Skip to content
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.

Sign up for GitHub

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

Add advisory for segfault in openssl-probe due to environment setters #2209

Closed
wants to merge 13 commits into from
Closed

Add advisory for segfault in openssl-probe due to environment setters #2209

Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
24d2d14
Create RUSTSEC-0000-0000.md
mmastrac Jan 22, 2025
42844c4
Update RUSTSEC-0000-0000.md
mmastrac Jan 22, 2025
26f7af9
Update RUSTSEC-0000-0000.md
mmastrac Jan 22, 2025
2c95894
Update RUSTSEC-0000-0000.md
mmastrac Jan 22, 2025
a285059
Update RUSTSEC-0000-0000.md
mmastrac Jan 23, 2025
7d47527
Update RUSTSEC-0000-0000.md
mmastrac Jan 23, 2025
83e4283
Update RUSTSEC-0000-0000.md
mmastrac Jan 23, 2025
baa0e08
Update RUSTSEC-0000-0000.md
mmastrac Jan 23, 2025
ece1d78
Update RUSTSEC-0000-0000.md
mmastrac Jan 23, 2025
09303e9
Update RUSTSEC-0000-0000.md
mmastrac Jan 23, 2025
88789ba
Update crates/openssl-probe/RUSTSEC-0000-0000.md
mmastrac Jan 30, 2025
874bdda
Update RUSTSEC-0000-0000.md
mmastrac Jan 30, 2025
f079478
Update RUSTSEC-0000-0000.md
mmastrac Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions crates/openssl-probe/RUSTSEC-0000-0000.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
```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"]
mmastrac marked this conversation as resolved.
Show resolved Hide resolved
"openssl_probe::init_ssl_cert_env_vars" = ["< 0.1.6"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link

@eric-seppanen eric-seppanen Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@tarcieri tarcieri Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imagine I depend on reqwest, which transitively depends on openssl-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.

Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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)

Copy link

@eric-seppanen eric-seppanen Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this advisory we're faced with two bad choices:

  1. Set the advisory to say "< 0.1.6"; downstream workspaces can bump openssl-probe to 0.1.6 in Cargo.lock and tooling (cargo audit or cargo deny) says the problem is gone. But nothing was fixed! The deprecation warning will be hidden if it's in a dependency, so these projects are still using dangerous code. This advisory didn't really have the desired effect.
  2. Set the advisory to say "0.1.x"; which is an advisory that can never be cleared (only manually ignored) because that covers all released versions. This advisory had a stronger effect but is annoying because every downstream project that has automated rustsec checks has to take actions beyond updating a dependency. This will also annoy maintainers of dependent crates that are "fixed" (not calling the dangerous functions), because automated rustsec checks can't tell the difference.

Which leads us to a new option:

  1. Release 0.2.0 with the safe-but-maybe-unsound function names removed, and have the advisory apply to all versions "< 0.2.0". For automated advisory tooling, this is the best outcome: downstream projects can resolve the advisory by bumping dependencies until 0.1.x is gone from Cargo.lock. This assumes that the major dependent crates cooperate by replacing 0.1.6 with 0.2.0. If they don't then this is a pointless exercise.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for being long-winded. I was trying to respond to

native-tls did use it, it's been fixed

Great! Unfortunately, nothing in this advisory says "update to native-tls 0.2.13". This advisory tells projects to bump openssl-probe to 0.1.6, which for the vast majority of workspaces will not fix anything.

Copy link
Member

Choose a reason for hiding this comment

The 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 chrono (RUSTSEC-2020-0159) which wasn't actionable and got a considerable amount of blowback, to the point that we ended up withdrawing it.

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 cargo update


[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

When these methods 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.
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.
Loading