Merge pull request #16238 from MathiasVP/fix-terator-to-expired-container-fp

Author: MathiasVP

cpp/ql/src/experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql

@@ -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
@@ -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
@@ -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
@@ -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
@@ -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()

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 |

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<std::vector<int>>& v = returnValue(); // GOOD [FALSE POSITIVE]
+  const std::vector<std::vector<int>>& v = returnValue(); // GOOD
   for(const std::vector<int>& x : v) {}
 }