Environment setters are unsound (aka Rust env setters are unsound) #30
Description
We've been tracking down a failure in our CI for a few days and in the end, the culprit was openssl_probe::init_ssl_cert_env_vars being called in a thread while other code that used getenv for a completely unrelated purpose was running in another thread.
https://github.com/sfackler/rust-native-tls/blob/master/src/imp/openssl.rs#L88
Unfortunately, these particular methods are landmines, and should probably be gated behind an unsafe feature while Rust 2024 is in the process of deprecating the "safe" version of set_env.
This is not a security vuln per-se, but it's certainly a major crash risk in complex code that makes use of any non-rust code.
What ended up happening for us is this:
- One thread called
init_ssl_cert_env_varsindirectly by triggering the firstreqwestaccess in a process. - A second thread was running Python asyncio, which called into
gettextto format an unrelated error, which calledgetenv("LANGUAGE") setenvtriggered arealloc, causing the environ to move while it was being scanned by the second thread- The second thread ends up reading an invalid pointer and segfaulting, bringing the whole process down
I would strongly encourage:
- Adding an "unsafe_set_env" feature and gating all environment-setting functions behind this
- Bumping the version and highly discouraging use of the older versions of the crate (possibly via a dependabot alert or CVE, regardless of the minimal actual security risk)
As a workaround, downstream crates like rust-native-tls should probe for paths as strings, and directly configure the SSL contexts with those paths, rather than attempting to set them in the environment directly.
Activity
ratmice commentedon Jan 23, 2025
Deprecate env-var-writing functions
alexcrichton commentedon Jan 24, 2025
Thanks for the report! As you can probably tell I haven't touched this crate in quite some time and this was more-or-less written for a different age. Nowadays the best solution to this issue is "stop using this crate" with libraries like rustls.
Nevertheless I'll try to do what at least I think is reasonable here. In #33 I'm going to deprecate the safe versions of functions and replace them with
unsafeequivalents. That means the functionality is all staying but it's#[deprecated]to warn-on-use. I don't think an 0.2 of this crate is going to happen so it's kind of the best option available being stuck on 0.1. This will still require all users of this crate to update their own usage but that was sort of inevitable anyway.mmastrac commentedon Jan 24, 2025
Thank you so much, @alexcrichton -- I started on the RUSTSEC advisory and I'll include this information once the crate publishes.
mmastrac commentedon Jan 24, 2025
FWIW, I'd be happy to tackle the maintenance of the crate if you'd like -- I suspect there will be a need for this crate for a few more years in some obscure cases that involve communication with obsolete systems.
alexcrichton commentedon Jan 24, 2025
That'd be very helpful, thank you! I've added you as a collaborator now. If you're ok with it I'll still hold on to the publish rights for a version or two (if necessary), but I'd be happy to add you as well to that in the future.
(no offsense meant, I just don't know much about you so would prefer to take things slowly here)
mmastrac commentedon Jan 24, 2025
@alexcrichton no problem, completely understandable.
eric-seppanen commentedon Jan 24, 2025
This crate is a dependency of
rustls-native-certs(a default dependency ofhyper-rustls) andnative-tls(a default dependency ofreqwest). So this is not an obscure case; a large fraction of projects containing http client code are indirectly using it.mmastrac commentedon Jan 24, 2025
Agreed -- to clarify, I mean that the option of moving to rustls is not available to all current users of OpenSSL, as rustls does not support certain older TLS and SSL standards that are still available in certain OpenSSL builds.