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

Whitelisting non-vulnerable usages / false positives in advisories #288

Closed
tarcieri opened this issue May 5, 2020 · 12 comments
Closed

Whitelisting non-vulnerable usages / false positives in advisories #288

tarcieri opened this issue May 5, 2020 · 12 comments

Comments

@tarcieri
Copy link
Member

tarcieri commented May 5, 2020

This came up via: clap-rs/clap#1569

The specific case is clap is pulling in rust-yaml which has RUSTSEC-2018-0006. They use the vulnerable API, but not in a way that triggers the vulnerability.

I've suggested updating the advisory to note their usage isn't vulnerable, but it'd be nice to be able to specify this programmatically somehow.

Something like:

[unaffected_consumers]
clap = ["2"]
@tarcieri tarcieri changed the title Whitelisting non-vulnerable usages / false positives Whitelisting non-vulnerable usages / false positives in advisories May 5, 2020
@kbknapp
Copy link

kbknapp commented May 5, 2020

Much appreciated! Just off the top of my head, maybe you'd probably want unaffected consumers to require a full version spec? In case the consumer has an update and starts using the vulnerability for real. It's more work to have to update the table to the new version being consumed, but ultimately its for the right reason to keep false negatives away which IMO are worse than false positives.

@ckaran
Copy link

ckaran commented May 5, 2020

[unaffected_consumers]
clap = ["2"]

You might want to force this to go all the way out to the patch level; right now it looks like all of version 2 is unaffected (which is currently correct for clap), but they could add a new non-breaking minor release that is affected. I do like that it's a list though!

EDIT

@kbknapp beat me to it while I was helping my son! 😀

@CreepySkeleton
Copy link
Contributor

And maybe require attaching a bit of context

[unaffected_consumers]
clap = { version = ["2"], reason = "never triggers, only trusted input", discussion = "link to thread" }

Where reason is a summary of why this is not a vulnerability and the discussion link is self evident.

@oherrala
Copy link
Contributor

oherrala commented May 7, 2020

I'm not against this proposal. It could eliminate a lot of noise and would be good for many users.

However, I really want to know if anything in my tree has vulnerability or some other known issue. Maybe it's not an real issue and it's easy to figure out. But I do want to know there is something somewhere.

I have two reasons for this:

  1. I need to know. I'm product owner and I want to keep the decisions about vulnerability management for myself and if something is deprecated I can start looking for alternatives, etc. Tools like cargo-audit and cargo-crev help tremendously with this.

  2. There are binary code analysis tools which are looking for vulnerable dependencies from binary code. These tools can't make the judgement if it's a real issue or not. However, they are used to audit a piece of software before purchase or before going to production. I would rather have cargo-audit tell me the issues as early as possible and before these binary analysis tools.

@Shnatsel
Copy link
Member

Shnatsel commented May 7, 2020

Exploitability is also far from being clear-cut. For example, Microsoft has famously misjudged their use of MD5 in certificates, which turned out to be far more exploitable than it seemed.

I agree that surfacing the presence of vulnerable packages this in some way is a good idea even if certain advisories are suppressed. The presence of suppression in itself can be important.

@ckaran
Copy link

ckaran commented May 8, 2020

What about a new switch? E.g., cargo audit --ignore-whitelists.

Alternatively, we could keep the current behavior, and make a new switch that goes the other way: cargo audit --filter-on-whitelists.

@WaffleLapkin
Copy link

WaffleLapkin commented May 8, 2020

cargo audit can also output something like

3 errors were suppressed because of whitelist. Use `cargo audit --ignore-whitelists` to see them.

@ckaran
Copy link

ckaran commented Jul 3, 2020

I just checked the advisory that is linked at https://rustsec.org/advisories/RUSTSEC-2018-0006, and its description for clap-rs/clap#1569 doesn't seem to have been updated yet. Can it be updated?

@tarcieri
Copy link
Member Author

tarcieri commented Jul 3, 2020

Yep, either open a PR to update the advisory or I can try to look at it later this weekend.

@ckaran
Copy link

ckaran commented Jul 6, 2020

@tarcieri I'm afraid I'm going to have to wait for you to look into it, I'm tracking down a nasty bug in my code involving serde_json right now.

@miaujennifer
Copy link

I'm not against this proposal. It could eliminate a lot of noise and would be good for many users.

However, I really want to know if anything in my tree has vulnerability or some other known issue. Maybe it's not an real issue and it's easy to figure out. But I do want to know there is something somewhere.

I have two reasons for this:

  1. I need to know. I'm product owner and I want to keep the decisions about vulnerability management for myself and if something is deprecated I can start looking for alternatives, etc. Tools like cargo-audit and cargo-crev help tremendously with this.
  2. There are binary code analysis tools which are looking for vulnerable dependencies from binary code. These tools can't make the judgement if it's a real issue or not. However, they are used to audit a piece of software before purchase or before going to production. I would rather have cargo-audit tell me the issues as early as possible and before these binary analysis tools.

@pinkforest
Copy link
Contributor

SInce both we cannot address this is in advisory-db today and there is now compherensive tooling issue under rustsec/rustsec

We'll close this one in favor of consolidating the attention to: rustsec/rustsec#505

Thanks everyone

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

9 participants