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.
tbu- · Jun 13, 2024 · a733df6 · a733df6
1 parent 56e112a
commit a733df6
Show file tree

Hide file tree

Showing 6 changed files with 96 additions and 35 deletions.
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,18 @@ 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 saw_invalid_env_key(&self) -> bool {
+        self.saw_invalid_env_key
     }
 
     pub fn get_program(&self) -> &OsStr {
@@ -361,7 +374,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 +439,21 @@ 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 {
+        {
+            let mut iter = k.as_bytes().iter();
+            if iter.next().is_none() {
+                *saw_invalid_env_key = true;
+                continue;
+            }
+            if iter.any(|&b| b == 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
@@ -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)?;
 
@@ -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) {

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,36 @@ 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 +881,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());