Merge branch 'main' into unitialized-local-as-path

Author: MathiasVP

cpp/ql/lib/semmle/code/cpp/models/implementations/Iterator.qll

@@ -440,17 +440,19 @@ private class IteratorAssignmentMemberOperatorModel extends IteratorAssignmentMe
  * A `begin` or `end` member function, or a related member function, that
  * returns an iterator.
  */
-private class BeginOrEndFunction extends MemberFunction, TaintFunction, GetIteratorFunction,
-  AliasFunction, SideEffectFunction
-{
+class BeginOrEndFunction extends MemberFunction {
   BeginOrEndFunction() {
     this.hasName([
         "begin", "cbegin", "rbegin", "crbegin", "end", "cend", "rend", "crend", "before_begin",
         "cbefore_begin"
       ]) and
     this.getType().getUnspecifiedType() instanceof Iterator
   }
+}
 
+private class BeginOrEndFunctionModels extends BeginOrEndFunction, TaintFunction,
+  GetIteratorFunction, AliasFunction, SideEffectFunction
+{
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     input.isQualifierObject() and
     output.isReturnValue()

cpp/ql/lib/semmle/code/cpp/models/implementations/StdContainer.qll

@@ -253,13 +253,15 @@ private class StdSequenceContainerAssign extends TaintFunction {
 /**
  * The standard container functions `at` and `operator[]`.
  */
-private class StdSequenceContainerAt extends TaintFunction {
+class StdSequenceContainerAt extends MemberFunction {
   StdSequenceContainerAt() {
     this.getClassAndName(["at", "operator[]"]) instanceof Array or
     this.getClassAndName(["at", "operator[]"]) instanceof Deque or
     this.getClassAndName(["at", "operator[]"]) instanceof Vector
   }
+}
 
+private class StdSequenceContainerAtModel extends StdSequenceContainerAt, TaintFunction {
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     // flow from qualifier to referenced return value
     input.isQualifierObject() and

cpp/ql/lib/semmle/code/cpp/models/implementations/StdMap.qll

@@ -129,9 +129,11 @@ private class StdMapMerge extends TaintFunction {
 /**
  * The standard map functions `at` and `operator[]`.
  */
-private class StdMapAt extends TaintFunction {
+class StdMapAt extends MemberFunction {
   StdMapAt() { this.getClassAndName(["at", "operator[]"]) instanceof MapOrUnorderedMap }
+}
 
+private class StdMapAtModels extends StdMapAt, TaintFunction {
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     // flow from qualifier to referenced return value
     input.isQualifierObject() and

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

@@ -0,0 +1,53 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+Using an iterator owned by a container after the lifetime of the container has expired can lead to undefined behavior.
+This is because the iterator may be invalidated when the container is destroyed, and dereferencing an invalidated iterator is undefined behavior.
+These problems can be hard to spot due to C++'s complex rules for temporary object lifetimes and their extensions.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Never create an iterator to a temporary container when the iterator is expected to be used after the container's lifetime has expired.
+</p>
+
+</recommendation>
+<example>
+<p>
+
+</p>
+
+<p>
+The rules for lifetime extension ensures that the code in <code>lifetime_of_temp_extended</code> is well-defined. This is because the
+lifetime of the temporary container returned by <code>get_vector</code> is extended to the end of the loop. However, prior to C++23,
+the lifetime extension rules do not ensure that the container returned by <code>get_vector</code> is extended in <code>lifetime_of_temp_not_extended</code>.
+This is because the temporary container is not bound to a rvalue reference.
+</p>
+<sample src="IteratorToExpiredContainerExtendedLifetime.cpp" />
+
+</example>
+<references>
+
+<li>CERT C Coding Standard:
+<a href="https://wiki.sei.cmu.edu/confluence/display/c/MEM30-C.+Do+not+access+freed+memory">MEM30-C. Do not access freed memory</a>.</li>
+<li>
+OWASP:
+<a href="https://owasp.org/www-community/vulnerabilities/Using_freed_memory">Using freed memory</a>.
+</li>
+<li>
+<a href="https://github.com/isocpp/CppCoreGuidelines/blob/master/docs/Lifetime.pdf">Lifetime safety: Preventing common dangling</a>
+</li>
+<li>
+<a href="https://en.cppreference.com/w/cpp/container">Containers library</a>
+</li>
+<li>
+<a href="https://en.cppreference.com/w/cpp/language/range-for">Range-based for loop (since C++11)</a>
+</li>
+
+</references>
+</qhelp>

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

@@ -0,0 +1,103 @@
+/**
+ * @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
+ * @id cpp/iterator-to-expired-container
+ * @problem.severity warning
+ * @tags reliability
+ *       security
+ *       external/cwe/cwe-416
+ *       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
+
+/**
+ * A configuration to track flow from a temporary variable to the qualifier of
+ * a destructor call
+ */
+module TempToDestructorConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source.asInstruction().(VariableAddressInstruction).getIRVariable() instanceof IRTempVariable
+  }
+
+  predicate isSink(DataFlow::Node sink) {
+    sink.asOperand().(ThisArgumentOperand).getCall().getStaticCallTarget() instanceof Destructor
+  }
+}
+
+module TempToDestructorFlow = DataFlow::Global<TempToDestructorConfig>;
+
+/**
+ * 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
+ * destroyed by a call to a destructor.
+ *
+ * For the latter case, consider something like:
+ * ```
+ * std::vector<std::vector<int>> get_2d_vector();
+ * auto& v = get_2d_vector()[0];
+ * ```
+ * Given the above, this predicate returns the node corresponding
+ * to `get_2d_vector()[0]` since the temporary `get_2d_vector()` gets
+ * destroyed by a call to `std::vector<std::vector<int>>::~vector`,
+ * 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()
+    or
+    exists(CallInstruction call |
+      result.asInstruction() = call and
+      DataFlow::localFlow(destroyedTemp.getNode(),
+        DataFlow::operandNode(call.getThisArgumentOperand()))
+    |
+      call.getStaticCallTarget() instanceof StdSequenceContainerAt or
+      call.getStaticCallTarget() instanceof StdMapAt
+    )
+  )
+}
+
+predicate isSinkImpl(DataFlow::Node sink, FunctionCall fc) {
+  exists(CallInstruction call |
+    call = sink.asOperand().(ThisArgumentOperand).getCall() and
+    fc = call.getUnconvertedResultExpression() and
+    call.getStaticCallTarget() instanceof BeginOrEndFunction
+  )
+}
+
+/**
+ * Flow from any destroyed object to the qualifier of a `begin` or `end` call
+ */
+module DestroyedToBeginConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source = getADestroyedNode() }
+
+  predicate isSink(DataFlow::Node sink) { isSinkImpl(sink, _) }
+
+  DataFlow::FlowFeature getAFeature() {
+    // By blocking argument-to-parameter flow we ensure that we don't enter a
+    // function body where the temporary outlives anything inside the function.
+    // This prevents false positives in cases like:
+    // ```cpp
+    // void foo(const std::vector<int>& v) {
+    //   for(auto x : v) { ... } // this is fine since v outlives the loop
+    // }
+    // ...
+    // foo(create_temporary())
+    // ```
+    result instanceof DataFlow::FeatureHasSinkCallContext
+  }
+}
+
+module DestroyedToBeginFlow = DataFlow::Global<DestroyedToBeginConfig>;
+
+from DataFlow::Node source, DataFlow::Node sink, FunctionCall beginOrEnd
+where DestroyedToBeginFlow::flow(source, sink) and isSinkImpl(sink, beginOrEnd)
+select source, "This object is destroyed before $@ is called.", beginOrEnd, beginOrEnd.toString()

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

@@ -0,0 +1,20 @@
+#include <vector>
+
+std::vector<int> get_vector();
+
+void use(int);
+
+void lifetime_of_temp_extended() {
+  for(auto x : get_vector()) {
+    use(x); // GOOD: The lifetime of the vector returned by `get_vector()` is extended until the end of the loop.
+  }
+}
+
+// Writes the the values of `v` to an external log and returns it unchanged.
+const std::vector<int>& log_and_return_argument(const std::vector<int>& v);
+
+void lifetime_of_temp_not_extended() {
+  for(auto x : log_and_return_argument(get_vector())) {
+    use(x); // BAD: The lifetime of the vector returned by `get_vector()` is not extended, and the behavior is undefined.
+  }
+}

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-416/IteratorToExpiredContainer.expected

@@ -0,0 +1 @@
+experimental/Security/CWE/CWE-416/IteratorToExpiredContainer.ql