Merge pull request #7543 from github/rdmarsh2/cpp/hex-format-range-analysis

C++: Use range analysis for maximum lengths of `%x` formats

Author: redsun82

cpp/ql/lib/change-notes/2022-01-14-hex-format-range-analysis.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* `FormatLiteral::getMaxConvertedLength` now uses range analysis to provide a
+  more accurate length for integers formatted with `%x`

cpp/ql/lib/semmle/code/cpp/commons/Printf.qll

@@ -399,6 +399,18 @@ private BufferWriteEstimationReason getEstimationReasonForIntegralExpression(Exp
   else result = TTypeBoundsAnalysis()
 }
 
+/**
+ * Gets the number of hex digits required to represent the integer represented by `f`.
+ *
+ * `f` is assumed to be nonnegative.
+ */
+bindingset[f]
+private int lengthInBase16(float f) {
+  f = 0 and result = 1
+  or
+  result = (f.log2() / 4.0).floor() + 1
+}
+
 /**
  * A class to represent format strings that occur as arguments to invocations of formatting functions.
  */
@@ -1253,23 +1265,42 @@ class FormatLiteral extends Literal {
         or
         this.getConversionChar(n).toLowerCase() = "x" and
         // e.g. "12345678"
-        exists(int sizeBytes, int baseLen |
-          sizeBytes =
-            min(int bytes |
-              bytes = this.getIntegralDisplayType(n).getSize()
+        exists(int baseLen, int typeBasedBound, int valueBasedBound |
+          typeBasedBound =
+            min(int digits |
+              digits = 2 * this.getIntegralDisplayType(n).getSize()
               or
               exists(IntegralType t |
                 t = this.getUse().getConversionArgument(n).getType().getUnderlyingType()
               |
-                t.isUnsigned() and bytes = t.getSize()
+                t.isUnsigned() and
+                digits = 2 * t.getSize()
               )
             ) and
-          baseLen = sizeBytes * 2 and
-          (
-            if this.hasAlternateFlag(n) then len = 2 + baseLen else len = baseLen // "0x"
-          )
-        ) and
-        reason = TTypeBoundsAnalysis()
+          exists(Expr arg, float lower, float upper, float typeLower, float typeUpper |
+            arg = this.getUse().getConversionArgument(n) and
+            lower = lowerBound(arg.getFullyConverted()) and
+            upper = upperBound(arg.getFullyConverted()) and
+            typeLower = exprMinVal(arg.getFullyConverted()) and
+            typeUpper = exprMaxVal(arg.getFullyConverted())
+          |
+            valueBasedBound =
+              lengthInBase16(max(float cand |
+                  // If lower can be negative we use `(unsigned)-1` as the candidate value.
+                  lower < 0 and
+                  cand = 2.pow(any(IntType t | t.isUnsigned()).getSize() * 8)
+                  or
+                  cand = upper
+                )) and
+            (
+              if lower > typeLower or upper < typeUpper
+              then reason = TValueFlowAnalysis()
+              else reason = TTypeBoundsAnalysis()
+            )
+          ) and
+          baseLen = valueBasedBound.minimum(typeBasedBound) and
+          if this.hasAlternateFlag(n) then len = 2 + baseLen else len = baseLen // "0x"
+        )
         or
         this.getConversionChar(n).toLowerCase() = "p" and
         exists(PointerType ptrType, int baseLen |

cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/VeryLikelyOverrunWrite.expected

@@ -14,6 +14,8 @@
 | tests.c:120:3:120:9 | call to sprintf | This 'call to sprintf' operation requires 17 bytes but the destination is only 1 bytes. |
 | tests.c:121:3:121:9 | call to sprintf | This 'call to sprintf' operation requires 17 bytes but the destination is only 16 bytes. |
 | tests.c:136:2:136:8 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 10 bytes. |
+| tests.c:186:3:186:9 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 2 bytes. |
+| tests.c:189:3:189:9 | call to sprintf | This 'call to sprintf' operation requires 3 bytes but the destination is only 2 bytes. |
 | unions.c:26:2:26:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 16 bytes. |
 | unions.c:27:2:27:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 15 bytes. |
 | unions.c:27:2:27:7 | call to strcpy | This 'call to strcpy' operation requires 21 bytes but the destination is only 16 bytes. |

cpp/ql/test/query-tests/Security/CWE/CWE-120/semmle/tests/tests.c

@@ -169,3 +169,27 @@ void testVarSizeStruct()
 
 	snprintf(s->data, 10, "abcdefghijklmnopqrstuvwxyz"); // GOOD
 }
+
+void tesHexBounds(int x) {
+	char buffer2[2];
+	char buffer3[3];
+	char buffer5[5];
+
+	sprintf(buffer2, "%x", 1);  // GOOD
+	sprintf(buffer3, "%x", 16); // GOOD
+	sprintf(buffer5, "%x", (unsigned short)x); // GOOD: bounded by conversion
+	if (x < 16 && x > 0) {
+		sprintf(buffer2, "%x", x); // GOOD: bounded by check
+	}
+
+	if (x < 16) {
+		sprintf(buffer2, "%x", x); // BAD:  negative values
+	}
+	if (x <= 16 && x > 0) {
+		sprintf(buffer2, "%x", x); // BAD: bound too loose
+	}
+
+	if(x < 0x10000 && x > 0) {
+		sprintf(buffer5, "%x", x); // GOOD: bounded by check
+	}
+}