From 36285ba2c5ef6e1667a69cffcd9da84f17da8f07 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 22 May 2023 17:39:43 -0700 Subject: [PATCH 1/2] C++: Fix pointer/pointee conflation. --- .../code/cpp/ir/dataflow/internal/DataFlowPrivate.qll | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll index efd33b82a89..cc8d0cdbe94 100644 --- a/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll +++ b/cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll @@ -210,8 +210,8 @@ class IndirectOperand extends Node { this.(RawIndirectOperand).getOperand() = operand and this.(RawIndirectOperand).getIndirectionIndex() = indirectionIndex or - this.(OperandNode).getOperand() = - Ssa::getIRRepresentationOfIndirectOperand(operand, indirectionIndex) + nodeHasOperand(this, Ssa::getIRRepresentationOfIndirectOperand(operand, indirectionIndex), + indirectionIndex - 1) } /** Gets the underlying operand. */ @@ -250,8 +250,8 @@ class IndirectInstruction extends Node { this.(RawIndirectInstruction).getInstruction() = instr and this.(RawIndirectInstruction).getIndirectionIndex() = indirectionIndex or - this.(InstructionNode).getInstruction() = - Ssa::getIRRepresentationOfIndirectInstruction(instr, indirectionIndex) + nodeHasInstruction(this, Ssa::getIRRepresentationOfIndirectInstruction(instr, indirectionIndex), + indirectionIndex - 1) } /** Gets the underlying instruction. */ From b32d55a21de144e0fe3840b159d68fd81754d5a3 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Mon, 22 May 2023 17:39:50 -0700 Subject: [PATCH 2/2] C++: Accept test changes. --- .../InvalidPointerDeref.expected | 28 ------------------- .../CWE/CWE-193/pointer-deref/test.cpp | 6 ++-- .../dataflow/taint-tests/localTaint.expected | 20 ++++++------- .../dataflow/taint-tests/vector.cpp | 15 +++------- .../MemoryFreed/UseAfterFree.expected | 5 ---- 5 files changed, 17 insertions(+), 57 deletions(-) diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected index 4ef8b163372..6da1913cfa2 100644 --- a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected @@ -654,30 +654,6 @@ edges | test.cpp:308:5:308:6 | xs | test.cpp:308:5:308:11 | access to array | | test.cpp:308:5:308:11 | access to array | test.cpp:308:5:308:29 | Store: ... = ... | | test.cpp:313:16:313:29 | new[] | test.cpp:314:17:314:18 | xs | -| test.cpp:314:17:314:18 | xs | test.cpp:314:17:314:25 | ... + ... | -| test.cpp:314:17:314:18 | xs | test.cpp:314:17:314:25 | ... + ... | -| test.cpp:314:17:314:18 | xs | test.cpp:318:13:318:20 | * ... | -| test.cpp:314:17:314:25 | ... + ... | test.cpp:318:14:318:20 | current | -| test.cpp:314:17:314:25 | ... + ... | test.cpp:318:14:318:20 | current | -| test.cpp:314:17:314:25 | ... + ... | test.cpp:320:13:320:20 | * ... | -| test.cpp:314:17:314:25 | ... + ... | test.cpp:320:13:320:20 | * ... | -| test.cpp:314:17:314:25 | ... + ... | test.cpp:320:14:320:20 | current | -| test.cpp:314:17:314:25 | ... + ... | test.cpp:320:14:320:20 | current | -| test.cpp:318:13:318:20 | * ... | test.cpp:318:14:318:20 | current | -| test.cpp:318:13:318:20 | * ... | test.cpp:320:13:320:20 | * ... | -| test.cpp:318:13:318:20 | * ... | test.cpp:320:14:320:20 | current | -| test.cpp:318:14:318:20 | current | test.cpp:314:17:314:25 | Store: ... + ... | -| test.cpp:318:14:318:20 | current | test.cpp:318:13:318:20 | Load: * ... | -| test.cpp:318:14:318:20 | current | test.cpp:320:10:320:21 | Store: -- ... | -| test.cpp:318:14:318:20 | current | test.cpp:320:12:320:21 | Load: (...) | -| test.cpp:320:13:320:20 | * ... | test.cpp:314:17:314:25 | Store: ... + ... | -| test.cpp:320:13:320:20 | * ... | test.cpp:318:13:318:20 | Load: * ... | -| test.cpp:320:13:320:20 | * ... | test.cpp:320:10:320:21 | Store: -- ... | -| test.cpp:320:13:320:20 | * ... | test.cpp:320:12:320:21 | Load: (...) | -| test.cpp:320:14:320:20 | current | test.cpp:314:17:314:25 | Store: ... + ... | -| test.cpp:320:14:320:20 | current | test.cpp:318:13:318:20 | Load: * ... | -| test.cpp:320:14:320:20 | current | test.cpp:320:10:320:21 | Store: -- ... | -| test.cpp:320:14:320:20 | current | test.cpp:320:12:320:21 | Load: (...) | subpaths #select | test.cpp:6:14:6:15 | Load: * ... | test.cpp:4:15:4:20 | call to malloc | test.cpp:6:14:6:15 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:4:15:4:20 | call to malloc | call to malloc | test.cpp:5:19:5:22 | size | size | @@ -703,7 +679,3 @@ 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:314:17:314:25 | Store: ... + ... | test.cpp:313:16:313:29 | new[] | test.cpp:314:17:314:25 | Store: ... + ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:313:16:313:29 | new[] | new[] | test.cpp:314:22:314:25 | size | size | -| test.cpp:318:13:318:20 | Load: * ... | test.cpp:313:16:313:29 | new[] | test.cpp:318:13:318:20 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:313:16:313:29 | new[] | new[] | test.cpp:314:22:314:25 | size | size | -| test.cpp:320:10:320:21 | Store: -- ... | test.cpp:313:16:313:29 | new[] | test.cpp:320:10:320:21 | Store: -- ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:313:16:313:29 | new[] | new[] | test.cpp:314:22:314:25 | size | size | -| test.cpp:320:12:320:21 | Load: (...) | test.cpp:313:16:313:29 | new[] | test.cpp:320:12:320:21 | Load: (...) | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:313:16:313:29 | new[] | new[] | test.cpp:314:22:314:25 | size | size | diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp index 05ae4a2ac57..54fa131f232 100644 --- a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp @@ -311,13 +311,13 @@ void test21() { void test22(unsigned size, int val) { char *xs = new char[size]; - char *end = xs + size; // GOOD [FALSE POSITIVE] + char *end = xs + size; // GOOD char **current = &end; do { - if( *current - xs < 1 ) // GOOD [FALSE POSITIVE] + if( *current - xs < 1 ) // GOOD return; - *--(*current) = 0; // GOOD [FALSE POSITIVE] + *--(*current) = 0; // GOOD val >>= 8; } while( val > 0 ); diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected index 85fc3526dc7..907cccd197b 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected @@ -8090,20 +8090,20 @@ | vector.cpp:520:25:520:31 | call to vector | vector.cpp:523:8:523:9 | vs | | | vector.cpp:520:25:520:31 | call to vector | vector.cpp:524:8:524:9 | vs | | | vector.cpp:520:25:520:31 | call to vector | vector.cpp:526:8:526:9 | vs | | -| vector.cpp:520:25:520:31 | call to vector | vector.cpp:539:8:539:9 | vs | | -| vector.cpp:520:25:520:31 | call to vector | vector.cpp:540:2:540:2 | vs | | +| vector.cpp:520:25:520:31 | call to vector | vector.cpp:532:8:532:9 | vs | | +| vector.cpp:520:25:520:31 | call to vector | vector.cpp:533:2:533:2 | vs | | | vector.cpp:520:30:520:30 | 0 | vector.cpp:520:25:520:31 | call to vector | TAINT | | vector.cpp:523:8:523:9 | ref arg vs | vector.cpp:524:8:524:9 | vs | | | vector.cpp:523:8:523:9 | ref arg vs | vector.cpp:526:8:526:9 | vs | | -| vector.cpp:523:8:523:9 | ref arg vs | vector.cpp:539:8:539:9 | vs | | -| vector.cpp:523:8:523:9 | ref arg vs | vector.cpp:540:2:540:2 | vs | | +| vector.cpp:523:8:523:9 | ref arg vs | vector.cpp:532:8:532:9 | vs | | +| vector.cpp:523:8:523:9 | ref arg vs | vector.cpp:533:2:533:2 | vs | | | vector.cpp:523:8:523:9 | vs | vector.cpp:523:10:523:10 | call to operator[] | TAINT | | vector.cpp:524:8:524:9 | ref arg vs | vector.cpp:526:8:526:9 | vs | | -| vector.cpp:524:8:524:9 | ref arg vs | vector.cpp:539:8:539:9 | vs | | -| vector.cpp:524:8:524:9 | ref arg vs | vector.cpp:540:2:540:2 | vs | | +| vector.cpp:524:8:524:9 | ref arg vs | vector.cpp:532:8:532:9 | vs | | +| vector.cpp:524:8:524:9 | ref arg vs | vector.cpp:533:2:533:2 | vs | | | vector.cpp:524:8:524:9 | vs | vector.cpp:524:10:524:10 | call to operator[] | TAINT | -| vector.cpp:526:8:526:9 | ref arg vs | vector.cpp:539:8:539:9 | vs | | -| vector.cpp:526:8:526:9 | ref arg vs | vector.cpp:540:2:540:2 | vs | | +| vector.cpp:526:8:526:9 | ref arg vs | vector.cpp:532:8:532:9 | vs | | +| vector.cpp:526:8:526:9 | ref arg vs | vector.cpp:533:2:533:2 | vs | | | vector.cpp:526:8:526:9 | vs | vector.cpp:526:11:526:15 | call to begin | TAINT | | vector.cpp:526:11:526:15 | call to begin | vector.cpp:526:3:526:17 | ... = ... | | | vector.cpp:526:11:526:15 | call to begin | vector.cpp:527:9:527:10 | it | | @@ -8128,5 +8128,5 @@ | vector.cpp:530:3:530:4 | ref arg it | vector.cpp:531:9:531:10 | it | | | vector.cpp:530:9:530:14 | call to source | vector.cpp:530:3:530:4 | ref arg it | TAINT | | vector.cpp:531:9:531:10 | it | vector.cpp:531:8:531:8 | call to operator* | TAINT | -| vector.cpp:539:8:539:9 | ref arg vs | vector.cpp:540:2:540:2 | vs | | -| vector.cpp:539:8:539:9 | vs | vector.cpp:539:10:539:10 | call to operator[] | TAINT | +| vector.cpp:532:8:532:9 | ref arg vs | vector.cpp:533:2:533:2 | vs | | +| vector.cpp:532:8:532:9 | vs | vector.cpp:532:10:532:10 | call to operator[] | TAINT | diff --git a/cpp/ql/test/library-tests/dataflow/taint-tests/vector.cpp b/cpp/ql/test/library-tests/dataflow/taint-tests/vector.cpp index 19824641560..a26ac8f0513 100644 --- a/cpp/ql/test/library-tests/dataflow/taint-tests/vector.cpp +++ b/cpp/ql/test/library-tests/dataflow/taint-tests/vector.cpp @@ -523,19 +523,12 @@ void test_vector_iterator() { sink(vs[1]); sink(vs[source()]); // $ MISSING: ast,ir - it = vs.begin(); // (1) + it = vs.begin(); sink(*it); it += 1; sink(*it); - it += source(); // (2) - sink(*it); // $ ast,ir // (3) - // This FP happens because of the following flows: - // 1. There's a write to the iterator at (2) - // 2. This write propagates to `it` on the next line at (3) - // 3. There's a taint step from `it` to `*it` at (3) - // 4. The `*it` is seen as a use of `vs` because of (1). - // 5. There's use-use flow from `*it` at (3) (which is a use of `vs`) to `vs` at (4) - // 6. There's a taint step from vs to vs[1] - sink(vs[1]); // $ SPURIOUS: ir // (4) + it += source(); + sink(*it); // $ ast,ir + sink(vs[1]); // clean } } diff --git a/cpp/ql/test/query-tests/Critical/MemoryFreed/UseAfterFree.expected b/cpp/ql/test/query-tests/Critical/MemoryFreed/UseAfterFree.expected index 54bad8e6cbc..16e74b982c1 100644 --- a/cpp/ql/test/query-tests/Critical/MemoryFreed/UseAfterFree.expected +++ b/cpp/ql/test/query-tests/Critical/MemoryFreed/UseAfterFree.expected @@ -23,8 +23,6 @@ edges | test_free.cpp:239:14:239:15 | * ... | test_free.cpp:241:9:241:10 | * ... | | test_free.cpp:239:14:239:15 | * ... | test_free.cpp:241:9:241:10 | * ... | | test_free.cpp:239:14:239:15 | * ... | test_free.cpp:241:9:241:10 | * ... | -| test_free.cpp:239:14:239:15 | * ... | test_free.cpp:241:10:241:10 | b | -| test_free.cpp:239:14:239:15 | * ... | test_free.cpp:241:10:241:10 | b | | test_free.cpp:245:10:245:11 | * ... | test_free.cpp:246:9:246:10 | * ... | | test_free.cpp:245:10:245:11 | * ... | test_free.cpp:246:9:246:10 | * ... | | test_free.cpp:245:10:245:11 | * ... | test_free.cpp:246:9:246:10 | * ... | @@ -61,7 +59,6 @@ nodes | test_free.cpp:239:14:239:15 | * ... | semmle.label | * ... | | test_free.cpp:241:9:241:10 | * ... | semmle.label | * ... | | test_free.cpp:241:9:241:10 | * ... | semmle.label | * ... | -| test_free.cpp:241:10:241:10 | b | semmle.label | b | | test_free.cpp:245:10:245:11 | * ... | semmle.label | * ... | | test_free.cpp:245:10:245:11 | * ... | semmle.label | * ... | | test_free.cpp:246:9:246:10 | * ... | semmle.label | * ... | @@ -92,8 +89,6 @@ subpaths | test_free.cpp:241:9:241:10 | * ... | test_free.cpp:239:14:239:15 | * ... | test_free.cpp:241:9:241:10 | * ... | Memory may have been previously freed by $@. | test_free.cpp:239:9:239:12 | call to free | call to free | | test_free.cpp:241:9:241:10 | * ... | test_free.cpp:239:14:239:15 | * ... | test_free.cpp:241:9:241:10 | * ... | Memory may have been previously freed by $@. | test_free.cpp:239:9:239:12 | call to free | call to free | | test_free.cpp:241:9:241:10 | * ... | test_free.cpp:239:14:239:15 | * ... | test_free.cpp:241:9:241:10 | * ... | Memory may have been previously freed by $@. | test_free.cpp:239:9:239:12 | call to free | call to free | -| test_free.cpp:241:10:241:10 | b | test_free.cpp:239:14:239:15 | * ... | test_free.cpp:241:10:241:10 | b | Memory may have been previously freed by $@. | test_free.cpp:239:9:239:12 | call to free | call to free | -| test_free.cpp:241:10:241:10 | b | test_free.cpp:239:14:239:15 | * ... | test_free.cpp:241:10:241:10 | b | Memory may have been previously freed by $@. | test_free.cpp:239:9:239:12 | call to free | call to free | | test_free.cpp:246:9:246:10 | * ... | test_free.cpp:245:10:245:11 | * ... | test_free.cpp:246:9:246:10 | * ... | Memory may have been previously freed by $@. | test_free.cpp:245:5:245:8 | call to free | call to free | | test_free.cpp:246:9:246:10 | * ... | test_free.cpp:245:10:245:11 | * ... | test_free.cpp:246:9:246:10 | * ... | Memory may have been previously freed by $@. | test_free.cpp:245:5:245:8 | call to free | call to free | | test_free.cpp:246:9:246:10 | * ... | test_free.cpp:245:10:245:11 | * ... | test_free.cpp:246:9:246:10 | * ... | Memory may have been previously freed by $@. | test_free.cpp:245:5:245:8 | call to free | call to free |