Discussion: Function-level reachability of vulnerabilities

[xizheyin]

I’ve been working on a research prototype around Rust vulnerability reachability, and I recently turned part of it into a tool. This tool complements cargo-audit by checking, at the function call level, whether vulnerabilities could actually affect the crate being analyzed. The analysis is based on MIR. As a prototype, there may be some inaccuracies, but I’ve included as many calling patterns as possible. This does not perform a precise pointer analysis; instead, it tends to flag all suspicious call chains. An example is v_frame of version 0.3.2, which depends on maligned 0.2.1.

cargo audit reports:

Crate:     maligned
Version:   0.2.1
Warning:   unsound
Title:     `maligned::align_first` causes incorrect deallocation
ID:        RUSTSEC-2023-0017
Dependency tree:
maligned 0.2.1
└── v_frame 0.3.2

The tool reports:

Found 1 advisories:

✗ VULNERABLE RUSTSEC-2023-0017
  Package: maligned 0.2.1
  Title: `maligned::align_first` causes incorrect deallocation
  URL: https://github.com/tylerhawkes/maligned/issues/5
  Affected functions:
    - maligned::align_first
    - maligned::align_first_boxed
    - maligned::align_first_boxed_cloned
    - maligned::align_first_boxed_default
  Call chains:
    → frame::Frame::<T>::new_with_padding -> plane::Plane::<T>::new -> plane::PlaneData::<T>::new -> maligned::align_first_boxed_cloned::<T, maligned::A64> -> maligned::align_first_boxed::<T, maligned::A64, {closure@maligned::align_first_boxed_cloned<T, maligned::A64>::{closure#0}}> -> maligned::align_first::<T, maligned::A64>
    → plane::PlaneData::<T>::new -> maligned::align_first_boxed_cloned::<T, maligned::A64> -> maligned::align_first_boxed::<T, maligned::A64, {closure@maligned::align_first_boxed_cloned<T, maligned::A64>::{closure#0}}> -> maligned::align_first::<T, maligned::A64>
    → plane::Plane::<T>::new -> plane::PlaneData::<T>::new -> maligned::align_first_boxed_cloned::<T, maligned::A64> -> maligned::align_first_boxed::<T, maligned::A64, {closure@maligned::align_first_boxed_cloned<T, maligned::A64>::{closure#0}}> -> maligned::align_first::<T, maligned::A64>
 
...(many other call chains)

  Description: `maligned::align_first` manually allocates with an alignment larger than T, and then uses `Vec::from_raw_parts` on that allocation to get a `Vec<T>`.  [`GlobalAlloc::dealloc`](https://doc.rust-lang.org/std/alloc/trait.GlobalAlloc.html#tymethod.dealloc) requires that the `layout` argument must be the same layout that was used to allocate that block of memory....

when no reachable call path is found, it will report No call chains.
Does this tool seem useful to the RustSec ecosystem?

[djc]

Does this tool seem useful to the RustSec ecosystem?

It definitely seems interesting. Not sure what you think the implication of that answer to your question should be?

[tarcieri]

False positive elimination is one of our oldest issues (#21), and I think we have problems with alert fatigue, so this is definitely nice to see. Starting with "No call chains" seems like a reasonable first step versus actually hiding the report, and we definitely need some way to lint the data we already have about affected functions, but this seems like some interesting first steps towards that goal.

You might borrow some ideas from RUST_BACKTRACE, only showing the first immediate call site, but informing the user they can pass something (additional argument, environment variable, whatever) to get the full call chain

[xizheyin]

Not sure what you think the implication of that answer to your question should be?

I’m trying to understand what improvements would make this tool useful to the RustSec ecosystem, rather than just as a prototype.

You might borrow some ideas from RUST_BACKTRACE, only showing the first immediate call site, but informing the user they can pass something (additional argument, environment variable, whatever) to get the full call chain.

That’s a good suggestion, I will try it; it would make it much more user-friendly.

If this project has any interesting practical applications, I’d be happy to help.

[djc]

Not sure what you think the implication of that answer to your question should be?

I’m trying to understand what improvements would make this tool useful to the RustSec ecosystem, rather than just as a prototype.

I think you should do some marketing for your tool (maybe advertise it on the /r/rust reddit and/or the users forum?) and get initial feedback from interested users. That might help out weed out teething issues.

I guess we could also review a PR to enable opt-in support in cargo-audit, with a longer-term goal to make it opt-out instead?

[xizheyin]

Based on the discussion above, I made some changes and created a one-click installation script. I think it’s too early to include this as an option in cargo-audit for now. Perhaps we could keep it as a standalone program for now and label it as an experimental project in README.md of cargo-audit. This might help us gather some feedback. What do you think?

I've reorganized the code and put it in a new repo: https://github.com/xizheyin/reachsec

If the vulnurability has no affected functions(from Rustsec) or no call chains are found, Analysis field will print the cause, like the figure below.

This figure presents the case with so many call chains, we can use option to control --max-call-chains or --show-all-call-chains, default up to 5.

[djc]

Based on the discussion above, I made some changes and created a one-click installation script. I think it’s too early to include this as an option in cargo-audit for now. Perhaps we could keep it as a standalone program for now and label it as an experimental project in README.md of cargo-audit. This might help us gather some feedback. What do you think?

I've reorganized the code and put it in a new repo: https://github.com/xizheyin/reachsec

Open to a PR that adds a note about this in our README.

Since this currently (AIUI) doesn't fully replace existing tooling, suggest to trim down the output to focus on the parts that are unique to your analysis, and prioritize it in the output (like by renaming "Analysis" to "Affected" and listing it as the first label).

Also suggest putting some screenshots like this front and center in your README.