Merge pull request github#12247 from smowton/smowton/fix/integer-conversion-sign

Go integer conversion: check against sink, not source signedness

Author: smowton

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

@@ -56,14 +56,14 @@ private predicate isIncorrectIntegerConversion(int sourceBitSize, int sinkBitSiz
  * integer types, which could cause unexpected values.
  */
 class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
-  boolean sourceIsSigned;
+  boolean sinkIsSigned;
   int sourceBitSize;
   int sinkBitSize;
 
   ConversionWithoutBoundsCheckConfig() {
-    sourceIsSigned in [true, false] and
+    sinkIsSigned in [true, false] and
     isIncorrectIntegerConversion(sourceBitSize, sinkBitSize) and
-    this = "ConversionWithoutBoundsCheckConfig" + sourceBitSize + sourceIsSigned + sinkBitSize
+    this = "ConversionWithoutBoundsCheckConfig" + sourceBitSize + sinkIsSigned + sinkBitSize
   }
 
   /** Gets the bit size of the source. */
@@ -75,11 +75,6 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
     |
       c.getTarget() = ip and source = c.getResult(0)
     |
-      (
-        if ip.getResultType(0) instanceof SignedIntegerType
-        then sourceIsSigned = true
-        else sourceIsSigned = false
-      ) and
       (
         apparentBitSize = ip.getTargetBitSize()
         or
@@ -112,10 +107,13 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
   predicate isSinkWithBitSize(DataFlow::TypeCastNode sink, int bitSize) {
     sink.asExpr() instanceof ConversionExpr and
     exists(IntegerType integerType | sink.getResultType().getUnderlyingType() = integerType |
-      bitSize = integerType.getSize()
-      or
-      not exists(integerType.getSize()) and
-      bitSize = getIntTypeBitSize(sink.getFile())
+      (
+        bitSize = integerType.getSize()
+        or
+        not exists(integerType.getSize()) and
+        bitSize = getIntTypeBitSize(sink.getFile())
+      ) and
+      if integerType instanceof SignedIntegerType then sinkIsSigned = true else sinkIsSigned = false
     ) and
     not exists(ShrExpr shrExpr |
       shrExpr.getLeftOperand().getGlobalValueNumber() =
@@ -134,7 +132,7 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
       if sinkBitSize != 0 then bitSize = sinkBitSize else bitSize = 32
     |
       node = DataFlow::BarrierGuard<upperBoundCheckGuard/3>::getABarrierNodeForGuard(g) and
-      g.isBoundFor(bitSize, sourceIsSigned)
+      g.isBoundFor(bitSize, sinkIsSigned)
     )
   }
 

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

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The query `go/incorrect-integer-conversion` now correctly recognises guards of the form `if val <= x` to protect a conversion `uintX(val)` when `x` is in the range `(math.MaxIntX, math.MaxUintX]`.

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

@@ -264,6 +264,9 @@ func testBoundsChecking(input string) {
 				_ = int16(parsed)
 			}
 		}
+		if parsed <= math.MaxUint16 {
+			_ = uint16(parsed)
+		}
 	}
 	{
 		parsed, err := strconv.ParseUint(input, 10, 32)