Add advisory for segfault in openssl-probe due to environment setters #2209
Conversation
Surround the TOML data with code markers.
Add security prose
|
If including a link to the source code, maybe it should be a link to a specific revision of that file, to guarantee readers months from now will see the version of the code you wanted them to see. (In the future that file may not even exist.) |
|
@eric-seppanen, thanks for the feedback -- I updated the advisory with a specific commit link. I've also added the path to the function and a "patched" version. @alexcrichton, please let me know if there is anything I missed here. Thanks for the quick turnaround on the new release. |
| keywords = ["ssl", "openssl", "environment"] | ||
|
|
||
| [affected.functions] | ||
| "openssl_probe::try_init_ssl_cert_env_vars" = ["< 0.1.6"] |
There was a problem hiding this comment.
It might make sense to also include the init_ssl_cert_env_vars function here too?
| The affected function is `try_init_ssl_cert_env_vars` in | ||
| <https://github.com/alexcrichton/openssl-probe/blob/db67c9e5b333b1b4164467b17f5d99207fad004c/src/lib.rs#L65>. | ||
|
|
||
| The crate's author released a fix in versions `>=0.1.6` which marks these functions as `unsafe` and `#[deprecated]`. |
There was a problem hiding this comment.
Two possible points of clarification here:
- Perhaps worth linking to libstd's documentation on
set_var? This advisory is basically "the crate callsset_varand it's bad for all the same reasons thatset_varis being madeunsafenow" - This might be a bit pedantic but the previously-safe versions of the functions are
#[deprecated], but the now-unsafeversions are not#[deprecated]they're justunsafe-and-documented-as-unsafe.
There was a problem hiding this comment.
I agree -- the text was not entirely clear. I've tried to write this in a blame-free retrospective style; please let me know if it reads OK with that in mind.
Update w/Alex's feedback.
Revert cat-on-keyboard issue and clarify text
| # `openssl-probe` may cause memory corruption in multi-threaded processes | ||
|
|
||
| `openssl-probe` offers non-`unsafe` methods that call `std::env::set_var`, which may be called | ||
| in a multithreaded environment, and potentially clash with environment access on other threads. |
There was a problem hiding this comment.
To confirm my own understanding on this, the unsafety here is only possible with non-Rust accesses to env vars, right? I was under the impression that if env vars are exclusively accessed through Rust then set_var is indeed safe, but once non-Rust users are added in there's no way to have it be safe.
It might be worth clarifying that here but the whole purpose of this crate is to interact with openssl which is, by definition, a non-Rust component working with env vars so it's also unlikely that a clarification would actually help much.
There was a problem hiding this comment.
I'll ponder and clarify this a bit -- it's true that we require a non-Rust component in the mix here.
IIRC the per-platform Rust env implementations take a lock for all reads and writes on platforms other than Windows, but given that the guarantee of safe concurrency within pure Rust code is not documented in set_env it might be a small shortcoming in the docs and perhaps an upstream bug is needed there.
|
|
||
| [affected.functions] | ||
| "openssl_probe::try_init_ssl_cert_env_vars" = ["< 0.1.6"] | ||
| "openssl_probe::init_ssl_cert_env_vars" = ["< 0.1.6"] |
There was a problem hiding this comment.
On second thought for the < 0.1.6 bit here (and the line above) - technically this isn't correct where init_ssl_cert_env_vars is still just as unsafe as it was before on 0.1.6+. I'm not sure how that interacts with tooling and this advisory though. Technically though just because someone updates to 0.1.6 doesn't really "fix" things, it's actually so long as these funtions are used then a "fix" is still necessary. In other words the true fix for this advisory isn't actually in openssl-probe, it's in all users of openssl-probe and fixing is more broadly scoped than just updating the openssl-probe crate
There was a problem hiding this comment.
That's a good point -- I believe that a deprecation warning probably means that the API consumer is aware and these functions are no longer "affected" but we may need someone else to weigh in.
There was a problem hiding this comment.
I took one more pass to clarify the role of the Rust platform locks around environment access.
There was a problem hiding this comment.
Imagine I depend on reqwest, which transitively depends on openssl-probe 0.1.5. I see the advisory, which appears to recommend updating to 0.1.6. I run cargo update -p openssl-probe and am satisfied that the problem is solved. But it's not!
I will never see this deprecation warning, because it only appears when compiling dependencies, and warnings in dependencies are normally suppressed.
I think don't think < 0.1.6 should go in the advisory, because it will lead to the wrong outcome for most projects.
There was a problem hiding this comment.
Imagine I depend on
reqwest, which transitively depends onopenssl-probe
Sounds like this advisory has large potential to be very noisy, which is something to consider. We've had a lot of backlash in the past from similarly noisy advisories.
There was a problem hiding this comment.
Perhaps some outreach to the dependent crates would be a good idea? If those crate maintainers receive some warning before this dependency hits everyone, they may be better prepared (e.g. with a fixed release that resolves the issue... somehow).
There was a problem hiding this comment.
I think don't think < 0.1.6 should go in the advisory, because it will lead to the wrong outcome for most projects.
On the other hand, without a "fixed" version, we get an advisory that always triggers, until the dependency is removed entirely. The only other way for tools like cargo-deny to prove that the danger is gone would be to update to a new major version of openssl-probe with the old function names removed. Which doesn't seem to be the plan?
There was a problem hiding this comment.
Right yeah I wasn't planning on doing a major release of openssl-probe as I suspected that would cause even more churn (e.g. warning about duplicate versions, etc).
There was a problem hiding this comment.
@alexcrichton If you'd like, I can put together a 0.2.0 patch where we 1) remove the deprecated functions entirely and 2) put the new unsafe functions behind a feature flag with an appropriately noisy name (__dangerous_single_threaded_environment_setters or similar). Consumers of the API can make a judgment call on whether their use is safe, and more targeted advisories can always be issued down the line if needed.
With that release out, the native-tls project can switch from configuring OpenSSL via env vars to one of the other methods that allow providing paths directly to the OpenSSL context (the vars are really just a convenience). Looks like git2 will also be affected in a similar way.
We can annotate the dangerous methods with appropriate guidance on how OpenSSL can be configured correctly.
Clarification
We tracked down a crash to an interaction with environment setters in openssl-probe and environment access in another thread. CVE scoring was cloned from a previous environment soundness issue (https://nvd.nist.gov/vuln/detail/CVE-2020-26235).
Filed issue with more details: alexcrichton/openssl-probe#30
This is not really a sensitive issue, so it could probably be delayed a bit to give the crate a chance to update.