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

Improve output for RustSec advisories #288

Closed
repi opened this issue Oct 17, 2020 · 4 comments · Fixed by #296
Closed

Improve output for RustSec advisories #288

repi opened this issue Oct 17, 2020 · 4 comments · Fixed by #296
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@repi
Copy link
Contributor

repi commented Oct 17, 2020

This is how cargo-audit outputs when it detects a security vulnerability:

ID:       RUSTSEC-2020-0041
Crate:    sized-chunks
Version:  0.6.2
Date:     2020-09-06
URL:      https://rustsec.org/advisories/RUSTSEC-2020-0041
Title:    Multiple soundness issues in Chunk and InlineArray
Solution:  No safe upgrade is available!
Dependency tree: 
sized-chunks 0.6.2
└── im 15.0.0
   ...

compared to current cargo-deny:

error[RUSTSEC-2020-0041]: Multiple soundness issues in Chunk and InlineArray
    ┌─ C:\git\embark\ark\Cargo.lock:454:1
    │
454 │ sized-chunks 0.6.2 registry+https://github.com/rust-lang/crates.io-index
    │ ------------------------------------------------------------------------ security vulnerability detected
    │
    = Chunk:

      * Array size is not checked when constructed with `unit()` and `pair()`.
      * Array size is not checked when constructed with `From<InlineArray<A, T>>`.
      * `Clone` and `insert_from` are not panic-safe; A panicking iterator causes memory safety issues with them.

      InlineArray:

      * Generates unaligned references for types with a large alignment requirement.
    = URL: https://github.com/bodil/sized-chunks/issues/11
    = sized-chunks v0.6.2
      └── im v15.0.0   

They are both quite good and both have their pros and cons.

The cargo-deny one can be somewhat verbose, but depends on how the description of the RustSec issue was. In the above example it was just technical implementation details which is likely less useful in the context directly from cargo-deny, but believe other issues are more clear there.

What I am missing though is the link, to be able to include in PRs/commits and to check if there is more info there directly: https://rustsec.org/advisories/RUSTSEC-2020-0041 in the cargo-deny output I have to copy'n'paste out the id and then web search for it.

I also like that both the RustSec website and cargo-audit spells out "Patched Versions: None!" and "Solution: No safe upgrade is available!" respectively.

Think we should add both the "Solution" (even if one is missing) and the full URL to the RustSec page in the cargo-deny output also.

@repi repi added enhancement New feature or request good first issue Good for newcomers labels Oct 17, 2020
@tomasfarias
Copy link
Contributor

If possible, I'd like to take this one 😄

Just had a question before I get to work: where should new output lines be located?
I think it would make sense to have them right under the ID/Title, i.e.:

error[RUSTSEC-2020-0041]: Multiple soundness issues in Chunk and InlineArray
URL: https://rustsec.org/advisories/RUSTSEC-2020-0041
Patched Versions: None!
Solution: No safe upgrade is available!

    ┌─ C:\git\embark\ark\Cargo.lock:454:1
    │
454 │ sized-chunks 0.6.2 registry+https://github.com/rust-lang/crates.io-index
    │ ------------------------------------------------------------------------ security vulnerability detected
    │
...

@repi
Copy link
Contributor Author

repi commented Oct 20, 2020

Thanks!

That does look quite good, but a bit hard to say what is the best :) Maybe have it indented somewhat also? Not sure

@tomasfarias
Copy link
Contributor

tomasfarias commented Oct 20, 2020

Alright, my development version is now reporting URL (I removed sized-chunks from the exclude list in deny.toml just to get this test output):

❯ target/debug/cargo-deny check
error[A001]: Multiple soundness issues in Chunk and InlineArray
    ┌─ /home/tomasfarias/src/github.com/tomasfarias/cargo-deny/Cargo.lock:107:1
    │
107 │ sized-chunks 0.6.2 registry+https://github.com/rust-lang/crates.io-index
    │ ------------------------------------------------------------------------ security vulnerability detected
    │
    = ID: RUSTSEC-2020-0041
    = URL: https://rustsec.org/advisories/RUSTSEC-2020-0041
    = Chunk:

      * Array size is not checked when constructed with `unit()` and `pair()`.
      * Array size is not checked when constructed with `From<InlineArray<A, T>>`.
      * `Clone` and `insert_from` are not panic-safe; A panicking iterator causes memory safety issues with them.

      InlineArray:

      * Generates unaligned references for types with a large alignment requirement.
    = URL: https://github.com/bodil/sized-chunks/issues/11
    = sized-chunks v0.6.2
      └── cargo-deny v0.7.3

advisories FAILED, bans ok, licenses ok, sources ok

Would be great to get some comments regarding how this looks, I'll open up a draft PR shortly so that changes can also be commented there.

@tomasfarias
Copy link
Contributor

tomasfarias commented Oct 20, 2020

While exploring cargo-audit to understand how to obtain the required patched versions for the Solution report, I started thinking having both "Patched Versions: None!" and "Solution: No safe upgrade is available!" may be somewhat redundant.

For reference, this is how cargo-audit reports "Solution":

if vulnerability.versions.patched.is_empty() {
    self.print_attr(Red, "Solution:     ", "No safe upgrade is available!");
} else {
    self.print_attr(
           Red,
           "Solution:     ",
           String::from("Upgrade to ")
                + &vulnerability
                     .versions
                     .patched
                     .iter()
                     .map(ToString::to_string)
                     .collect::<Vec<_>>()
                     .as_slice()
                     .join(" OR "),
      );
}

Wouldn't adding a "Patched Versions" line simply repeat all available versions already being listed by iterating over vulnerability.versions.patched?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
2 participants