Implement code review feedback

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 {

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

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 |