From b2974ba1c6dfe089dd1be4b5d17e937854768685 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 16 Apr 2024 22:05:39 +0100 Subject: [PATCH 1/5] C++: Factor body of `isSink` into its own predicate. --- .../Security/CWE/CWE-416/IteratorToExpiredContainer.ql | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql b/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql index 15f34aa8d151..e926bed6edd3 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql @@ -11,7 +11,6 @@ * external/cwe/cwe-664 */ -// IMPORTANT: This query does not currently find anything since it relies on extractor and analysis improvements that hasn't yet been released import cpp import semmle.code.cpp.ir.IR import semmle.code.cpp.dataflow.new.DataFlow @@ -19,6 +18,11 @@ import semmle.code.cpp.models.implementations.StdContainer import semmle.code.cpp.models.implementations.StdMap import semmle.code.cpp.models.implementations.Iterator +private predicate tempToDestructorSink(DataFlow::Node sink, CallInstruction call) { + call = sink.asOperand().(ThisArgumentOperand).getCall() and + call.getStaticCallTarget() instanceof Destructor +} + /** * A configuration to track flow from a temporary variable to the qualifier of * a destructor call @@ -28,9 +32,7 @@ module TempToDestructorConfig implements DataFlow::ConfigSig { source.asInstruction().(VariableAddressInstruction).getIRVariable() instanceof IRTempVariable } - predicate isSink(DataFlow::Node sink) { - sink.asOperand().(ThisArgumentOperand).getCall().getStaticCallTarget() instanceof Destructor - } + predicate isSink(DataFlow::Node sink) { tempToDestructorSink(sink, _) } } module TempToDestructorFlow = DataFlow::Global; From d22e2bae8e19c6167042c82fbdc9632346c9a431 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 16 Apr 2024 22:06:15 +0100 Subject: [PATCH 2/5] C++: Select the post-update node in 'getADestroyedNode'. --- .../CWE/CWE-416/IteratorToExpiredContainer.ql | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql b/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql index e926bed6edd3..3a171f15c12e 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql @@ -37,6 +37,11 @@ module TempToDestructorConfig implements DataFlow::ConfigSig { module TempToDestructorFlow = DataFlow::Global; +/** Holds if `pun` is the post-update node of the qualifier of `Call`. */ +private predicate isPostUpdateOfQualifier(CallInstruction call, DataFlow::PostUpdateNode pun) { + call.getThisArgumentOperand() = pun.getPreUpdateNode().asOperand() +} + /** * Gets a `DataFlow::Node` that represents a temporary that will be destroyed * by a call to a destructor, or a `DataFlow::Node` that will transitively be @@ -53,8 +58,12 @@ module TempToDestructorFlow = DataFlow::Global; * and thus the result of `get_2d_vector()[0]` is also an invalid reference. */ DataFlow::Node getADestroyedNode() { - exists(TempToDestructorFlow::PathNode destroyedTemp | destroyedTemp.isSource() | - result = destroyedTemp.getNode() + exists(DataFlow::Node n | TempToDestructorFlow::flowTo(n) | + // Case 1: The pointer that goes into the destructor call is destroyed + exists(CallInstruction destructorCall | + tempToDestructorSink(n, destructorCall) and + isPostUpdateOfQualifier(destructorCall, result) + ) or exists(CallInstruction call | result.asInstruction() = call and From eb2790ae63497e30b7420eaf8f6e6c8d4db45340 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 16 Apr 2024 22:07:38 +0100 Subject: [PATCH 3/5] C++: Fix 'case 2' in 'destroyedToBeginSink' now that we're working with the sink instead of the source. --- .../Security/CWE/CWE-416/IteratorToExpiredContainer.ql | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql b/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql index 3a171f15c12e..f0eedb60fb31 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql @@ -65,10 +65,11 @@ DataFlow::Node getADestroyedNode() { isPostUpdateOfQualifier(destructorCall, result) ) or + // Case 2: Anything that was derived from the temporary that is now destroyed + // is also destroyed. exists(CallInstruction call | result.asInstruction() = call and - DataFlow::localFlow(destroyedTemp.getNode(), - DataFlow::operandNode(call.getThisArgumentOperand())) + DataFlow::localFlow(DataFlow::operandNode(call.getThisArgumentOperand()), n) | call.getStaticCallTarget() instanceof StdSequenceContainerAt or call.getStaticCallTarget() instanceof StdMapAt From 96ba3ec88e4ab13c79356def4a805564a2d71fbc Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 16 Apr 2024 22:07:55 +0100 Subject: [PATCH 4/5] C++: Rename predicate. --- .../Security/CWE/CWE-416/IteratorToExpiredContainer.ql | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql b/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql index f0eedb60fb31..c1a67db4c43b 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql @@ -77,7 +77,7 @@ DataFlow::Node getADestroyedNode() { ) } -predicate isSinkImpl(DataFlow::Node sink, FunctionCall fc) { +predicate destroyedToBeginSink(DataFlow::Node sink, FunctionCall fc) { exists(CallInstruction call | call = sink.asOperand().(ThisArgumentOperand).getCall() and fc = call.getUnconvertedResultExpression() and @@ -91,7 +91,7 @@ predicate isSinkImpl(DataFlow::Node sink, FunctionCall fc) { module DestroyedToBeginConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node source) { source = getADestroyedNode() } - predicate isSink(DataFlow::Node sink) { isSinkImpl(sink, _) } + predicate isSink(DataFlow::Node sink) { destroyedToBeginSink(sink, _) } DataFlow::FlowFeature getAFeature() { // By blocking argument-to-parameter flow we ensure that we don't enter a @@ -111,5 +111,5 @@ module DestroyedToBeginConfig implements DataFlow::ConfigSig { module DestroyedToBeginFlow = DataFlow::Global; from DataFlow::Node source, DataFlow::Node sink, FunctionCall beginOrEnd -where DestroyedToBeginFlow::flow(source, sink) and isSinkImpl(sink, beginOrEnd) +where DestroyedToBeginFlow::flow(source, sink) and destroyedToBeginSink(sink, beginOrEnd) select source, "This object is destroyed before $@ is called.", beginOrEnd, beginOrEnd.toString() From 592ca061599ff2c06769ffb115f0f9d3fd3c92d6 Mon Sep 17 00:00:00 2001 From: Mathias Vorreiter Pedersen Date: Tue, 16 Apr 2024 22:08:13 +0100 Subject: [PATCH 5/5] C++: Accept test changes. --- .../CWE/CWE-416/IteratorToExpiredContainer.expected | 7 +------ .../experimental/query-tests/Security/CWE/CWE-416/test.cpp | 4 ++-- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-416/IteratorToExpiredContainer.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-416/IteratorToExpiredContainer.expected index 2063838f1070..c29e7953ea64 100644 --- a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-416/IteratorToExpiredContainer.expected +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-416/IteratorToExpiredContainer.expected @@ -2,15 +2,10 @@ | test.cpp:680:30:680:30 | call to operator[] | This object is destroyed before $@ is called. | test.cpp:680:17:680:17 | call to end | call to end | | test.cpp:683:31:683:32 | call to at | This object is destroyed before $@ is called. | test.cpp:683:17:683:17 | call to begin | call to begin | | test.cpp:683:31:683:32 | call to at | This object is destroyed before $@ is called. | test.cpp:683:17:683:17 | call to end | call to end | -| test.cpp:689:17:689:29 | temporary object | This object is destroyed before $@ is called. | test.cpp:689:31:689:35 | call to begin | call to begin | -| test.cpp:689:46:689:58 | temporary object | This object is destroyed before $@ is called. | test.cpp:689:60:689:62 | call to end | call to end | +| test.cpp:689:46:689:58 | pointer to ~vector output argument | This object is destroyed before $@ is called. | test.cpp:689:60:689:62 | call to end | call to end | | test.cpp:702:27:702:27 | call to operator[] | This object is destroyed before $@ is called. | test.cpp:703:19:703:23 | call to begin | call to begin | | test.cpp:702:27:702:27 | call to operator[] | This object is destroyed before $@ is called. | test.cpp:703:36:703:38 | call to end | call to end | -| test.cpp:716:36:716:48 | temporary object | This object is destroyed before $@ is called. | test.cpp:716:17:716:17 | call to begin | call to begin | -| test.cpp:716:36:716:48 | temporary object | This object is destroyed before $@ is called. | test.cpp:716:17:716:17 | call to end | call to end | | test.cpp:727:23:727:23 | call to operator[] | This object is destroyed before $@ is called. | test.cpp:750:17:750:17 | call to begin | call to begin | | test.cpp:727:23:727:23 | call to operator[] | This object is destroyed before $@ is called. | test.cpp:750:17:750:17 | call to end | call to end | | test.cpp:735:23:735:23 | call to operator[] | This object is destroyed before $@ is called. | test.cpp:759:17:759:17 | call to begin | call to begin | | test.cpp:735:23:735:23 | call to operator[] | This object is destroyed before $@ is called. | test.cpp:759:17:759:17 | call to end | call to end | -| test.cpp:771:44:771:56 | temporary object | This object is destroyed before $@ is called. | test.cpp:772:35:772:35 | call to begin | call to begin | -| test.cpp:771:44:771:56 | temporary object | This object is destroyed before $@ is called. | test.cpp:772:35:772:35 | call to end | call to end | diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-416/test.cpp b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-416/test.cpp index b3504e7d9b54..e1c0193c9cdb 100644 --- a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-416/test.cpp +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-416/test.cpp @@ -713,7 +713,7 @@ void test() { for(auto it = v.begin(); it != v.end(); ++it) {} // GOOD } - for (auto x : return_self_by_ref(returnValue())) {} // BAD + for (auto x : return_self_by_ref(returnValue())) {} // BAD [NOT DETECTED] for (auto x : return_self_by_value(returnValue())) {} // GOOD } @@ -768,6 +768,6 @@ void test2() { } void test3() { - const std::vector>& v = returnValue(); // GOOD [FALSE POSITIVE] + const std::vector>& v = returnValue(); // GOOD for(const std::vector& x : v) {} } \ No newline at end of file