Merge pull request #14606 from owen-mc/go/incorrect-integer-conversion-fixes

Go: Two fixes to upper bound checks in "incorrect integer conversion" query

Author: owen-mc

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

@@ -1,16 +1,48 @@
 import go
 
-/**
- * Gets the maximum value of an integer (signed if `isSigned`
- * is true, unsigned otherwise) with `bitSize` bits.
- */
-float getMaxIntValue(int bitSize, boolean isSigned) {
-  bitSize in [8, 16, 32] and
-  (
-    isSigned = true and result = 2.pow(bitSize - 1) - 1
-    or
-    isSigned = false and result = 2.pow(bitSize) - 1
-  )
+/** The constant `math.MaxInt` or the constant `math.MaxUint`. */
+abstract private class MaxIntOrMaxUint extends DeclaredConstant {
+  /**
+   * Gets the (binary) order of magnitude when the architecture has bit size
+   * `architectureBitSize`, which is defined to be the integer `x` such that
+   * `2.pow(x) - 1` is the value of this constant.
+   */
+  abstract int getOrder(int architectureBitSize);
+
+  /**
+   * Holds if the value of this constant given `architectureBitSize` minus
+   * `strictnessOffset` is less than or equal to `2.pow(b) - 1`.
+   */
+  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
+      )
+    )
+  }
+}
+
+/** The constant `math.MaxInt`. */
+private class MaxInt extends MaxIntOrMaxUint {
+  MaxInt() { this.hasQualifiedName("math", "MaxInt") }
+
+  override int getOrder(int architectureBitSize) {
+    architectureBitSize = [32, 64] and result = architectureBitSize - 1
+  }
+}
+
+/** The constant `math.MaxUint`. */
+private class MaxUint extends MaxIntOrMaxUint {
+  MaxUint() { this.hasQualifiedName("math", "MaxUint") }
+
+  override int getOrder(int architectureBitSize) {
+    architectureBitSize = [32, 64] and result = architectureBitSize
+  }
 }
 
 /**
@@ -204,7 +236,7 @@ private class MaxValueState extends TMaxValueState {
   predicate architectureBitSizeUnknown() { this.architectureBitSize().isUnknown() }
 
   /**
-   * Gets the bitsize we should use for a sink.
+   * Gets the bitsize we should use for a sink of type `uint`.
    *
    * If the architecture bit size is known, then we should use that. Otherwise,
    * we should use 32 bits, because that will find results that only exist on
@@ -257,15 +289,27 @@ private predicate upperBoundCheckGuard(DataFlow::Node g, Expr e, boolean branch)
 /** An upper bound check that compares a variable to a constant value. */
 class UpperBoundCheckGuard extends DataFlow::RelationalComparisonNode {
   UpperBoundCheckGuard() {
+    // Note that even though `x > c` and `x >= c` do not look like upper bound
+    // checks, on the branches where they are false the conditions are `x <= c`
+    // and `x < c` respectively, which are upper bound checks.
     count(expr.getAnOperand().getExactValue()) = 1 and
     expr.getAnOperand().getType().getUnderlyingType() instanceof IntegerType
   }
 
   /**
-   * Holds if this upper bound check ensures the non-constant operand is less
-   * than or equal to `2^(bitsize) - 1`. In this  case, the upper bound check
-   * is a barrier guard. `architectureBitSize` is used if the constant operand
-   * is `math.MaxInt` or `math.MaxUint`.
+   * Holds if this upper bound check should stop flow for a flow state with bit
+   * size `bitSize` and architecture bit size `architectureBitSize`.
+   *
+   * A flow state has bit size `bitSize` if that is the smallest valid bit size
+   * `b` such that the maximum value that could get to that point is less than
+   * or equal to `2^(b) - 1`. So the flow should be stopped if there is a valid
+   * bit size `b` which is less than `bitSize` such that the maximum value that
+   * could get to that point is than or equal to `2^(b) - 1`. In this  case,
+   * the upper bound check is a barrier guard, because the flow should have bit
+   * size equal to the smallest such `b` instead of `bitSize`.
+   *
+   * The argument `architectureBitSize` is only used if the constant operand is
+   * `math.MaxInt` or `math.MaxUint`.
    *
    * Note that we have to use floats here because integers in CodeQL are
    * represented by 32-bit signed integers, which cannot represent some of the
@@ -274,25 +318,25 @@ class UpperBoundCheckGuard extends DataFlow::RelationalComparisonNode {
   predicate isBoundFor(int bitSize, int architectureBitSize) {
     bitSize = validBitSize() and
     architectureBitSize = [32, 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`.
+    exists(int b, float strictnessOffset |
+      // It is sufficient to check for the next valid bit size below `bitSize`.
+      b = max(int a | a = validBitSize() and a < bitSize) and
+      // We will use the format `x <= c - strictnessOffset`. Since `x < c` is
+      // the same as `x <= c-1`, we set `strictnessOffset` to 1 in this case.
+      // For `x >= c` we will be dealing with the case where 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
     |
-      exists(DeclaredConstant maxint, DeclaredConstant maxuint |
-        maxint.hasQualifiedName("math", "MaxInt") and maxuint.hasQualifiedName("math", "MaxUint")
-      |
-        if expr.getAnOperand() = maxint.getAReference()
-        then bound = getMaxIntValue(architectureBitSize, true)
-        else
-          if expr.getAnOperand() = maxuint.getAReference()
-          then bound = getMaxIntValue(architectureBitSize, false)
-          else bound = expr.getAnOperand().getExactValue().toFloat()
-      ) and
-      bound - strictnessOffset < 2.pow(bitSize) - 1
+      if expr.getAnOperand() = any(MaxIntOrMaxUint m).getAReference()
+      then
+        any(MaxIntOrMaxUint m | expr.getAnOperand() = m.getAReference())
+            .isBoundFor(b, architectureBitSize, strictnessOffset)
+      else
+        // We want `x <= c - strictnessOffset` to guarantee that `x <= 2^b - 1`,
+        // which is equivalent to saying `c - strictnessOffset <= 2^b - 1`.
+        expr.getAnOperand().getExactValue().toFloat() - strictnessOffset <= 2.pow(b) - 1
     )
   }
 
@@ -316,7 +360,7 @@ class UpperBoundCheckGuard extends DataFlow::RelationalComparisonNode {
  *   _ = uint16(parsed)
  * }
  * ```
- * `parsed < math.MaxInt16` is an `UpperBoundCheckGuard` and `uint16(parsed)`
+ * `parsed <= math.MaxInt16` is an `UpperBoundCheckGuard` and `uint16(parsed)`
  * is an `UpperBoundCheck` that would be a barrier for flow states with bit
  * size greater than 15 and would transform them to a flow state with bit size
  * 15 and the same architecture bit size.

go/ql/src/change-notes/2023-10-27-incorrect-integer-conversion-guards.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The query `go/incorrect-integer-conversion` now correctly recognizes more guards of the form `if val <= x` to protect a conversion `uintX(val)`.

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

@@ -333,8 +333,11 @@ func testBoundsChecking(input string) {
 			_ = uint16(parsed)
 			_ = int16(parsed) // $ hasValueFlow="type conversion"
 		}
-		if parsed < 5 {
-			_ = uint16(parsed)
+		if parsed <= 255 {
+			_ = uint8(parsed)
+		}
+		if parsed <= 256 {
+			_ = uint8(parsed) // $ hasValueFlow="type conversion"
 		}
 		if err == nil && 1 == 1 && parsed < math.MaxInt8 {
 			_ = int8(parsed)
@@ -349,6 +352,15 @@ func testBoundsChecking(input string) {
 	}
 }
 
+func testBoundsChecking2(input string) error {
+	version, err := strconv.ParseUint(input, 10, 0)
+	if err != nil || version > math.MaxInt {
+		return fmt.Errorf("checksum has invalid version: %w", err)
+	}
+	_ = int(version)
+	return nil
+}
+
 func testRightShifted(input string) {
 	{
 		parsed, err := strconv.ParseInt(input, 10, 32)