Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing documentation #25

Closed
Shnatsel opened this issue Jun 27, 2018 · 11 comments
Closed

Missing documentation #25

Shnatsel opened this issue Jun 27, 2018 · 11 comments

Comments

@Shnatsel
Copy link
Member

By looking at this repository I do not see any answers to the following questions:

  1. How do I file an advisory? (Presumably by opening a PR, but it's not stated)
  2. Under what conditions should I file an advisory? Is a panic eligible for an advisory? Memory leak? Math that calculates buffer sizes incorrectly but is not dangerous unless fed to unsafe by API consumer? I've already found all of these.
  3. What do I do if the crate maintainers are not responding / fixing the issues? My experience with filing bugs has ranged from best response I've seen yet to pretty much ignored.

The reason I'm asking is that I've spent the last several days fuzzing various popular library crates, so now I'm sitting on a pile of bugs, wondering what to do with them aside of fixing them and opening PRs in absence of action from maintainers.

Also, somewhat out of scope of this issue:

  • I have looked for a place to report security issues in Rust packages on crates.io and found nothing of use. There is security page for Rust itself that does not mention anything about crates.io packages. If I'd found something more serious than panics or OOMs that I got so far, I would not be able to report it anywhere, as my experience with CVEs so far can be summed up as "don't even bother".
  • I have failed to find a guide for shipping security updates on crates.io. Do I yank the vulnerable versions? How do I announce that a certain update is a security update? What other steps do I need to take?
  • This is the first time I hear about cargo-audit, despite being security-inclined. Cargo quickstart guides did not mention it. Neither did the Nomicon mention any kind of actions to take after you've found and fixed a memory safety issue. The folks over at rust-fuzz make no mention of this either.
  • I kind of expected cargo to notify me about security updates for dependencies of my crate without me invoking obscure tools. It's not like I can run cargo audit on everything twice a day, even if I'd known about it. Yet the readme for it says it doesn't even ship with cargo by default?
  • As an aside, I hope something like email updates is set up for maintainers of crates on crates.io shipping with cargo.lock pointing to vulnerable versions and cargo.toml to vulnerable series, but I am not one of them, so I wouldn't know.
@tarcieri
Copy link
Member

tarcieri commented Jun 27, 2018

Hi there! Some great questions, thanks for opening this issue.

For context this is a project I spiked out last year and have probably not devoted enough time to maintaining/improving after it hit something close to an MVP, but I'd love to work through these issues now and better document the project.

RustSec questions

How do I file an advisory? (Presumably by opening a PR, but it's not stated)

You are correct that opening a PR is the correct way to file an advisory. We should probably add a CONTRIBUTING.md that describes this, along with basic instructions for filing issues (perhaps just a reference to CONTRIBUTING.md) to this repo and the cargo-audit repo.

The original impetus behind this project was a cargo advisory subcommand that would file advisories from the CLI, but I never got around to implementing that and I'm not sure it's really necessary. See rust-lang/rfcs#1752 which was adapted into the as-yet-unmerged-and-only RustSec RFC https://github.com/RustSec/rfcs/pull/1.

Under what conditions should I file an advisory? Is a panic eligible for an advisory? Memory leak?

I would categorize both of these as a potential DoS if they can be induced by an attacker. I just merged a panic-related issue in #24.

Math that calculates buffer sizes incorrectly but is not dangerous unless fed to unsafe by API consumer?

As in something that miscalculates a buffer size and returns e.g. a usize? Erring on the side of caution I'd say yes, particularly if you've found places it's happening in practice.

What do I do if the crate maintainers are not responding / fixing the issues? My experience with filing bugs has ranged from best response I've seen yet to pretty much ignored.

File away here. So long as the issues are legitimate I will merge them into the advisory database, with our without feedback from the crate maintainers (preferably with, but I'd personally rather err on the side of comprehensiveness)

The reason I'm asking is that I've spent the last several days fuzzing various popular library crates, so now I'm sitting on a pile of bugs, wondering what to do with them aside of fixing them and opening PRs in absence of action from maintainers.

Haha fun! I'd suggest opening both an issue on the upstream project and advisories here, and cross-referencing them in the issue description/comments.

This is the first time I hear about cargo-audit, despite being security-inclined. Cargo quickstart guides did not mention it. Neither did the Nomicon mention any kind of actions to take after you've found and fixed a memory safety issue. The folks over at rust-fuzz make no mention of this either.

I haven't exactly done a great job popularizing RustSec, especially since its release. It's linked from the Cargo Subcommands documentation but not really anywhere else to my knowledge. Adding references to it in the Cargo documentation is a great idea.

I kind of expected cargo to notify me about security updates for dependencies of my crate without me invoking obscure tools. It's not like I can run cargo audit on everything twice a day, even if I'd known about it. Yet the readme for it says it doesn't even ship with cargo by default?

You might take a look at the cargo advisory rust-internals thread (in addition to rust-lang/rfcs#1752) for some history of how RustSec came about. The verdict at the time was directly integrating a feature like this into Cargo was premature. However, perhaps if you can fill up the database with vulnerabilities, it will be a good time to discuss upstreaming it into Cargo proper again.

As an aside, I hope something like email updates is set up for maintainers of crates on crates.io shipping with cargo.lock pointing to vulnerable versions and cargo.toml to vulnerable series, but I am not one of them, so I wouldn't know.

That is also a great idea, particularly for downstream dependencies.

Generic Rust Questions

I have looked for a place to report security issues in Rust packages on crates.io and found nothing of use.

It seems like an attribute in Cargo.toml that contains a URI or email address for a project's vulnerability reporting policy or contact (something like vulnerability_reporting) would be useful, particularly if it were displayed/linked from crates.io

I have failed to find a guide for shipping security updates on crates.io. Do I yank the vulnerable versions?

There are mixed opinions on yanking but I think yanking versions with severe vulnerabilities (e.g RCE) would probably be for the best. That's just my opinion though.

That said RustSec is designed to alert people to vulnerabilities without requiring crate publishers yank every last vulnerable crate.

@Shnatsel
Copy link
Member Author

Thanks for the swift and comprehensive reply!

However, perhaps if you can fill up the database with vulnerabilities, it will be a good time to discuss upstreaming it into Cargo proper again.

cargo-fuzz trophy case is a good starting point for filling up the DB.

I think it's a chicken-and-egg problem: people are not aware of a way to announce security fixes, so they do not announce them and it doesn't give them much visibility. It should be fairly easy to test this hypothesis by scrubbing bug trackers for popular crates for keywords like "panic" or "overflow"; I'm sure they're there.

For reference, here are the somewhat security-related bugs I've found so far:
image-rs/image-png#79
image-rs/image-png#80
kornelski/lodepng-rust#28
RustAudio/lewton#27
No RCEs or information disclosure bugs yet, fortunately.

Also, I'd appreciate documentation on specifying version ranges for vulnerabilities, e.g. how to express that 1.2.0 and 2.0.0 are vulnerable, but 1.2.1 and 2.0.1 are not.

@alex
Copy link
Member

alex commented Jun 27, 2018

FWIW, I wouldn't bother including panics as security issues. (a) I think it'll bloat the DB, reducing it's utility, (b) most other folks don't count them this way; I'm not aware of any browser or OS that include null-derefs in advisories for example, OSS-Fuzz marks null-deref as regular bugs, not security.

@tarcieri
Copy link
Member

Regarding a) I'm not particularly concerned about bloating the database as we presently have a total of 6 published advisories, so if anything I'd like to encourage more contributions.

As for b) from what I've observed crashes/DoS advisories are published quite frequently for at least Windows and OS X/iOS. I think #24 in particular warrants inclusion.

@Shnatsel
Copy link
Member Author

The problem is exactly in this difference of opinion: some people will assume they are important and report them, while others will not, resulting in wildly incomplete coverage of panics. Also, upon seeing advisories about panics, some people will consider them noise (or at least something that does not warrant upgrading to an API with a breaking change) and ignore actually critical advisories as well.

So I'd suggest only including panics if the crate specifically guarantees that it will not panic.

@tarcieri
Copy link
Member

That's a good metric. And I'd agree we should have some clearer guidelines about what sort of vulnerabilities should be reported to the database. Seems like good content for a CONTRIBUTING.md.

@Shnatsel
Copy link
Member Author

Shnatsel commented Jul 18, 2018

During my recent work on fuzzing popular Rust crates I've noticed that there are basically two kinds of DoS issues: panics that unwind the current thread and resource exhaustion issues (memory leaks, unbounded allocations, stack overflow, etc) that crash the entire process.

I think it would make sense to omit panics unless the crate explicitly guarantees that it won't panic, because interested parties can easily guard against them by isolating unreliable code in a separate thread. I would include DoS issues that crash the entire process because you cannot reasonably guard against them in a DoS-critical application.

@tarcieri
Copy link
Member

@Shnatsel those criteria seem reasonable to me

tarcieri added a commit that referenced this issue Jul 23, 2018
This is long overdue! (see #25) It provides basic instructions for
filing advisories against the database, and also some guidelines
for what types of vulnerabilities qualify.
tarcieri added a commit that referenced this issue Jul 23, 2018
This is long overdue! (see #25) It provides basic instructions for
filing advisories against the database, and also some guidelines
for what types of vulnerabilities qualify.
@tarcieri
Copy link
Member

tarcieri commented Jul 23, 2018

Here is a PR for an initial CONTRIBUTING.md (Edit: Now merged, but please feel free to leave feedback or send PRs against it)

@tarcieri
Copy link
Member

I think we should be good on this now. Please reopen if you disagree.

@Shnatsel
Copy link
Member Author

I was unable to comment earlier, but this is great, thanks!

I might or might not propose some clarifications to the criteria for whether or not a vulnerability qualifies.

Also, I really dig the Big Red Button in the readme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants