diff --git a/cpp/ql/lib/semmle/code/cpp/security/InvalidPointerDereference/AllocationToInvalidPointer.qll b/cpp/ql/lib/semmle/code/cpp/security/InvalidPointerDereference/AllocationToInvalidPointer.qll index 976524316a8..83017aec353 100644 --- a/cpp/ql/lib/semmle/code/cpp/security/InvalidPointerDereference/AllocationToInvalidPointer.qll +++ b/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 diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-193/InvalidPointerDeref.expected b/cpp/ql/test/query-tests/Security/CWE/CWE-193/InvalidPointerDeref.expected index b93b69398ce..1148e98980c 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-193/InvalidPointerDeref.expected +++ b/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 | ... + ... | ... + ... | diff --git a/cpp/ql/test/query-tests/Security/CWE/CWE-193/test.cpp b/cpp/ql/test/query-tests/Security/CWE/CWE-193/test.cpp index 58b3e843424..e1598902ac6 100644 --- a/cpp/ql/test/query-tests/Security/CWE/CWE-193/test.cpp +++ b/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 + } } \ No newline at end of file