Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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 }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this check is better here, in the sink definition, or in the place where the query uses the sink (as it is for rust/access-after-lifetime-ended)? Should we be consistent between the two?

(not exists(TypeInference::inferType(p)) or TypeInference::inferType(p) instanceof PtrType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm interested to see if this PtrType restriction effects real world [edge case] results (DCA and/or MRVA).

)
}
}

/**
* 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 = "<core::ptr::non_null::NonNull>::new" and pos.asPosition() = 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a barrier or a sink???

Either way I'd like to see a test case for this.

)
}
}

private class NumericTypeBarrier extends Barrier instanceof Barriers::NumericTypeBarrier { }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this end up blocking legitimate results where a null value is cast through an integer type and back, perhaps for integration with C-like interfaces or something? Admittedly literal 0 isn't currently a source for this query, so we don't really support this case as it is now.


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"
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see an example (test case) for this as well. I'm not quite clear what it looks like.

}

/**
* A barrier for invalid pointer access vulnerabilities for values checked to
* be non-`null`.
Expand Down
6 changes: 4 additions & 2 deletions rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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())
)
}

Expand Down
1 change: 1 addition & 0 deletions rust/ql/src/queries/summary/Stats.qll
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down