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:
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:
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 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 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. 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.
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).
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/ 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?
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.
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.
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.
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.
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?
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 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?
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?
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:
*** 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.
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.
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), 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:
*** warning: security vulnerabilities found in 2 dependencies! Run `cargo audit` for details.
Could alternatively look like this:
*** warning: 2 insecure and 3 yanked dependencies in Cargo.lock! Run `cargo audit` for details.
Indeed... I think a lot of showstopper bugs of this nature might ultimately be advisory-worthy
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.
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.
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?
If a crate is fixed for a security reason, the old versions can be yanked and the new version can be suggested
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:
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?
