Clap: using a vulnerable rust-yaml version in clap v2

[Licenser]

clap depends on a version of rust-yaml that has a vulnerability - it would be nice to update to a newer version: https://rustsec.org/advisories/RUSTSEC-2018-0006

Affected Version of clap

• 2.33.0

[rlabrecque]

I just started using https://github.com/RustSec/cargo-audit via https://github.com/actions-rs/audit-check and clap is the only library causing issues because of rust-yaml. Would definitely be nice to get this updated.

[CreepySkeleton]

clap 3.0 gets this fixed in #1541 , so this issue is about backporting the update to 2.34.

Also, I don't think the uncontrolled recursion case is possible with clap, so this is entirely about appeasing the audit gods :confused:

[Licenser]

In part yes, but it also removes the burden from maintainers to figure out that it's 'only' in clap and that clap isn't affected.

As a user I'm not certain where exactly in the clap workflow yaml files are read, so it's hard to decide if it's an actual issue or a false positive. I sure could dig through the clap codebase and find out, but that's quite a lot of work.

So it's not only appeasing the audit gods but making security easier for everyone, which is a big + :D

[CreepySkeleton]

As a user I'm not certain where exactly in the clap workflow yaml files are read, so it's hard to decide if it's an actual issue or a false positive. I sure could dig through the clap codebase and find out, but that's quite a lot of work.

Yaml files are an alternative to building the clap::App directly in your code. A developer of a CLI does it, those files never originate from a third party source. In fact, the maximum possible depth is about 10 for "no subcommands" CLI, every nested subcomman could add up to 10. I'm pretty sure there are not many apps out there that use mare than 4 nested subcommands.

As a user I'm not certain where exactly in the clap workflow yaml files are read, so it's hard to decide if it's an actual issue or a false positive. I sure could dig through the clap codebase and find out, but that's quite a lot of work.

To be clear - I'm not saying that we shouldn't do it, I'm just pointing out that there's no practical way this could go sought today :smile:

[CreepySkeleton]

@Dylan-DPC So how do we do it? I suggest to create v2-backports branch based on this commit and merge it there. Once done, you can release 2.34.0

[Licenser]

No hurry on it :)

[CreepySkeleton]

The situation: clap v2 depends on rust-yaml v0.3 and the fix is not backported to it. Blocked on it.

[CreepySkeleton]

Blocked on https://github.com/chyh1990/yaml-rust/issues/126

[CreepySkeleton]

To be fair this is not a vulnerability. Vulnerability in computing is defined as (emphasis mine)

In computer security, a vulnerability is a weakness which can be exploited by a threat actor, such as an attacker, to perform unauthorized actions within a computer system. To exploit a vulnerability, an attacker must have at least one applicable tool or technique that can connect to a system weakness.

There's no such tool for an attacker because yml files are included at compile time via include_str!; it's a trusted input. I'm totally for fixing this - as soon as it's fixed upstream, but the T: vulnerability label is uncalled for.

[ckaran]

@CreepySkeleton you're right that this isn't a true vulnerability, but @Licenser is definitely correct when he said:

In part yes, but it also removes the burden from maintainers to figure out that it's 'only' in clap and that clap isn't affected.

False positives waste time and lead to fatigue, which can lead to people finding ways of turning off the warnings, which leads to true positives getting through. So this needs to get fixed as quickly as possible, even if it isn't a vulnerability.

[CreepySkeleton]

@ckaran Well yes, except we can't fix it unless it's backported to yaml 0.3, not really, because doing so would be a breaking change and potentially break god knows how many crates out there for no benefit.

We _might_ be willing to publish a breaking change to fix a _real_ vulnerability, but this one does not qualify as such.

Come to think of it, clap is not the one to blame here, it does everything right. It's cargo-audit who emits a false-positive warning and causes the fatigue. If you ask me, an audit system must have a way to mark certain software as a known false positive and suppress the warning. Is there one?

[ckaran]

If you ask me, an audit system must have a way to mark certain software as a known false positive and suppress the warning. Is there one?

I don't think so, but I've just started using it. @tarcieri, is there a way to mark a vulnerability as a false positive?

[CreepySkeleton]

Clarification: @tarcieri, is there a way to mark a specific vulnerability in a specific version or range of versions of a specific crate that depends on a vulnerable crate but never triggers the vulnerability as a false positive?

[tarcieri]

The main way to flag vulnerabilities as false positives is either via a command line --ignore argument to cargo audit or via its configuration.

Regarding transitive dependencies where the vulnerable code path is never called (which appears to be what's happening here?), detecting these scenarios is a long-time requested feature. We'd like to be able to do a call graph analysis. Here's a relevant issue:

https://github.com/RustSec/cargo-audit/issues/89

[CreepySkeleton]

Regarding transitive dependencies where the vulnerable code path is never called

Not quite. The allege vulnerability in question is unrestrained recursion that leads to stack overflow, and this code is indeed called here. But the input we ever feed to the yaml parser (the one with that triggers recursion) is trusted (is included via include_str! at compile time) and the nesting level is never big enough to trigger the overflow, not even close. So yeah, it's pretty subtle, but not a vulnerability.

The main way to flag vulnerabilities as false positives is either via a command line --ignore argument to cargo audit or via its configuration.

The problem with this workflow is that users have to see the warning first, then discover that the report in question is a false one, then individually mark it as false positive for themselves. This is the fatigue @ckaran was talking about and it won't go away until an automatic solution (from user's perspective) is developed.

[tarcieri]

I'm afraid there aren't presently any good solutions for that other than publishing a release which is able to use transitive dependency which isn't in the vulnerable range.

[kbknapp]

Just throwing my thoughts in the ring as well. I 100% agree with @CreepySkeleton here, there isn't much we can do unless yaml_rust backports the fix into a 0.3.x branch linked above. I think the clap team has put in a lot of effort here, as a clap maintainer has backported the fix to yaml_rust and opened the PR. Realistically the only thing we can do next is wait for the PR to be merged, or make a breaking change in clap. If this were an active vulnerability, I would be willing to make a breaking change in clap, but this isn't.

Having said that, I also agree with the sentiment that it's difficult for users to go through the headache of finding out what deps are causing which errors, and whether or not it's a real vulnerability. However, I would caveat that with that's just part of dependency management. Without better tooling, that's just the state of things.

The only other solution I can think of at the moment is if we add a blurb in our docs for the affected API that says something to the effect of, "There is a known false positive when using cargo-audit, link to discussion, use this cargo-audit config to ignore the error."

[ckaran]

The only other solution I can think of at the moment is if we add a blurb in our docs for the affected API that says something to the effect of, "There is a known false positive when using cargo-audit, link to discussion, use this cargo-audit config to ignore the error."

Assuming that the PR for the backport is accepted soon, this might be the best answer.

[tarcieri]

"There is a known false positive when using cargo-audit, link to discussion, use this cargo-audit config to ignore the error."

I'd also be fine with adding this same text to the advisory itself.

[CreepySkeleton]

The only other solution I can think of at the moment is if we add a blurb in our docs for the affected API that says something to the effect of, "There is a known false positive when using cargo-audit, link to discussion, use this cargo-audit config to ignore the error."

This kind of solution is unlikely to change anything because users get the warning from cargo audit. They don't even have to know that this API - and the attached note - exists in the dirst place. t\all they see is

ID:       RUSTSEC-2018-0006
Crate:    yaml-rust
Version:  0.3.5
Date:     2018-09-17
URL:      https://rustsec.org/advisories/RUSTSEC-2018-0006
Title:    Uncontrolled recursion leads to abort in deserialization
Solution:  upgrade to >= 0.4.1
Dependency tree: 
yaml-rust 0.3.5
└── clap 2.33.0

Which doesn't mention anything about the relevant API.

In order to be useful, this attachment must be visible. I'd suggest put it in readme and docs.rs front page, somewhere near the top, and paint it bold.

[CreepySkeleton]

I'd also be fine with adding this same text to the advisory itself.

_That_ would be marvelous.

[ckaran]

I'd also be fine with adding this same text to the advisory itself.

OK, so just so I'm 100% clear, this is going to be in cargo audit's database itself, correct? How do you handle a situation where a crate is using both clap and rust-yaml? That is, this is not a vulnerability for clap, but for other uses it may be, so you want to make sure that you don't accidentally turn off all auditing against rust-yaml just because you happen to include clap in your project.

[CreepySkeleton]

so you want to make sure that you don't accidentally turn off all auditing against rust-yaml just because you happen to include clap in your project.

Right, it must be worded in a way that explicitly conveys that clap is NOT vulnerable, but rust-yaml IS.

[tarcieri]

@ckaran this would just be in the description of the advisory. cargo audit shows a dependency tree of how crates with advisories are used in the project, allowing the user to see whether it's just clap pulling in yaml-rust or something else.

For what it's worth, I also opened a ticket about potentially doing some sort of structured whitelisting in advisories as well:

https://github.com/RustSec/advisory-db/issues/288

In that case, cargo audit could check the dependency tree automatically, rather than relying on the user to do it.

[ckaran]

@tarcieri that's perfect! The description can be a stopgap, and the ticket can be a future enhancement.