search

rustsec / advisory-db

Security advisory database for Rust crates published through crates.io
https://rustsec.org
Other
909 stars 357 forks source link

Added advisory for undefined behavior in openssl #2021

Closed alex closed 2 months ago

Shnatsel commented 2 months ago

Is the length of the slice attacker-controlled? If not, I would rather file this as informational = unsound.

Also, this makes me wonder if there are any other similarly problematic calls to slice::from_raw_parts in the codebase that should be fixed before we publish an advisory, so that users could upgrade all at once instead of getting several different notifications.

alex commented 2 months ago

This API is used all over the place, there's at least a few places where the string length will be attacker controlled, yes.

And there may be others, I filed this before doing the variant analysis.

Shnatsel commented 2 months ago

There's about a dozen places where a slice is constructed from raw parts:

https://github.com/search?q=repo%3Asfackler%2Frust-openssl%20slice%3A%3Afrom_raw_parts&type=code

Perhaps we should just wrap all of them in a helper function that explicitly checks for the zero length, just in case?

alex commented 2 months ago

It's not a bad idea, a quick review gives me a few that I'm skeptical of (but which I don't have a PoC for), and most of them aren't problems or already have checks.

alex commented 2 months ago

Ok, on further review (and reading OpenSSL's code) I don't see any others that should be able to trigger UB.

It may still be worth it to have a utility function that handles this case as a best practice, but I don't think it needs to block this advisory.

Shnatsel commented 2 months ago

Could you open a PR to refactor them with a helper function with a zero check then? And after that I'd be happy to publish an advisory once that's released to crates.io. Since it's not clear how one would go about exploiting this, I wouldn't want to publish a non-actionable advisory, or several different advisories.

alex commented 2 months ago

https://github.com/sfackler/rust-openssl/pull/2268 has the refactor, but I'm not going to do another release for this in the absence of any way to trigger this issue.