Skip to content

Validate environment variable names in std::process #126391

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

Closed
wants to merge 2 commits into from
Closed

Validate environment variable names in std::process #126391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
45a2620
std::ffi: Remove unused link
tbu- Jun 20, 2024
6e57e34
Validate environment variable names in `std::process`
tbu- Jun 13, 2024
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
1 change: 0 additions & 1 deletion library/std/src/ffi/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,6 @@
//!
//! [Unicode scalar value]: https://www.unicode.org/glossary/#unicode_scalar_value
//! [Unicode code point]: https://www.unicode.org/glossary/#code_point
//! [`env::set_var()`]: crate::env::set_var "env::set_var"
//! [`env::var_os()`]: crate::env::var_os "env::var_os"
//! [unix.OsStringExt]: crate::os::unix::ffi::OsStringExt "os::unix::ffi::OsStringExt"
//! [`from_vec`]: crate::os::unix::ffi::OsStringExt::from_vec "os::unix::ffi::OsStringExt::from_vec"
Expand Down
36 changes: 36 additions & 0 deletions library/std/src/process/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,42 @@ fn test_interior_nul_in_env_value_is_error() {
}
}

#[test]
fn test_empty_env_key_is_error() {
match env_cmd().env("", "value").spawn() {
Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
Ok(_) => panic!(),
}
}

#[test]
fn test_interior_equals_in_env_key_is_error() {
match env_cmd().env("has=equals", "value").spawn() {
Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
Ok(_) => panic!(),
}
}
Comment on lines +388 to +394
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird, https://learn.microsoft.com/en-us/windows/win32/procthread/environment-variables The name of an environment variable cannot include an equal sign (=).

Can this have test:

#[test]
fn test_env_name_starts_with_equals_is_error() {
    match env_cmd().env("=startsequals", "value").spawn() {
        Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
        Ok(_) => panic!(),
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't test for env_cmd().env("has=equals", "value") check what value actually set? I expect that this will be has with value =equalsvalue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand what you're asking me to do.

There's a test for interior equals sign (above) that checks that the functions return an error.

There's a test for leading equals sign (below) that also checks the environment variable values.

Copy link
Member

Choose a reason for hiding this comment

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

The name of an environment variable cannot include an equal sign

It can as the first character. CMD.exe uses them as "hidden" environment variables. This is technically an implementation detail but we have to allow it otherwise, for example, copying environment variables from one environment to another would break.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a test for leading equals sign (below) that also checks the environment variable values.

Ok, looks like i missed that.

This is technically an implementation detail but we have to allow it otherwise.

Should this be documented for Command's envs?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we can make any promises here. As you saw, it's not documented by Microsoft. We can't really make strong guarantees than the official docs.

Copy link
Contributor

@klensy klensy Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No guarantees, just note about implementation detail, so there will not "wtf i added equals and it didn't blown up" cases.

Copy link
Member

Choose a reason for hiding this comment

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

🤷I mean, maybe with appropriate caveats. I don't feel too strongly about it either way. I'm not sure what users can do with that information.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/python/cpython/blame/8e8a4baf652f6e1cee7acde9d78c4b6154539748/Modules/posixmodule.c#L12065-L12066 Oh, fun.


#[test]
fn test_env_leading_equals() {
let mut cmd;
if cfg!(not(target_os = "windows")) {
cmd = env_cmd();
} else {
cmd = Command::new("cmd");
cmd.arg("/c");
cmd.arg("echo =RUN_TEST_LEADING_EQUALS=%=RUN_TEST_LEADING_EQUALS%");
}
cmd.env("=RUN_TEST_LEADING_EQUALS", "789=012");
let result = cmd.output().unwrap();
let output = String::from_utf8_lossy(&result.stdout).to_string();

assert!(
output.contains("=RUN_TEST_LEADING_EQUALS=789=012"),
"didn't find =RUN_TEST_LEADING_EQUALS inside of:\n\n{output}",
);
}

/// Tests that process creation flags work by debugging a process.
/// Other creation flags make it hard or impossible to detect
/// behavioral changes in the process.
Expand Down
29 changes: 25 additions & 4 deletions library/std/src/sys/pal/unix/process/process_common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ pub struct Command {
uid: Option<uid_t>,
gid: Option<gid_t>,
saw_nul: bool,
saw_invalid_env_key: bool,
closures: Vec<Box<dyn FnMut() -> io::Result<()> + Send + Sync>>,
groups: Option<Box<[gid_t]>>,
stdin: Option<Stdio>,
Expand Down Expand Up @@ -190,6 +191,7 @@ impl Command {
uid: None,
gid: None,
saw_nul,
saw_invalid_env_key: false,
closures: Vec::new(),
groups: None,
stdin: None,
Expand All @@ -214,6 +216,7 @@ impl Command {
uid: None,
gid: None,
saw_nul,
saw_invalid_env_key: false,
closures: Vec::new(),
groups: None,
stdin: None,
Expand Down Expand Up @@ -276,8 +279,17 @@ impl Command {
self.create_pidfd
}

pub fn saw_nul(&self) -> bool {
self.saw_nul
pub fn validate_input(&self) -> io::Result<()> {
if self.saw_invalid_env_key {
Err(io::const_io_error!(
io::ErrorKind::InvalidInput,
"env key empty or equals sign found in env key",
))
} else if self.saw_nul {
Err(io::const_io_error!(io::ErrorKind::InvalidInput, "nul byte found in provided data"))
} else {
Ok(())
}
}

pub fn get_program(&self) -> &OsStr {
Expand Down Expand Up @@ -358,7 +370,7 @@ impl Command {

pub fn capture_env(&mut self) -> Option<CStringArray> {
let maybe_env = self.env.capture_if_changed();
maybe_env.map(|env| construct_envp(env, &mut self.saw_nul))
maybe_env.map(|env| construct_envp(env, &mut self.saw_nul, &mut self.saw_invalid_env_key))
}

#[allow(dead_code)]
Expand Down Expand Up @@ -423,9 +435,18 @@ impl CStringArray {
}
}

fn construct_envp(env: BTreeMap<OsString, OsString>, saw_nul: &mut bool) -> CStringArray {
fn construct_envp(
env: BTreeMap<OsString, OsString>,
saw_nul: &mut bool,
saw_invalid_env_key: &mut bool,
) -> CStringArray {
let mut result = CStringArray::with_capacity(env.len());
for (mut k, v) in env {
if k.is_empty() || k.as_bytes()[1..].contains(&b'=') {
*saw_invalid_env_key = true;
continue;
}

// Reserve additional space for '=' and null terminator
k.reserve_exact(v.len() + 2);
k.push("=");
Expand Down
14 changes: 3 additions & 11 deletions library/std/src/sys/pal/unix/process/process_fuchsia.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,7 @@ impl Command {
) -> io::Result<(Process, StdioPipes)> {
let envp = self.capture_env();

if self.saw_nul() {
return Err(io::const_io_error!(
io::ErrorKind::InvalidInput,
"nul byte found in provided data",
));
}
self.validate_input()?;

let (ours, theirs) = self.setup_io(default, needs_stdin)?;

Expand All @@ -37,11 +32,8 @@ impl Command {
}

pub fn exec(&mut self, default: Stdio) -> io::Error {
if self.saw_nul() {
return io::const_io_error!(
io::ErrorKind::InvalidInput,
"nul byte found in provided data",
);
if let Err(err) = self.validate_input() {
return err;
}

match self.setup_io(default, true) {
Expand Down
31 changes: 13 additions & 18 deletions library/std/src/sys/pal/unix/process/process_unix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,12 @@ use libc::{c_int, pid_t};
)))]
use libc::{gid_t, uid_t};

use crate::io::{self, Error, ErrorKind};
use crate::num::NonZero;
use crate::sys::cvt;
#[cfg(target_os = "linux")]
use crate::sys::pal::unix::linux::pidfd::PidFd;
use crate::sys::process::process_common::*;
use crate::{fmt, mem, sys};
use crate::{fmt, io, mem, sys};

cfg_if::cfg_if! {
// This workaround is only needed for QNX 7.0 and 7.1. The bug should have been fixed in 8.0
Expand Down Expand Up @@ -60,12 +59,7 @@ impl Command {

let envp = self.capture_env();

if self.saw_nul() {
return Err(io::const_io_error!(
ErrorKind::InvalidInput,
"nul byte found in provided data",
));
}
self.validate_input()?;

let (ours, theirs) = self.setup_io(default, needs_stdin)?;

Expand Down Expand Up @@ -146,7 +140,7 @@ impl Command {
);
let errno = i32::from_be_bytes(errno.try_into().unwrap());
assert!(p.wait().is_ok(), "wait() should either return Ok or panic");
return Err(Error::from_raw_os_error(errno));
return Err(io::Error::from_raw_os_error(errno));
}
Err(ref e) if e.is_interrupted() => {}
Err(e) => {
Expand Down Expand Up @@ -175,8 +169,8 @@ impl Command {
// allowed to exist in dead code), but it sounds bad, so we go out of our
// way to avoid that all-together.
#[cfg(any(target_os = "tvos", target_os = "watchos"))]
const ERR_APPLE_TV_WATCH_NO_FORK_EXEC: Error = io::const_io_error!(
ErrorKind::Unsupported,
const ERR_APPLE_TV_WATCH_NO_FORK_EXEC: io::Error = io::const_io_error!(
io::ErrorKind::Unsupported,
"`fork`+`exec`-based process spawning is not supported on this target",
);

Expand Down Expand Up @@ -219,7 +213,7 @@ impl Command {
thread::sleep(delay);
} else {
return Err(io::const_io_error!(
ErrorKind::WouldBlock,
io::ErrorKind::WouldBlock,
"forking returned EBADF too often",
));
}
Expand All @@ -234,8 +228,8 @@ impl Command {
pub fn exec(&mut self, default: Stdio) -> io::Error {
let envp = self.capture_env();

if self.saw_nul() {
return io::const_io_error!(ErrorKind::InvalidInput, "nul byte found in provided data",);
if let Err(err) = self.validate_input() {
return err;
}

match self.setup_io(default, true) {
Expand Down Expand Up @@ -561,7 +555,7 @@ impl Command {
thread::sleep(delay);
} else {
return Err(io::const_io_error!(
ErrorKind::WouldBlock,
io::ErrorKind::WouldBlock,
"posix_spawnp returned EBADF too often",
));
}
Expand Down Expand Up @@ -729,7 +723,7 @@ impl Command {
// But we cannot obtain its pid even though pidfd_getpid support was verified earlier.
// This might happen if libc can't open procfs because the file descriptor limit has been reached.
libc::close(pidfd);
return Err(Error::new(
return Err(io::Error::new(
e.kind(),
"pidfd_spawnp succeeded but the child's PID could not be obtained",
));
Expand Down Expand Up @@ -1190,7 +1184,6 @@ impl ExitStatusError {
mod linux_child_ext {

use crate::os::linux::process as os;
use crate::sys::pal::unix::ErrorKind;
use crate::sys::pal::unix::linux::pidfd as imp;
use crate::sys_common::FromInner;
use crate::{io, mem};
Expand All @@ -1203,7 +1196,9 @@ mod linux_child_ext {
.as_ref()
// SAFETY: The os type is a transparent wrapper, therefore we can transmute references
.map(|fd| unsafe { mem::transmute::<&imp::PidFd, &os::PidFd>(fd) })
.ok_or_else(|| io::Error::new(ErrorKind::Uncategorized, "No pidfd was created."))
.ok_or_else(|| {
io::Error::new(io::ErrorKind::Uncategorized, "No pidfd was created.")
})
}

fn into_pidfd(mut self) -> Result<os::PidFd, Self> {
Expand Down
8 changes: 2 additions & 6 deletions library/std/src/sys/pal/unix/process/process_vxworks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,8 @@ impl Command {
use crate::sys::cvt_r;
let envp = self.capture_env();

if self.saw_nul() {
return Err(io::const_io_error!(
ErrorKind::InvalidInput,
"nul byte found in provided data",
));
}
self.validate_input()?;

let (ours, theirs) = self.setup_io(default, needs_stdin)?;
let mut p = Process { pid: 0, status: None };

Expand Down
40 changes: 34 additions & 6 deletions library/std/src/sys/pal/windows/process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,12 +142,40 @@ impl AsRef<OsStr> for EnvKey {
}
}

pub(crate) fn ensure_no_nuls<T: AsRef<OsStr>>(str: T) -> io::Result<T> {
if str.as_ref().encode_wide().any(|b| b == 0) {
Err(io::const_io_error!(ErrorKind::InvalidInput, "nul byte found in provided data"))
} else {
Ok(str)
/// Returns an error if the provided string has a NUL byte anywhere or a `=`
/// after the first character.
fn ensure_env_var_name<T: AsRef<OsStr>>(s: T) -> io::Result<T> {
fn inner(s: &OsStr) -> io::Result<()> {
let err = || {
Err(io::const_io_error!(
ErrorKind::InvalidInput,
"nul or '=' byte found in provided data",
))
};
let mut iter = s.as_encoded_bytes().iter();
if iter.next() == Some(&0) {
return err();
}
if iter.any(|&b| b == 0 || b == b'=') {
return err();
}
Ok(())
}
inner(s.as_ref())?;
Ok(s)
}

/// Returns an error if the provided string has a NUL byte anywhere.
pub(crate) fn ensure_no_nuls<T: AsRef<OsStr>>(s: T) -> io::Result<T> {
fn inner(s: &OsStr) -> io::Result<()> {
if s.as_encoded_bytes().iter().any(|&b| b == 0) {
Err(io::const_io_error!(ErrorKind::InvalidInput, "nul byte found in provided data"))
} else {
Ok(())
}
}
inner(s.as_ref())?;
Ok(s)
}

pub struct Command {
Expand Down Expand Up @@ -873,7 +901,7 @@ fn make_envp(maybe_env: Option<BTreeMap<EnvKey, OsString>>) -> io::Result<(*mut
}

for (k, v) in env {
ensure_no_nuls(k.os_string)?;
ensure_env_var_name(k.os_string)?;
blk.extend(k.utf16);
blk.push('=' as u16);
blk.extend(ensure_no_nuls(v)?.encode_wide());
Expand Down
Loading