From 6a7b2e4aa4cae46a6342fe5510530f4aa0ab0550 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 16 Oct 2023 10:47:45 +0100 Subject: [PATCH 1/3] C++: Add failing test. --- cpp/ql/test/query-tests/Security/CWE/CWE-193/test.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) 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..f6983191110 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); + + if(p <= chend) { + *p = 42; // BAD [NOT DETECTED] + } } \ No newline at end of file From 7e6857d36bb3fc71c377a7489bc8fdfaa9eb572d Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 16 Oct 2023 10:48:28 +0100 Subject: [PATCH 2/3] C++: Make 'hasSize' slightly smarter when handling ternary operators. --- .../AllocationToInvalidPointer.qll | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) 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 From d8a049f5cc79e8e837b0ac5f4b4a73a69ccde166 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 16 Oct 2023 10:48:44 +0100 Subject: [PATCH 3/3] C++: Accept test changes. --- .../Security/CWE/CWE-193/InvalidPointerDeref.expected | 11 +++++++++++ cpp/ql/test/query-tests/Security/CWE/CWE-193/test.cpp | 4 ++-- 2 files changed, 13 insertions(+), 2 deletions(-) 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 f6983191110..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 @@ -854,9 +854,9 @@ void test16_with_malloc(size_t index) { void test_regression(size_t size) { int* p = (int*)MyMalloc(size + 1); - int* chend = p + (size + 1); + int* chend = p + (size + 1); // $ alloc=L856+1 if(p <= chend) { - *p = 42; // BAD [NOT DETECTED] + *p = 42; // $ deref=L860 // BAD } } \ No newline at end of file