Merge pull request github#13355 from jketema/ptr-deref-forward

C++: Ensure that the sink instruction occurs last in `cpp/invalid-pointer-deref`

Author: jketema

cpp/ql/src/experimental/Security/CWE/CWE-193/InvalidPointerDeref.ql

@@ -179,15 +179,34 @@ predicate isSinkImpl(
   pointerAddInstructionHasBounds(pai, sink1, sink2, delta)
 }
 
+/**
+ * Yields any instruction that is control-flow reachable from `instr`.
+ */
+bindingset[instr, result]
+pragma[inline_late]
+Instruction getASuccessor(Instruction instr) {
+  exists(IRBlock b, int instrIndex, int resultIndex |
+    result.getBlock() = b and
+    instr.getBlock() = b and
+    b.getInstruction(instrIndex) = instr and
+    b.getInstruction(resultIndex) = result
+  |
+    resultIndex >= instrIndex
+  )
+  or
+  instr.getBlock().getASuccessor+() = result.getBlock()
+}
+
 /**
  * Holds if `sink` is a sink for `InvalidPointerToDerefConfig` and `i` is a `StoreInstruction` that
  * writes to an address that non-strictly upper-bounds `sink`, or `i` is a `LoadInstruction` that
  * reads from an address that non-strictly upper-bounds `sink`.
  */
 pragma[inline]
 predicate isInvalidPointerDerefSink(DataFlow::Node sink, Instruction i, string operation, int delta) {
-  exists(AddressOperand addr |
-    bounded1(addr.getDef(), sink.asInstruction(), delta) and
+  exists(AddressOperand addr, Instruction s |
+    s = sink.asInstruction() and
+    bounded1(addr.getDef(), s, delta) and
     delta >= 0 and
     i.getAnOperand() = addr
   |
@@ -247,7 +266,8 @@ newtype TMergedPathNode =
   TPathNodeSink(Instruction i) {
     exists(DataFlow::Node n |
       InvalidPointerToDerefFlow::flowTo(n) and
-      isInvalidPointerDerefSink(n, i, _, _)
+      isInvalidPointerDerefSink(n, i, _, _) and
+      i = getASuccessor(n.asInstruction())
     )
   }
 

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected

@@ -663,8 +663,6 @@ edges
 | test.cpp:326:15:326:23 | ... + ... | test.cpp:342:8:342:17 | * ... |
 | test.cpp:338:8:338:15 | * ... | test.cpp:342:8:342:17 | * ... |
 | test.cpp:341:8:341:17 | * ... | test.cpp:342:8:342:17 | * ... |
-| test.cpp:342:8:342:17 | * ... | test.cpp:333:5:333:21 | Store: ... = ... |
-| test.cpp:342:8:342:17 | * ... | test.cpp:341:5:341:21 | Store: ... = ... |
 | test.cpp:347:14:347:27 | new[] | test.cpp:348:15:348:16 | xs |
 | test.cpp:348:15:348:16 | xs | test.cpp:350:16:350:19 | ... ++ |
 | test.cpp:348:15:348:16 | xs | test.cpp:350:16:350:19 | ... ++ |
@@ -1031,9 +1029,7 @@ nodes
 | test.cpp:326:15:326:16 | xs | semmle.label | xs |
 | test.cpp:326:15:326:23 | ... + ... | semmle.label | ... + ... |
 | test.cpp:326:15:326:23 | ... + ... | semmle.label | ... + ... |
-| test.cpp:333:5:333:21 | Store: ... = ... | semmle.label | Store: ... = ... |
 | test.cpp:338:8:338:15 | * ... | semmle.label | * ... |
-| test.cpp:341:5:341:21 | Store: ... = ... | semmle.label | Store: ... = ... |
 | test.cpp:341:8:341:17 | * ... | semmle.label | * ... |
 | test.cpp:342:8:342:17 | * ... | semmle.label | * ... |
 | test.cpp:347:14:347:27 | new[] | semmle.label | new[] |
@@ -1088,8 +1084,6 @@ subpaths
 | test.cpp:264:13:264:14 | Load: * ... | test.cpp:260:13:260:24 | new[] | test.cpp:264:13:264:14 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:260:13:260:24 | new[] | new[] | test.cpp:261:19:261:21 | len | len |
 | test.cpp:274:5:274:10 | Store: ... = ... | test.cpp:270:13:270:24 | new[] | test.cpp:274:5:274:10 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:270:13:270:24 | new[] | new[] | test.cpp:271:19:271:21 | len | len |
 | test.cpp:308:5:308:29 | Store: ... = ... | test.cpp:304:15:304:26 | new[] | test.cpp:308:5:308:29 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:304:15:304:26 | new[] | new[] | test.cpp:308:8:308:10 | ... + ... | ... + ... |
-| test.cpp:333:5:333:21 | Store: ... = ... | test.cpp:325:14:325:27 | new[] | test.cpp:333:5:333:21 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:325:14:325:27 | new[] | new[] | test.cpp:326:20:326:23 | size | size |
-| test.cpp:341:5:341:21 | Store: ... = ... | test.cpp:325:14:325:27 | new[] | test.cpp:341:5:341:21 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:325:14:325:27 | new[] | new[] | test.cpp:326:20:326:23 | size | size |
 | test.cpp:350:15:350:19 | Load: * ... | test.cpp:347:14:347:27 | new[] | test.cpp:350:15:350:19 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:347:14:347:27 | new[] | new[] | test.cpp:348:20:348:23 | size | size |
 | test.cpp:358:14:358:26 | Load: * ... | test.cpp:355:14:355:27 | new[] | test.cpp:358:14:358:26 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@ + 1. | test.cpp:355:14:355:27 | new[] | new[] | test.cpp:356:20:356:23 | size | size |
 | test.cpp:359:14:359:32 | Load: * ... | test.cpp:355:14:355:27 | new[] | test.cpp:359:14:359:32 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@ + 2. | test.cpp:355:14:355:27 | new[] | new[] | test.cpp:356:20:356:23 | size | size |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp

@@ -330,15 +330,15 @@ void test23(unsigned size, int val) {
     if(*current - xs < 1)
       return;
 
-    *--(*current) = 0; // GOOD [FALSE POSITIVE]
+    *--(*current) = 0; // GOOD
     return;
   }
 
   if (val < 2) {
     if(*current - xs < 2)
       return;
 
-    *--(*current) = 0; // GOOD [FALSE POSITIVE]
+    *--(*current) = 0; // GOOD
     *--(*current) = 0; // GOOD
   }
 }