Merge pull request #15128 from owen-mc/go/fix-fp-incorrect-integer-conversion-signedness

Go: fix FP in incorrect integer conversion query relating to strict comparisons with MaxInt and MaxUint

Author: owen-mc

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

@@ -15,15 +15,11 @@ abstract private class MaxIntOrMaxUint extends DeclaredConstant {
    */
   predicate isBoundFor(int b, int architectureBitSize, float strictnessOffset) {
     // 2.pow(x) - 1 - strictnessOffset <= 2.pow(b) - 1
-    exists(int x |
-      x = this.getOrder(architectureBitSize) and
-      b = validBitSize() and
-      (
-        strictnessOffset = 0 and x <= b
-        or
-        strictnessOffset = 1 and x <= b - 1
-      )
-    )
+    // For the values that we are restricting `b` to, `strictnessOffset` has no
+    // effect on the result, so we can ignore it.
+    b = validBitSize() and
+    strictnessOffset = [0, 1] and
+    this.getOrder(architectureBitSize) <= b
   }
 }
 

go/ql/src/change-notes/2023-12-17-incorrect-integer-conversion-fix.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* There was a bug in the query `go/incorrect-integer-conversion` which meant that upper bound checks using a strict inequality (`<`) and comparing against `math.MaxInt` or `math.MaxUint` were not considered correctly, which led to false positives. This has now been fixed.

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

@@ -1,2 +1,2 @@
-failures
 testFailures
+failures

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

@@ -491,3 +491,10 @@ func typeAssertion(s string) {
 	}
 
 }
+
+func dealWithArchSizeCorrectly(s string) uint {
+	if i, err := strconv.ParseUint(s, 10, 64); err == nil && i < math.MaxUint {
+		return uint(i)
+	}
+	return 0
+}