Merge pull request #7386 from github/redsun82/cpp-overrunning-write-precision-split

C++: split `cpp/overrunning-write` into two

Author: MathiasVP

cpp/config/suites/security/cwe-120

@@ -5,9 +5,11 @@
   @name Badly bounded write (CWE-120)
 + semmlecode-cpp-queries/Security/CWE/CWE-120/OverrunWrite.ql: /CWE/CWE-120
   @name Potentially overrunning write (CWE-120)
++ semmlecode-cpp-queries/Security/CWE/CWE-120/VeryLikelyOverrunWrite.ql: /CWE/CWE-120
+  @name Likely overrunning write
 + semmlecode-cpp-queries/Security/CWE/CWE-120/OverrunWriteFloat.ql: /CWE/CWE-120
   @name Potentially overrunning write with float to string conversion (CWE-120)
 + semmlecode-cpp-queries/Best Practices/Likely Errors/OffsetUseBeforeRangeCheck.ql: /CWE/CWE-120
   @name Array offset used before range check (CWE-120)
 + semmlecode-cpp-queries/Likely Bugs/Memory Management/UnsafeUseOfStrcat.ql: /CWE/CWE-120
-    @name Potentially unsafe use of strcat (CWE-120)
+  @name Potentially unsafe use of strcat (CWE-120)

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

@@ -10,10 +10,22 @@ private import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 private import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
 
 private newtype TBufferWriteEstimationReason =
-  TNoSpecifiedEstimateReason() or
+  TUnspecifiedEstimateReason() or
   TTypeBoundsAnalysis() or
+  TWidenedValueFlowAnalysis() or
   TValueFlowAnalysis()
 
+private predicate gradeToReason(int grade, TBufferWriteEstimationReason reason) {
+  // when combining reasons, lower grade takes precedence
+  grade = 0 and reason = TUnspecifiedEstimateReason()
+  or
+  grade = 1 and reason = TTypeBoundsAnalysis()
+  or
+  grade = 2 and reason = TWidenedValueFlowAnalysis()
+  or
+  grade = 3 and reason = TValueFlowAnalysis()
+}
+
 /**
  * A reason for a specific buffer write size estimate.
  */
@@ -32,24 +44,24 @@ abstract class BufferWriteEstimationReason extends TBufferWriteEstimationReason
    * Combine estimate reasons. Used to give a reason for the size of a format string
    * conversion given reasons coming from its individual specifiers.
    */
-  abstract BufferWriteEstimationReason combineWith(BufferWriteEstimationReason other);
+  BufferWriteEstimationReason combineWith(BufferWriteEstimationReason other) {
+    exists(int grade, int otherGrade |
+      gradeToReason(grade, this) and gradeToReason(otherGrade, other)
+    |
+      if otherGrade < grade then result = other else result = this
+    )
+  }
 }
 
 /**
  * No particular reason given. This is currently used for backward compatibility so that
  * classes derived from BufferWrite and overriding `getMaxData/0` still work with the
  * queries as intended.
  */
-class NoSpecifiedEstimateReason extends BufferWriteEstimationReason, TNoSpecifiedEstimateReason {
-  override string toString() { result = "NoSpecifiedEstimateReason" }
+class UnspecifiedEstimateReason extends BufferWriteEstimationReason, TUnspecifiedEstimateReason {
+  override string toString() { result = "UnspecifiedEstimateReason" }
 
   override string getDescription() { result = "no reason specified" }
-
-  override BufferWriteEstimationReason combineWith(BufferWriteEstimationReason other) {
-    // this reason should not be used in format specifiers, so it should not be combined
-    // with other reasons
-    none()
-  }
 }
 
 /**
@@ -60,9 +72,24 @@ class TypeBoundsAnalysis extends BufferWriteEstimationReason, TTypeBoundsAnalysi
   override string toString() { result = "TypeBoundsAnalysis" }
 
   override string getDescription() { result = "based on type bounds" }
+}
 
-  override BufferWriteEstimationReason combineWith(BufferWriteEstimationReason other) {
-    other != TNoSpecifiedEstimateReason() and result = TTypeBoundsAnalysis()
+/**
+ * The estimation comes from non trivial bounds found via actual flow analysis,
+ * but a widening aproximation might have been used for variables in loops.
+ * For example
+ * ```
+ * for (int i = 0; i < 10; ++i) {
+ *    int j = i + i;
+ *    //...  <- estimation done here based on j
+ * }
+ * ```
+ */
+class WidenedValueFlowAnalysis extends BufferWriteEstimationReason, TWidenedValueFlowAnalysis {
+  override string toString() { result = "WidenedValueFlowAnalysis" }
+
+  override string getDescription() {
+    result = "based on flow analysis of value bounds with a widening approximation"
   }
 }
 
@@ -80,10 +107,6 @@ class ValueFlowAnalysis extends BufferWriteEstimationReason, TValueFlowAnalysis
   override string toString() { result = "ValueFlowAnalysis" }
 
   override string getDescription() { result = "based on flow analysis of value bounds" }
-
-  override BufferWriteEstimationReason combineWith(BufferWriteEstimationReason other) {
-    other != TNoSpecifiedEstimateReason() and result = other
-  }
 }
 
 class PrintfFormatAttribute extends FormatAttribute {
@@ -359,6 +382,23 @@ private int lengthInBase10(float f) {
   result = f.log10().floor() + 1
 }
 
+bindingset[expr]
+private BufferWriteEstimationReason getEstimationReasonForIntegralExpression(Expr expr) {
+  // we consider the range analysis non trivial if it
+  // * constrained non-trivially both sides of a signed value, or
+  // * constrained non-trivially the positive side of an unsigned value
+  // expr should already be given as getFullyConverted
+  if
+    upperBound(expr) < exprMaxVal(expr) and
+    (exprMinVal(expr) >= 0 or lowerBound(expr) > exprMinVal(expr))
+  then
+    // next we check whether the estimate may have been widened
+    if upperBoundMayBeWidened(expr)
+    then result = TWidenedValueFlowAnalysis()
+    else result = TValueFlowAnalysis()
+  else result = TTypeBoundsAnalysis()
+}
+
 /**
  * A class to represent format strings that occur as arguments to invocations of formatting functions.
  */
@@ -1157,12 +1197,10 @@ class FormatLiteral extends Literal {
             1 + lengthInBase10(2.pow(this.getIntegralDisplayType(n).getSize() * 8 - 1)) and
           // The second case uses range analysis to deduce a length that's shorter than the length
           // of the number -2^31.
-          exists(Expr arg, float lower, float upper, float typeLower, float typeUpper |
+          exists(Expr arg, float lower, float upper |
             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())
+            upper = upperBound(arg.getFullyConverted())
           |
             valueBasedBound =
               max(int cand |
@@ -1179,11 +1217,9 @@ class FormatLiteral extends Literal {
                   else cand = lengthInBase10(upper)
                 )
               ) and
-            (
-              if lower > typeLower or upper < typeUpper
-              then reason = TValueFlowAnalysis()
-              else reason = TTypeBoundsAnalysis()
-            )
+            // we don't want to call this on `arg.getFullyConverted()` as we want
+            // to detect non-trivial range analysis without taking into account up-casting
+            reason = getEstimationReasonForIntegralExpression(arg)
           ) and
           len = valueBasedBound.minimum(typeBasedBound)
         )
@@ -1195,12 +1231,10 @@ class FormatLiteral extends Literal {
           typeBasedBound = lengthInBase10(2.pow(this.getIntegralDisplayType(n).getSize() * 8) - 1) and
           // The second case uses range analysis to deduce a length that's shorter than
           // the length of the number 2^31 - 1.
-          exists(Expr arg, float lower, float upper, float typeLower, float typeUpper |
+          exists(Expr arg, float lower, float upper |
             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())
+            upper = upperBound(arg.getFullyConverted())
           |
             valueBasedBound =
               lengthInBase10(max(float cand |
@@ -1210,11 +1244,9 @@ class FormatLiteral extends Literal {
                   or
                   cand = upper
                 )) and
-            (
-              if lower > typeLower or upper < typeUpper
-              then reason = TValueFlowAnalysis()
-              else reason = TTypeBoundsAnalysis()
-            )
+            // we don't want to call this on `arg.getFullyConverted()` as we want
+            // to detect non-trivial range analysis without taking into account up-casting
+            reason = getEstimationReasonForIntegralExpression(arg)
           ) and
           len = valueBasedBound.minimum(typeBasedBound)
         )

cpp/ql/lib/semmle/code/cpp/security/BufferWrite.qll

@@ -76,7 +76,7 @@ abstract class BufferWrite extends Expr {
    * can be found), specifying the reason for the estimation.
    */
   int getMaxData(BufferWriteEstimationReason reason) {
-    reason instanceof NoSpecifiedEstimateReason and result = getMaxData()
+    reason instanceof UnspecifiedEstimateReason and result = getMaxData()
   }
 
   /**

cpp/ql/src/Security/CWE/CWE-120/BadlyBoundedWrite.qhelp

@@ -18,8 +18,5 @@
 <p>To fix the problem, either the second argument to <code>snprintf</code> should be changed to 80, or the buffer extended to 256 characters.  A further improvement is to use a preprocessor define so that the size is only specified in one place, potentially preventing future recurrence of this issue.</p>
 
 </example>
-<references>
-
-
-</references>
+<include src="OverrunWriteReferences.inc.qhelp" />
 </qhelp>

cpp/ql/src/Security/CWE/CWE-120/OverrunWrite.c

@@ -1,9 +1,9 @@
-void sayHello()
+void sayHello(uint32_t userId)
 {
-	char buffer[10];
+	char buffer[18];
 
-	// BAD: this message overflows the buffer
-	strcpy(buffer, "Hello, world!");
+	// BAD: this message overflows the buffer if userId >= 10000
+	sprintf(buffer, "Hello, user %d!", userId);
 
 	MessageBox(hWnd, buffer, "New Message", MB_OK);
 }

cpp/ql/src/Security/CWE/CWE-120/OverrunWrite.qhelp

@@ -6,30 +6,20 @@
 <p>The program performs a buffer copy or write operation with no upper limit on the size of the copy, and it appears that certain inputs will cause a buffer overflow to occur in this case.  In addition to causing program instability, techniques exist which may allow an attacker to use this vulnerability to execute arbitrary code.</p>
 
 </overview>
-<recommendation>
-<p>Always control the length of buffer copy and buffer write operations.  <code>strncpy</code> should be used over <code>strcpy</code>, <code>snprintf</code> over <code>sprintf</code>, and in other cases 'n-variant' functions should be preferred.</p>
-
-</recommendation>
+<include src="OverrunWriteRecommendation.inc.qhelp" />
 <example>
 <sample src="OverrunWrite.c" />
 
-<p>In this example, the call to <code>strcpy</code> copies a message of 14 characters (including the terminating null) into a buffer with space for just 10 characters.  As such, the last four characters overflow the buffer resulting in undefined behavior.</p>
+<p>In this example, the call to <code>sprintf</code> writes a message of 14 characters (including the terminating null) plus the length of the string conversion of `userId` into a buffer with space for just 18 characters.  As such, if `userId` is greater or equal to `10000`, the last characters overflow the buffer resulting in undefined behavior.</p>
 
-<p>To fix this issue three changes should be made:</p>
+<p>To fix this issue these changes should be made:</p>
 <ul>
-  <li>Control the size of the buffer using a preprocessor define.</li>
-  <li>Replace the call to <code>strcpy</code> with <code>strncpy</code>, specifying the define as the maximum length to copy.  This will prevent the buffer overflow.</li>
-  <li>Consider increasing the buffer size, say to 20 characters, so that the message is displayed correctly.</li>
+  <li>Control the size of the buffer by declaring it with a compile time constant.</li>
+  <li>Preferably, replace the call to <code>sprintf</code> with <code>snprintf</code>, using the defined constant size of the buffer or `sizeof(buffer)` as maximum length to write. This will prevent the buffer overflow.</li>
+  <li>Optionally, if `userId` is expected to be less than `10000`, then return or throw an error if `userId` is out of bounds.</li>
+  <li>Otherwise, consider increasing the buffer size to at least 25 characters, so that the message is displayed correctly regardless of the value of `userId`.</li>
 </ul>
 
 </example>
-<references>
-
-<li>CERT C Coding Standard: <a href="https://www.securecoding.cert.org/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator">STR31-C. Guarantee that storage for strings has sufficient space for character data and the null terminator</a>.</li>
-
-
-<!--  LocalWords:  preprocessor CWE STR
- -->
-
-</references>
+<include src="OverrunWriteReferences.inc.qhelp" />
 </qhelp>

cpp/ql/src/Security/CWE/CWE-120/OverrunWrite.ql

@@ -21,12 +21,14 @@ import semmle.code.cpp.commons.Alloc
  * See CWE-120/UnboundedWrite.ql for a summary of CWE-120 alert cases.
  */
 
-from BufferWrite bw, Expr dest, int destSize, int estimated
+from BufferWrite bw, Expr dest, int destSize, int estimated, BufferWriteEstimationReason reason
 where
   not bw.hasExplicitLimit() and // has no explicit size limit
   dest = bw.getDest() and
   destSize = getBufferSize(dest, _) and
-  estimated = bw.getMaxDataLimited(_) and
+  estimated = bw.getMaxDataLimited(reason) and
+  // we exclude ValueFlowAnalysis as it is reported in cpp/very-likely-overruning-write
+  not reason instanceof ValueFlowAnalysis and
   // we can deduce that too much data may be copied (even without
   // long '%f' conversions)
   estimated > destSize

cpp/ql/src/Security/CWE/CWE-120/OverrunWriteFloat.qhelp

@@ -6,30 +6,19 @@
 <p>The program performs a buffer copy or write operation that includes one or more float to string conversions (i.e. the %f format specifier), which may overflow the destination buffer if extreme inputs are given.  In addition to causing program instability, techniques exist which may allow an attacker to use this vulnerability to execute arbitrary code.</p>
 
 </overview>
-<recommendation>
-<p>Always control the length of buffer copy and buffer write operations.  <code>strncpy</code> should be used over <code>strcpy</code>, <code>snprintf</code> over <code>sprintf</code>, and in other cases 'n-variant' functions should be preferred.</p>
-
-</recommendation>
+<include src="OverrunWriteRecommendation.inc.qhelp" />
 <example>
 <sample src="OverrunWriteFloat.c" />
 
-<p>In this example, the call to <code>sprintf</code> contains a %f format specifier.  Though a 256 character buffer has been allowed, it is not sufficient for the most extreme floating point inputs.  For example the representation of double value 1e304 (that is 1 with 304 zeroes after it) will overflow a buffer of this length.</p>
+<p>In this example, the call to <code>sprintf</code> contains a <code>%f</code> format specifier.  Though a 256 character buffer has been allowed, it is not sufficient for the most extreme floating point inputs.  For example the representation of double value 1e304 (that is 1 with 304 zeroes after it) will overflow a buffer of this length.</p>
 
 <p>To fix this issue three changes should be made:</p>
 <ul>
   <li>Control the size of the buffer using a preprocessor define.</li>
   <li>Replace the call to <code>sprintf</code> with <code>snprintf</code>, specifying the define as the maximum length to copy.  This will prevent the buffer overflow.</li>
-  <li>Consider using the %g format specifier instead of %f.</li>
+  <li>Consider using the <code>%g</code> format specifier instead of <code>%f</code>.</li>
 </ul>
 
 </example>
-<references>
-
-<li>CERT C Coding
-Standard: <a href="https://www.securecoding.cert.org/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator">STR31-C. Guarantee
-that storage for strings has sufficient space for character data and
-the null terminator</a>.</li>
-
-
-</references>
+<include src="OverrunWriteReferences.inc.qhelp" />
 </qhelp>

cpp/ql/src/Security/CWE/CWE-120/OverrunWriteRecommendation.inc.qhelp

@@ -0,0 +1,8 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<recommendation>
+<p>Always control the length of buffer copy and buffer write operations. <code>strncpy</code> should be used over <code>strcpy</code>, <code>snprintf</code> over <code>sprintf</code>, and in other cases 'n-variant' functions should be preferred.</p>
+</recommendation>
+</qhelp>

cpp/ql/src/Security/CWE/CWE-120/OverrunWriteReferences.inc.qhelp

@@ -0,0 +1,14 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<references>
+
+<li>CERT C Coding Standard: <a href="https://www.securecoding.cert.org/confluence/display/c/STR31-C.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator">STR31-C. Guarantee that storage for strings has sufficient space for character data and the null terminator</a>.</li>
+<li>CERT C++ Coding Standard: <a href="https://www.securecoding.cert.org/confluence/display/cplusplus/STR50-CPP.+Guarantee+that+storage+for+strings+has+sufficient+space+for+character+data+and+the+null+terminator">STR50-CPP. Guarantee that storage for strings has sufficient space for character data and the null terminator</a>.</li>
+
+<!--  LocalWords:  preprocessor CWE STR
+ -->
+
+</references>
+</qhelp>