From 45a26205ac5a4928a0d3593f6f86bff0ecd201c7 Mon Sep 17 00:00:00 2001 From: Tobias Bucher Date: Thu, 20 Jun 2024 21:26:10 +0200 Subject: [PATCH 1/2] std::ffi: Remove unused link --- library/std/src/ffi/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/library/std/src/ffi/mod.rs b/library/std/src/ffi/mod.rs index 469136be8838a..323a754e7f7e7 100644 --- a/library/std/src/ffi/mod.rs +++ b/library/std/src/ffi/mod.rs @@ -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" From 6e57e3460a6916d667a6bed924b7ebf2eccd7274 Mon Sep 17 00:00:00 2001 From: Tobias Bucher Date: Thu, 13 Jun 2024 12:13:22 +0200 Subject: [PATCH 2/2] Validate environment variable names in `std::process` Make sure that they're not empty and do not contain `=` signs beyond the first character. This prevents environment variable injection, because previously, setting the `PATH=/opt:` variable to `foobar` would lead to the `PATH` variable being overridden. Fixes #122335. --- library/std/src/process/tests.rs | 36 +++++++++++++++++ .../sys/pal/unix/process/process_common.rs | 29 ++++++++++++-- .../sys/pal/unix/process/process_fuchsia.rs | 14 ++----- .../src/sys/pal/unix/process/process_unix.rs | 31 ++++++-------- .../sys/pal/unix/process/process_vxworks.rs | 8 +--- library/std/src/sys/pal/windows/process.rs | 40 ++++++++++++++++--- 6 files changed, 113 insertions(+), 45 deletions(-) diff --git a/library/std/src/process/tests.rs b/library/std/src/process/tests.rs index 88cc95caf4073..2f1d1fa8ac68e 100644 --- a/library/std/src/process/tests.rs +++ b/library/std/src/process/tests.rs @@ -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!(), + } +} + +#[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. diff --git a/library/std/src/sys/pal/unix/process/process_common.rs b/library/std/src/sys/pal/unix/process/process_common.rs index d9c41d4348756..57c5a6492e1ca 100644 --- a/library/std/src/sys/pal/unix/process/process_common.rs +++ b/library/std/src/sys/pal/unix/process/process_common.rs @@ -96,6 +96,7 @@ pub struct Command { uid: Option, gid: Option, saw_nul: bool, + saw_invalid_env_key: bool, closures: Vec io::Result<()> + Send + Sync>>, groups: Option>, stdin: Option, @@ -190,6 +191,7 @@ impl Command { uid: None, gid: None, saw_nul, + saw_invalid_env_key: false, closures: Vec::new(), groups: None, stdin: None, @@ -214,6 +216,7 @@ impl Command { uid: None, gid: None, saw_nul, + saw_invalid_env_key: false, closures: Vec::new(), groups: None, stdin: None, @@ -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 { @@ -358,7 +370,7 @@ impl Command { pub fn capture_env(&mut self) -> Option { 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)] @@ -423,9 +435,18 @@ impl CStringArray { } } -fn construct_envp(env: BTreeMap, saw_nul: &mut bool) -> CStringArray { +fn construct_envp( + env: BTreeMap, + 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("="); diff --git a/library/std/src/sys/pal/unix/process/process_fuchsia.rs b/library/std/src/sys/pal/unix/process/process_fuchsia.rs index 5d0110cf55dcb..3c5c5f6436909 100644 --- a/library/std/src/sys/pal/unix/process/process_fuchsia.rs +++ b/library/std/src/sys/pal/unix/process/process_fuchsia.rs @@ -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)?; @@ -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) { diff --git a/library/std/src/sys/pal/unix/process/process_unix.rs b/library/std/src/sys/pal/unix/process/process_unix.rs index 5d30f388da18e..5fe556b65ffca 100644 --- a/library/std/src/sys/pal/unix/process/process_unix.rs +++ b/library/std/src/sys/pal/unix/process/process_unix.rs @@ -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 @@ -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)?; @@ -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) => { @@ -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", ); @@ -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", )); } @@ -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) { @@ -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", )); } @@ -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", )); @@ -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}; @@ -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 { diff --git a/library/std/src/sys/pal/unix/process/process_vxworks.rs b/library/std/src/sys/pal/unix/process/process_vxworks.rs index 2d9a304c49512..371b6f77a49ed 100644 --- a/library/std/src/sys/pal/unix/process/process_vxworks.rs +++ b/library/std/src/sys/pal/unix/process/process_vxworks.rs @@ -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 }; diff --git a/library/std/src/sys/pal/windows/process.rs b/library/std/src/sys/pal/windows/process.rs index 95b51e704f9d4..25195b0a78a21 100644 --- a/library/std/src/sys/pal/windows/process.rs +++ b/library/std/src/sys/pal/windows/process.rs @@ -142,12 +142,40 @@ impl AsRef for EnvKey { } } -pub(crate) fn ensure_no_nuls>(str: T) -> io::Result { - 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>(s: T) -> io::Result { + 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>(s: T) -> io::Result { + 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 { @@ -873,7 +901,7 @@ fn make_envp(maybe_env: Option>) -> 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());