C++: Add range analysis to fix FPs.

Author: MathiasVP

cpp/ql/src/Critical/OverflowStatic.ql

@@ -14,6 +14,7 @@
 
 import cpp
 import semmle.code.cpp.commons.Buffer
+import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 import LoopBounds
 
 private predicate staticBufferBase(VariableAccess access, Variable v) {
@@ -51,6 +52,8 @@ predicate overflowOffsetInLoop(BufferAccess bufaccess, string msg) {
     loop.getStmt().getAChild*() = bufaccess.getEnclosingStmt() and
     loop.limit() >= bufaccess.bufferSize() and
     loop.counter().getAnAccess() = bufaccess.getArrayOffset() and
+    // Ensure that we don't have an upper bound on the array index that's less than the buffer size.
+    not upperBound(bufaccess.getArrayOffset().getFullyConverted()) < bufaccess.bufferSize() and
     msg =
       "Potential buffer-overflow: counter '" + loop.counter().toString() + "' <= " +
         loop.limit().toString() + " but '" + bufaccess.buffer().getName() + "' has " +
@@ -94,10 +97,15 @@ class CallWithBufferSize extends FunctionCall {
   }
 
   int statedSizeValue() {
-    exists(Expr statedSizeSrc |
-      DataFlow::localExprFlow(statedSizeSrc, statedSizeExpr()) and
-      result = statedSizeSrc.getValue().toInt()
-    )
+    // `upperBound(e)` defaults to `exprMaxVal(e)` when `e` isn't analyzable. So to get a meaningful
+    // result in this case we pick the minimum value obtainable from dataflow and range analysis.
+    result =
+      upperBound(statedSizeExpr())
+          .minimum(any(Expr statedSizeSrc |
+              DataFlow::localExprFlow(statedSizeSrc, statedSizeExpr())
+            |
+              statedSizeSrc.getValue().toInt()
+            ))
   }
 }
 

cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/OverflowStatic.expected

@@ -5,8 +5,6 @@
 | tests.cpp:245:42:245:42 | 6 | Potential buffer-overflow: 'global_array_5' has size 5 not 6. |
 | tests.cpp:349:2:349:14 | access to array | Potential buffer-overflow: 'charArray' has size 10 but 'charArray[10]' is accessed here. |
 | tests.cpp:350:17:350:29 | access to array | Potential buffer-overflow: 'charArray' has size 10 but 'charArray[10]' is accessed here. |
-| tests.cpp:594:4:594:12 | access to array | Potential buffer-overflow: counter 'k' <= 100 but 'buffer' has 16 elements. |
-| tests.cpp:603:24:603:24 | n | Potential buffer-overflow: 'dest' has size 128 not 132. |
 | var_size_struct.cpp:54:5:54:14 | access to array | Potential buffer-overflow: 'str' has size 1 but 'str[1]' is accessed here. |
 | var_size_struct.cpp:55:5:55:14 | access to array | Potential buffer-overflow: 'str' has size 1 but 'str[1]' is accessed here. |
 | var_size_struct.cpp:103:39:103:41 | 129 | Potential buffer-overflow: 'str' has size 128 not 129. |

cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/tests.cpp

@@ -591,7 +591,7 @@ void test22(bool b, const char* source) {
 	int k;
 	for (k = 0; k <= 100; k++) {
 		if(k < 16) {
-			buffer[k] = 'x'; // GOOD [FALSE POSITIVE]
+			buffer[k] = 'x'; // GOOD
 		}
 	}
 
@@ -600,7 +600,7 @@ void test22(bool b, const char* source) {
 	if (n >= 128) {
     return;
   }
-  memcpy(dest, source, n); // GOOD [FALSE POSITIVE]
+  memcpy(dest, source, n); // GOOD
 }
 
 int main(int argc, char *argv[])