Refactor #1

This has one problem: we aren't able to properly set sinks to be
barriers, to avoid the issue that we get flows from a source A to sink1,
and also from A to sink1 and then on to sink2, and
so on. These are
seen in SPURIOUS results in:
go/ql/test/query-tests/Security/CWE-681/IncorrectIntegerConversion.go

Author: owen-mc

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

@@ -17,11 +17,12 @@ float getMaxIntValue(int bitSize, boolean isSigned) {
  * Get the size of `int` or `uint` in `file`, or 0 if it is
  * architecture-specific.
  */
-int getIntTypeBitSize(File file) {
+bindingset[architectureSpecificBitSize]
+int getIntTypeBitSize(File file, int architectureSpecificBitSize) {
   file.constrainsIntBitSize(result)
   or
   not file.constrainsIntBitSize(_) and
-  result = 0
+  result = architectureSpecificBitSize
 }
 
 /**
@@ -90,7 +91,7 @@ deprecated class ConversionWithoutBoundsCheckConfig extends TaintTracking::Confi
       ) and
       (
         if apparentBitSize = 0
-        then effectiveBitSize = getIntTypeBitSize(source.getFile())
+        then effectiveBitSize = getIntTypeBitSize(source.getFile(), 0)
         else effectiveBitSize = apparentBitSize
       ) and
       // `effectiveBitSize` could be any value between 0 and 64, but we
@@ -113,7 +114,7 @@ deprecated class ConversionWithoutBoundsCheckConfig extends TaintTracking::Confi
         bitSize = integerType.getSize()
         or
         not exists(integerType.getSize()) and
-        bitSize = getIntTypeBitSize(sink.getFile())
+        bitSize = getIntTypeBitSize(sink.getFile(), 0)
       ) and
       if integerType instanceof SignedIntegerType then sinkIsSigned = true else sinkIsSigned = false
     ) and
@@ -152,50 +153,73 @@ deprecated class ConversionWithoutBoundsCheckConfig extends TaintTracking::Confi
 
 /** Flow state for ConversionWithoutBoundsCheckConfig. */
 newtype IntegerConversionFlowState =
-  /** Keep track of info about the source and potential sinks. */
-  TFlowstate(boolean sinkIsSigned, int sourceBitSize, int sinkBitSize) {
-    sinkIsSigned in [true, false] and
-    isIncorrectIntegerConversion(sourceBitSize, sinkBitSize)
+  TFlowstate(int bitSize, boolean isSigned) {
+    bitSize = [0, 8, 16, 32, 64] and
+    isSigned = [true, false]
   }
 
-/** Gets whether the sink is signed. */
-boolean getSinkIsSigned(IntegerConversionFlowState state) { state = TFlowstate(result, _, _) }
+/**
+ * The integer type with bit size `bitSize` and signedness `isSigned` has
+ * maximum value `2^result - 1`.
+ */
+bindingset[bitSize, bitSizeForZero]
+private int f(int bitSize, boolean isSigned, int bitSizeForZero) {
+  exists(int effectiveBitSize | effectiveBitSize = replaceZeroWith(bitSize, bitSizeForZero) |
+    isSigned = true and result = effectiveBitSize - 1
+    or
+    isSigned = false and result = effectiveBitSize
+  )
+}
 
-/** Gets the bit size of the source. */
-int getSourceBitSize(IntegerConversionFlowState state) { state = TFlowstate(_, result, _) }
+private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConfigSig {
+  class FlowState extends IntegerConversionFlowState {
+    /** Gets the signedness of the flow state. */
+    boolean getIsSigned() { this = TFlowstate(_, result) }
 
-/** Gets the bit size of the sink. */
-int getSinkBitSize(IntegerConversionFlowState state) { state = TFlowstate(_, _, result) }
+    /** Gets the bit size of the flow state. */
+    int getBitSize() { this = TFlowstate(result, _) }
 
-private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConfigSig {
-  class FlowState = IntegerConversionFlowState;
+    string toString() { result = "FlowState(" + this.getBitSize() + "," + this.getIsSigned() + ")" }
+  }
 
   predicate isSource(DataFlow::Node source, FlowState state) {
     exists(
       DataFlow::CallNode c, IntegerParser::Range ip, int apparentBitSize, int effectiveBitSize
     |
-      c.getTarget() = ip and source = c.getResult(0)
-    |
+      c.getTarget() = ip and
+      source = c.getResult(0) and
       (
         apparentBitSize = ip.getTargetBitSize()
         or
         // If we are reading a variable, check if it is
         // `strconv.IntSize`, and use 0 if it is.
-        exists(DataFlow::Node rawBitSize | rawBitSize = ip.getTargetBitSizeInput().getNode(c) |
+        exists(DataFlow::Node rawBitSize |
+          rawBitSize = ip.getTargetBitSizeInput().getNode(c) and
           if rawBitSize = any(Strconv::IntSize intSize).getARead()
           then apparentBitSize = 0
           else apparentBitSize = rawBitSize.getIntValue()
         )
       ) and
-      (
-        if apparentBitSize = 0
-        then effectiveBitSize = getIntTypeBitSize(source.getFile())
-        else effectiveBitSize = apparentBitSize
-      ) and
-      // `effectiveBitSize` could be any value between 0 and 64, but we
-      // can round it up to the nearest size of an integer type without
-      // changing behavior.
-      getSourceBitSize(state) = min(int b | b in [0, 8, 16, 32, 64] and b >= effectiveBitSize)
+      // Note that `getIntTypeBitSize` can return 0, so `effectiveBitSize`
+      // can be 0. Also `effectiveBitSize` is not necessarily the bit-size
+      // of an integer type - it can be any integer between 0 and 64.
+      effectiveBitSize = replaceZeroWith(apparentBitSize, getIntTypeBitSize(source.getFile(), 0)) and
+      // `state` represents an integer type for the sink which will overflow
+      // if assigned the maximum value that can be parsed by `ip`. If both the
+      // source and the sink are `int` or `uint` then overflow can only happen
+      // if we are converting from `uint` to `int`.
+      if state.getBitSize() = 0 and effectiveBitSize = 0
+      then ip.isSigned() = false and state.getIsSigned() = true
+      else
+        // The maximum value for a signed type with n bits is 2^(n - 1) - 1.
+        // The maximum value for an unsigned type with n bits is 2^n - 1. To
+        // deal with `int` and `uint` we treat them as 64-bit if they are at
+        // the source (so we catch that flow from `int` to `int32` is incorrect
+        // on a 64-bit architecture) and 32-bit if they are at the sink (so we
+        // catch that flow from `int64` to `int` is incorrect on a 32-bit
+        // architecture), and we have already dealt with the case that both
+        // source and sink are `int` or `uint`.
+        f(state.getBitSize(), state.getIsSigned(), 32) < f(effectiveBitSize, ip.isSigned(), 64)
     )
   }
 
@@ -205,18 +229,18 @@ private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConf
    * not also in a right-shift expression. We allow this case because it is
    * a common pattern to serialise `byte(v)`, `byte(v >> 8)`, and so on.
    */
-  additional predicate isSinkWithBitSize(
-    DataFlow::TypeCastNode sink, boolean sinkIsSigned, int bitSize
-  ) {
+  additional predicate isSink2(DataFlow::TypeCastNode sink, FlowState state) {
     sink.asExpr() instanceof ConversionExpr and
     exists(IntegerType integerType | sink.getResultType().getUnderlyingType() = integerType |
       (
-        bitSize = integerType.getSize()
+        state.getBitSize() = integerType.getSize()
         or
         not exists(integerType.getSize()) and
-        bitSize = getIntTypeBitSize(sink.getFile())
+        state.getBitSize() = getIntTypeBitSize(sink.getFile(), 0)
       ) and
-      if integerType instanceof SignedIntegerType then sinkIsSigned = true else sinkIsSigned = false
+      if integerType instanceof SignedIntegerType
+      then state.getIsSigned() = true
+      else state.getIsSigned() = false
     ) and
     not exists(ShrExpr shrExpr |
       shrExpr.getLeftOperand().getGlobalValueNumber() =
@@ -231,28 +255,20 @@ private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConf
     // 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).
-    isSinkWithBitSize(sink.getASuccessor(), getSinkIsSigned(state), getSinkBitSize(state))
+    isSink2(sink.getASuccessor(), state)
   }
 
   predicate isBarrier(DataFlow::Node node, FlowState state) {
-    exists(boolean sinkIsSigned, int sinkBitSize |
-      sinkIsSigned = getSinkIsSigned(state) and
-      sinkBitSize = getSinkBitSize(state)
+    // To catch flows that only happen on 32-bit architectures we consider an
+    // architecture-dependent sink bit size to be 32.
+    exists(UpperBoundCheckGuard g, int effectiveSinkBitSize |
+      effectiveSinkBitSize = replaceZeroWith(state.getBitSize(), 32)
     |
-      // To catch flows that only happen on 32-bit architectures we
-      // consider an architecture-dependent sink bit size to be 32.
-      exists(UpperBoundCheckGuard g, int effectiveSinkBitSize |
-        if sinkBitSize != 0 then effectiveSinkBitSize = sinkBitSize else effectiveSinkBitSize = 32
-      |
-        node = DataFlow::BarrierGuard<upperBoundCheckGuard/3>::getABarrierNodeForGuard(g) and
-        g.isBoundFor(effectiveSinkBitSize, sinkIsSigned)
-      )
-      or
-      exists(int bitSize |
-        isIncorrectIntegerConversion(getSourceBitSize(state), bitSize) and
-        isSinkWithBitSize(node, sinkIsSigned, bitSize)
-      )
+      node = DataFlow::BarrierGuard<upperBoundCheckGuard/3>::getABarrierNodeForGuard(g) and
+      g.isBoundFor(effectiveSinkBitSize, state.getIsSigned())
     )
+    or
+    isSink2(node, state)
   }
 }
 
@@ -324,3 +340,8 @@ string describeBitSize(int bitSize, int intTypeBitSize) {
         "a number with architecture-dependent bit-width, which is constrained to be " +
           intTypeBitSize + "-bit by build constraints,"
 }
+
+bindingset[inputBitSize, replacementForZero]
+private int replaceZeroWith(int inputBitSize, int replacementForZero) {
+  if inputBitSize = 0 then result = replacementForZero else result = inputBitSize
+}

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

@@ -25,6 +25,6 @@ where
   sinkConverted = sink.getNode().getASuccessor()
 select sinkConverted, source, sink,
   "Incorrect conversion of " +
-    describeBitSize(getSourceBitSize(sink.getState()), getIntTypeBitSize(source.getNode().getFile()))
-    + " from $@ to a lower bit size type " + sinkConverted.getType().getUnderlyingType().getName() +
+    describeBitSize(sink.getState().getBitSize(), getIntTypeBitSize(source.getNode().getFile(), 0)) +
+    " 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.expected

@@ -1,2 +1,3 @@
 failures
 testFailures
+| IncorrectIntegerConversion.go:339:23:339:67 | comment | Fixed spurious result:hasValueFlow="type conversion" |

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

@@ -134,7 +134,7 @@ func testParseUint() {
 		if err != nil {
 			panic(err)
 		}
-		_ = int8(parsed)
+		_ = int8(parsed) // $ hasValueFlow="type conversion"
 		_ = uint8(parsed)
 		_ = int16(parsed)
 		_ = uint16(parsed)
@@ -152,7 +152,7 @@ func testParseUint() {
 		}
 		_ = int8(parsed)  // $ hasValueFlow="type conversion"
 		_ = uint8(parsed) // $ hasValueFlow="type conversion"
-		_ = int16(parsed)
+		_ = int16(parsed) // $ hasValueFlow="type conversion"
 		_ = uint16(parsed)
 		_ = int32(parsed)
 		_ = uint32(parsed)
@@ -170,11 +170,11 @@ func testParseUint() {
 		_ = uint8(parsed)  // $ hasValueFlow="type conversion"
 		_ = int16(parsed)  // $ hasValueFlow="type conversion"
 		_ = uint16(parsed) // $ hasValueFlow="type conversion"
-		_ = int32(parsed)
+		_ = int32(parsed)  // $ hasValueFlow="type conversion"
 		_ = uint32(parsed)
 		_ = int64(parsed)
 		_ = uint64(parsed)
-		_ = int(parsed)
+		_ = int(parsed) // $ hasValueFlow="type conversion"
 		_ = uint(parsed)
 	}
 	{
@@ -188,7 +188,7 @@ func testParseUint() {
 		_ = uint16(parsed) // $ hasValueFlow="type conversion"
 		_ = int32(parsed)  // $ hasValueFlow="type conversion"
 		_ = uint32(parsed) // $ hasValueFlow="type conversion"
-		_ = int64(parsed)
+		_ = int64(parsed)  // $ hasValueFlow="type conversion"
 		_ = uint64(parsed)
 		_ = int(parsed)  // $ hasValueFlow="type conversion"
 		_ = uint(parsed) // $ hasValueFlow="type conversion"
@@ -204,9 +204,9 @@ func testParseUint() {
 		_ = uint16(parsed) // $ hasValueFlow="type conversion"
 		_ = int32(parsed)  // $ hasValueFlow="type conversion"
 		_ = uint32(parsed) // $ hasValueFlow="type conversion"
-		_ = int64(parsed)
+		_ = int64(parsed)  // $ hasValueFlow="type conversion"
 		_ = uint64(parsed)
-		_ = int(parsed)
+		_ = int(parsed) // $ hasValueFlow="type conversion"
 		_ = uint(parsed)
 	}
 }
@@ -336,15 +336,15 @@ func testPathWithMoreThanOneSink(input string) {
 			panic(err)
 		}
 		v1 := int16(parsed) // $ hasValueFlow="type conversion"
-		_ = int16(v1)
+		_ = int16(v1)       // $ SPURIOUS: hasValueFlow="type conversion"
 	}
 	{
 		parsed, err := strconv.ParseInt(input, 10, 32)
 		if err != nil {
 			panic(err)
 		}
 		v := int16(parsed) // $ hasValueFlow="type conversion"
-		_ = int8(v)
+		_ = int8(v)        // $ SPURIOUS: hasValueFlow="type conversion"
 	}
 	{
 		parsed, err := strconv.ParseInt(input, 10, 32)
@@ -353,7 +353,7 @@ func testPathWithMoreThanOneSink(input string) {
 		}
 		v1 := int32(parsed)
 		v2 := int16(v1) // $ hasValueFlow="type conversion"
-		_ = int8(v2)
+		_ = int8(v2)    // $ SPURIOUS: hasValueFlow="type conversion"
 	}
 	{
 		parsed, err := strconv.ParseInt(input, 10, 16)

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

@@ -20,7 +20,7 @@ func testIntSource386() {
 		if err != nil {
 			panic(err)
 		}
-		_ = int32(parsed)
+		_ = int32(parsed) // $ hasValueFlow="type conversion"
 		_ = uint32(parsed)
 	}
 	{

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

@@ -23,7 +23,7 @@ func testIntSource32() {
 		if err != nil {
 			panic(err)
 		}
-		_ = int32(parsed)
+		_ = int32(parsed) // $ hasValueFlow="type conversion"
 		_ = uint32(parsed)
 	}
 	{

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

@@ -20,7 +20,7 @@ func testIntSinkAmd64() {
 		if err != nil {
 			panic(err)
 		}
-		_ = int(parsed)
+		_ = int(parsed) // $ hasValueFlow="type conversion"
 		_ = uint(parsed)
 	}
 }

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

@@ -23,7 +23,7 @@ func testIntSink64() {
 		if err != nil {
 			panic(err)
 		}
-		_ = int(parsed)
+		_ = int(parsed) // $ hasValueFlow="type conversion"
 		_ = uint(parsed)
 	}
 }

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

@@ -24,7 +24,7 @@ func oldTestIntSink64() {
 		if err != nil {
 			panic(err)
 		}
-		_ = int(parsed)
+		_ = int(parsed) // $ hasValueFlow="type conversion"
 		_ = uint(parsed)
 	}
 }