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

bors · bors · commit d8f447d4b06a · 2024-06-21T20:24:55.000Z
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 rust-lang#122335.
diff --git 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"
diff --git a/library/std/src/process/tests.rs b/library/std/src/process/tests.rs
@@ -378,6 +378,35 @@ 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 = env_cmd();
+    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
@@ -100,6 +100,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>,
@@ -193,6 +194,7 @@ impl Command {
             uid: None,
             gid: None,
             saw_nul,
+            saw_invalid_env_key: false,
             closures: Vec::new(),
             groups: None,
             stdin: None,
@@ -217,6 +219,7 @@ impl Command {
             uid: None,
             gid: None,
             saw_nul,
+            saw_invalid_env_key: false,
             closures: Vec::new(),
             groups: None,
             stdin: None,
@@ -279,8 +282,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 {
@@ -361,7 +373,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)]
@@ -426,9 +438,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("=");
diff --git a/library/std/src/sys/pal/unix/process/process_fuchsia.rs b/library/std/src/sys/pal/unix/process/process_fuchsia.rs
@@ -21,12 +21,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)?;
 
@@ -41,11 +36,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
@@ -1,5 +1,5 @@
 use crate::fmt;
-use crate::io::{self, Error, ErrorKind};
+use crate::io::{self, Error};
 use crate::mem;
 use crate::num::NonZero;
 use crate::sys;
@@ -66,12 +66,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)?;
 
@@ -182,7 +177,7 @@ impl Command {
     // 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,
+        io::ErrorKind::Unsupported,
         "`fork`+`exec`-based process spawning is not supported on this target",
     );
 
@@ -223,7 +218,7 @@ impl Command {
                     thread::sleep(delay);
                 } else {
                     return Err(io::const_io_error!(
-                        ErrorKind::WouldBlock,
+                        io::ErrorKind::WouldBlock,
                         "forking returned EBADF too often",
                     ));
                 }
@@ -238,8 +233,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) {
@@ -499,7 +494,7 @@ impl Command {
                             thread::sleep(delay);
                         } else {
                             return Err(io::const_io_error!(
-                                ErrorKind::WouldBlock,
+                                io::ErrorKind::WouldBlock,
                                 "posix_spawnp returned EBADF too often",
                             ));
                         }
@@ -1111,14 +1106,14 @@ impl crate::os::linux::process::ChildExt for crate::process::Child {
         self.handle
             .pidfd
             .as_ref()
-            .ok_or_else(|| Error::new(ErrorKind::Uncategorized, "No pidfd was created."))
+            .ok_or_else(|| Error::new(io::ErrorKind::Uncategorized, "No pidfd was created."))
     }
 
     fn take_pidfd(&mut self) -> io::Result<PidFd> {
         self.handle
             .pidfd
             .take()
-            .ok_or_else(|| Error::new(ErrorKind::Uncategorized, "No pidfd was created."))
+            .ok_or_else(|| Error::new(io::ErrorKind::Uncategorized, "No pidfd was created."))
     }
 }
 
diff --git 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
@@ -149,12 +149,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 {
@@ -857,7 +885,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());
