diff --git a/ql/src/experimental/Unsafe/UnsafeUsageBad.go b/ql/src/experimental/Unsafe/UnsafeUsageBad.go index d699799eef6..ba83f31ade9 100644 --- a/ql/src/experimental/Unsafe/UnsafeUsageBad.go +++ b/ql/src/experimental/Unsafe/UnsafeUsageBad.go @@ -7,7 +7,7 @@ import ( func main() {} -func bad0() { +func badArrays() { // A harmless piece of data: harmless := [8]byte{'A', 'A', 'A', 'A', 'A', 'A', 'A', 'A'} // Something secret: @@ -25,85 +25,22 @@ func bad0() { fmt.Println("hello world") } } -func bad1() { - // A harmless piece of data: - harmless := [8]string{"A", "A", "A", "A", "A", "A", "A", "A"} - // Something secret: - secret := [9]string{"s", "e", "n", "s", "i", "t", "i", "v", "e"} - - // The declared `leaking` contains data from `harmless` - // plus the data from `secret`; - // (notice we read more than the length of harmless) - var leaking = (*[8 + 9]string)(unsafe.Pointer(&harmless)) // BAD - - fmt.Println(*leaking) - fmt.Println([17]string((*leaking))) - - // Avoid optimization: - if secret[0] == "42" { - fmt.Println("hello world") - } -} - -func bad2() { +func badIndexExpr() { // A harmless piece of data: harmless := [8]byte{'A', 'A', 'A', 'A', 'A', 'A', 'A', 'A'} // Something secret: secret := [9]byte{'s', 'e', 'n', 's', 'i', 't', 'i', 'v', 'e'} - // Read before secret: - var leaking = buffer_request(unsafe.Pointer(&harmless)) // BAD (see inside buffer_request func) + // Read before secret, overflowing into secret. + // NOTE: unsafe.Pointer(&harmless) != unsafe.Pointer(&harmless[2]) + // Even tough harmless and leaking have the same size, + // the new variable `leaking` will contain data starting from + // the address of the 3rd element of the `harmless` array, + // and continue for 8 bytes, going out of the boundaries of + // `harmless` and crossing into the memory occupied by `secret`. + var leaking = (*[8]byte)(unsafe.Pointer(&harmless[2])) // BAD - fmt.Println((string)(leaking[:])) - - // Avoid optimization: - if secret[0] == 123 { - fmt.Println("hello world") - } -} -func buffer_request(req unsafe.Pointer) [8 + 9]byte { - // The length of req is 8 bytes (see origin in above function), - // but we cast it to a longer array, - // which means that when the resulting array - // will be read, the read will also contain pieces of - // data from anything occupying memory after the target - // (namely, memory occupied by`secret`). - var buf [8 + 9]byte - buf = *(*[8 + 9]byte)(req) - return buf -} - -func bad3() { - // A harmless piece of data: - harmless := [1]int64{23} - // Something secret: - secret := [9]byte{'s', 'e', 'n', 's', 'i', 't', 'i', 'v', 'e'} - - // Read before secret, overflowing into secret - // (notice we read more than the length of harmless); - // the leaking array will not contain letters, - // but integers representing bytes from `secret`. - var leaking = (*[4]int64)(unsafe.Pointer(&harmless)) // BAD - - fmt.Println(*leaking) - - // Avoid optimization: - if secret[0] == 123 { - fmt.Println("hello world") - } -} -func bad4() { - // A harmless piece of data: - harmless := int8(123) - // Something secret: - secret := [9]byte{'s', 'e', 'n', 's', 'i', 't', 'i', 'v', 'e'} - - // Read before secret, overflowing into secret - // (notice we read more than the length of harmless); - // the leaking data will contain some bits from `secret`. - var leaking = (*int64)(unsafe.Pointer(&harmless)) // BAD - - fmt.Println(*leaking) + fmt.Println(string((*leaking)[:])) // Avoid optimization: if secret[0] == 123 { diff --git a/ql/src/experimental/Unsafe/WrongUsageOfUnsafe.ql b/ql/src/experimental/Unsafe/WrongUsageOfUnsafe.ql index 5ac1ed96782..fc27216167f 100644 --- a/ql/src/experimental/Unsafe/WrongUsageOfUnsafe.ql +++ b/ql/src/experimental/Unsafe/WrongUsageOfUnsafe.ql @@ -26,7 +26,7 @@ class ConversionToUnsafePointer extends ConversionExpr { ConversionToUnsafePointer() { getFinalType(getType()) instanceof UnsafePointerType } } -/* Type casting through the use of unsafe pointers.*/ +/* Type casting from a `unsafe.Pointer`.*/ class UnsafeTypeCastingConf extends TaintTracking::Configuration { UnsafeTypeCastingConf() { this = "UnsafeTypeCastingConf" } @@ -54,31 +54,41 @@ predicate castShortArrayToLongerArray( ) { exists( UnsafeTypeCastingConf cfg, ConversionExpr castBig, ConversionToUnsafePointer castLittle, - ArrayType arrTo, ArrayType arrFrom, int arrFromAvailableSize + ArrayType arrTo, ArrayType arrFrom, int arrFromSize | cfg.hasFlowPath(source, sink) and cfg.isSource(source.getNode(), castLittle) and cfg.isSink(sink.getNode(), castBig) and arrTo = getFinalType(castBig.getTypeExpr().getType()) and ( + // Array (whole) to array: + // The `unsafe.Pointer` expression is on the array + // (e.g. unsafe.Pointer(&someArray)), + // meaning that the pointer points to the start of the array: arrFrom = getFinalType(castLittle.getOperand().getType()) and - arrFromAvailableSize = arrFrom.getLength() and - message = - "Dangerous array type casting to [" + arrTo.getLength() + "]" + arrTo.getElementType() + - " from [" + arrFrom.getLength() + "]" + arrFrom.getElementType() + arrFromSize = arrFrom.getLength() and + message = "Dangerous array type casting to " + arrTo.pp() + " from " + arrFrom.pp() or + // Piece of array (starting from an index), to array: + // The `unsafe.Pointer` expression can also point to a specific + // element of an array + // (e.g. unsafe.Pointer(&someArray[2])), + // which will be the starting point in memory for the newly cast + // variable. exists(IndexExpr indexExpr | indexExpr = castLittle.getOperand().getChildExpr(0) and + // The `arrFrom` is the base of the index expression: arrFrom = indexExpr.getBase().getType() and - arrFromAvailableSize = arrFrom.getLength() - indexExpr.getIndex().getIntValue() and + // Calculate the size of the `arrFrom`: + arrFromSize = arrFrom.getLength() - indexExpr.getIndex().getIntValue() and message = - "Dangerous array type casting to [" + arrTo.getLength() + "]" + arrTo.getElementType() + - " from an index [" + arrFrom.getLength() + "]" + arrFrom.getElementType() + "[" + - indexExpr.getIndex().getIntValue() + "]" + "Dangerous array type casting to " + arrTo.pp() + " from an index expression " + + arrFrom.pp() + "[" + indexExpr.getIndex().getIntValue() + "]" + " (overflowing by " + + (arrTo.getLength() - arrFromSize) + " bytes)" ) ) and arrTo.getLength() > 0 and //TODO - arrTo.getLength() > arrFromAvailableSize + arrTo.getLength() > arrFromSize ) } @@ -100,9 +110,7 @@ predicate castTypeToArray(DataFlow::PathNode source, DataFlow::PathNode sink, st not typeFrom instanceof PointerType and not castLittle.getOperand().getChildExpr(0).(IndexExpr).getBase().getType() instanceof ArrayType and typeFrom = getFinalType(castLittle.getOperand().getType()) and - message = - "Dangerous type up-casting to [" + arrTo.getLength() + "]" + arrTo.getElementType() + " from " - + typeFrom + message = "Dangerous type up-casting to " + arrTo.pp() + " from " + typeFrom ) } @@ -116,47 +124,36 @@ predicate castDifferentBitSizeNumbers( ) { exists( UnsafeTypeCastingConf cfg, ConversionExpr castBig, ConversionToUnsafePointer castLittle, - CustomNumericType numTo, CustomNumericType numFrom + NumericType numTo, NumericType numFrom | cfg.hasFlowPath(source, sink) and cfg.isSource(source.getNode(), castLittle) and cfg.isSink(sink.getNode(), castBig) and numTo = getFinalType(castBig.getTypeExpr().getType()) and - ( - numFrom = getFinalType(castLittle.getOperand().getType()) or - numFrom = - getFinalType(getFinalType(castLittle.getOperand().getType()) - .(StructType) - .getField(_) - .getType()) - ) and + numFrom = getFinalType(castLittle.getOperand().getType()) and // TODO: also consider cast from uint to int? - numTo.getSize() != numFrom.getSize() and + getNumericTypeSize(numTo) != getNumericTypeSize(numFrom) and message = "Dangerous numeric type casting to " + numTo.getName() + " from " + numFrom.getName() ) } /* - * A numeric type that returns bit size 0 if it is one of the numeric + * Returns 0 if the NumericType is one of the numeric * types that have architecture-specific bit sizes. */ -class CustomNumericType extends NumericType { - CustomNumericType() { this instanceof NumericType } - - override int getSize() { - ( - // If the numeric types have arch-specific - // bit sizes, then set the size to 0 to distinguish - // it from others. - this instanceof UintType - or - this instanceof IntType - ) and - result = 0 +int getNumericTypeSize(NumericType typ) { + ( + // If the numeric types have arch-specific + // bit sizes, then set the size to 0 to distinguish + // it from others. + typ instanceof UintType or - result = this.(NumericType).getSize() - } + typ instanceof IntType + ) and + result = 0 + or + result = typ.getSize() } from DataFlow::PathNode source, DataFlow::PathNode sink, string message diff --git a/ql/test/experimental/Unsafe/WrongUsageOfUnsafe.expected b/ql/test/experimental/Unsafe/WrongUsageOfUnsafe.expected index e0d9f0cac41..976b2e094dc 100644 --- a/ql/test/experimental/Unsafe/WrongUsageOfUnsafe.expected +++ b/ql/test/experimental/Unsafe/WrongUsageOfUnsafe.expected @@ -48,8 +48,8 @@ nodes | WrongUsageOfUnsafe.go:292:16:292:48 | type conversion | semmle.label | type conversion | | WrongUsageOfUnsafe.go:292:23:292:47 | type conversion : unsafe.Pointer | semmle.label | type conversion : unsafe.Pointer | #select -| WrongUsageOfUnsafe.go:77:16:77:55 | type conversion | WrongUsageOfUnsafe.go:77:27:77:54 | type conversion : unsafe.Pointer | WrongUsageOfUnsafe.go:77:16:77:55 | type conversion | $@. | WrongUsageOfUnsafe.go:77:27:77:54 | type conversion | Dangerous array type casting to [8]uint8 from an index [8]uint8[2] | -| WrongUsageOfUnsafe.go:111:16:111:59 | type conversion | WrongUsageOfUnsafe.go:111:31:111:58 | type conversion : unsafe.Pointer | WrongUsageOfUnsafe.go:111:16:111:59 | type conversion | $@. | WrongUsageOfUnsafe.go:111:31:111:58 | type conversion | Dangerous array type casting to [17]uint8 from an index [8]uint8[0] | +| WrongUsageOfUnsafe.go:77:16:77:55 | type conversion | WrongUsageOfUnsafe.go:77:27:77:54 | type conversion : unsafe.Pointer | WrongUsageOfUnsafe.go:77:16:77:55 | type conversion | $@. | WrongUsageOfUnsafe.go:77:27:77:54 | type conversion | Dangerous array type casting to [8]uint8 from an index expression [8]uint8[2] (overflowing by 2 bytes) | +| WrongUsageOfUnsafe.go:111:16:111:59 | type conversion | WrongUsageOfUnsafe.go:111:31:111:58 | type conversion : unsafe.Pointer | WrongUsageOfUnsafe.go:111:16:111:59 | type conversion | $@. | WrongUsageOfUnsafe.go:111:31:111:58 | type conversion | Dangerous array type casting to [17]uint8 from an index expression [8]uint8[0] (overflowing by 9 bytes) | | WrongUsageOfUnsafe.go:129:16:129:56 | type conversion | WrongUsageOfUnsafe.go:129:31:129:55 | type conversion : unsafe.Pointer | WrongUsageOfUnsafe.go:129:16:129:56 | type conversion | $@. | WrongUsageOfUnsafe.go:129:31:129:55 | type conversion | Dangerous array type casting to [17]uint8 from [8]uint8 | | WrongUsageOfUnsafe.go:149:16:149:56 | type conversion | WrongUsageOfUnsafe.go:149:31:149:55 | type conversion : unsafe.Pointer | WrongUsageOfUnsafe.go:149:16:149:56 | type conversion | $@. | WrongUsageOfUnsafe.go:149:31:149:55 | type conversion | Dangerous array type casting to [17]uint8 from [8]uint8 | | WrongUsageOfUnsafe.go:166:16:166:58 | type conversion | WrongUsageOfUnsafe.go:166:33:166:57 | type conversion : unsafe.Pointer | WrongUsageOfUnsafe.go:166:16:166:58 | type conversion | $@. | WrongUsageOfUnsafe.go:166:33:166:57 | type conversion | Dangerous array type casting to [17]string from [8]string |