Merge pull request github#14512 from MathiasVP/fix-size-in-invalid-ptr-deref

C++: Fix size deduction in `cpp/invalid-pointer-deref`

Author: Dave Bartolomeo

cpp/ql/lib/semmle/code/cpp/security/InvalidPointerDereference/AllocationToInvalidPointer.qll

@@ -60,17 +60,31 @@ private import semmle.code.cpp.rangeanalysis.new.RangeAnalysisUtil
 
 private VariableAccess getAVariableAccess(Expr e) { e.getAChild*() = result }
 
+/**
+ * Gets a (sub)expression that may be the result of evaluating `size`.
+ *
+ * For example, `getASizeCandidate(a ? b : c)` gives `a ? b : c`, `b` and `c`.
+ */
+bindingset[size]
+pragma[inline_late]
+private Expr getASizeCandidate(Expr size) {
+  result = size
+  or
+  result = [size.(ConditionalExpr).getThen(), size.(ConditionalExpr).getElse()]
+}
+
 /**
  * Holds if the `(n, state)` pair represents the source of flow for the size
  * expression associated with `alloc`.
  */
 predicate hasSize(HeuristicAllocationExpr alloc, DataFlow::Node n, int state) {
-  exists(VariableAccess va, Expr size, int delta |
+  exists(VariableAccess va, Expr size, int delta, Expr s |
     size = alloc.getSizeExpr() and
+    s = getASizeCandidate(size) and
     // Get the unique variable in a size expression like `x` in `malloc(x + 1)`.
-    va = unique( | | getAVariableAccess(size)) and
+    va = unique( | | getAVariableAccess(s)) and
     // Compute `delta` as the constant difference between `x` and `x + 1`.
-    bounded1(any(Instruction instr | instr.getUnconvertedResultExpression() = size),
+    bounded1(any(Instruction instr | instr.getUnconvertedResultExpression() = s),
       any(LoadInstruction load | load.getUnconvertedResultExpression() = va), delta) and
     n.asExpr() = va and
     state = delta

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

@@ -181,6 +181,12 @@ edges
 | test.cpp:833:37:833:39 | end | test.cpp:815:52:815:54 | end |
 | test.cpp:841:18:841:35 | call to malloc | test.cpp:842:3:842:20 | ... = ... |
 | test.cpp:848:20:848:37 | call to malloc | test.cpp:849:5:849:22 | ... = ... |
+| test.cpp:856:12:856:35 | call to malloc | test.cpp:857:16:857:29 | ... + ... |
+| test.cpp:856:12:856:35 | call to malloc | test.cpp:857:16:857:29 | ... + ... |
+| test.cpp:856:12:856:35 | call to malloc | test.cpp:860:5:860:11 | ... = ... |
+| test.cpp:857:16:857:29 | ... + ... | test.cpp:857:16:857:29 | ... + ... |
+| test.cpp:857:16:857:29 | ... + ... | test.cpp:860:5:860:11 | ... = ... |
+| test.cpp:857:16:857:29 | ... + ... | test.cpp:860:5:860:11 | ... = ... |
 nodes
 | test.cpp:4:15:4:33 | call to malloc | semmle.label | call to malloc |
 | test.cpp:5:15:5:22 | ... + ... | semmle.label | ... + ... |
@@ -307,6 +313,10 @@ nodes
 | test.cpp:842:3:842:20 | ... = ... | semmle.label | ... = ... |
 | test.cpp:848:20:848:37 | call to malloc | semmle.label | call to malloc |
 | test.cpp:849:5:849:22 | ... = ... | semmle.label | ... = ... |
+| test.cpp:856:12:856:35 | call to malloc | semmle.label | call to malloc |
+| test.cpp:857:16:857:29 | ... + ... | semmle.label | ... + ... |
+| test.cpp:857:16:857:29 | ... + ... | semmle.label | ... + ... |
+| test.cpp:860:5:860:11 | ... = ... | semmle.label | ... = ... |
 subpaths
 #select
 | test.cpp:6:14:6:15 | * ... | test.cpp:4:15:4:33 | call to malloc | test.cpp:6:14:6:15 | * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:4:15:4:33 | call to malloc | call to malloc | test.cpp:5:19:5:22 | size | size |
@@ -344,3 +354,4 @@ subpaths
 | test.cpp:821:7:821:12 | ... = ... | test.cpp:793:14:793:32 | call to malloc | test.cpp:821:7:821:12 | ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:793:14:793:32 | call to malloc | call to malloc | test.cpp:794:21:794:24 | size | size |
 | test.cpp:842:3:842:20 | ... = ... | test.cpp:841:18:841:35 | call to malloc | test.cpp:842:3:842:20 | ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:841:18:841:35 | call to malloc | call to malloc | test.cpp:842:11:842:15 | index | index |
 | test.cpp:849:5:849:22 | ... = ... | test.cpp:848:20:848:37 | call to malloc | test.cpp:849:5:849:22 | ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:848:20:848:37 | call to malloc | call to malloc | test.cpp:849:13:849:17 | index | index |
+| test.cpp:860:5:860:11 | ... = ... | test.cpp:856:12:856:35 | call to malloc | test.cpp:860:5:860:11 | ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:856:12:856:35 | call to malloc | call to malloc | test.cpp:857:21:857:28 | ... + ... | ... + ... |

cpp/ql/test/query-tests/Security/CWE/CWE-193/test.cpp

@@ -848,4 +848,15 @@ void test16_with_malloc(size_t index) {
     int* newname = (int*)malloc(size);
     newname[index] = 0; // $ SPURIOUS: alloc=L848 deref=L849 // GOOD [FALSE POSITIVE]
   }
+}
+
+# define MyMalloc(size) malloc(((size) == 0 ? 1 : (size)))
+
+void test_regression(size_t size) {
+  int* p = (int*)MyMalloc(size + 1);
+  int* chend = p + (size + 1); // $ alloc=L856+1
+
+  if(p <= chend) {
+    *p = 42; // $ deref=L860 // BAD
+  }
 }