Refactor #2

Author: owen-mc

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

@@ -233,7 +233,7 @@ private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConf
         state.getConcreteBitsize() =
           min(int bitsize |
             bitsize = validBitsize() and
-            f(effectiveBitSize, ip.isSigned(), 64) <= state.getConcreteBitsize()
+            f(effectiveBitSize, ip.isSigned(), 64) <= bitsize
           |
             bitsize
           )
@@ -248,16 +248,25 @@ private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConf
    */
   additional predicate isSink2(DataFlow::TypeCastNode sink, FlowState state) {
     sink.asExpr() instanceof ConversionExpr and
-    exists(IntegerType integerType | sink.getResultType().getUnderlyingType() = integerType |
+    exists(IntegerType integerType, int sinkBitsize, boolean sinkIsSigned |
+      sink.getResultType().getUnderlyingType() = integerType and
       (
-        state.getBitSize() = integerType.getSize()
+        sinkBitsize = integerType.getSize()
         or
         not exists(integerType.getSize()) and
-        state.getBitSize() = getIntTypeBitSize(sink.getFile(), 0)
+        sinkBitsize = getIntTypeBitSize(sink.getFile(), 0)
       ) and
-      if integerType instanceof SignedIntegerType
-      then state.getIsSigned() = true
-      else state.getIsSigned() = false
+      if integerType instanceof SignedIntegerType then sinkIsSigned = true else sinkIsSigned = false
+    |
+      // Flow state has a concrete value
+      f(sinkBitsize, sinkIsSigned, 32) < state.getConcreteBitsize()
+      or
+      // Flow state has an architecture-independent value
+      exists(boolean signed | signed = state.getArchictureDependentSignedness() |
+        if sinkBitsize = 0
+        then sinkIsSigned = true and signed = false
+        else f(sinkBitsize, sinkIsSigned, 32) < f(0, signed, 64)
+      )
     ) and
     not exists(ShrExpr shrExpr |
       shrExpr.getLeftOperand().getGlobalValueNumber() =
@@ -276,16 +285,42 @@ private module ConversionWithoutBoundsCheckConfig implements DataFlow::StateConf
   }
 
   predicate isBarrier(DataFlow::Node node, FlowState state) {
+    isUpperBoundCheckBarrier(node, state, _)
+    or
+    isSink2(node, state)
+  }
+
+  additional predicate isUpperBoundCheckBarrier(
+    DataFlow::Node node, FlowState state, UpperBoundCheckGuard g
+  ) {
     // 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)
+    exists(int effectiveSinkBitSize |
+      effectiveSinkBitSize = state.getConcreteBitsize() or
+      effectiveSinkBitSize = f(0, state.getArchictureDependentSignedness(), 32)
     |
       node = DataFlow::BarrierGuard<upperBoundCheckGuard/3>::getABarrierNodeForGuard(g) and
-      g.isBoundFor(effectiveSinkBitSize, state.getIsSigned())
+      // use `false` for `isSigned` as we have already subtracted 1 from
+      // `effectiveSinkBitSize` if necessary
+      g.isBoundFor2(effectiveSinkBitSize)
+    )
+  }
+
+  predicate isAdditionalFlowStep(
+    DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
+  ) {
+    exists(UpperBoundCheckGuard g |
+      isUpperBoundCheckBarrier(node1, state1, g) and
+      node2 = node1.getASuccessor() and
+      state2.getConcreteBitsize() =
+        max(int bitsize |
+          bitsize = validBitsize() and
+          bitsize < state1.getConcreteBitsize() and
+          not g.isBoundFor2(bitsize)
+        |
+          bitsize
+        )
     )
-    or
-    isSink2(node, state)
   }
 }
 
@@ -337,6 +372,33 @@ class UpperBoundCheckGuard extends DataFlow::RelationalComparisonNode {
     )
   }
 
+  predicate isBoundFor2(int bitSize) {
+    bitSize = [7, 8, 15, 16, 31, 32, 63, 64] and
+    exists(float bound, float strictnessOffset |
+      // For `x < c` the bound is `c-1`. For `x >= c` we will be an upper bound
+      // on the `branch` argument of `checks` is false, which is equivalent to
+      // `x < c`.
+      if expr instanceof LssExpr or expr instanceof GeqExpr
+      then strictnessOffset = 1
+      else strictnessOffset = 0
+    |
+      (
+        bound = expr.getAnOperand().getExactValue().toFloat()
+        or
+        exists(DeclaredConstant maxint | maxint.hasQualifiedName("math", "MaxInt") |
+          expr.getAnOperand() = maxint.getAReference() and
+          bound = getMaxIntValue(32, true)
+        )
+        or
+        exists(DeclaredConstant maxuint | maxuint.hasQualifiedName("math", "MaxUint") |
+          expr.getAnOperand() = maxuint.getAReference() and
+          bound = getMaxIntValue(32, false)
+        )
+      ) and
+      bound - strictnessOffset <= 2.pow(bitSize) - 1
+    )
+  }
+
   /** Holds if this guard validates `e` upon evaluating to `branch`. */
   predicate checks(Expr e, boolean branch) {
     this.leq(branch, DataFlow::exprNode(e), _, _) and

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

@@ -1,3 +1,2 @@
 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

@@ -336,15 +336,15 @@ func testPathWithMoreThanOneSink(input string) {
 			panic(err)
 		}
 		v1 := int16(parsed) // $ hasValueFlow="type conversion"
-		_ = int16(v1)       // $ SPURIOUS: hasValueFlow="type conversion"
+		_ = int16(v1)
 	}
 	{
 		parsed, err := strconv.ParseInt(input, 10, 32)
 		if err != nil {
 			panic(err)
 		}
 		v := int16(parsed) // $ hasValueFlow="type conversion"
-		_ = int8(v)        // $ SPURIOUS: hasValueFlow="type conversion"
+		_ = int8(v)
 	}
 	{
 		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)    // $ SPURIOUS: hasValueFlow="type conversion"
+		_ = int8(v2)
 	}
 	{
 		parsed, err := strconv.ParseInt(input, 10, 16)