Merge pull request #524 from geoffw0/cpp-299

CPP: Add (partial) dataflow to OverflowStatic.ql

Author: jbj

change-notes/1.19/analysis-cpp.md

@@ -25,6 +25,7 @@
 | Resource not released in destructor | Fewer false positive results | Placement new is now excluded from the query. Also fixed an issue where false positives could occur if the destructor body was not in the snapshot. |
 | Missing return statement (`cpp/missing-return`) | Visible by default | The precision of this query has been increased from 'medium' to 'high', which makes it visible by default in LGTM. It was 'medium' in release 1.17 and 1.18 because it had false positives due to an extractor bug that was fixed in 1.18. |
 | Missing return statement | Fewer false positive results | The query is now produces correct results when a function returns a template-dependent type, or makes a non-returning call to another function. |
+| Static array access may cause overflow | More correct results | Data flow to the size argument of a buffer operation is now checked in this query. |
 | Call to memory access function may overflow buffer | More correct results | Array indexing with a negative index is now detected by this query. |
 | Self comparison | Fewer false positive results | Code inside macro invocations is now excluded from the query. |
 | Suspicious call to memset | Fewer false positive results | Types involving decltype are now correctly compared. |

cpp/ql/src/Critical/OverflowStatic.ql

@@ -82,22 +82,31 @@ class CallWithBufferSize extends FunctionCall
   Expr buffer() {
     exists(int i |
       bufferAndSizeFunction(this.getTarget(), i, _) and
-      result = this.getArgument(i))
+      result = this.getArgument(i)
+    )
   }
-  Expr statedSize() {
+  Expr statedSizeExpr() {
     exists(int i |
       bufferAndSizeFunction(this.getTarget(), _, i) and
-      result = this.getArgument(i))
+      result = this.getArgument(i)
+    )
+  }
+  int statedSizeValue() {
+    exists(Expr statedSizeSrc |
+      DataFlow::localFlow(DataFlow::exprNode(statedSizeSrc), DataFlow::exprNode(statedSizeExpr())) and
+      result = statedSizeSrc.getValue().toInt()
+    )
   }
 }
 
 predicate wrongBufferSize(Expr error, string msg) {
-  exists(CallWithBufferSize call, int bufsize, Variable buf |
+  exists(CallWithBufferSize call, int bufsize, Variable buf, int statedSize |
     staticBuffer(call.buffer(), buf, bufsize) and
-    call.statedSize().getValue().toInt() > bufsize and
-    error = call.statedSize() and
+    statedSize = min(call.statedSizeValue()) and
+    statedSize > bufsize and
+    error = call.statedSizeExpr() and
     msg = "Potential buffer-overflow: '" + buf.getName() +
-    "' has size " + bufsize.toString() + " not " + call.statedSize().getValue() + ".")
+    "' has size " + bufsize.toString() + " not " + statedSize + ".")
 }
 
 predicate outOfBounds(BufferAccess bufaccess, string msg)

cpp/ql/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected

@@ -6,3 +6,4 @@
 | test.cpp:20:3:20:12 | access to array | Potential buffer-overflow: counter 'i' <= 3 but 'buffer2' has 3 elements. |
 | test.cpp:24:27:24:27 | 4 | Potential buffer-overflow: 'buffer1' has size 3 not 4. |
 | test.cpp:26:27:26:27 | 4 | Potential buffer-overflow: 'buffer2' has size 3 not 4. |
+| test.cpp:40:22:40:27 | amount | Potential buffer-overflow: 'buffer' has size 100 not 101. |

cpp/ql/test/query-tests/Critical/OverflowStatic/test.cpp

@@ -25,3 +25,24 @@ void f1(void)
 	memcpy(buffer2, buffer1, 3); // GOOD
 	memcpy(buffer2, buffer1, 4); // BAD
 }
+
+void f2(char *src)
+{
+	char buffer[100];
+	char *ptr;
+	int amount;
+
+	amount = 100;
+	memcpy(buffer, src, amount); // GOOD
+	amount = amount + 1;
+	memcpy(buffer, src, amount); // BAD [NOT DETECTED]
+	amount = 101;
+	memcpy(buffer, src, amount); // BAD
+
+	ptr = buffer;
+	memcpy(ptr, src, 101); // BAD [NOT DETECTED]
+	ptr = &(buffer[0]);
+	memcpy(ptr, src, 101); // BAD [NOT DETECTED]
+	ptr = &(buffer[1]);
+	memcpy(ptr, src, 100); // BAD [NOT DETECTED]
+}