flate2-rs unsound due to calls to getenv allowing safe data race

[xerxes12354]

rust-lang/flate2-rs#272

Closed without being addressed leaving security vulnerability. Unsound.

[thomcc]

This is honestly a bug in the stdlib that setenv is exposed in safe code.

Work is underway to potentially fix this in the stdlib, although it's tricky to find a way without breakage. In the meantime though, I'm kind of with Alex here that going "looking for FFI crates where this could be a problem" isn't exactly helpful 🤷

[tarcieri]

Based on a quick look I agree. We don't tend to track transitive unsoundness and would rather track a vuln against std for this.

If we don't already have one for this (I don't see one) I'd suggest filing a soundness vuln against std. But even that seems tricky as I don't see a canonical upstream tracking issue for this. This is the closest I've seen so far:

rust-lang/rust#27970
https://github.com/rust-lang/rust/pull/24741/files

It seems like set_var is a huge footgun and perhaps unfixable in its current form, but there's not much consensus about what to do about it.

[xerxes12354]

You cannot create soundness bug using only Rust with this, flate2-rs allows this to occur by unsoundly exposing an unsafe API.

[thomcc]

Again, the bug here is in the stdlib exposing the setenv API as safe in the first place. I suspect the logic you're using is the one that was used at a time, but it's flawed: Rust doesn't exist in a vacuum. For years, C code has held that setenv is unsafe to use in the context of multiple threads, but that getenv is safe (this fact is even documented as part of POSIX). This API is too pervasively used to imagine that realistically Rust can assume safety given that the only users of it are from within Rust.

Note that in many cases, there's no viable solution here for C bindings. In some cases you can cut out the functionality or make it an unsafe fn, in others you can patch the original C library, but even that isn't always viable¹.

IMO, Rust should change the behavior of the std::env::set* functions to only impact the environment as viewed by std::env::var* (and by other rust-visible things like std::process), although possibly this should only take effect after the first thread is spawned.

Also, in practice, this isn't a problem 99% of the time, which is why despite the same issue being present in libstd, it still hasn't been fixed yet. Don't get me wrong: it should be fixed, but... realistically, nobody who isn't setting out to try and trigger this sort of bug will hit it. You have to aggressively perform set_var in a loop from a different thread than the reader, and that just isn't common behavior.

Again, this isn't to say that things are fine without a fix here, but just to say waiting on a fix from stdlib is probably fine, rather than either arguing every C library binding needs to do something about a stdlib soundness hole.

---

¹ And even if patching is viable (e.g. in -sys crates that bundle/vendor) it is honestly kinda dodgy. Personally, I would really rather random -sys crates I use not be applying various custom patches to the underlying C libs they're binding to, especially for something unlikely to cause issues like this.

[jhpratt]

nobody who isn't setting out to try and trigger this sort of bug will hit it.

While I initially said the same thing, it turns out this isn't true. Here's a user saying that the bug (in time & chrono) was discovered in real-world usage.

Personally, I made the decision to eliminate the unsound behavior, though I am reconsidering that decision.

[leo60228]

You cannot create soundness bug using only Rust with this, flate2-rs allows this to occur by unsoundly exposing an unsafe API.

I'm not sure how relevant this is, but wouldn't this implementation of setenv be perfectly POSIX-compliant?

#[no_mangle]
pub unsafe extern "C" fn setenv(key: *const c_char, value: *const c_char, overwrite: c_int) -> c_int {
    if get_running_threads() > 1 {
        unreachable_unchecked()
    } else {
        real_setenv(key, value, overwrite)
    }
}

[tarcieri]

I'm not sure how get_running_threads is intended to be implemented, but regardless that has an inherent race / TOCTTOU in that threads can be created after you invoke it.

Also in what way is the existence of more than one thread "unreachable"? If there's more than one thread, that appears to have undefined behavior.

From what I can tell the only way to soundly implement this sort of thing is acquiring the OS-specific environment lock:

• https://github.com/rust-lang/rust/blob/074f63648bd2368d5ca19aed02b5763a144e5d05/library/std/src/sys/unix/os.rs#L489-L493
• https://github.com/rust-lang/rust/blob/074f63648bd2368d5ca19aed02b5763a144e5d05/library/std/src/sys/unix/os.rs#L531-L554

...and since that isn't directly exposed, the only way to do so is by (vicariously) invoking e.g. std::src:sys::unix::os::{getenv, setenv}

[leo60228]

I was using Rust syntax as an example. My point was that, to my knowledge, calling setenv in the presence of multiple threads is UB according to POSIX.

[tarcieri]

FYI, I opened a rust-internals thread asking if it would make sense to expose functions for obtaining ENV_LOCK in Unix environments:

https://internals.rust-lang.org/t/synchronized-ffi-access-to-posix-environment-variable-functions/15475

[tarcieri]

I was using Rust syntax as an example. My point was that, to my knowledge, calling setenv in the presence of multiple threads is UB according to POSIX.

Yes, notably setenv(3) is marked as MT-Unsafe by glibc: https://www.gnu.org/software/libc/manual/html_node/Environment-Access.html

[tarcieri]

Also, I've changed my mind from my earlier position. I think it would make sense to go ahead and file an advisory for this.

AFAIK there's no change to std possible which will make the existing implementation of flate2 sound in and of itself, only new APIs which flate2 can possibly consume in the future, and given that, we should probably file an advisory.

[thomcc]

AFAIK there's no change to std possible which will make the existing implementation of flate2 sound in and of itself

This isn't true. Here are some of the possible changes that would fix

1. Well, making env::set_var an unsafe function. Which it should be, given that adding locks isn't really/always sufficient to make a MT-Unsafe function safe.
2. Making it so that env::set_var changes are only visible to Rust, possibly deferring this until a second thread has been spawned

The ship has probably sailed for 1, but last I checked, 2 was still being discussed. There are likely others still as well.

[RalfJung]

regardless that has an inherent race / TOCTTOU in that threads can be created after you invoke it.

I don't think it has: if there are no other threads right now, then that means there are also no threads that could be spawning new threads that would make this a race.

[tarcieri]

@thomcc I don't see how 1 helps. The problem is one of synchronization, and in that regard it already has locks and provides a safe API.

The problem is one of unsoundness if there is another caller which isn't participating in that synchronization. Slapping unsafe on it isn't a solution if there's no sound way to use it.

I don't think it has: if there are no other threads right now, then that means there are also no threads that could be spawning new threads that would make this a race.

Aah, indeed...

[thomcc]

Slapping unsafe on it isn't a solution if there's no sound way to use it.

The safety contract of env::set_var would have to include that there are no concurrent readers, which is trivial to prove if you use it before spawning threads, or only from Rust.

I think there's a possible solution between 1 and 2 though.

1. Make set_var only change things visible from std::env functions (as proposed in 2 above).
2. Add a new unsafe function for modifying the real honest-to-god global env, visible to everyone.

As there's no sound way to have the changes visible to FFI at the moment since the lock is not exposed, the first is not really a breaking change. And the second recovers the lost ability, but makes it actually explicit that there's a real thread safety issue.

[joshtriplett]

As there's no sound way to have the changes visible to FFI at the moment since the lock is not exposed, the first is not really a breaking change.

It's very much a breaking change, insofar as people today call set_var and expect the changes to be visible immediately, and that would stop working.

[leo60228]

What about something like this? (Rust-flavored pseudocode, don't pay too much attention to the details)

static RUST_ENV: RwLock<HashMap<String, String>> = Default::default();

pub fn set_var(key: &str, value: &str) {
    RUST_ENV.write().insert(key, value);
}

pub unsafe fn set_system_var(key: &str, value: &str) {
    libc::setenv(key, value);
}

pub fn var(key: &str) -> Option<String> {
    RUST_ENV.get(key).or_else(|| libc::getenv(key))
}

impl Command {
    pub fn spawn(&mut self) -> Result<Child> {
        self.envs(RUST_ENV.iter()).spawn_impl()
    }
}

[DanielJoyce]

What about something like this? (Rust-flavored pseudocode, don't pay too much attention to the details)

static RUST_ENV: RwLock<HashMap<String, String>> = Default::default();

pub fn set_var(key: &str, value: &str) {
    RUST_ENV.write().insert(key, value);
}

pub unsafe fn set_system_var(key: &str, value: &str) {
    libc::setenv(key, value);
}

pub fn var(key: &str) -> Option<String> {
    RUST_ENV.get(key).or_else(|| libc::getenv(key))
}

impl Command {
    pub fn spawn(&mut self) -> Result<Child> {
        self.envs(RUST_ENV.iter()).spawn_impl()
    }
}

This is kinda how Java does with the mutable shared state being System properties, and only providing a System.getenv() method but no set method.

So already recognized as bad practice in java land

Maybe we should torpedo set_var entirely as Java effectively does, and add notes to the libc crate on the gotchas.

[leo60228]

We can't do that without breaking backwards compatibility. I suppose a very literal reading of the breaking change policy could allow this, but it's definitely not reasonable.

[tarcieri]

To me it's really looking like the best course of action would be to deprecate env::set_var. If people really need it for whatever reason, libc::setenv is still available.

env::home_dir was deprecated for less.

[leo60228]

That's fair, but unsoundness requiring a deprecated function is still unsoundness. It seems fair to ignore it, though, considering the alternatives.

[pinkforest]

Sum summarum: flat2-rs unsoundness

There are upstream rust-lang/rust issue(s) and libs are well aware of this by now with extensive discussions.

There is also another issue more general in nature: #1190

time was only re-filed because the maintainer wanted it back - #1242 (comment)

chrono was only re-filed to advertise fix because there was RiR localtime_r in 0.4.20 - #1310

Filing something against something where the flate2 maintainer disagrees that this should be done has made this controversial in addition that there can be no fix in flate2 itself.

Closing this issue in favor of the upstream issue rust-lang/rust#90308 - if needed re: specific to flate2 - please re-open.

[FrankHB]

Again, the bug here is in the stdlib exposing the setenv API as safe in the first place. I suspect the logic you're using is the one that was used at a time, but it's flawed: Rust doesn't exist in a vacuum. For years, C code has held that setenv is unsafe to use in the context of multiple threads, but that getenv is safe (this fact is even documented as part of POSIX). This API is too pervasively used to imagine that realistically Rust can assume safety given that the only users of it are from within Rust.

The logic is for various reasons in conventional all software development:

• This is the place to tracking issues, even not all of them are bugs.
• This is a public place searchable by all users, not necesarily of your project. Actually I come from Google result for some questions about getenv usability in multithreading environment even not related to Rust. That does not make the report in this issue useless.
• On the other hand, "Close" is a conventional primitive for all kinds of issue tracking. This includes the status of "CLOSED INVALID"/"CLOSED WONTFIX"/"CLOSED WORKSFORME"/"CLOSED DUP"/"CLOSED ERRATA"/... in traditional bugzilla instances. (Ironically, bugzilla is actually not just for "bugs"...) But instead of "the resolution is known and settled down", the reason you choose to close is just "we decide to have nothing to do further due to exteranl limitations", which is traditonally a separated top-level status like "SUSPENDED" or "ON HOLD".
• Sadly GitHub issues cannot reflect this need (and worse, the final action is marked closed "as completed"). This is confusing in nature, because the remaining majority of developers will find the status is more closed to "OPEN", while you have no public documentation to override the commen sense of how to handle issues in general. If you choose to just inherit GitHub's issue model to ease of your life at the cost of inconvenience for arbitrary readers from public space, that is your freedom; but note that you will eventually reiterate your reasoning, as you are still the minority.

[RalfJung]

Please don't necro-post. The discussion here is long over, and if there's new things to discuss it should be done in a new issue.