Security advisories in crates.io

[untitaker]

pre-RFC here: https://internals.rust-lang.org/t/pre-rfc-security-advisories-as-part-of-crates-io-metadata/4045

[EPashkin]

may be --generate here?

[untitaker]

The idea behind that awkward phrasing is that both cargo advisory and cargo advisory --generate are valid for this step. "without --upload" is a phrasing that captures both.

(damn "start a review" button)

[gkoz]

I'm not aware of pinning minor versions in Cargo.toml being encouraged (except the 0.x.y case but then x is the major version for cargo-semver purposes). Can you elaborate on this?

[untitaker]

You're right about that, I only thought about the 0.x.y-case, but changing MINOR to MAJOR in that sentence is also wrong. What about this:

...with the strongly encouraged practice of pinning version ranges (sometimes specific versions)

Specific versions are pinned if a semver-compatible version of a dependency introduces a bug that breaks the dependent crate.

[gkoz]

I feel this bit of motivation doesn't work because

• pinning to specific versions is not the norm,
• cargo makes the ultimate consumer track precise versions via lockfile, giving them more power and responsibility.

So applying the fix should normally not be a problem. Only if the intermediate crates actively prevent this by strict version pinning do you need to act differently (e.g. prevent publishing a crate which makes it impossible to avoid vulnerable dependency versions).

[untitaker]

In the pre-pre-RFC thread I gave a different motivation. Money quote:

Unfortunately I'm depending on open source libraries where nobody gets paid for their work. This means that the maintainer of Z is less likely to backport the patch to other MINOR versions, only the latest version gets a PATCH release. The maintainers of X and Y are less likely to update their dependencies either, because they do all of this in their freetime as well. This is a social problem though, and people are just getting started12 writing about this issue.

I agree with the motivation being a bit weak. Do you think the scenario described in that thread above is more realistic?

[untitaker]

I rewrote the entire motivation. Better?

[untitaker]

Well, as opposed to "*" as version specifier, which Crates.io rejects.

[untitaker]

I updated the text to concretely mention why pinning is encouraged. Note that I wrote "version ranges" (e.g. ^1.0.0) as well.

[gkoz]

as opposed to "*" as version specifier, which Crates.io rejects

I see, it's the "pinning" part because I wouldn't consider "^1.0.0" pinning, it implies something like "=1.0.0" to me. But is it right to treat them the same way? I don't think "^1.0.0" poses any propagation problems. Dealing with more exotic cases is important but probably won't be the primary focus of this feature.

I don't understand what you mean. Dealing with upgrades is not concern of this RFC at all.

It is because you're opening with slow propagation of security fixes. It's probably stronger to first discuss only the communication part: all users need to be made aware of the advisory. (BTW I wouldn't mind cargo outright failing the build by default if any dependency is found to be vulnerable.) The propagation discussion can build on that but the feature is valuable even without it.

[gkoz]

Perhaps I'm not taking seriously enough the scenario of a "1.x" branch being unmaintained and vulnerable (i.e. cargo update definitely won't work).

[untitaker]

The new example I wrote in 06259c1 doesn't imply that.

[gkoz]

If X and Yuse the (default) caret requirement, performing cargo update on W will update Z (once again, for 0.a.b MINOR version is b as far as caret requirement is concerned).

[untitaker]

What about "semver-incompatible version"? Also I probably should clarify what the version pinning for all crates looks like. I also made the assumption that each crate exposes its dependency's API and therefore needs to do a new semver-incompatible release when their dependencies do.

[gkoz]

I guess if the fix requires a major version bump, you get the same scenario as with pinned versions.

[gkoz]

Well it seems like you're solving a pretty specific problem (a discontinued B 0.9 is found out to be vulnerable and A depends on B 0.9 and won't upgrade), which is an extension of more general goals (it's by itself valuable to be notified if you got a vulnerable B 0.9 in your lockfile).

[untitaker]

No, that's not the problem I'm trying to solve, that's a pretty common example that exhibits that problem, where the entire issue is not just solved by running cargo update regularly. The problem is that people don't know if their dependencies contain security vulnerabilities, and that it's currently hard to keep on top of that.

[arielb1]

Enterprise users might only want to do cargo update at Designated Release Points except for security issues. Automated security notifications would remind them to update.

[untitaker]

Is this something I should write into the Motivation? I feel like it's a bit
dishonest as I wouldn't know of any current enterprise users of Rust, so that
claim is hard to verify.

On Thu, Sep 22, 2016 at 01:56:48PM -0700, arielb1 wrote:

Enterprise users might only want to do cargo update at Designated Release Points except for security issues. Automated security notifications would remind them to update.

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#1752 (comment)

[untitaker]

@gkoz @arielb1 I rewrote the motivation again in 06d5174. I think this clarifies the problem this RFC is trying to solve without making any unnecessarily strong claims. What do you think?

[gkoz]

@untitaker thanks, it looks great now!

[tarcieri]

This looks good to me on my first pass through

[untitaker]

Next week it'll be a month this PR has been open. I understand that this is hard to decide on but I suspect that it is also slipping through the cracks.

[vi]

How global should be cached the vulnerability list? Per-project or per-cargo-deployment?

Shall it keep global list of crates (from multiple projects) to keep informed about advisories, so they got checked in one move?

Shall it just download all new vulnerabilities for all crates (not scalable)?

---

With unmodified point "3." (especially if the list is per-project) users may get too many warnings: doing occasional little fixes on many mini-projects => getting the warning almost every time.

Alternative solution:

3b. cargo build/run/test: If advisory list is not updated for N days, start updating it in background (while continuing compilation and testing and running, etc.). If the list gets successfully downloaded, check against it after cargo test/run/build main run. If can't download, print a short warning. The warning about an advisory should be bigger, redder and scarier than the warning about inability to download the advisory list.

Something like this:

$ cargo test
    Downloading advisory list for dependent crates in background...
    Finished debug [unoptimized + debuginfo] target(s) in 0.0 secs
     Running target/debug/rt-10dd314f496c8c5a

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured
warning: unable to download advisory list

[vi]

Shall there be advisories for Rust system components (like libstd or libcore)?

Shall definition of a security vulnerability included in RFC? For some things (especially in libraries) it can be tricky to discriminate just bugs and vulnerabilities. For example, is memory corruption by a data structure with a safe interface always an advisory-worthy vulnerability?

[tarcieri]

@vi the ruby-advisory-db tracks similar vulnerabilities in the Ruby standard library. I think it would be nice to have functionality-wise, but seems a bit orthogonal to advisories for specific crates, and might be something to punt on initially in the hopes of getting anything out the door.

[eternaleye]

@tarcieri, @vi: In addition, the push towards stdlib-aware Cargo (and the possible future of much of the stdlib being on crates.io natively) may make it a moot point.

[untitaker]

1. I don't think adding advisories for the stdlib is within scope, as the stdlib doesn't have a independent versioning scheme and it's unclear how allow_vulnerable is supposed to work with it. Also with the stdlib as-is (and without stdlib-aware cargo), this would push the entire logic into rustc rather than having it in cargo.
2. I didn't make this clear, but I think forcing a fetch of the advisory list on cargo publish is enough. This list is also updated with cargo update, but not build. A separate command would be nice, but having warnings if the list is old seems very intrusive to me.

What does Cargo do with yanked packages?

[vi]

I didn't make this clear, but I think forcing a fetch of the advisory list on cargo publish is enough.

What about private Rust projects that are never published to crates.io? Shouldn't they also benefit from advisories?

That's why advisories also make sence for crates without reverse dependencies.

Also cargo publish is a point where developer may think that the job is almost done, and an advisory may knock it back into designing and architecting. With cargo build-triggered warnings one may just switch away from problematic dependency earlier.

[tarcieri]

What about private Rust projects that are never published to crates.io? Shouldn't they also benefit from advisories?

My inclination is no, or rather, if they'd like to use this feature, they should run an internal Cargo repository with something that speaks the same API as crates.io (similar to Gemstash) and can thereby manage advisories for internal crates.

[untitaker]

With cargo build-triggered warnings one may just switch away from problematic dependency earlier.

Network access during compilation is a dealbreaker for many bigger Linux distributions, which is why I'm trying to avoid it. For the dev workflow it would be better of course.

[vi]

There may be --offline option or distributions may use local crates.io-like proxy/adaptor to its packages.

[alexcrichton]

Reading over this, I unfortunately don't have a lot of time to follow the conversation and help push this over the finish line, but some thoughts I had were:

• I feel like the alternative with yanking isn't suitably explored. As you've mentioned many of the problems you listed are simply fixed by adding a yank reason, and I think the remaining problems would require an arguably similar delta of work/understanding the current RFC proposes from the current behavior of cargo to implement. That is, I agree with @wycats that it may be best to explore that route before adding more infrastructure to Cargo with a whole new subcommand.
• I'm very worried about the warning behavior here, so much so that I could consider it a hard blocker for this feature. The proposal is to unconditionally warn on all builds about vulnerable packages with an optional whitelist in Cargo.toml to turn off warnings. The RFC then later claims that this feature could be used for vulnerabilities that may affect optional functionality. This to me sounds like I'm going to start getting warning ridden builds in Rust for functionality I never use. This seem to me like very much the wrong default and would simply condition everyone to ignore these warnings (defeating their purpose in the first place)
• The allow_vulnerable key seems very underspecified. In one location it's a string array, in another it's a boolean, and it's also not the conventional separated-with-dashes that Cargo configuration typically has.

Next week it'll be a month this PR has been open. I understand that this is hard to decide on but I suspect that it is also slipping through the cracks.

I'm sorry that this is taking awhile, but please remember that we're all very busy with lots of projects in flight, so we don't always have a lot of time to allocate to all RFCs coming down the pike.

[untitaker]

I feel like the alternative with yanking isn't suitably explored.

The assertion @tarcieri and I have made in this and other threads is that overloading yank with a multitude of "yank reasons" compromises the security aspect of the proposed feature. Tony said it better than I could. My personal viewpoint is that cargo should provide a panic button for security incidents where the user is guided through proper disclosure, not a swiss-army knife where the answer to "What do I do next?" is "it depends, there are endless possibilities". Whether this RFC achieves that is left to discuss.

EDIT: Note that internally you could model the API such that it uses yank and cargo advisory becomes a wizard for a particular yank reason. This is why the entire API for Crates.io is left unspecified. I care more about the UX part of whatever the implementation looks like than anything else.

The proposal is to unconditionally warn on all builds about vulnerable packages with an optional whitelist in Cargo.toml to turn off warnings. The RFC then later claims that this feature could be used for vulnerabilities that may affect optional functionality. This to me sounds like I'm going to start getting warning ridden builds in Rust for functionality I never use.

I'm very confused about this reasoning. If a dependency has a vulnerability, and I can disable the corresponding warning with a whitelist, how does this generate warning ridden builds? The purpose of the whitelist is to turn off security warnings that affect featuresets I don't use.

The allow_vulnerable key seems very underspecified. In one location it's a string array, in another it's a boolean, and it's also not the conventional separated-with-dashes that Cargo configuration typically has.

That's totally fair criticism. The bool value is a leftover from older drafts and the preference for underscores is my own bias as a Python developer, and because there are no keys with dashes except for package names in the TOML. In either case it's not a conceptual problem.

[seanmonstar]

There exists dev-dependencies.

[untitaker]

Fair enough, fixed.

[tarcieri]

@alexcrichton I'm not necessarily opposed to yank being a means to an end here, and indeed yanking crates with vulnerabilities makes sense, so having a separate command may be redundant.

However, the goal of a feature like this is to build a security advisory database which security-related tooling can consume. One like this (except as an integrated part of https://crates.io as opposed to a git repo of YAML files on GitHub):

https://github.com/rubysec/ruby-advisory-db

More than just eliminating security vulnerabilities during development, such a system can aid security teams in monitoring deployed versions of apps containing vulnerable crates with the assistance of endpoint agents.

If this can be accomplished with adding a set of security-related flags to yank, great, but I'm not sure that actually makes sense.

Security advisories by nature span multiple versions (while still being associated with a single logical CVE/DWF-like ID), but yank (as far as I can tell) only operates on one version at a time. That feels like an impedance mismatch to me: I think I'd rather the filing of an advisory carry an optional flag to yank the affected crates. Perhaps yank can be modified to consult the advisory for the affected versions so you can omit the --vers flag, but at that point, is it really the same command?

Having a generic description associated with a yank event is much less useful than a dedicated security advisory database.

[vi]

I don't like fusing of yanking and advisories.

• Yanking seem to be better usef for retracting crates quickly after they are published. Yanking things that was published long ago is more destabilizing.
• Security vulnerabilities may be found and filed, but not fixed. Yanking all versions of a crate without providing non-yanked version seems to be too breaking.
• With yanking for security, old versions of big important frameworks may become increasingly yanked. If one expect that 2-year-old versions are 70%-probable to be yanked because of security, overriding the yanking though Cargo.lock hacks may become a norm. One may need to try various old versions to bisect bugs or for doing gradual, step-by-step upgrades when reiving something long abandoned. I want projects to be generally buildable even with Cargo.lock removed.
• Applications may not be affected by a particular vulnerability. Having vulnerability ID in special "we have checked that it's OK" is more explicit reasoning about security.
• Advisories may have more didactic effect, making programmers think more about security. Cargo may make the first move, reifying abstract far away security problems into an actual potential vulnerability in your project. Because of this it is also important that the system should be in effect by default, not opt-in.
• Advisory tracking system can be used by more things, not just Rust/Cargo. For example, somebody may want an unmaintained long-running program to automatically shutdown if some even potential vulnerability is found.

[brson]

Although this seems like a reasonable thing to want, I'm wary to devote a lot of infrastructure to this use case without having more evidence that it's needed now and the design is right.

There are a number of specifics here and it seems like there's a fair opportunity to overdesign something that ends up being the wrong design in practice, or just underutilized for whatever reason.

Text in the RFC indicating precedent in other package managers, their success, and how this design relates to them, would bolster the argument.

A more general, and more incremental approach that doesn't preclude adding a full security advisory feature as specified here, might be "yank with message".

Another incremental approach would be to develop something out-of-tree and prove it out, before trying to add the feature to cargo. As an example, I'm not sure that there are even enough crates with known security vulnerabilities to warrant the infrastructure necessary to add a feature to cargo to let one publish advisories from the command line. We could start by just setting up a repo on github containing a blacklist, which people can submit PRs to when security problems become known. Then either cargo itself or a cargo advisory plugin could source that list and print notifications. Implementing this would be a simple step that lets the concept prove itself without requiring cargo commit to a specific user interface.

[tarcieri]

@brson one of the Bundler/RubyGems maintainers is working on a similar feature (cc @indirect), so hopefully you'll have a data point there

[tarcieri]

Another incremental approach would be to develop something out-of-tree and prove it out, before trying to add the feature to cargo.

I would agree it might be best to start out by developing this out-of-tree. Maybe just an advisory database on GitHub you contribute to with pull requests storing the advisory data format, and a CLI tool you can cargo install to clone that repo and check it against your Cargo.lock.

[untitaker]

Since so many core members of Rust are against this only form that I consider viable I'm going to close this pull request. I agree that developing this feature in a separate repo outside of crates.io is good as PoC, but I don't think that a separate project would attract the amount of users necessary to make it actually useful.

If anybody wants to work with me on a PoC feel free to email me and I'll be happy to help wherever I can, but my motivation to e.g. implement this feature all by myself (which I initially had) has faded away.

[tarcieri]

If anyone is interested in continuing this work, I've created a project to continue pursuing it:

https://github.com/rustsec/

I've imported the latest version of this RFC into a repo here if anyone would like to continue discussion:

https://github.com/rustsec/rfcs/pull/1

At my job we're looking at using Rust in a "high assurance" capacity, so vulnerability tracking is definitely something I'd love to see tackled.