Auto merge of #126391 - tbu-:pr_command_env_equals, r=cuviper

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.
try-job: x86_64-msvc

Author: bors

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"

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.

library/std/src/sys/pal/unix/process/process_common.rs

@@ -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>,
@@ -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<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)]
@@ -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("=");

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) {

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<os::PidFd, Self> {

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 };
 

library/std/src/sys/pal/windows/process.rs

@@ -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 {
@@ -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());