Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++: Fix cpp/iterator-to-expired-container FPs #16238

Merged
merged 5 commits into from
Apr 18, 2024
Merged

C++: Fix cpp/iterator-to-expired-container FPs #16238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b2974ba
C++: Factor body of `isSink` into its own predicate.
MathiasVP Apr 16, 2024
d22e2ba
C++: Select the post-update node in 'getADestroyedNode'.
MathiasVP Apr 16, 2024
eb2790a
C++: Fix 'case 2' in 'destroyedToBeginSink' now that we're working wi…
MathiasVP Apr 16, 2024
96ba3ec
C++: Rename predicate.
MathiasVP Apr 16, 2024
592ca06
C++: Accept test changes.
MathiasVP Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 23 additions & 11 deletions cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,18 @@
* 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
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
Expand All @@ -28,13 +32,16 @@ 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<TempToDestructorConfig>;

/** 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
Expand All @@ -51,21 +58,26 @@ module TempToDestructorFlow = DataFlow::Global<TempToDestructorConfig>;
* 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
// 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
)
)
}

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
Expand All @@ -79,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
Expand All @@ -99,5 +111,5 @@ module DestroyedToBeginConfig implements DataFlow::ConfigSig {
module DestroyedToBeginFlow = DataFlow::Global<DestroyedToBeginConfig>;

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()
7 changes: 1 addition & 6 deletions ...ql/test/experimental/query-tests/Security/CWE/CWE-416/IteratorToExpiredContainer.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
4 changes: 2 additions & 2 deletions cpp/ql/test/experimental/query-tests/Security/CWE/CWE-416/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -768,6 +768,6 @@ void test2() {
}

void test3() {
const std::vector<std::vector<int>>& v = returnValue(); // GOOD [FALSE POSITIVE]
const std::vector<std::vector<int>>& v = returnValue(); // GOOD
for(const std::vector<int>& x : v) {}
}
Loading