# Pre-RFC: Reviving "Security advisories in crates.io" (RFC PR #1752) **URL:** \ **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:** 15\ **Page:** 1
**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:** [December 13, 2018, 8:46pm UTC](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017/1 "2018-12-13T20:46:39Z")
Once upon a time in 2016 there was some discussion of a first-class security advisory feature for `cargo`, with a corresponding advisory database for crates published through [crates.io](http://crates.io): > [@\[pre-RFC\] Security advisories as part of crates.io metadata](https://internals.rust-lang.org/t/pre-rfc-security-advisories-as-part-of-crates-io-metadata/4045): > > This is a continuation of [Idea: Security advisories as part of crates.io metadata](https://internals.rust-lang.org/t/idea-security-advisories-as-part-of-crates-io-metadata/3899). The following text is a copy-paste of [https://github.com/untitaker/rfcs/blob/security-advisories/text/0000-security-advisories.md](https://github.com/untitaker/rfcs/blob/security-advisories/text/0000-security-advisories.md), the text on GitHub and this post will be updated as the discussion progresses. Since we’ve already had relatively much of discussion about this, please tell me if this is already ready for a pull request. Summary [Crates.io](http://Crates.io) should offer an API to release security advisories for crate… > > > pre-RFC here: https://internals.rust-lang.org/t/pre-rfc-security-advisories-as-p…art-of-crates-io-metadata/4045 The response from the Rust core team at the time, paraphrasing liberally, was to prototype this functionality out-of-tree first, and then circle back some time in the future to discuss upstreaming it. So myself and a few others did that, and the result is RustSec: - [https://rustsec.org/](https://rustsec.org/) - [https://crates.rustsec.org/](https://crates.rustsec.org/) - [RustSec · GitHub](https://github.com/rustsec/) It's now been a couple years, and several recent security incidents in other language package repositories have drawn attention to the overall idea of crate security. Additionally, we've created the [Rust Secure Code Working Group](https://github.com/rust-secure-code/wg) as a place for working on improving overall security of the Rust language ecosystem. Of all of the possible concrete tasks for such a group we've discussed, ensuring vulnerabilities in the ecosystem are tracked and surfaced to developers seems like one of the few things most people involved are on board with. I think the [original RFC](https://github.com/rust-lang/rfcs/pull/1752) provides a good starting point, and I'm interested in forking that and updating it with some things we have learned over the past two years operating the project in practice. One thing to note is that we never implemented a `cargo advisory` subcommand, and instead have a [manually curated GitHub repo which accepts advisories as PRs](https://github.com/RustSec/advisory-db/blob/master/CONTRIBUTING.md). I think a `cargo advisory` command is definitely worth revisiting, perhaps not necessarily with the exact described semantics, but more of as a corollary to `cargo publish` for automatically publishing an advisory on [crates.io](http://crates.io). All that said, I'm not necessarily looking to "merge RustSec into cargo", and am happy to explore other approaches, including ones which aren't discussed in the original RFC. For example, I have heard rumors that GitHub is adding first-class vulnerability tracking features, and may potentially allow you to do things like automatically request a CVE through the GitHub UI (if anyone from GitHub is reading this and is allowed to comment, that'd be great). ---
**Author:** ![nikomatsakis](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/nikomatsakis/32/5410_2.png) [@nikomatsakis](https://internals.rust-lang.org/u/nikomatsakis)\ **Post date:** [December 14, 2018, 3:02pm UTC](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017/2 "2018-12-14T15:02:56Z")
Question: it seems like this feature ought to be tied to `cargo yank`, and to generally require as little “extra action” as possible. For example, you might imagine that if a yanked package is part of your lockfile, you get a very visible warning – maybe even have to do `cargo build -f` or something to proceed. I’ve not really followed the discussion up to this point, so I’m not sure if this has been raised and considered. Mostly I clicked on [https://rustsec.org/](https://rustsec.org/) and `cargo audit`, which seems to require an “extra step” – makes sense for an out-of-tree effort, but if we move this in-tree, maybe we want to make it more invisible and automatic? ---
**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:** [December 14, 2018, 3:59pm UTC](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017/3 "2018-12-14T15:59:12Z")
> [@nikomatsakis](#): > > Question: it seems like this feature ought to be tied to `cargo yank` , and to generally require as little “extra action” as possible. I think this feature and `cargo yank` are, at the very least, closely related. For RustSec as an out-of-tree effort, I recently suggested that we should be [linting that all vulnerable versions of a crate are yanked](https://github.com/RustSec/advisory-db/issues/74). Attaching some metadata to `cargo yank` is a potential solution, and there was a lot of discussion about that on the previous pre-RFC thread in 2016. The original RFC also contained an "Alternatives" section with discussion of extending `cargo yank` in lieu of adding a separate feature: > The main way it's somewhat problematic for the purposes of building a security advisory database is that one security advisory can span multiple crate versions. From a domain modeling perspective, I think there's something of a "has many" relationship between a security advisory and `cargo yank` events. Perhaps that's a solvable problem though, and `cargo yank` could be updated to support some sort of batch yanking to which we could tie additional metadata i.e. a security advisory. ---
**Author:** ![sfackler](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/sfackler/32/9_2.png) [@sfackler](https://internals.rust-lang.org/u/sfackler)\ **Post date:** [December 14, 2018, 4:53pm UTC](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017/4 "2018-12-14T16:53:10Z")
I definitely think it should be easy to publish an advisory and yank all affected versions, but I don’t think yanking should be required. Imagine a relatively low impact issue that affects every previous release of a crate, for example. ---
**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:** [December 14, 2018, 5:26pm UTC](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017/5 "2018-12-14T17:26:39Z")
One idea people were tossing around before was having `cargo advisory` support a `--yank` (or possibly a `--no-yank`) command-line option to automatically yank (or don’t yank, if we want yanking to be the default) affected releases. ---
**Author:** ![djc](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/djc/32/1592_2.png) [@djc](https://internals.rust-lang.org/u/djc)\ **Post date:** [December 14, 2018, 7:57pm UTC](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017/6 "2018-12-14T19:57:45Z")
It would be really great if there was a (possibly opt-in) way to make CI fail for projects that include vulnerable dependencies. ---
**Author:** ![Shnatsel](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/shnatsel/32/4613_2.png) [@Shnatsel](https://internals.rust-lang.org/u/Shnatsel)\ **Post date:** [December 14, 2018, 10:09pm UTC](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017/7 "2018-12-14T22:09:02Z")
Some interesting areas to explore: 1. Systems like npm, yarn, etc only notify developer about vulnerable versions on `npm update`, which people actually do very rarely. This is largely because the package management is standardized, but the build system is not, so that’s the most visible place they could put it. Cargo does both builds and package management, so it could inform you about vulnerable dependencies sooner, e.g. on `cargo build`. 2. Once a vulnerability in a crate with a lot of dependencies is discovered, whom and how do we notify about it? Normally `cargo update` would solve this for anyone depending on it, even transitively. But what if one of the dependencies pulls in a specific version? What if no semver-compatible version with a fix is available? 3. How do we notify people who have installed a binary via Cargo about vulnerabilities in the binaries they’re running? For example, if there is [a bug in Rust OpenSSL bindings](https://github.com/sfackler/rust-openssl/pull/942) and the user has installed a binary that uses them, how do they find out that they are running a vulnerable version and need to update? 4. Do we have guidelines against vendoring C code in the crate? Again, OpenSSL comes to mind. Linking against a system library instead which does receive security updates should be the way to go. How does that interact with platforms that do not have a sane way to install and update a library, like Windows? ---
**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:** [December 16, 2018, 6:38pm UTC](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017/8 "2018-12-16T18:38:50Z")
> [@Shnatsel](#): > > Systems like npm, yarn, etc only notify developer about vulnerable versions on `npm update` , which people actually do very rarely. This is largely because the package management is standardized, but the build system is not, so that’s the most visible place they could put it. Cargo does both builds and package management, so it could inform you about vulnerable dependencies sooner, e.g. on `cargo build` . For me, the main argument for adding security advisories as a first-class `cargo` feature would be to provide an integrated, "always on" (or at least "default on") experience for these audits. RustSec seems to have suffered from low discoverability, and while there are many other options to explore like trying to add information about it to official Rust documentation, the simplest answer to me for the "How Do We Teach This?" question is: it's just there, and will tell you if there's a problem. I would agree I'd like to see them as part of the `cargo build` workflow. The main argument for that IMO is the state of the advisory database may have changed since the last time `cargo update` was run. Unless users are running `cargo update` frequently (something I think we shouldn't encourage doing indiscriminately), or run `cargo audit` in CI, information about those vulnerabilities won't be surfaced. I think there are ways we could keep it unobtrusive but still provide value. For example, we could keep the information displayed to a one-liner: ```rust *** warning: security vulnerabilities found in 2 dependencies! Run `cargo audit` for details. ``` ...or the above could be a hard error, or just a hard error for crates with `#[deny(warnings)]`. Something else that might make sense if integrating this sort of check directly into `cargo` by default is viewed as being too onerous is to make `cargo audit` into a `rustup` component, particularly if that would somehow allow it to run as part of the `cargo build` workflow, but only if the component is installed. ---
**Author:** ![rocallahan](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/rocallahan/32/2261_2.png) [@rocallahan](https://internals.rust-lang.org/u/rocallahan)\ **Post date:** [December 19, 2018, 1:11am UTC](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017/9 "2018-12-19T01:11:11Z")
Forgive me for scope creep, but I would like `cargo build` to warn about known-bad dependency versions that don’t necessarily have security advisories. For example last year I tracked down an intermittent memory corruption bug to a version of `crossbeam` that had a data race (in unsafe code of course). The bug had been fixed in later versions, but I didn’t know about it. IMHO fundamental breakage like that is worth at least a deprecation warning in `cargo build`. ---
**Author:** ![sfackler](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/sfackler/32/9_2.png) [@sfackler](https://internals.rust-lang.org/u/sfackler)\ **Post date:** [December 19, 2018, 1:26am UTC](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017/10 "2018-12-19T01:26:49Z")
Memory corruption would result in a security advisory. ---
**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:** [December 19, 2018, 1:35am UTC](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017/11 "2018-12-19T01:35:18Z")
> [@rocallahan](#): > > Forgive me for scope creep, but I would like `cargo build` to warn about known-bad dependency versions that don’t necessarily have security advisories. No worries about scope creep! I think a bulk `cargo yank` feature, combined with a structured "yank with reason" feature (see e.g. [rust-lang/cargo#2608](https://github.com/rust-lang/cargo/issues/2608)), could potentially meet the use cases of both security advisories and these sorts of "showstopper bug" use cases. I think there might be merit in pursuing this sort of approach as a generally useful feature and a building block for security advisories. Indeed these sorts of error messages in the `cargo build` workflow: ```rust *** warning: security vulnerabilities found in 2 dependencies! Run `cargo audit` for details. ``` Could alternatively look like this: ```rust *** warning: 2 insecure and 3 yanked dependencies in Cargo.lock! Run `cargo audit` for details. ``` > [@sfackler](#): > > Memory corruption would result in a security advisory. Indeed... I think a lot of showstopper bugs of this nature might ultimately be advisory-worthy ---
**Author:** ![rocallahan](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/rocallahan/32/2261_2.png) [@rocallahan](https://internals.rust-lang.org/u/rocallahan)\ **Post date:** [December 19, 2018, 1:53am UTC](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017/12 "2018-12-19T01:53:19Z")
> [@sfackler](#): > > Memory corruption would result in a security advisory. That sounds like a reasonable policy, but in practice there was no security advisory in this case AFAICT. I can imagine plausible scenarios where crate authors might want to warn users about a bad version even though no security bug was present. I can't imagine scenarios where crate authors would want to warn users about a bad version, but the bad version should not be yanked. So tying this to yanking would be fine with me. ---
**Author:** ![JeffBurdges](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/jeffburdges/32/2116_2.png) [@JeffBurdges](https://internals.rust-lang.org/u/JeffBurdges)\ **Post date:** [January 8, 2019, 9:11am UTC](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017/13 "2019-01-08T09:11:47Z")
We should definitely push for dynamic linking system libraries on sane distributions, even ignoring the security benefits. I do think eliminate all vendered C code sounds impossible, but such crates should own those vulnerabilities. ---
**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")
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? 😜 > > > @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: > > > 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)) ---
**Author:** ![system](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/system/32/14092_2.png) [@system](https://internals.rust-lang.org/u/system)\ **Post date:** [April 18, 2019, 7:45am UTC](https://internals.rust-lang.org/t/pre-rfc-reviving-security-advisories-in-crates-io-rfc-pr-1752/9017/15 "2019-04-18T07:45:33Z")
This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.