diff --git a/rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll b/rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll index 06438fef0c8f..1c2ca95d3342 100644 --- a/rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll +++ b/rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll @@ -4,7 +4,9 @@ */ import rust +private import codeql.rust.Concepts private import codeql.rust.dataflow.DataFlow +private import codeql.rust.dataflow.FlowSink private import codeql.rust.security.AccessInvalidPointerExtensions private import codeql.rust.internal.Type private import codeql.rust.internal.TypeInference as TypeInference @@ -34,6 +36,13 @@ module AccessAfterLifetime { */ class Sink = AccessInvalidPointer::Sink; + // /** + // * A data flow sink for accesses to a pointer after its lifetime has ended, + // * that is, a dereference. + // */ + // abstract class Sink extends QuerySink::Range { + // override string getSinkType() { result = "AccessAfterLifetime" } + // } /** * A barrier for accesses to a pointer after its lifetime has ended. */ @@ -112,7 +121,10 @@ module AccessAfterLifetime { private class RefExprSource extends Source { Expr targetValue; - RefExprSource() { this.asExpr().(RefExpr).getExpr() = targetValue } + RefExprSource() { + this.asExpr().(RefExpr).getExpr() = targetValue and + this.asExpr().(RefExpr).isRaw() + } override Expr getTarget() { result = targetValue } } diff --git a/rust/ql/lib/codeql/rust/security/AccessInvalidPointerExtensions.qll b/rust/ql/lib/codeql/rust/security/AccessInvalidPointerExtensions.qll index b8b40ffa2578..ad4a5cbda56d 100644 --- a/rust/ql/lib/codeql/rust/security/AccessInvalidPointerExtensions.qll +++ b/rust/ql/lib/codeql/rust/security/AccessInvalidPointerExtensions.qll @@ -4,12 +4,15 @@ */ import rust +private import codeql.rust.elements.Call private import codeql.rust.dataflow.DataFlow private import codeql.rust.dataflow.FlowSource private import codeql.rust.dataflow.FlowSink private import codeql.rust.Concepts private import codeql.rust.dataflow.internal.Node private import codeql.rust.security.Barriers as Barriers +private import codeql.rust.internal.TypeInference as TypeInference +private import codeql.rust.internal.Type /** * Provides default sources, sinks and barriers for detecting accesses to @@ -47,20 +50,58 @@ module AccessInvalidPointer { ModelsAsDataSource() { sourceNode(this, "pointer-invalidate") } } - /** - * A pointer access using the unary `*` operator. - */ + /** A raw pointer access using the unary `*` operator. */ private class DereferenceSink extends Sink { - DereferenceSink() { any(DerefExpr p).getExpr() = this.asExpr() } + DereferenceSink() { + exists(Expr p, DerefExpr d | p = d.getExpr() and p = this.asExpr() | + // Dereferencing a raw pointer is an unsafe operation. Hence relevant + // dereferences must occur inside code marked as unsafe. + // See: https://doc.rust-lang.org/reference/types/pointer.html#r-type.pointer.raw.safety + (p.getEnclosingBlock*().isUnsafe() or p.getEnclosingCallable().(Function).isUnsafe()) and + (not exists(TypeInference::inferType(p)) or TypeInference::inferType(p) instanceof PtrType) + ) + } } - /** - * A pointer access from model data. - */ + /** A pointer access from model data. */ private class ModelsAsDataSink extends Sink { ModelsAsDataSink() { sinkNode(this, "pointer-access") } } + private class BarrierCall extends Barrier { + BarrierCall() { + exists(Call call, ArgumentPosition pos, string canonicalName | + call.getStaticTarget().getCanonicalPath() = canonicalName and + this.asExpr() = call.getArgument(pos) + | + canonicalName = "::new" and pos.asPosition() = 0 + ) + } + } + + private class NumericTypeBarrier extends Barrier instanceof Barriers::NumericTypeBarrier { } + + private class BooleanTypeBarrier extends Barrier instanceof Barriers::BooleanTypeBarrier { } + + private class FieldlessEnumTypeBarrier extends Barrier instanceof Barriers::FieldlessEnumTypeBarrier + { } + + private class DefaultBarrier extends Barrier { + DefaultBarrier() { + // A barrier for calls that statically resolve to the `Default::default` + // trait function. Such calls are imprecise, and can always resolve to the + // implementations for raw pointers that return a null pointer. This + // creates many false positives in combination with other inaccuracies + // (too many `pointer-access` sinks created by the model generator). + // + // We could try removing this barrier in the future when either 1/ the + // model generator creates fewer spurious sinks or 2/ data flow for calls + // to trait functions is more precise. + this.asExpr().(Call).getStaticTarget().getCanonicalPath() = + "<_ as core::default::Default>::default" + } + } + /** * A barrier for invalid pointer access vulnerabilities for values checked to * be non-`null`. diff --git a/rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql b/rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql index b9bf80c94749..dd5bb816d8f8 100644 --- a/rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql +++ b/rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql @@ -26,7 +26,8 @@ module AccessAfterLifetimeConfig implements DataFlow::ConfigSig { predicate isSource(DataFlow::Node node) { node instanceof AccessAfterLifetime::Source and // exclude cases with sources in macros, since these results are difficult to interpret - not node.asExpr().isFromMacroExpansion() + not node.asExpr().isFromMacroExpansion() and + AccessAfterLifetime::sourceValueScope(node, _, _) } predicate isSink(DataFlow::Node node) { @@ -36,7 +37,8 @@ module AccessAfterLifetimeConfig implements DataFlow::ConfigSig { // include only results inside `unsafe` blocks, as other results tend to be false positives ( node.asExpr().getEnclosingBlock*().isUnsafe() or - node.asExpr().getEnclosingCallable().(Function).isUnsafe() + node.asExpr().getEnclosingCallable().(Function).isUnsafe() or + not exists(node.asExpr()) ) } diff --git a/rust/ql/src/queries/summary/Stats.qll b/rust/ql/src/queries/summary/Stats.qll index d06389fb6a77..64368db22b82 100644 --- a/rust/ql/src/queries/summary/Stats.qll +++ b/rust/ql/src/queries/summary/Stats.qll @@ -18,6 +18,7 @@ private import codeql.rust.security.SensitiveData private import TaintReach // import all query extensions files, so that all extensions of `QuerySink` are found private import codeql.rust.security.regex.RegexInjectionExtensions +private import codeql.rust.security.AccessAfterLifetimeExtensions private import codeql.rust.security.AccessInvalidPointerExtensions private import codeql.rust.security.CleartextLoggingExtensions private import codeql.rust.security.CleartextStorageDatabaseExtensions