Merge pull request github#12441 from smowton/smowton/fix/golang-incorrect-integer-conversion-sanitizer

Go: fix incorrect-integer-conversion sanitizer

Author: smowton

go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll

@@ -123,7 +123,13 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
     )
   }
 
-  override predicate isSink(DataFlow::Node sink) { this.isSinkWithBitSize(sink, sinkBitSize) }
+  override predicate isSink(DataFlow::Node sink) {
+    // We use the argument of the type conversion as the configuration sink so that we
+    // can sanitize the result of the conversion to prevent flow on to further sinks
+    // without needing to use `isSanitizerOut`, which doesn't work with flow states
+    // (and therefore the legacy `TaintTracking::Configuration` class).
+    this.isSinkWithBitSize(sink.getASuccessor(), sinkBitSize)
+  }
 
   override predicate isSanitizer(DataFlow::Node node) {
     // To catch flows that only happen on 32-bit architectures we
@@ -135,10 +141,9 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
       g.isBoundFor(bitSize, sinkIsSigned)
     )
     or
-    exists(DataFlow::Node sink, int bitSize |
+    exists(int bitSize |
       isIncorrectIntegerConversion(sourceBitSize, bitSize) and
-      this.isSinkWithBitSize(sink, bitSize) and
-      TaintTracking::localTaintStep(sink, node)
+      this.isSinkWithBitSize(node, bitSize)
     )
   }
 }

go/ql/src/Security/CWE-681/IncorrectIntegerConversionQuery.ql

@@ -19,10 +19,13 @@ import semmle.go.security.IncorrectIntegerConversionLib
 
 from
   DataFlow::PathNode source, DataFlow::PathNode sink, ConversionWithoutBoundsCheckConfig cfg,
-  DataFlow::CallNode call
-where cfg.hasFlowPath(source, sink) and call.getResult(0) = source.getNode()
-select sink.getNode(), source, sink,
+  DataFlow::CallNode call, DataFlow::Node sinkConverted
+where
+  cfg.hasFlowPath(source, sink) and
+  call.getResult(0) = source.getNode() and
+  sinkConverted = sink.getNode().getASuccessor()
+select sinkConverted, source, sink,
   "Incorrect conversion of " +
     describeBitSize(cfg.getSourceBitSize(), getIntTypeBitSize(source.getNode().getFile())) +
-    " from $@ to a lower bit size type " + sink.getNode().getType().getUnderlyingType().getName() +
+    " from $@ to a lower bit size type " + sinkConverted.getType().getUnderlyingType().getName() +
     " without an upper bound check.", source, call.getTarget().getQualifiedName()

go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.ql

@@ -1,11 +1,23 @@
 import go
-import TestUtilities.InlineFlowTest
+import TestUtilities.InlineExpectationsTest
 import semmle.go.security.IncorrectIntegerConversionLib
 
-class IncorrectIntegerConversionTest extends InlineFlowTest {
-  override DataFlow::Configuration getValueFlowConfig() {
-    result = any(ConversionWithoutBoundsCheckConfig config)
-  }
+class TestIncorrectIntegerConversion extends InlineExpectationsTest {
+  TestIncorrectIntegerConversion() { this = "TestIncorrectIntegerConversion" }
+
+  override string getARelevantTag() { result = "hasValueFlow" }
 
-  override DataFlow::Configuration getTaintFlowConfig() { none() }
+  override predicate hasActualResult(Location location, string element, string tag, string value) {
+    tag = "hasValueFlow" and
+    exists(DataFlow::Node sink, DataFlow::Node sinkConverted |
+      any(ConversionWithoutBoundsCheckConfig config).hasFlowTo(sink) and
+      sinkConverted = sink.getASuccessor()
+    |
+      sinkConverted
+          .hasLocationInfo(location.getFile().getAbsolutePath(), location.getStartLine(),
+            location.getStartColumn(), location.getEndLine(), location.getEndColumn()) and
+      element = sinkConverted.toString() and
+      value = "\"" + sinkConverted.toString() + "\""
+    )
+  }
 }