Rust: New query rust/access-after-lifetime-ended

rust/ql/integration-tests/query-suite/rust-security-and-quality.qls.expected

@@ -16,6 +16,7 @@ ql/rust/ql/src/queries/security/CWE-327/BrokenCryptoAlgorithm.ql
 ql/rust/ql/src/queries/security/CWE-328/WeakSensitiveDataHashing.ql
 ql/rust/ql/src/queries/security/CWE-696/BadCtorInitialization.ql
 ql/rust/ql/src/queries/security/CWE-770/UncontrolledAllocationSize.ql
+ql/rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql
 ql/rust/ql/src/queries/security/CWE-825/AccessInvalidPointer.ql
 ql/rust/ql/src/queries/summary/LinesOfCode.ql
 ql/rust/ql/src/queries/summary/LinesOfUserCode.ql

rust/ql/integration-tests/query-suite/rust-security-extended.qls.expected

@@ -15,6 +15,7 @@ ql/rust/ql/src/queries/security/CWE-312/CleartextLogging.ql
 ql/rust/ql/src/queries/security/CWE-327/BrokenCryptoAlgorithm.ql
 ql/rust/ql/src/queries/security/CWE-328/WeakSensitiveDataHashing.ql
 ql/rust/ql/src/queries/security/CWE-770/UncontrolledAllocationSize.ql
+ql/rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql
 ql/rust/ql/src/queries/security/CWE-825/AccessInvalidPointer.ql
 ql/rust/ql/src/queries/summary/LinesOfCode.ql
 ql/rust/ql/src/queries/summary/LinesOfUserCode.ql

rust/ql/lib/codeql/rust/elements/internal/AstNodeImpl.qll

@@ -59,6 +59,17 @@ module Impl {
       )
     }
 
+    /** Gets the block that encloses this node, if any. */
+    cached
+    BlockExpr getEnclosingBlock() {
+      exists(AstNode p | p = this.getParentNode() |
+        result = p
+        or
+        not p instanceof BlockExpr and
+        result = p.getEnclosingBlock()
+      )
+    }
+
     /** Holds if this node is inside a macro expansion. */
     predicate isInMacroExpansion() { MacroCallImpl::isInMacroExpansion(_, this) }
 

rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll

@@ -127,6 +127,10 @@ module Impl {
      */
     Name getName() { variableDecl(definingNode, result, text) }
 
+    /** Gets the block that encloses this variable, if any. */
+    cached
+    BlockExpr getEnclosingBlock() { result = definingNode.getEnclosingBlock() }
+
     /** Gets the `self` parameter that declares this variable, if any. */
     SelfParam getSelfParam() { result.getName() = this.getName() }
 

rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll

@@ -0,0 +1,119 @@
+/**
+ * Provides classes and predicates for reasoning about accesses to a pointer
+ * after its lifetime has ended.
+ */
+
+import rust
+private import codeql.rust.dataflow.DataFlow
+private import codeql.rust.security.AccessInvalidPointerExtensions
+private import codeql.rust.internal.Type
+private import codeql.rust.internal.TypeInference as TypeInference
+
+/**
+ * Provides default sources, sinks and barriers for detecting accesses to a
+ * pointer after its lifetime has ended, as well as extension points for
+ * adding your own. Note that a particular `(source, sink)` pair must be
+ * checked with `dereferenceAfterLifetime` to determine if it is a result.
+ */
+module AccessAfterLifetime {
+  /**
+   * A data flow source for accesses to a pointer after its lifetime has ended,
+   * that is, creation of a pointer or reference.
+   */
+  abstract class Source extends DataFlow::Node {
+    /**
+     * Gets the value this pointer or reference points to.
+     */
+    abstract Expr getTarget();
+  }
+
+  /**
+   * A data flow sink for accesses to a pointer after its lifetime has ended,
+   * that is, a dereference. We re-use the same sinks as for the accesses to
+   * invalid pointers query.
+   */
+  class Sink = AccessInvalidPointer::Sink;
+
+  /**
+   * A barrier for accesses to a pointer after its lifetime has ended.
+   */
+  abstract class Barrier extends DataFlow::Node { }
+
+  /**
+   * Holds if the pair `(source, sink)`, that represents a flow from a
+   * pointer or reference to a dereference, has its dereference outside the
+   * lifetime of the target variable `target`.
+   */
+  bindingset[source, sink]
+  predicate dereferenceAfterLifetime(Source source, Sink sink, Variable target) {
+    exists(BlockExpr valueScope, BlockExpr accessScope |
+      valueScope(source.getTarget(), target, valueScope) and
+      accessScope = sink.asExpr().getExpr().getEnclosingBlock() and
+      not maybeOnStack(valueScope, accessScope)
+    )
+  }
+
+  /**
+   * Holds if `var` has scope `scope`.
+   */
+  private predicate variableScope(Variable var, BlockExpr scope) {
+    // local variable
+    scope = var.getEnclosingBlock()
+    or
+    // parameter
+    exists(Callable c |
+      var.getParameter().getEnclosingCallable() = c and
+      scope.getParentNode() = c
+    )
+  }
+
+  /**
+   * Holds if `value` accesses a variable `target` with scope `scope`.
+   */
+  private predicate valueScope(Expr value, Variable target, BlockExpr scope) {
+    // variable access (to a non-reference)
+    target = value.(VariableAccess).getVariable() and
+    variableScope(target, scope) and
+    not TypeInference::inferType(value) instanceof RefType
+    or
+    // field access
+    valueScope(value.(FieldExpr).getContainer(), target, scope)
+  }
+
+  /**
+   * Holds if block `a` contains block `b`, in the sense that a variable in
+   * `a` may be on the stack during execution of `b`. This is interprocedural,
+   * but is an overapproximation that doesn't accurately track call contexts
+   * (for example if `f` and `g` both call `b`, then then depending on the
+   * caller a variable in `f` or `g` may or may-not be on the stack during `b`).
+   */
+  private predicate maybeOnStack(BlockExpr a, BlockExpr b) {
+    // `b` is a child of `a`
+    a = b.getEnclosingBlock*()
+    or
+    // propagate through function calls
+    exists(CallExprBase ce |
+      maybeOnStack(a, ce.getEnclosingBlock()) and
+      ce.getStaticTarget() = b.getEnclosingCallable()
+    )
+  }
+
+  /**
+   * A source that is a `RefExpr`.
+   */
+  private class RefExprSource extends Source {
+    Expr targetValue;
+
+    RefExprSource() { this.asExpr().getExpr().(RefExpr).getExpr() = targetValue }
+
+    override Expr getTarget() { result = targetValue }
+  }
+
+  /**
+   * A barrier for nodes inside closures, as we don't model lifetimes of
+   * variables through closures properly.
+   */
+  private class ClosureBarrier extends Barrier {
+    ClosureBarrier() { this.asExpr().getExpr().getEnclosingCallable() instanceof ClosureExpr }
+  }
+}

rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.qhelp

@@ -0,0 +1,50 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+
+<p>
+Dereferencing a pointer after the lifetime of its target has ended causes undefined behavior. Memory
+may be corrupted, causing the program to crash or behave incorrectly, in some cases exposing the program
+to potential attacks.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+When dereferencing a pointer in <code>unsafe</code> code, take care that the pointer is still valid
+at the time it is dereferenced. Code may need to be rearranged or changed to extend lifetimes. If
+possible, rewrite the code using safe Rust types to avoid this kind of problem altogether.
+</p>
+
+</recommendation>
+<example>
+
+<p>
+In the following example, <code>val</code> is local to <code>get_pointer</code> so its lifetime
+ends when that function returns. However, a pointer to <code>val</code> is returned and dereferenced
+after that lifetime has ended, causing undefined behavior:
+</p>
+
+<sample src="AccessAfterLifetimeBad.rs" />
+
+<p>
+One way to fix this is to change the return type of the function from a pointer to a <code>Box</code>,
+which ensures that the value it points to remains on the heap for the lifetime of the <code>Box</code>
+itself. Note that there is no longer a need for an <code>unsafe</code> block as the code no longer
+handles pointers directly:
+</p>
+
+<sample src="AccessAfterLifetimeGood.rs" />
+
+</example>
+<references>
+
+<li>Rust Documentation: <a href="https://doc.rust-lang.org/reference/behavior-considered-undefined.html#dangling-pointers">Behavior considered undefined &gt;&gt; Dangling pointers</a>.</li>
+<li>Rust Documentation: <a href="https://doc.rust-lang.org/std/ptr/index.html#safety">Module ptr - Safety</a>.</li>
+<li>Massachusetts Institute of Technology: <a href="https://web.mit.edu/rust-lang_v1.25/arch/amd64_ubuntu1404/share/doc/rust/html/book/second-edition/ch19-01-unsafe-rust.html#dereferencing-a-raw-pointer">Unsafe Rust - Dereferencing a Raw Pointer</a>.</li>
+
+</references>
+</qhelp>

rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql

@@ -0,0 +1,47 @@
+/**
+ * @name Access of a pointer after its lifetime has ended
+ * @description Dereferencing a pointer after the lifetime of its target has ended
+ *              causes undefined behavior and may result in memory corruption.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 9.8
+ * @precision medium
+ * @id rust/access-after-lifetime-ended
+ * @tags reliability
+ *       security
+ *       external/cwe/cwe-825
+ */
+
+import rust
+import codeql.rust.dataflow.DataFlow
+import codeql.rust.dataflow.TaintTracking
+import codeql.rust.security.AccessAfterLifetimeExtensions
+import AccessAfterLifetimeFlow::PathGraph
+
+/**
+ * A data flow configuration for detecting accesses to a pointer after its
+ * lifetime has ended.
+ */
+module AccessAfterLifetimeConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) { node instanceof AccessAfterLifetime::Source }
+
+  predicate isSink(DataFlow::Node node) { node instanceof AccessAfterLifetime::Sink }
+
+  predicate isBarrier(DataFlow::Node barrier) { barrier instanceof AccessAfterLifetime::Barrier }
+}
+
+module AccessAfterLifetimeFlow = TaintTracking::Global<AccessAfterLifetimeConfig>;
+
+from
+  AccessAfterLifetimeFlow::PathNode sourceNode, AccessAfterLifetimeFlow::PathNode sinkNode,
+  Variable target
+where
+  // flow from a pointer or reference to the dereference
+  AccessAfterLifetimeFlow::flowPath(sourceNode, sinkNode) and
+  // check that the dereference is outside the lifetime of the target
+  AccessAfterLifetime::dereferenceAfterLifetime(sourceNode.getNode(), sinkNode.getNode(), target) and
+  // exclude cases with sources / sinks in macros, since these results are difficult to interpret
+  not sourceNode.getNode().asExpr().getExpr().isFromMacroExpansion() and
+  not sinkNode.getNode().asExpr().getExpr().isFromMacroExpansion()
+select sinkNode.getNode(), sourceNode, sinkNode,
+  "Access of a pointer to $@ after its lifetime has ended.", target, target.toString()

rust/ql/src/queries/security/CWE-825/AccessAfterLifetimeBad.rs

@@ -0,0 +1,19 @@
+
+fn get_pointer() -> *const i64 {
+	let val = 123;
+
+	return &val;
+} // lifetime of `val` ends here, the pointer becomes dangling
+
+fn example() {
+	let ptr = get_pointer();
+	let val;
+
+	// ...
+
+	unsafe {
+		val = *ptr; // BAD: dereferences `ptr` after the lifetime of `val` has ended
+	}
+
+	// ...
+}

rust/ql/src/queries/security/CWE-825/AccessAfterLifetimeGood.rs

@@ -0,0 +1,17 @@
+
+fn get_box() -> Box<i64> {
+	let val = 123;
+
+	return Box::new(val); // copies `val` onto the heap, where it remains for the lifetime of the `Box`.
+}
+
+fn example() {
+	let ptr = get_box();
+	let val;
+
+	// ...
+
+	val = *ptr; // GOOD
+
+	// ...
+}

rust/ql/test/extractor-tests/crate_graph/crates.expected

@@ -18,7 +18,7 @@
 #-----| core -> Crate([email protected])
 #-----| compiler_builtins -> Crate([email protected])
 
-#-----| Crate([email protected].0)
+#-----| Crate([email protected].1)
 #-----| proc_macro -> Crate([email protected])
 #-----| alloc -> Crate([email protected])
 #-----| core -> Crate([email protected])
@@ -89,7 +89,7 @@ main.rs:
 #-----| core -> Crate([email protected])
 #-----| std -> Crate([email protected])
 #-----| test -> Crate([email protected])
-#-----| cfg_if -> Crate([email protected].0)
+#-----| cfg_if -> Crate([email protected].1)
 #-----| digest -> Crate([email protected])
 
 #-----| Crate([email protected])