Security advisories in crates.io #1752

untitaker · 2016-09-18T13:05:57Z

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

Discourse's markdown parser doesn't like the current ones

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

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

Also unify some terminology: "report" / "vulnerability report" => "advisory" / "security advisory"

EPashkin · 2016-09-21T19:02:33Z

text/0000-security-advisories.md

+
+Here's the workflow:
+
+1. The user invokes `cargo advisory` without the `--upload` option. Cargo will


may be --generate here?

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 · 2016-09-22T13:23:32Z

text/0000-security-advisories.md

+When compared to other ecosystems such as Python's, Rust's broader community
+prefers many single-purpose crates over larger monoliths. This situation,
+together with the strongly encouraged practice of pinning MINOR versions of
+dependencies, slows down the propagation of critical security fixes.


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?

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.

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).

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?

I rewrote the entire motivation. Better?

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

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

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.

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).

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

gkoz · 2016-09-22T13:38:53Z

text/0000-security-advisories.md

+Assume a crate `W`, which depends on `X`, which depends on `Y`, which depends
+on `Z`.  If `Z` releases a new MINOR version including a security fix, it
+requires the attention of `Y`'s  and `X`'s maintainers to propagate that
+security fix to `W`. What makes this situation worse is that the author of `W`


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).

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.

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

gkoz · 2016-09-22T19:24:53Z

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 · 2016-09-22T19:46:54Z

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 · 2016-09-22T20:56:41Z

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 · 2016-09-23T08:41:23Z

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 · 2016-09-24T13:11:09Z

@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 · 2016-09-29T15:37:37Z

@untitaker thanks, it looks great now!

tarcieri · 2016-10-03T02:22:53Z

This looks good to me on my first pass through

untitaker · 2016-10-13T12:48:57Z

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 · 2016-10-24T21:15:57Z

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 · 2016-10-25T19:04:41Z

@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 · 2016-10-25T20:07:27Z

@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 · 2016-10-26T10:23:28Z

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.
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 · 2016-10-26T10:33:40Z

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 · 2016-10-28T18:14:30Z

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 · 2016-10-29T15:08:31Z

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 · 2016-10-29T19:39:15Z

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

alexcrichton · 2016-10-31T18:50:49Z

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 · 2016-10-31T19:09:01Z

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 · 2016-10-31T19:34:12Z

There exists dev-dependencies.

untitaker · 2016-10-31T20:41:32Z

Fair enough, fixed.

tarcieri · 2016-10-31T21:02:13Z

@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 · 2016-10-31T22:59:25Z

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 · 2016-11-01T21:50:15Z

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 · 2016-11-01T22:47:36Z

@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 · 2016-11-01T23:19:09Z

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 · 2016-11-09T18:34:51Z

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 · 2017-01-28T04:45:59Z

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.

Audit Cargo.lock for crates with security vulnerabilities reported to the RustSec Advisory Database. This is a PoC implementation of the closed RFC 1752: rust-lang/rfcs#1752 WWW: https://rustsec.org/ git-svn-id: svn+ssh://svn.freebsd.org/ports/head@481321 35697150-7ecd-e111-bb59-0022644237b5

Audit Cargo.lock for crates with security vulnerabilities reported to the RustSec Advisory Database. This is a PoC implementation of the closed RFC 1752: rust-lang/rfcs#1752 WWW: https://rustsec.org/

Audit Cargo.lock for crates with security vulnerabilities reported to the RustSec Advisory Database. This is a PoC implementation of the closed RFC 1752: rust-lang/rfcs#1752 WWW: https://rustsec.org/ git-svn-id: svn+ssh://svn.freebsd.org/ports/head@481321 35697150-7ecd-e111-bb59-0022644237b5

Audit Cargo.lock for crates with security vulnerabilities reported to the RustSec Advisory Database. This is a PoC implementation of the closed RFC 1752: rust-lang/rfcs#1752 WWW: https://rustsec.org/

untitaker added 14 commits August 24, 2016 21:53

Init 0000-security-advisories.md

c4d7142

Fix formatting

95b6927

Clarify dependencies and Cargo warnings

2a5231b

Add DWFs, Drawbacks, Alternatives and Questions

761bf49

Reformat codeblocks

430943c

Discourse's markdown parser doesn't like the current ones

Rename command to advisory.

cb75ca3

Reformat vulnerability report, add url field

e6c7ee7

Apply suggestion from @tomaka

45a7c2a

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

Apply suggestion from @lifthrasiir

68bb0b1

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

Update motivation

5a06156

Allow optional keys to be absent

fa78731

Don't invoke EDITOR, let the user do it.

b0691a1

Introduce advisory IDs, allow multiple per version

b11cc12

Also unify some terminology: "report" / "vulnerability report" => "advisory" / "security advisory"

Remove error behavior for cargo test.

d8aedf2

nrc added the T-dev-tools Relevant to the development tools team, which will review and decide on the RFC. label Sep 18, 2016

EPashkin reviewed Sep 21, 2016

View reviewed changes

gkoz reviewed Sep 22, 2016

View reviewed changes

untitaker added 3 commits September 22, 2016 18:51

Rewrite motivation

06259c1

Clarify how version pinning is encouraged.

4956855

Alternative: npm deprecate in cargo

8a64d0c

Rewrite motivation again

06d5174

Fix typo

8eb1e4d

vi mentioned this pull request Oct 31, 2016

Does it sanitize reserved name like CON or COM1 on Windows? andybarron/app-dirs-rs#14

Open

Fix leftover syntax

2cbf15f

Rename allow{_,-}vulnerable

bca167f

untitaker closed this Nov 9, 2016

tarcieri mentioned this pull request Jun 27, 2018

Missing documentation rustsec/advisory-db#25

Closed

sonalkr132 mentioned this pull request Jul 1, 2019

Prevent yanking of popular gem versions with downloads greater than 15000 rubygems/rubygems.org#1396

Closed


		Here's the workflow:

		1. The user invokes `cargo advisory` without the `--upload` option. Cargo will

Security advisories in crates.io #1752

Security advisories in crates.io #1752

Conversation

untitaker commented Sep 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkoz Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkoz Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkoz Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkoz commented Sep 22, 2016 • edited Loading

untitaker commented Sep 22, 2016

arielb1 commented Sep 22, 2016

untitaker commented Sep 23, 2016

untitaker commented Sep 24, 2016

gkoz commented Sep 29, 2016

tarcieri commented Oct 3, 2016

untitaker commented Oct 13, 2016

vi commented Oct 24, 2016 • edited Loading

tarcieri commented Oct 25, 2016

eternaleye commented Oct 25, 2016

untitaker commented Oct 26, 2016

vi commented Oct 26, 2016

tarcieri commented Oct 28, 2016

untitaker commented Oct 29, 2016

vi commented Oct 29, 2016 • edited Loading

alexcrichton commented Oct 31, 2016

untitaker commented Oct 31, 2016 • edited Loading

seanmonstar commented Oct 31, 2016

untitaker commented Oct 31, 2016

tarcieri commented Oct 31, 2016 • edited Loading

vi commented Oct 31, 2016 • edited Loading

brson commented Nov 1, 2016 • edited Loading

tarcieri commented Nov 1, 2016

tarcieri commented Nov 1, 2016

untitaker commented Nov 9, 2016 • edited Loading

tarcieri commented Jan 28, 2017 • edited Loading

gkoz Sep 22, 2016 •

edited

Loading

gkoz Sep 22, 2016 •

edited

Loading

gkoz Sep 22, 2016 •

edited

Loading

gkoz commented Sep 22, 2016 •

edited

Loading

vi commented Oct 24, 2016 •

edited

Loading

vi commented Oct 29, 2016 •

edited

Loading

untitaker commented Oct 31, 2016 •

edited

Loading

tarcieri commented Oct 31, 2016 •

edited

Loading

vi commented Oct 31, 2016 •

edited

Loading

brson commented Nov 1, 2016 •

edited

Loading

untitaker commented Nov 9, 2016 •

edited

Loading

tarcieri commented Jan 28, 2017 •

edited

Loading