rustsec icon indicating copy to clipboard operation
rustsec copied to clipboard

Cargo audit false positives for optional dependencies pulled in by disabled features.

Open 0xForerunner opened this issue 2 years ago • 11 comments

I'll give a minimal example here. In my toml I have sqlx

sqlx = { version = "0.7", default-features = false, features = [
    "macros",
    "runtime-tokio-native-tls",
    "migrate",
    "postgres",
    "chrono",
] }

Checking my actual dependencies with cargo tree reveals:

cargo tree | grep sql                                                                                                                                                                                                                                                                                                                                                
│   ├── sqlx-core v0.7.3
│   │   ├── sqlformat v0.2.3
├── sqlx v0.7.3
│   ├── sqlx-core v0.7.3 (*)
│   ├── sqlx-macros v0.7.3 (proc-macro)
│   │   ├── sqlx-core v0.7.3
│   │   │   ├── sqlformat v0.2.3 (*)
│   │   ├── sqlx-macros-core v0.7.3
│   │   │   ├── sqlx-core v0.7.3 (*)
│   │   │   ├── sqlx-postgres v0.7.3
│   │   │   │   ├── sqlx-core v0.7.3 (*)
│   └── sqlx-postgres v0.7.3
│       ├── sqlx-core v0.7.3 (*)

And yet I still get the error

Crate:     rsa
Version:   0.9.6
Title:     Marvin Attack: potential key recovery through timing sidechannels
Date:      2023-11-22
ID:        RUSTSEC-2023-0071
URL:       https://rustsec.org/advisories/RUSTSEC-2023-0071
Severity:  5.9 (medium)
Solution:  No fixed upgrade is available!
Dependency tree:
rsa 0.9.6
└── sqlx-mysql 0.7.3
    ├── sqlx-macros-core 0.7.3
    │   └── sqlx-macros 0.7.3
    │       └── sqlx 0.7.3
    │           └── signup-sequencer 2.0.0
    └── sqlx 0.7.3

I understand this is due to cargo-audit simply scanning the lock file, but I imagine if cargo tree is smart enough to omit these deps then the same should be possible in cargo-audit. Let me know what you think!

0xForerunner avatar Feb 14 '24 17:02 0xForerunner

Given cargo-audit's role as a Cargo.lock analyzer, there's little we can do without implementing a much more complicated analysis which can incorporate Cargo.toml as well, which has been discussed in the past.

IMO the real issue here is that the Cargo resolver is including these dependencies in Cargo.lock in the first place. I believe this issue might be relevant:

https://github.com/rust-lang/cargo/issues/10801

tarcieri avatar Feb 14 '24 17:02 tarcieri

Yeah I can definitely see your point there. That's a pretty old issue though and this is a pretty important feature for a project like this. Would it be possible for some type of temporary solution when we run cargo tree in the background and filter results based off that? I know that feels a bit dirty, but perhaps warranted?

0xForerunner avatar Feb 14 '24 18:02 0xForerunner

cargo-tree isn't exactly designed for programmatic consumption, and it's a heavy dependency.

To properly consume data from Cargo.toml files / Rust workspaces it would be better if we optionally linked with cargo the same way cargo-tree does. That could probably use its own issue (I'm kind of surprised we don't have a general tracking issue for it already, just a lot of discussion of potentially doing that).

tarcieri avatar Feb 14 '24 18:02 tarcieri

Gotcha. Okay well we've got this documented here for now. Let me know if you come up with any clever solutions to this, cheers!

0xForerunner avatar Feb 14 '24 18:02 0xForerunner

Yeah, this is definitely a worthwhile issue to track, whether the solution happens upstream in Cargo or via new features added to cargo-audit.

tarcieri avatar Feb 14 '24 18:02 tarcieri

The way to go for filtering would be cargo metadata, but that runs into its own issues and limitations, as I've learned by working on cargo auditable.

Shnatsel avatar Feb 14 '24 18:02 Shnatsel

The cargo issue I linked earlier suggests this issue occurs with cargo metadata as well

tarcieri avatar Feb 14 '24 18:02 tarcieri

It seems this issue is fixed in cargo deny: https://github.com/rust-lang/cargo/issues/10801

~~@ewoolsey using cargo deny instead could be a reasonable workaround.~~

Shnatsel avatar Feb 14 '24 19:02 Shnatsel

Nevermind, I think it's only fixed for other lints, and security audits still run into this due to looking only at Cargo.lock

Shnatsel avatar Feb 14 '24 19:02 Shnatsel

I think the issue is fixed inside the krate library that uses cargo deny under the hood. More specifically it was fixed in response to this https://github.com/EmbarkStudios/krates/issues/41 Maybe using krate would be a viable option.

stormshield-gt avatar Jul 26 '24 09:07 stormshield-gt

krate looks interesting!

tarcieri avatar Jul 26 '24 12:07 tarcieri