Rust: Query for dereferencing an invalid pointer

[geoffw0]

New query rust/access-invalid-pointer that spots dereferences of pointers that are invalid to dereference. There are tests for two general cases, but this query is only intended to catch the first one:

• (this query) dereference after a pointer is explicitly made invalid, for example by calling a dealloc function before dereferencing. Analogous to cpp/use-after-free.
  • an edge case we don't cover yet is when the deallocation is done through a distinct pointer to the same memory.
• (future query) dereference of a pointer after the lifetime of the thing it points two has (implicitly) ended.

TODO:

• DCA run
• code review
• docs review

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[github-actions]

QHelp previews:

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

Access of invalid pointer

Dereferencing an invalid or dangling pointer may cause 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 valid and points to the intended data. Code may need to be rearranged or additional checks added to ensure safety in all circumstances. If possible, rewrite the code using safe Rust types to avoid this kind of problem altogether.

Example

In the following example, std::ptr::drop_in_place is used to execute the destructor of an object. However, a pointer to that object is dereferenced later in the program, causing undefined behavior:

unsafe {
    std::ptr::drop_in_place(ptr); // executes the destructor of `*ptr`
}

// ...

unsafe {
    do_something(&*ptr); // BAD: dereferences `ptr`
}

In this case, undefined behavior can be avoided by rearranging the code so that the dereferencing comes before the call to std::ptr::drop_in_place:

unsafe {
    do_something(&*ptr); // GOOD: dereferences `ptr` while it is still valid
}

// ...

{
    std::ptr::drop_in_place(ptr); // executes the destructor of `*ptr`
}

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-476.
• Common Weakness Enumeration: CWE-825.

[geoffw0]

DCA:

• lots of new query sinks in all projects; this is unsurprising as dereferences are considered a sink for this query and they're quite common.
  • we should probably block flow past sinks to prevent excessive data flow and potentially reporting lots of similar results.
• 8 query results across 4 projects.
  • most are inside Drop implementations, which is not something the query design anticipated and I need to look into whether they're acceptable results.

[mchammer01]

@geoffw0 👋🏻 - approving on behalf of Docs.
Left a few minor suggestions. Feel free to ignore the ones you don't agree with 😅

[geoffw0]

@mchammer01 suggestions accepted, thank you.

I still need to decide what to do about the results inside Drop implementations, then this should be ready to merge.

[geoffw0]

Re: results inside Drop implementations, this was not the actual issue just typically where code exposing the issue occurred. The issue was that calls to drop act on an object, not a pointer, and we're not modelling that difference correctly. For now I have removed the drop model entirely so that we don't get spurious results. I would like to return to this at some point but it's not necessary for the initial version of this query, for which we have plenty of other correctly behaving models.

I'll do another DCA run to confirm we no longer get false positive results.

[geoffw0]

DCA

• there are still two FP results (down from eight); I've added a test exposing what's going on, and I can implement a workaround - but I think the results are only there because of the existing workaround in the sinks. So fixing that (planned as follow-up) seems like a better approach than introducing a second workaround. --- update: the test case is fixed and I've started another new DCA run to confirm.
• we get handful of new dataflow inconsistencies; "PostUpdateNode should not be the target of local flow"; but this is a drop in the bucket compared to existing inconsistencies. Inconsistencies are something we should spend some time on, but not in this PR.
• TL;DR: good enough.

[hvitved]

#19195 should hopefully remove the data flow inconsistencies.

[geoffw0]

I've merged in the fix for the consistency check. I think this PR is ready for approval now.

[hvitved]

The two empty DataFlowConsistency.expected files should be deleted.

[geoffw0]

Good point. Done.

[hvitved]

I have started a final DCA run.

[geoffw0]

DCA LGTM. There are 2 results, in both cases the sources and sinks are good but the flow misses an is_null() check that looks like it renders it safe. I've added a note to the issue for this query that we should add barriers, though frankly we probably need more coverage before this matters.