C++: Fix pointer/pointee conflation and handle flow through '++' when tracking indirect flow.

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

@@ -28,11 +28,13 @@ predicate memberMayBeVarSize(Class c, MemberVariable v) {
  * Gets the expression associated with `n`. Unlike `n.asExpr()` this also gets the
  * expression underlying an indirect dataflow node.
  */
-private Expr getExpr(DataFlow::Node n) {
-  result = [n.asExpr(), n.asIndirectArgument(), n.asIndirectArgument()]
+private Expr getExpr(DataFlow::Node n, boolean isIndirect) {
+  result = n.asExpr() and isIndirect = false
+  or
+  result = n.asIndirectExpr() and isIndirect = true
 }
 
-private DataFlow::Node exprNode(Expr e) { e = getExpr(result) }
+private DataFlow::Node exprNode(Expr e, boolean isIndirect) { e = getExpr(result, isIndirect) }
 
 /**
  * Holds if `bufferExpr` is an allocation-like expression.
@@ -83,6 +85,80 @@ private int isSource(Expr bufferExpr, Element why) {
   )
 }
 
+/** Holds if the value of `n2 + delta` may be equal to the value of `n1`. */
+private predicate localFlowIncrStep(DataFlow::Node n1, DataFlow::Node n2, int delta) {
+  DataFlow::localFlowStep(n1, n2) and
+  (
+    exists(IncrementOperation incr |
+      n1.asIndirectExpr() = incr.getOperand() and
+      delta = -1
+    )
+    or
+    exists(DecrementOperation decr |
+      n1.asIndirectExpr() = decr.getOperand() and
+      delta = 1
+    )
+    or
+    exists(AddExpr add, Expr e1, Expr e2 |
+      add.hasOperands(e1, e2) and
+      n1.asIndirectExpr() = e1 and
+      delta = -e2.getValue().toInt()
+    )
+    or
+    exists(SubExpr add, Expr e1, Expr e2 |
+      add.hasOperands(e1, e2) and
+      n1.asIndirectExpr() = e1 and
+      delta = e2.getValue().toInt()
+    )
+  )
+}
+
+/**
+ * Holds if `n1` may flow to `n2` without passing through any back-edges.
+ *
+ * Back-edges are excluded to prevent infinite loops on examples like:
+ * ```
+ * while(true) { ++n; }
+ * ```
+ * which, when used in `localFlowStepRec`, would create infinite loop that continuously
+ * increments the `delta` parameter.
+ */
+private predicate localFlowNotIncrStep(DataFlow::Node n1, DataFlow::Node n2) {
+  not localFlowIncrStep(n1, n2, _) and
+  DataFlow::localFlowStep(n1, n2) and
+  not n1 = n2.(DataFlow::SsaPhiNode).getAnInput(true)
+}
+
+private predicate localFlowToExprStep(DataFlow::Node n1, DataFlow::Node n2) {
+  not exists([n1.asExpr(), n1.asIndirectExpr()]) and
+  localFlowNotIncrStep(n1, n2)
+}
+
+/** Holds if `mid2 + delta` may be equal to `n1`. */
+private predicate localFlowStepRec0(DataFlow::Node n1, DataFlow::Node mid2, int delta) {
+  exists(DataFlow::Node mid1, int d1, int d2 |
+    // Or we take a number of steps that adds `d1` to the pointer
+    localFlowStepRec(n1, mid1, d1) and
+    // followed by a step that adds `d2` to the pointer
+    localFlowIncrStep(mid1, mid2, d2) and
+    delta = d1 + d2
+  )
+}
+
+/** Holds if `n2 + delta` may be equal to `n1`. */
+private predicate localFlowStepRec(DataFlow::Node n1, DataFlow::Node n2, int delta) {
+  // Either we take one or more steps that doesn't modify the size of the buffer
+  localFlowNotIncrStep+(n1, n2) and
+  delta = 0
+  or
+  exists(DataFlow::Node mid2 |
+    // Or we step from `n1` to `mid2 + delta`
+    localFlowStepRec0(n1, mid2, delta) and
+    // and finally to the next `ExprNode`.
+    localFlowToExprStep*(mid2, n2)
+  )
+}
+
 /**
  * Holds if `e2` is an expression that is derived from `e1` such that if `e1[n]` is a
  * well-defined expression for some number `n`, then `e2[n + delta]` is also a well-defined
@@ -105,8 +181,9 @@ private predicate step(Expr e1, Expr e2, int delta) {
     delta = bufferSize - parentClass.getSize()
   )
   or
-  DataFlow::localFlowStep+(exprNode(e1), exprNode(e2)) and
-  delta = 0
+  exists(boolean isIndirect |
+    localFlowStepRec(exprNode(e1, isIndirect), exprNode(e2, isIndirect), delta)
+  )
 }
 
 /**

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

@@ -38,14 +38,19 @@
 | tests.cpp:376:3:376:13 | access to array | This array indexing operation accesses byte offset 101 but the $@ is only 101 bytes. | tests.cpp:369:47:369:52 | call to malloc | array |
 | tests.cpp:446:3:446:24 | access to array | This array indexing operation accesses a negative index -3 on the $@. | tests.cpp:444:7:444:14 | intArray | array |
 | tests.cpp:454:3:454:11 | access to array | This array indexing operation accesses a negative index -21 on the $@. | tests.cpp:450:7:450:11 | multi | array |
+| tests.cpp:455:3:455:14 | access to array | This array indexing operation accesses a negative index -5 on the $@. | tests.cpp:450:7:450:11 | multi | array |
 | tests.cpp:456:3:456:11 | access to array | This array indexing operation accesses a negative index -21 on the $@. | tests.cpp:450:7:450:11 | multi | array |
+| tests.cpp:456:3:456:15 | access to array | This array indexing operation accesses a negative index -5 on the $@. | tests.cpp:450:7:450:11 | multi | array |
+| tests.cpp:457:3:457:14 | access to array | This array indexing operation accesses a negative index -5 on the $@. | tests.cpp:450:7:450:11 | multi | array |
 | tests.cpp:459:3:459:11 | access to array | This array indexing operation accesses byte offset 639 but the $@ is only 400 bytes. | tests.cpp:450:7:450:11 | multi | array |
 | tests.cpp:461:3:461:11 | access to array | This array indexing operation accesses byte offset 639 but the $@ is only 400 bytes. | tests.cpp:450:7:450:11 | multi | array |
 | tests.cpp:476:2:476:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
 | tests.cpp:477:2:477:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
+| tests.cpp:479:2:479:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
 | tests.cpp:481:2:481:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
 | tests.cpp:485:2:485:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:469:7:469:12 | buffer | array |
 | tests.cpp:487:2:487:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:473:21:473:26 | call to malloc | array |
+| tests.cpp:489:2:489:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:473:21:473:26 | call to malloc | array |
 | tests.cpp:491:2:491:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:474:21:474:26 | call to malloc | array |
 | tests.cpp:495:2:495:7 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:474:21:474:26 | call to malloc | array |
 | tests.cpp:519:3:519:8 | call to memset | This 'memset' operation accesses 20 bytes but the $@ is only 10 bytes. | tests.cpp:502:15:502:20 | call to malloc | destination buffer |
@@ -54,6 +59,7 @@
 | tests.cpp:541:6:541:10 | call to fread | This 'fread' operation may access 101 bytes but the $@ is only 100 bytes. | tests.cpp:532:7:532:16 | charBuffer | destination buffer |
 | tests.cpp:546:6:546:10 | call to fread | This 'fread' operation may access 400 bytes but the $@ is only 100 bytes. | tests.cpp:532:7:532:16 | charBuffer | destination buffer |
 | tests.cpp:569:6:569:15 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
+| tests.cpp:575:7:575:13 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
 | tests.cpp:577:7:577:13 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
 | tests.cpp:579:6:579:12 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
 | tests.cpp:586:6:586:12 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |

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

@@ -454,7 +454,7 @@ void test17(long long *longArray)
 		multi[-5][5] = 0; // BAD: underrun write [INCORRECT MESSAGE]
 		multi[5][-5] = 0; // DUBIOUS: underrun write (this one is still within the bounds of the whole array)
 		multi[-5][-5] = 0; // BAD: underrun write [INCORRECT MESSAGE]
-		multi[0][-5] = 0; // BAD: underrun write [NOT DETECTED]
+		multi[0][-5] = 0; // BAD: underrun write
 
 		multi[15][5] = 0; // BAD: overrun write
 		multi[5][15] = 0; // DUBIOUS: overrun write (this one is still within the bounds of the whole array)
@@ -476,7 +476,7 @@ void test18()
 	p1[-1] = 0; // BAD: underrun write
 	p2[-1] = 0; // BAD: underrun write
 	p2++;
-	p2[-1] = 0; // GOOD
+	p2[-1] = 0; // GOOD [FALSE POSITIVE]
 
 	p3[-1] = 0; // BAD
 	while (*p3 != 0) {
@@ -486,7 +486,7 @@ void test18()
 
 	p4[-1] = 0; // BAD: underrun write
 	p4++;
-	p4[-1] = 0; // GOOD
+	p4[-1] = 0; // GOOD [FALSE POSITIVE]
 
 	p5[-1] = 0; // BAD
 	while (*p5 != 0) {
@@ -572,7 +572,7 @@ void test21(bool cond)
 	if (cond)
 	{
 		ptr++;
-		if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[0]
+		if (ptr[-1] == 0) { return; } // GOOD: accesses buffer[0] [FALSE POSITIVE]
 	} else {
 		if (ptr[-1] == 0) { return; } // BAD: accesses buffer[-1]
 	}

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

@@ -1,4 +1,5 @@
 | tests.cpp:272:2:272:8 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 8 bytes. |
+| tests.cpp:273:2:273:8 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 8 bytes. |
 | tests.cpp:308:3:308:9 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 8 bytes. |
 | tests.cpp:321:2:321:8 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 4 bytes. |
 | tests.cpp:324:3:324:9 | call to sprintf | This 'call to sprintf' operation requires 11 bytes but the destination is only 4 bytes. |