-
Notifications
You must be signed in to change notification settings - Fork 342
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
Collecting metadata for impacted functions in advisories #68
Comments
I think we should just collect a canonical path starting with the crate name, and sans any parameters as an optional attribute of the advisory. Using the advisory from the above description as an example: affected_functions = ["smallvec::SmallVec::insert_many"] |
Sorry to chime in on this after it's been merged. One problem with the current design is that it does not allow for the specification of a version. Why could this be important? Example v1 introduce vulnerable function Just naming all affected_functions What would the change proposal be?
What are the consequences of the proposed change?
Another question would be whether we would want to restrict this to functions, only. Couldn't a data structure have a vulnerability, too? |
Haha, no worries. Modifying the existing
I definitely agree with this. There have been a few advisories filed which don't pertain to a function, but e.g. C FFI-related memory corruption (e.g. How about this:
[affected_paths]
">= 1.0.0, < 1.2.3" = ["mycrate::foo::bar"]
"1.2.3" = ["mycrate::foo::baz"] Or (these are two different TOML syntaxes for the same thing): affected_paths."1.2.3" = [
"mycrate::foo::bar",
"mycrate::foo::baz",
"mycrate::foo::quux",
] |
That sounds good! Personally, I would prefer the first affected_paths syntax you have given. |
Both syntaxes will work, although we can document the first one. |
We need to decide and document whether |
It could potentially list both. Curious what @Inventitech thinks |
Listing both would probably result in conflicting information, too much room for human error, and increased burden for filing advisories. |
Hmm ... hard question. Pros for listing public affected paths are that these are easy to consume for most users. Pros for listing just the offending (maybe private, maybe public) functionality is that it is easy to do for the advisory creator, it is easiest to check for correctness and it keeps the advisories short (this might be an important practical argument). In theory, once we have the directly affected paths, we can just auto-expand to all affected paths in the module. Of course, the danger is that people take a quick manual look at the advisory, see they're not using anything of what is listed under I also discussed this with @jhejderup, and like you, we see pros and cons for both approaches, but in the end, would only mention the root cause paths. Main arguments are that it is easy for the advisory creator, it keeps the advisories short, it does not include parts that can be auto-generated (transitive uses), and it seems to be more future-proof. |
An alternative to listing both under |
We tried using the new syntax in #89, which provides a pretty good real-world example: [affected_paths]
">= 0.4.0, <= 0.10.0" = ["safe_transmute::guarded_transmute_vec_permissive"]
"= 0.10.0" = ["safe_transmute::guarded_transmute_to_bytes_vec"] This now has me wondering if instead of a map from paths to [affected_paths]
"safe_transmute::guarded_transmute_vec_permissive" = [">= 0.4.0, <= 0.10.0"]
"safe_transmute::guarded_transmute_to_bytes_vec" = ["= 0.10.0"] (although ironically this seems to be breaking GitHub's TOML parser?) Perhaps we should use an array of tables instead? [[affected_path]]
name = "safe_transmute::guarded_transmute_vec_permissive"
versions = [">= 0.4.0, <= 0.10.0"]
[[affected_path]]
name = "safe_transmute::guarded_transmute_to_bytes_vec"
versions = ["= 0.10.0"] |
IMHO, it should (see my post #68 (comment)). Anyway, this is only a minor change. Your other suggestion seems a bit more verbose than needs-be, by repeating [[affected_path]]. |
@Inventitech another way of writing the same thing is: affected_paths = [
{ name = "safe_transmute::guarded_transmute_vec_permissive", versions = [">= 0.4.0, <= 0.10.0"] }
] Your version is pretty close to that, aside from using nested arrays instead of an array of tables. The reason for the more verbose form is the less verbose form seems to freak out TOML parsers (see the other version in my GitHub comment). |
@tarcieri How would you specify implemented methods of a trait? |
@vks right now the path parser doesn't support parameters, so it'd be I'm not sure it's worth the effort to disambiguate in the case there are methods with the same name in multiple traits impl'd on the same type: it's fairly uncommon, and even if it were to happen that there's a vulnerability in a method with the same name impl'd in multiple traits, where the code in question happens to be using the not-vulnerable method of a different trait, the worst case is a false positive in the taint analysis, which is the state of affairs for everything today. |
RustPräzi is a crater-like tool which builds a call graph across all of the crates published to crates.io:
https://internals.rust-lang.org/t/prototype-dev-tool-rustprazi-a-tool-to-build-an-entire-call-graph-of-crates-io/8912
It would be interesting to use this tool, perhaps in conjunction with crates-audit, to generate a list of impacted crates based on this call graph.
To do that we'd need to collect structured information about vulnerable functions in advisories. Looking at our existing advisories, that information is often already there, but buried in an unstructured prose description, e.g.:
https://rustsec.org/advisories/RUSTSEC-2018-0003
I think it'd be good to come up with a structured format for this sort of information that can be fed into tools like RustPräzi to determine impacted users of vulnerable crates.
The text was updated successfully, but these errors were encountered: