diff --git a/rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll b/rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll index 64b806331a66..c404f13b5314 100644 --- a/rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll +++ b/rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll @@ -47,20 +47,6 @@ module AccessAfterLifetime { valueScope(source.getTarget(), target, scope) } - /** - * 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 | - sourceValueScope(source, target, valueScope) and - accessScope = sink.asExpr().getEnclosingBlock() and - not mayEncloseOnStack(valueScope, accessScope) - ) - } - /** * Holds if `var` has scope `scope`. */ @@ -88,24 +74,6 @@ module AccessAfterLifetime { valueScope(value.(FieldExpr).getContainer(), target, scope) } - /** - * Holds if block `a` contains block `b`, in the sense that a stack allocated variable in - * `a` may still 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 mayEncloseOnStack(BlockExpr a, BlockExpr b) { - // `b` is a child of `a` - a = b.getEnclosingBlock*() - or - // propagate through function calls - exists(Call call | - mayEncloseOnStack(a, call.getEnclosingBlock()) and - call.getARuntimeTarget() = b.getEnclosingCallable() - ) - } - /** * A source that is a `RefExpr`. */ diff --git a/rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql b/rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql index edc22a86409b..2f2991678930 100644 --- a/rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql +++ b/rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql @@ -15,7 +15,7 @@ import rust import codeql.rust.dataflow.DataFlow import codeql.rust.dataflow.TaintTracking -import codeql.rust.security.AccessAfterLifetimeExtensions +import codeql.rust.security.AccessAfterLifetimeExtensions::AccessAfterLifetime import AccessAfterLifetimeFlow::PathGraph /** @@ -24,14 +24,14 @@ import AccessAfterLifetimeFlow::PathGraph */ module AccessAfterLifetimeConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node node) { - node instanceof AccessAfterLifetime::Source and + node instanceof Source and // exclude cases with sources in macros, since these results are difficult to interpret not node.asExpr().isFromMacroExpansion() and - AccessAfterLifetime::sourceValueScope(node, _, _) + sourceValueScope(node, _, _) } predicate isSink(DataFlow::Node node) { - node instanceof AccessAfterLifetime::Sink and + node instanceof Sink and // Exclude cases with sinks in macros, since these results are difficult to interpret not node.asExpr().isFromMacroExpansion() and // TODO: Remove this condition if it can be done without negatively @@ -40,13 +40,13 @@ module AccessAfterLifetimeConfig implements DataFlow::ConfigSig { exists(node.asExpr()) } - predicate isBarrier(DataFlow::Node barrier) { barrier instanceof AccessAfterLifetime::Barrier } + predicate isBarrier(DataFlow::Node barrier) { barrier instanceof Barrier } predicate observeDiffInformedIncrementalMode() { any() } Location getASelectedSourceLocation(DataFlow::Node source) { exists(Variable target | - AccessAfterLifetime::sourceValueScope(source, target, _) and + sourceValueScope(source, target, _) and result = [target.getLocation(), source.getLocation()] ) } @@ -54,6 +54,56 @@ module AccessAfterLifetimeConfig implements DataFlow::ConfigSig { module AccessAfterLifetimeFlow = TaintTracking::Global; +predicate sourceBlock(Source s, Variable target, BlockExpr be) { + AccessAfterLifetimeFlow::flow(s, _) and + sourceValueScope(s, target, be.getEnclosingBlock*()) +} + +predicate sinkBlock(Sink s, BlockExpr be) { + AccessAfterLifetimeFlow::flow(_, s) and + be = s.asExpr().getEnclosingBlock() +} + +private predicate tcStep(BlockExpr a, BlockExpr b) { + // propagate through function calls + exists(Call call | + a = call.getEnclosingBlock() and + call.getARuntimeTarget() = b.getEnclosingCallable() + ) +} + +private predicate isTcSource(BlockExpr be) { sourceBlock(_, _, be) } + +private predicate isTcSink(BlockExpr be) { sinkBlock(_, be) } + +/** + * Holds if block `a` contains block `b`, in the sense that a stack allocated variable in + * `a` may still 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 depending on the + * caller a variable in `f` or `g` may or may-not be on the stack during `b`). + */ +private predicate mayEncloseOnStack(BlockExpr a, BlockExpr b) = + doublyBoundedFastTC(tcStep/2, isTcSource/1, isTcSink/1)(a, b) + +/** + * 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`. + */ +predicate dereferenceAfterLifetime(Source source, Sink sink, Variable target) { + AccessAfterLifetimeFlow::flow(source, sink) and + sourceValueScope(source, target, _) and + not exists(BlockExpr beSource, BlockExpr beSink | + sourceBlock(source, target, beSource) and + sinkBlock(sink, beSink) + | + beSource = beSink + or + mayEncloseOnStack(beSource, beSink) + ) +} + from AccessAfterLifetimeFlow::PathNode sourceNode, AccessAfterLifetimeFlow::PathNode sinkNode, Variable target @@ -61,6 +111,6 @@ 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) + dereferenceAfterLifetime(sourceNode.getNode(), sinkNode.getNode(), target) select sinkNode.getNode(), sourceNode, sinkNode, "Access of a pointer to $@ after its lifetime has ended.", target, target.toString()