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

[geoffw0]

New query rust/access-after-lifetime-ended, for detecting pointer dereferences after the lifetime of the pointed-to object has ended. Makes use of some existing tests that were created for rust/access-invalid-pointer (before I realized that the idea for that query needed breaking into two separate queries). Also adds quite a lot of new test cases as well.

Note that the query is currently @precision medium due to several remaining false positive results in the tests (and in MRVA results). I made some effort to fix these, but I didn't get them all, I feel it's time to get what we have merged and plan further improvements as follow-up work.

Before merging:

• QL review
• docs review
• DCA run

[github-actions]

QHelp previews:

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

Access of a pointer after its lifetime has ended

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.

Recommendation

When dereferencing a pointer in unsafe 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.

Example

In the following example, val is local to get_pointer so its lifetime ends when that function returns. However, a pointer to val is returned and dereferenced after that lifetime has ended, causing undefined behavior:

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
	}

	// ...
}

One way to fix this is to change the return type of the function from a pointer to a Box, which ensures that the value it points to remains on the heap for the lifetime of the Box itself. Note that there is no longer a need for an unsafe block as the code no longer handles pointers directly:

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

	// ...
}

References

• Rust Documentation: Behavior considered undefined >> Dangling pointers.
• Rust Documentation: Module ptr - Safety.
• Massachusetts Institute of Technology: Unsafe Rust - Dereferencing a Raw Pointer.
• Common Weakness Enumeration: CWE-825.

[Copilot]

Pull Request Overview

Adds a new CodeQL query to detect pointer dereferences after the lifetime of their target has ended, including examples, documentation, and scope‐tracking support.

• Introduce AccessAfterLifetime.ql with configuration, flow definition, and filtering logic
• Add QL test harness entries (.qlref), examples (Good.rs/Bad.rs), and documentation (.qhelp)
• Extend the internal QL library to track enclosing blocks (getEnclosingBlock) and support the new query

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
rust/ql/test/query-tests/security/CWE-825/options.yml	Add futures dependency for async tests
rust/ql/test/query-tests/security/CWE-825/main.rs	Invoke newly added test cases in main()
rust/ql/test/query-tests/security/CWE-825/deallocation.rs	Update existing invalid-pointer test annotations
rust/ql/test/query-tests/security/CWE-825/AccessAfterLifetime.qlref	Register the new query with test harness
rust/ql/src/queries/security/CWE-825/AccessAfterLifetimeGood.rs	Add good example using Box
rust/ql/src/queries/security/CWE-825/AccessAfterLifetimeBad.rs	Add bad example that returns a dangling pointer
rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql	Implement the new query logic
rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.qhelp	Add documentation and examples
rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll	Define sources, sinks, barriers, and scope checks
rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll	Add Variable.getEnclosingBlock()
rust/ql/lib/codeql/rust/elements/internal/AstNodeImpl.qll	Add AstNode.getEnclosingBlock()

Comments suppressed due to low confidence (1)

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

• The predicate isFromMacroExpansion() does not exist; you likely meant isInMacroExpansion() to filter out macro expansions.

not sinkNode.getNode().asExpr().getExpr().isFromMacroExpansion()

[geoffw0]

DCA:

• 95 results for the new query; that's quite a lot...
  • confusing results that have sources in macros - fixed ✅
  • results matching self; I think this kind of thing is probably OK: match self { EnumValue(x) => &x.y }
    • but the query is @precision medium, so deferring that for now should be OK I think
• otherwise LGTM

[mchammer01]

@geoffw0 - LGTM ✨
Added a couple of very minor comments/suggestions.

[geoffw0]

Thanks for the review @mchammer01 , I've accepted both suggestions. :)

[hvitved]

No need to cache.

[geoffw0]

Updated, thanks.

[paldepind]

Looks really great to me! Lots of true positives.

[paldepind]

Suggested change

	return &val;
	&val

[paldepind]

Suggested change

	return Box::new(val); // copies `val` onto the heap, where it remains for the lifetime of the `Box`.
	Box::new(val) // copies `val` onto the heap, where it remains for the lifetime of the `Box`.

[paldepind]

An idea for getting rid of this false positive could be to change DereferenceSink such that only dereferences inside unsafe blocks are considered.

[paldepind]

This predicate name threw me a bit off. I initially thought it would be something about heap allocated vs stack allocated variables.

This changes the name and tries to tweak the documentation a bit. Of course if you have a better idea then feel free to use that instead :)

Suggested change

	* 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) {
	* 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 blockStackEnclosing(BlockExpr a, BlockExpr b) {

[paldepind]

It's a bit confusing that there is two vals here and the lifetime of the val 5 lines up has not ended. Just renaming one one them would help I think.