Pre-RFC: Reviving "Security advisories in crates.io" (RFC PR #1752)

Ok, so, after thinking about this for awhile, I have a new proposal. How about the thing @alexcrichton suggested 3 years ago that the core team keeps telling us we should consider? :stuck_out_tongue_winking_eye:

If a crate is fixed for a security reason, the old versions can be yanked and the new version can be suggested

Good idea @alexcrichton!

So how about this: each cargo yank event has associated metadata, in the form of a TOML file. If we allow this data to be mutable, it can be backfilled for existing yank events if crate owners so desire.

Here is what I would like for RustSec's purposes:

reason="security"
description="MsQueue and SegQueue suffer from double-free"

[advisory]
id = "RUSTSEC-2018-0009"

For the moment I'd like to gloss over whatever sequence of events would allow this to happen, but here is the user experience I have in mind. Imagine you type cargo build (no cargo update required) on your project. You see this:

$ cargo build
[...]
warning: Cargo.lock contains 1 crate with security advisory: crossbeam (RUSTSEC-2018-0009)
help: run `cargo install cargo-audit` and then `cargo audit` to determine potential solutions

Not particularly sure how that will scale to large numbers of vulnerable crates or complex combinations of reasons (really I think the only other reason would be something like bug.

The reasons I really like this particular approach:

It provides what I think is the minimally obtrusive path to integrating with cargo, and also visibility and discovery for RustSec in general, but also keeps cargo audit completely decoupled from cargo, and therefore nicely sidesteps questions like "how do we integrate the rustsec crate into cargo?".

I think it's nice for RustSec to stay decoupled because there are many ways we could potentially extend cargo audit, for example using a tool like RustPräzi to determine if a project actually uses vulnerable codepaths of a dependency, thereby eliminating false positives when the parent project doesn't call any of the vulnerable code. To pull this off we might need cargo audit to talk to a RustSec-as-a-service application (which is already live at https://crates.rustsec.org). We might try many experiments, and perhaps many of them will fail, but I think this will be significantly harder if we're blocked on getting through the cargo code review process.

One important thing to pay attention to is that [advisory] is only the id. This is actually for an important reason: vulnerabilities can span multiple versions of a crate. Before I had suggested using yank metadata was inappropriate for that particular reason. However, if we just store the advisory ID, we have all the information we need to display a short message to the user (including one which could be integrated into https://crates.io as well) but avoid duplicating the extended advisory metadata across the yank events.

In other words, we make cargo and https://crates.io aware of just enough advisory metadata to display useful information intended to guide the user to cargo audit (and in the case of https://crates.io, display a short message about the advisory and link to the https://rustsec.org web site)

If we can do this, it would also enable us to remove all VersionReq-related data from advisories (e.g. affected_versions, unaffected_versions), which would be fantastic because this has been an ongoing pain point:

https://github.com/RustSec/rustsec-crate/issues/51

Instead of using a bunch of VersionReq expressions to try to declare which crates are vulnerable, patched, or never impacted in the first place, cargo audit can just read the metadata for all yanked versions of crates it's auditing, and then it would know exactly which versions are vulnerable in a way which matches what crates are yanked because they are the same mechanism.

This allows for an MVP of a cargo integration which is also dramatically reduced in scope from the original OP, but is also generally useful for all cargo yank events and would also close a longstanding cargo issue. To me that's great because I think it dramatically increases the chance of successfully shipping.

Perhaps in the future we could circle back on something closer to the vision in the OP (or what npm provides today), where all of RustSec's functionality is integrated directly into cargo. Or maybe cargo audit would make more sense as a rustup component. Either way, we wouldn't have to decide those things today and can just keep those ideas in our pocket, and perhaps focus that energy instead on questions like:

  • What exact messages should cargo display to the user, and when?
  • How much information is too much, or too little?
  • In what ways could we potentially surface this information on https://crates.io (see also rust-secure-code/wg#16)
12 Likes