std::os::unix::process::CommandExt::exec should be unsafe

[Diggsey]

This method assigns to environ outside of any lock, meaning that it's automatically unsafe for multi-threaded programs.

For single-threaded programs, if the exec fails, environ will point to memory that has been freed. The documentation gives a false sense of security:

The process may be in a "broken state" if this function returns in error. For example the working directory, environment variables, signal handling settings, various user/group information, or aspects of stdio file descriptors may have changed.

[bstrie]

Smells like #39575 , marking as unsound.

[joshlf]

@Diggsey If you could write some PoC code which demonstrates this, it'd be very helpful. This came up on chat today.

[Diggsey]

@joshlf not right now, but I can point you to the code:

rust/src/libstd/sys/unix/process/process_unix.rs

Line 197 in ca2639e

*sys::os::environ() = envp.as_ptr();

If you look at the comments on that method, you can see that it's clearly been written under the assumption that it will be running in a child process (immediately after a fork). If this were the case, the code would be correct, because it would be guaranteed that there are no other threads running in the child, due to the nature of fork. However, this do_exec method is also directly called by exec and is therefore completely unsafe in a multi-threaded program.

[alex]

I think there's an even more simple memory unsafety here, unless I'm missing something:

• *sys::os::environ() = envp.as_ptr() escapes that ptr to global storage
• However in exec(), we can see that envp is on the stack, so it'll be dropped if you return, which can only happen if exec fails.

In other words, any attempt to use the environment after a failed exec is memory unsafe, because the global environment is now pointing at some random heap memory?

Have I grievously missed something?

[alex]

POC:

alexgaynor@penguin /tmp> cat t.rs 
use std::process::Command;
use std::os::unix::process::CommandExt;

fn main() {
    eprintln!("About to exec");
    Command::new("doesnt-exist").env("VARIABLE", "X").exec();
    eprintln!("Didn't exist!");
    eprintln!("VARIABLE: {:?}", std::env::var("VARAIABLE"));
}
alexgaynor@penguin /tmp> rustc +nightly -Z sanitizer=address t.rs 
alexgaynor@penguin /tmp> env ASAN_OPTIONS=detect_odr_violation=0 ./t 
About to exec
Didn't exist!
fish: “env ASAN_OPTIONS=detect_odr_vio…” terminated by signal SIGSEGV (Address boundary error)

[alex]

It seems like a reasonable fix would be to use execvpe and let it be responsible for mutating the world.

[RalfJung]

@alex This does not reproduce on playground, likely because asan doesn't run there.

Could you describe what is going on, for someone not knowing how env vars are implemented on Linux/Unix platforms?

[alex]

On Linux, env vars are really just a pointer to an array of pointers to nul-terminated strings, with a static lifetime.

So in this case we set that pointer to a Vec's, which is on the stack, data buffer. And then that pointer lives beyond the lifetime of the value on the stack. Now all reads/writes to env vars are pointing to freed memory. A classic UAF.

FWIW a reproducer that will probably work regardless of memory allocator is to check if an env var exists before the exec call, and then try accessing it again afterwards.

You'll notice that I didn't get an ASAN report, that's because the standard lib code doing the read wasn't compiled with ASAN, but I do get the SEGV because it uses an allocator where as soon as some memory is freed it becomes unmapped.

[RalfJung]

Thanks!

So in this case we set that pointer to a Vec's, which is on the stack, data buffer. And then that pointer lives beyond the lifetime of the value on the stack. Now all reads/writes to env vars are pointing to freed memory. A classic UAF.

Changing the env var ptr is happening here, right?

rust/src/libstd/sys/unix/process/process_unix.rs

Lines 196 to 198 in 365b900

	if let Some(envp) = maybe_envp {
	*sys::os::environ() = envp.as_ptr();
	}

Also I was confused a bit here because I thought you said the data buffer is on the stack. But what you mean is that the Vec is on the stack, and usually the stack just disappears because of exec so its buffer in the heap gets leaked, but when exec fails we do pop and deallocate.

Seems like an "easy fix" would be to mem::forget the envp. That wouldn't change anything about the concurrency issue though that the issue was originally about.

[alex]

Using execvpe seems much easier to me, that way we don't have to setup the environment ourselves.

[RalfJung]

Reading the man page I agree, it seems designed for this situation (changing the executed program's environment).

Can we assume that it would also solve the data race pointed out at the beginning of this issue?

[alex]

Yes, it would.

…

On Thu, Oct 25, 2018 at 8:29 AM Ralf Jung ***@***.***> wrote: Reading the man page I agree, it seems designed for this situation (changing the executed program's environment). Can we assume that it would also solve the data race pointed out at the beginning of this issue? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#46775 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAADBDwbERYUnTg6C6IxLlObFp1Jl8nGks5uoa7HgaJpZM4REa02> .

-- All that is necessary for evil to succeed is for good people to do nothing.

[alex]

I'll try to whip up a patch for this.

[RalfJung]

Awesome :)