Skip to content

Environment setters are unsound (aka Rust env setters are unsound) #30

Closed
@mmastrac

Description

@mmastrac

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_vars indirectly by triggering the first reqwest access in a process.
  • A second thread was running Python asyncio, which called into gettext to format an unrelated error, which called getenv("LANGUAGE")
  • setenv triggered a realloc, 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

ratmice commented on Jan 23, 2025

@ratmice
added a commit that references this issue on Jan 24, 2025
3ea7c1a
alexcrichton

alexcrichton commented on Jan 24, 2025

@alexcrichton
Owner

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 unsafe equivalents. 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

mmastrac commented on Jan 24, 2025

@mmastrac
CollaboratorAuthor

Thank you so much, @alexcrichton -- I started on the RUSTSEC advisory and I'll include this information once the crate publishes.

mmastrac

mmastrac commented on Jan 24, 2025

@mmastrac
CollaboratorAuthor

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

alexcrichton commented on Jan 24, 2025

@alexcrichton
Owner

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

mmastrac commented on Jan 24, 2025

@mmastrac
CollaboratorAuthor

@alexcrichton no problem, completely understandable.

eric-seppanen

eric-seppanen commented on Jan 24, 2025

@eric-seppanen

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.

This crate is a dependency of rustls-native-certs (a default dependency of hyper-rustls) and native-tls (a default dependency of reqwest). So this is not an obscure case; a large fraction of projects containing http client code are indirectly using it.

$ cargo new --lib test_tls; cd test_tls; cargo add reqwest; cargo tree -i openssl-probe
openssl-probe v0.1.6
└── native-tls v0.2.12
    ├── hyper-tls v0.6.0
    │   └── reqwest v0.12.12
    │       └── test_tls v0.1.0 (/home/eric/work/play/test_tls)
    ├── reqwest v0.12.12 (*)
    └── tokio-native-tls v0.3.1
        ├── hyper-tls v0.6.0 (*)
        └── reqwest v0.12.12 (*)
$ cargo new --lib test_tls; cd test_tls; cargo add hyper-rustls; cargo tree -i openssl-probe
openssl-probe v0.1.6
└── rustls-native-certs v0.8.1
    └── hyper-rustls v0.27.5
        └── test_tls v0.1.0 (/home/eric/work/play/test_tls)
mmastrac

mmastrac commented on Jan 24, 2025

@mmastrac
CollaboratorAuthor

So this is not an obscure case; a large fraction of projects containing http client code are indirectly using it.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Participants

    @ratmice@alexcrichton@mmastrac@eric-seppanen

    Issue actions

      Environment setters are unsound (aka Rust env setters are unsound) · Issue #30 · alexcrichton/openssl-probe