Unsafe std::env::set_var change

The difficulty is that you can't wrap all FFI code on (at least) Windows, Linux and macOS, because of deferred dynamic initialization; in other words, there is a route for non-Rust code to run such that the Rust code is unable to wrap it, and instead has to assume it might be running concurrently.

One of the preconditions that the caller of setenv in POSIX is responsible for upholding is that no threads exist that might be calling getenv concurrently with setenv. Because you can't do anything about threads started during deferred dynamic initialization in the general case, you can't uphold this precondition in all cases - you need special knowledge of the non-Rust code you're calling to confirm that it hasn't done this.

Yes, but every call to externally-compiler C code is unsafe. Thus you can add requirement to call that C lock to the requirements of FFI builders.

Not impossible, but that's where it turns into use of the nuke to kill a cockroach: instead of asking [very few] users of std::env::set_var change you are now asking thousands if not millions of users of FFI to alter their crates for safety.

Hardly a usable solution, but it's not impossible. Heck, some languages have actually done similar choice, namely C# and Java.

This isn't just about calls to externally-compiled C code. It's about linking externally compiled C code which you don't call via FFI; just having C code linked into your ELF or PE binary can be enough to start threads outside your control, and you need those threads (which are not called via FFI) to respect the environment lock, not just the FFI calls.

Somehow, you need to ensure that when my C++ constructor starts a thread, and that thread calls getenv at arbitrary times (possibly indirectly if it's looking up a logging host to do a background flush of data to the cloud), it does so respecting the Rust rules for setenv and similar. But there's no Rust involved in the path from "binary on disk" to "call to getenv", so Rust can't interpose a lock here, since there's no FFI call to lock around.

Java's solution (I've not looked at C#) is to not have setenv and other such calls that are not MT-safe exposed in the Java language. Once you've done that, you're restricting the danger to places that are already doing FFI, and are thus exposed to danger via FFI. Rust could remove set_env completely, and say that if you want to manipulate your own environment, you must do so via FFI calls, and then the Java solution can be made to work. But that's a non-starter if you want to both expose POSIX setenv and permit Rust to link against arbitrary C code.

I'm not sure what you are talking about. Rust already couldn't guarantee any safety if you allow one to link random libraries to your Rust process as XZ story have shown us.

Yes, it's inherently unsafe act to link externally compiled C code which you don't call via FFI which means someone who does that have to guarantee safety.

But there is Rust developer involved in such process and s/he would have to ensure that everything works as it should. With or without set_var change.

Arbitrary code linked into your process may compromise safety of your program in many ways, why call to set_var or pthread_create executed in such code should be considered safe while other things are off-limits?

You couldn't permit Rust to link against arbitrary C code. Not even worth discussing. Impossible. Full stop.

You may permit Rust to link against arbitrary non-malicious C code and then the question of what code is malicious and which code is non-malicious opens.

Because we're saying that on the Rust side, set_var is unsafe, which tells the Rust caller that it's not considered safe. And the reason we're doing that is that the C code could call getenv and breach Rust's safety rules at an arbitrary moment, where we can't put an FFI lock around it (since the thread that calls getenv has never been directly involved in FFI).

If you want set_var to be considered safe, then you need to find a way to address the problem of threads that never go near Rust code calling getenv; it's already documented that calling setenv is unsafe if you have multiple threads in your program. Or we go down the route of marking env::set_var as unsafe, and then it's on the Rust code to meet the safety requirements, per normal Rust rules.

Declare these threads unsafe. Problem solved. You couldn't declare all C code “safe”, anyway.

But they're not unsafe - everything they do is, per POSIX rules, safe. It's the caller of setenv who's breaking the POSIX safety rules.

Shrugs. What “enhanced” liblzma library did was also per POSIX rules safe.

Argument “we have to support arbitrary POSIX-safe code” is deeply on the “feasibility” side of the fence, not “possibility” side of the fance.

And I already told many times that I don't think this path is feasible.

It's perfectly possible, though.

Rust has as a goal being a systems language to replace C on POSIX systems.. That implies supporting arbitrary POSIX-safe code, because that's part of being a systems language that replaces C on POSIX systems.

Now, if we declare that Rust is no longer going to have the goals it has today, we can make our POSIX interface safe, by wrapping up all of our calls to POSIX-defined functionality like setenv behind locks, and by defining it as the fault of the other code that it doesn't follow Rust's safety rules for setenv and similar, but instead follows POSIX's rules for POSIX-defined functionality.

Or we could not support setenv, and do what Java has done with ProcessBuilder such that you never need setenv.

But both of those are in conflict with the Rust goal of being a systems language replacing C on POSIX systems; Java doesn't want to be a systems language for POSIX systems (it wants to be a language for JVM systems), so escapes this.

I cannot see a way to reconcile Rust's goal of being a systems language for POSIX systems in which you can express all the programs you might otherwise express in safe POSIX C (where the safety rules are all human-checked) with the goal of making getenv and setenv POSIX calls safe per Rust's safety rules, given that the Rust compiler cannot check POSIX's safety rules for those calls. And that's why std::env::set_var is becoming unsafe - there is a POSIX safety rule for setenv that cannot be compiler-checked or compiler-enforced, and our choices are to rule out being a POSIX systems language in which you can express all useful POSIX-safe code (albeit you have to use unsafe for cases where the POSIX rules require you to check conditions required for Rust rules manually), or to mark setenv calls as unsafe, since POSIX requires you to check a global condition that Rust cannot check.

you're confusing memory safety with cybersecurty. if i write ransomware in pure safe rust, it will be memory safe, but not many people would describe it as safe to run.

Absolutely not. Have you actually looked on the Wikipedia article? Here's the relevant quote:

The code uses the glibc IFUNC mechanism to replace an existing function in OpenSSH called RSA_public_decrypt with a malicious version.

And before confusion would settle: it uses IFUNC mechanism to replace RSA_public_decrypt function with another one, but said RSA_public_decrypt function is not based on IFUNC, it could replace, monkey-patch or process any function in Rust standard library (and elsewhere), too.

Sure, but ransomware written in safe rust wouldn't be able to replace pieces of other code except via some kind of unsafe tool.

I was talking about this “code replacement at distance via shared library loaded in the process” part, not about other aspects of the attack.

No, but I was an active contributor to nixpkgs when it happened, so I saw everything unfold in realtime. The security holes it took advantage of were:

  1. release tarballs being different from the repo
  2. noone can read m4 macros, and they are rarely audited
  3. hiding a payload within fake test data

once you run unsandboxed malicious code, it's already game over. memory safe languages won't save you.

completely untrue. there's several interfaces that can sidestep rusts security guarantees, such as /proc/self/mem, or by simply modifying the executable so that it will execute malicious code next time it is run.

Yeah, but the interesting thing in the exploit was the fact that no one ever explicitly “run” any lzma or xz functions! Malicious code was “simply linked in” and yet it modified structures that are, normally, protected.

That's the interesting part that from Rust safety and FFI, not how m4 macros were [ab]used.

I've found some time to try it out... and after all, I've no idea where the problem is, that set_var should be marked unsafe.

This threading code, calling std::env::var and std::env::set_var the whole time, runs forever. Tried with gnu and musl libc. There is no error, no crash and the data is correct, of course you can't be sure which value is set when and if the getter-thread runs before or after a setter-thread, because you have no control when the thread is scheduled. But same problem exists during file writing:

use std::{
    io::{self, Write as _},
    thread,
    time::Duration,
};

use nanorand::Rng;

const ENV_VARNAME: &str = "ENV_TEST_VAR";
const ERROR_WAIT_US: Duration = Duration::from_micros(10);
const THREAD_COUNT: usize = 20;
const THREAD_SLEEP_MS: Duration = Duration::from_millis(1000);

fn rnd_value(len: u8, rng: &mut nanorand::tls::TlsWyRand) -> String {
    let charset = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
    let mut value = format!("{}", len + 1);
    for _ in 0..len {
        let index = rng.generate_range(0..charset.len());
        value.push_str(&charset[index..=index]);
    }
    value
}

fn main() {
    let env_value = std::env::var(ENV_VARNAME);
    assert!(env_value.is_err());

    const JH: Option<thread::JoinHandle<()>> = None;
    let mut th_getenv = [JH; THREAD_COUNT];

    for thread in &mut th_getenv {
        *thread = Some(thread::spawn(|| loop {
            let env_value = std::env::var(ENV_VARNAME);
            match env_value {
                Ok(env_value) => {
                    let _ = writeln!(io::stdout(), "env::var: {ENV_VARNAME}: {env_value}");
                    let c = env_value.chars().next().expect("no char");
                    assert!(env_value.len() as u32 == c.to_digit(10).expect("no digit"));
                }
                Err(err) => {
                    let _ = writeln!(io::stderr(), "env::var fail: {err:?}");
                    thread::sleep(ERROR_WAIT_US);
                }
            }
        }));
    }

    let mut th_setenv = [JH; THREAD_COUNT];

    for thread in &mut th_setenv {
        *thread = Some(thread::spawn(|| {
            let mut rng = nanorand::tls_rng();

            loop {
                let value_len = rng.generate_range(1u8..9);
                let value = rnd_value(value_len, &mut rng);
                // edition > 2021 - Why???
                // unsafe { std::env::set_var(ENV_VARNAME, value); }
                // edition <= 2021
                std::env::set_var(ENV_VARNAME, &value);
                // no error handling possible
                let _ = writeln!(io::stdout(), "env::set_var: {ENV_VARNAME}: {value}");
            }
        }));
    }

    loop {
        thread::sleep(THREAD_SLEEP_MS);
    }
}

More or less the same works also in C++23. Here stdout/err is not locked by default, like in Rust...

#include <algorithm>
#include <array>
#include <cassert>
#include <cerrno>
#include <chrono>
#include <cstdint>
#include <cstdlib>
#include <cstring>
#include <iostream>
#include <string>
#include <thread>

namespace {
    constexpr const char *k_env_varname = "ENV_TEST_VAR";
    constexpr const int64_t k_error_wait_us = 10;
    constexpr const std::size_t k_thread_count = 20;
    constexpr const int64_t k_thread_sleep_ms = 1000;
} // namespace

/// @brief Generate random `std::string` with length digit at start
///
/// @param len Length of `std::string` [1..=9]
///
/// @return random `std::string`
auto rnd_value(uint8_t len) -> std::string {
    auto randchar = []() -> char {
        const auto charset = std::to_array(
            "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz");
        const auto max_index = charset.size() - 1;
        const auto index =
            static_cast<unsigned long>(rand()) % max_index; // NOLINT
        return charset.at(index);
    };
    std::string value(len + 1, 0);
    std::generate_n(value.begin() + 1, len, randchar);
    value[0] = static_cast<char>('1' + len);
    return value;
}

auto main() -> int {
    // Flawfinder: ignore
    const auto *env_value = std::getenv(k_env_varname); // NOLINT
    assert(!env_value);

    auto th_getenv = std::array<std::jthread, k_thread_count>{};

    for (auto &thread : th_getenv) {
        thread = std::jthread{[] {
            for (;;) {
                // Flawfinder: ignore
                const auto *env_value = std::getenv(k_env_varname); // NOLINT
                if (env_value == nullptr) {
                    // NOLINTNEXTLINE
                    std::cerr << "getenv fail: " << std::strerror(errno)
                              << "\n";
                    std::this_thread::sleep_for(
                        std::chrono::microseconds{k_error_wait_us});
                } else {
                    std::cout << "getenv: " << k_env_varname << ": "
                              << env_value << "\n";
                    const auto value = std::string{env_value};
                    // Flawfinder: ignore
                    assert(value.length() ==
                           static_cast<std::size_t>(value[0] - '0'));
                }
            }
        }};
    }

    auto th_setenv = std::array<std::jthread, k_thread_count>{};

    for (auto &thread : th_setenv) {
        thread = std::jthread{[] {
            for (;;) {
                // NOLINTNEXTLINE(cert-msc30-c,cert-msc50-cpp,concurrency-mt-unsafe)
                const uint8_t value_len = 1 + static_cast<uint8_t>(rand() % 9);
                const auto value = rnd_value(value_len);
                // Flawfinder: ignore
                auto ret = setenv(k_env_varname, value.data(), 1); // NOLINT
                if (ret != 0) {
                    // NOLINTNEXTLINE
                    std::cerr << "setenv fail: " << std::strerror(errno)
                              << "\n";
                    std::this_thread::sleep_for(
                        std::chrono::microseconds{k_error_wait_us});
                } else {
                    std::cout << "setenv: " << k_env_varname << ": " << value
                              << "\n";
                }
            }
        }};
    }

    for (;;) {
        std::this_thread::sleep_for(
            std::chrono::milliseconds{k_thread_sleep_ms});
    }

    return 0;
}

That is because on the Rust side we do use a lock for env::var() and env::set_var(), so if only Rust code is accessing the process environment, there is no way to observe the problem. The problem is that there is no way to force the C side to use the same lock.

Did you use something like valgrind to check that there is no use after free here? Also what OS did you run it on?

Easily verifiable is that getaddrinfo requests info from nscd, and nscd makes getenv calls.

But nevertheless your gist-link is the 1st info showing the exact problem.
Seems you have to modify the environ with setenv in some more stressing way all the time so that memory allocation is happening and getenv accesses invalid memory.
Then it is reproducible also with calling setenv and getenv directly.

Please post a link to the source with this lock for Linux!

The only source I can find where there is some locking for environ is Fortanix SGX and Hermit OS.
For Linux the libc functions are called without any special code, like for Windows GetEnvironmentVariableW, but later is thread safe.

Yes compiled with address sanitizer.

But see Unsafe std::env::set_var change - #58 by arlecchino setenv has to be called more or less without rest to provoke the segv, creating all the time new entries not only changing the value.

Here

The writes to stdout and stderr within the thread caught my attention. I have yet to find such writes not synchronized; protected by a lock. In most cases two. One within the standard library and one within the operating system. If I'm correct then those writes will nicely line up your threads like train cars making it unlikely any will run simultaneously.

Are you certain your implementation of rand is thread safe?

"Resting" is not a thread safety primitive.

Well, then your lib/bin can't simply forbid unsafe code.
It would be preferable that Rust provides a safe std API for as many parts as possible and so it is really disappointing that the solution is like it is now.