Merge pull request #5923 from MathiasVP/range-analysis-in-overflow-static

C++: Add range analysis to `cpp/static-buffer-overflow`

cpp/change-notes/2021-05-18-static-buffer-overflow.md

@@ -0,0 +1,2 @@
+lgtm
+* The "Static buffer overflow" query (cpp/static-buffer-overflow) has been improved to produce fewer false positives.

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,17 +97,22 @@ 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(min(Expr statedSizeSrc |
+              DataFlow::localExprFlow(statedSizeSrc, statedSizeExpr())
+            |
+              statedSizeSrc.getValue().toInt()
+            ))
   }
 }
 
 predicate wrongBufferSize(Expr error, string msg) {
   exists(CallWithBufferSize call, int bufsize, Variable buf, int statedSize |
     staticBuffer(call.buffer(), buf, bufsize) and
-    statedSize = min(call.statedSizeValue()) and
+    statedSize = call.statedSizeValue() and
     statedSize > bufsize and
     error = call.statedSizeExpr() and
     msg =

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

@@ -586,6 +586,23 @@ void test21(bool cond)
 	if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[1]
 }
 
+void test22(bool b, const char* source) {
+	char buffer[16];
+	int k;
+	for (k = 0; k <= 100; k++) {
+		if(k < 16) {
+			buffer[k] = 'x'; // GOOD
+		}
+	}
+
+	char dest[128];
+	int n = b ? 1024 : 132;
+	if (n >= 128) {
+    return;
+  }
+  memcpy(dest, source, n); // GOOD
+}
+
 int main(int argc, char *argv[])
 {
 	long long arr17[19];
@@ -609,6 +626,7 @@ int main(int argc, char *argv[])
 	test19(argc == 0);
 	test20();
 	test21(argc == 0);
+	test22(argc == 0, argv[0]);
 
 	return 0;
 }