Merge pull request #16355 from MathiasVP/promote-iterator-to-expired-container-out-of-experimental

C++: Promote `cpp/iterator-to-expired-container` out of experimental

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

@@ -2,9 +2,10 @@
  * @name Iterator to expired container
  * @description Using an iterator owned by a container whose lifetime has expired may lead to unexpected behavior.
  * @kind problem
- * @precision high
+ * @precision medium
  * @id cpp/iterator-to-expired-container
  * @problem.severity warning
+ * @security-severity 8.8
  * @tags reliability
  *       security
  *       external/cwe/cwe-416
@@ -69,6 +70,31 @@ predicate destroyedToBeginSink(DataFlow::Node sink, FunctionCall fc) {
   )
 }
 
+/**
+ * Holds if `node1` is the node corresponding to a qualifier of a destructor
+ * call and `node2` is a node that is destroyed as a result of `node1` being
+ * destroyed.
+ */
+private predicate qualifierToDestroyed(DataFlow::Node node1, DataFlow::Node node2) {
+  tempToDestructorSink(node1, _) and
+  node2 = getADestroyedNode(node1)
+}
+
+/**
+ * A configuration to track flow from a destroyed node to a qualifier of
+ * a `begin` or `end` function call.
+ *
+ * This configuration exists to prevent a cartesian product between all sinks and
+ * all states in `Config::isSink`.
+ */
+module Config0 implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { qualifierToDestroyed(_, source) }
+
+  predicate isSink(DataFlow::Node sink) { destroyedToBeginSink(sink, _) }
+}
+
+module Flow0 = DataFlow::Global<Config0>;
+
 /**
  * A configuration to track flow from a temporary variable to the qualifier of
  * a destructor call, and subsequently to a qualifier of a call to `begin` or
@@ -78,12 +104,15 @@ module Config implements DataFlow::StateConfigSig {
   newtype FlowState =
     additional TempToDestructor() or
     additional DestroyedToBegin(DataFlow::Node n) {
-      exists(DataFlow::Node thisOperand |
-        tempToDestructorSink(thisOperand, _) and
-        n = getADestroyedNode(thisOperand)
-      )
+      any(Flow0::PathNode pn | pn.isSource()).getNode() = n
     }
 
+  /**
+   * Holds if `sink` is a qualifier to a call to `begin`, and `mid` is an
+   * object that is destroyed.
+   */
+  private predicate relevant(DataFlow::Node mid, DataFlow::Node sink) { Flow0::flow(mid, sink) }
+
   predicate isSource(DataFlow::Node source, FlowState state) {
     source.asInstruction().(VariableAddressInstruction).getIRVariable() instanceof IRTempVariable and
     state = TempToDestructor()
@@ -92,16 +121,16 @@ module Config implements DataFlow::StateConfigSig {
   predicate isAdditionalFlowStep(
     DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2
   ) {
-    tempToDestructorSink(node1, _) and
     state1 = TempToDestructor() and
     state2 = DestroyedToBegin(node2) and
-    node2 = getADestroyedNode(node1)
+    qualifierToDestroyed(node1, node2)
   }
 
   predicate isSink(DataFlow::Node sink, FlowState state) {
-    // Note: This is a non-trivial cartesian product!
-    // Hopefully, both of these sets are quite small in practice
-    destroyedToBeginSink(sink, _) and state instanceof DestroyedToBegin
+    exists(DataFlow::Node mid |
+      relevant(mid, sink) and
+      state = DestroyedToBegin(mid)
+    )
   }
 
   DataFlow::FlowFeature getAFeature() {

cpp/ql/src/change-notes/2024-04-29-iterator-to-expired-container.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `cpp/iterator-to-expired-container`, to detect the creation of iterators owned by a temporary objects that are about to be destroyed.