-
Notifications
You must be signed in to change notification settings - Fork 406
Add advisory for segfault in openssl-probe due to environment setters #2209
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
Changes from 10 commits
24d2d14
42844c4
26f7af9
2c95894
a285059
7d47527
83e4283
baa0e08
ece1d78
09303e9
88789ba
874bdda
f079478
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
```toml | ||
[advisory] | ||
id = "RUSTSEC-0000-0000" | ||
package = "openssl-probe" | ||
date = "2025-01-10" | ||
url = "https://github.com/alexcrichton/openssl-probe/issues/30" | ||
references = ["https://www.edgedb.com/blog/c-stdlib-isn-t-threadsafe-and-even-safe-rust-didn-t-save-us"] | ||
informational = "unsound" | ||
categories = ["memory-corruption"] | ||
cvss = "CVSS:3.1/AV:N/AC:H/PR:L/UI:N/S:U/C:N/I:N/A:H" | ||
keywords = ["ssl", "openssl", "environment"] | ||
|
||
[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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On second thought for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I took one more pass to clarify the role of the Rust platform locks around environment access. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Imagine I depend on 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eric-seppanen native-tls did use it, it's been fixed. rustls-native-certs wasn't affected but was bumped. All relevant crate owners above were brought into the thread. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (if we end up releasing a 0.2.0 with the affected functions behind a bigger wall, we'll have to do that dance again) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this advisory we're faced with two bad choices:
Which leads us to a new option:
Is it worth it, considering the ecosystem churn caused by 0.1.x and 0.2.0 coexisting for a period? That's harder to answer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apologies for being long-winded. I was trying to respond to
Great! Unfortunately, nothing in this advisory says "update to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the advisory definitely needs to be actionable. We published a similar advisory for It would be good to coordinate with major notable downstream dependencies (there are at least 7 with more than a million downloads) to ensure that when we publish the advisory, users of those crates simply need to |
||
|
||
[affected] | ||
os = ["linux"] | ||
|
||
[versions] | ||
patched = [">= 0.1.6"] | ||
``` | ||
|
||
# `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. | ||
mmastrac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
In pure Rust code, concurrent read and write access to the environment is actually safe due to a lock | ||
taken in the platform implementations of the environment accessors (the documentation does not | ||
state this, and it's possible it _could_ change in the future). Libraries using other runtimes | ||
(including Python, those written in pure C and others) do not make use of these internal Rust | ||
environment locks, however, and instead use their own locks, or unprotected raw access to `libc`'s | ||
`getenv`, `setenv`, or even worse, `char** environ`. | ||
|
||
When these methods in `openssl-probe` (or that matter, any other pure Rust code calling `std::env::set_env`) | ||
are called while other threads are active and accessing the environment, it | ||
may cause other threads to access dangling environment pointers in the cases where the underlying | ||
environment data is moved or resized in response to an additional environment variable being | ||
added, or a variable's contents being enlarged. | ||
|
||
This is shown to occur on Linux, but it will also likely occur on any other platform where `getenv` | ||
and `setenv` are not thread-safe, though trigger conditions may vary widely. | ||
|
||
Note that these function calls are completely safe and sound in purely single-threaded environments, | ||
or multi-threaded environments where it can be proven that no simultaneous read and writes to the | ||
environment occur. | ||
|
||
## Rust's `set_env` | ||
|
||
This crate, and all other callers of the Rust `set_env` function (<https://doc.rust-lang.org/std/env/fn.set_var.html>) | ||
are unsound due to the unfortunate reality of the POSIX standard which defines these enviornment access methods | ||
mmastrac marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
without making any sort of thread-safety guarantees. | ||
|
||
In Rust's 2024 edition `std::env::set_var` is marked as `unsafe` and the documentation was updated to note | ||
that the only safe way to use these functions is in a single-threaded context. | ||
|
||
## Affected Code | ||
|
||
The affected functions are `init_ssl_cert_env_vars` and `try_init_ssl_cert_env_vars` in | ||
<https://github.com/alexcrichton/openssl-probe/blob/db67c9e5b333b1b4164467b17f5d99207fad004c/src/lib.rs#L52> and <https://github.com/alexcrichton/openssl-probe/blob/db67c9e5b333b1b4164467b17f5d99207fad004c/src/lib.rs#L65>, respectively, and | ||
any other crate's call-graph which may call this function directly or indirectly | ||
<[https://github.com/search?q=try_init_ssl_cert_env_vars&type=code](https://github.com/search?q=try_init_ssl_cert_env_vars+OR+init_ssl_cert_env_vars&type=code)>. `native_tls <= 0.2.12` may | ||
do so in certain configurations <https://github.com/sfackler/rust-native-tls/blob/2424bc5efd1b8b4bcf60dbda93259a3f29db7f06/Cargo.toml>. | ||
|
||
The crate's author released a fix in versions `>=0.1.6` which marks these functions as `#[deprecated]` and adds | ||
new `unsafe` equivalents with safety guidance <https://github.com/alexcrichton/openssl-probe/commit/3ea7c1af24d7f03c5786872f06ff066e03b75138>. | ||
|
||
## Alternative Mitigations | ||
|
||
In the case of glibc users, some future thread-safety improvements may protect you from `setenv`/`getenv` clashes | ||
which were introduced in <https://github.com/bminor/glibc/commit/7a61e7f557a97ab597d6fca5e2d1f13f65685c61>, | ||
however direct `environ` access in multithreaded programs will still risk dangling pointer access. | ||
|
||
Users of other `libc` implementations should consult their sourcecode listings for thread-safety guarantees | ||
around multithreaded environment read/write access, though readers should be prepared to be disappointed. |
Uh oh!
There was an error while loading. Please reload this page.