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

**URL:** <https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017>\
**Category:** cargo\
**Created:** [December 13, 2018, 8:46pm UTC](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017 "2018-12-13T20:46:39Z")\
**Posts on this page:** 1\
**Showing post:** 14

<div class="post-metadata">

**Author:** ![bascule](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/bascule/32/3057_2.png) [@bascule](https://internals.rust-lang.org/u/bascule)\
**Post date:** [January 18, 2019, 7:45am UTC](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017/14 "2019-01-18T07:45:32Z")

</div>

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? 😜

> <https://github.com/rust-lang/cargo/issues/2608>
>
> @wycats, @nikomatsakis and I have discussed recently the ability to yank a crate… with a reason, and then whenever that yanked crate is compiled locally it will present the reason as a warning.
> 
> The intended use cases for this would be:
> \- When a crate is fixed because it will be broken in the next version of the compiler (e.g. a soundness fix or bug fix) then the previous versions can be yanked and nudge users forward.
> \- If a crate is fixed for a security reason, the old versions can be yanked and the new version can be suggested
> \- If a crate is renamed (or perhaps deprecated) to another then the yank message can indicate what to do in that situation.
> 
> Technically, I believe this should be implemented through:
> 1. Update crates.io to accept an optional message for yanks. Store it in the database.
> 2. Update \`cargo yank\` to \*\*require a message\*\* indicating why the crate is being yanked.
> 3. Change the logic for \*\*when a yanked crate is compiled\*\* to something like:
> - Use whatever state the registry is currently in, do not update it.
> - Only issue a warning if the crate is compiled, not if it is "fresh" (e.g. has already been compiled).
> - When compilation starts, fire off an asynchronous HTTP request to crates.io to fetch the yank message.
> - Print out the warning whenever the HTTP request finishes or block the build finishing on these HTTP requests to return.
> - Ignore any errors that happen fetching the yank message, they should not fail the build.
> 
> I think this should suffice as an "opportunistic nudge" for these situations where possible, although it's certainly no form of ironclad guarantee.
> 
> Thoughts @wycats, @nikomatsakis?

> 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:

```toml
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:

```rust
$ 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](https://github.com/RustSec/advisory-db/issues/68), 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](https://github.com/rust-secure-code/wg/issues/13) (which is already live at [https://crates.rustsec.org](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](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](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](https://crates.io), display a short message about the advisory and link to the [https://rustsec.org](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/dtolnay/semver/issues/172>
>
> This test fails:
> 
> \`\`\`diff
> diff --git a/tests/regression.rs b/tests/regression….rs
> index a47a777..041bd7c 100644
> \--- a/tests/regression.rs
> +++ b/tests/regression.rs
> @@ -28,3 +28,22 @@ fn test\_regressions() {
> }
> }
> }
> +
> +// See https://github.com/RustSec/cargo-audit/issues/17
> +#\[test\]
> +fn test\_suffix() {
> + let req = semver::VersionReq::parse("\>= 0.10.2").unwrap();
> + let version = semver::Version::parse("0.12.0-pre.0").unwrap();
> +
> + // Test parsing
> + assert\_eq!(version.major, 0);
> + assert\_eq!(version.minor, 12);
> + assert\_eq!(version.patch, 0);
> + assert\_eq!(version.pre, vec!\[
> + semver::Identifier::AlphaNumeric("pre".into()),
> + semver::Identifier::Numeric(0),
> + \]);
> +
> + // Test matching
> + assert!(req.matches(&version));
> +}
> \`\`\`
> 
> It matches the req if I remove the "-pre.0" suffix though.
> 
> This causes cargo-audit to \[report wrong vulnerabilities\](https://github.com/RustSec/cargo-audit/issues/17).
> 
> I assume this is a bug, and does not follow the spec, right?

[https://github.com/RustSec/rustsec-crate/issues/51](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](https://crates.io) (see also [rust-secure-code/wg#16](https://github.com/rust-secure-code/wg/issues/16))

---

_[View the full topic](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017)._