fix(windows): reject malformed process environments (#3500)

* fix(windows): reject malformed process environments

refs #3430

* fix(windows): validate registry environment strings

refs #3430

* fix(windows): preserve hidden environment variables

refs #3430

1 files changed, 140 insertions, 32 deletions

diff --git a/vendor/portable-pty/src/cmdbuilder.rs b/vendor/portable-pty/src/cmdbuilder.rs
index 385fe81c..a9395f41 100644
--- a/vendor/portable-pty/src/cmdbuilder.rs
+++ b/vendor/portable-pty/src/cmdbuilder.rs
@@ -71,6 +71,70 @@ fn get_shell() -> String {
     "/bin/sh".into()
 }
 
+#[cfg(windows)]
+fn reg_value_to_string(value: &winreg::RegValue) -> anyhow::Result<OsString> {
+    use std::os::windows::ffi::OsStringExt;
+    use winapi::um::processenv::ExpandEnvironmentStringsW;
+    use winreg::enums::RegType;
+
+    if !matches!(value.vtype, RegType::REG_SZ | RegType::REG_EXPAND_SZ) {
+        anyhow::bail!(
+            "unsupported registry type {:?} for environment variable",
+            value.vtype
+        );
+    }
+
+    let mut bytes = value.bytes.chunks_exact(2);
+    let mut units: Vec<u16> = bytes
+        .by_ref()
+        .map(|pair| u16::from_le_bytes([pair[0], pair[1]]))
+        .collect();
+    anyhow::ensure!(
+        bytes.remainder().is_empty(),
+        "invalid UTF-16 byte length for environment variable"
+    );
+    while let Some(0) = units.last() {
+        units.pop();
+    }
+    anyhow::ensure!(
+        !units.contains(&0),
+        "embedded null in registry environment variable"
+    );
+
+    if value.vtype == RegType::REG_SZ {
+        return Ok(OsString::from_wide(&units));
+    }
+
+    units.push(0);
+    let size = unsafe { ExpandEnvironmentStringsW(units.as_ptr(), std::ptr::null_mut(), 0) };
+    if size == 0 {
+        anyhow::bail!(
+            "ExpandEnvironmentStringsW failed: {}",
+            std::io::Error::last_os_error()
+        );
+    }
+    let mut buf = vec![0u16; size as usize];
+    let written = unsafe {
+        ExpandEnvironmentStringsW(units.as_ptr(), buf.as_mut_ptr(), buf.len() as u32)
+    };
+    if written == 0 {
+        anyhow::bail!(
+            "ExpandEnvironmentStringsW failed: {}",
+            std::io::Error::last_os_error()
+        );
+    }
+    anyhow::ensure!(
+        written as usize <= buf.len(),
+        "environment changed while expanding registry value"
+    );
+
+    buf.truncate(written as usize);
+    while let Some(0) = buf.last() {
+        buf.pop();
+    }
+    Ok(OsString::from_wide(&buf))
+}
+
 fn get_base_env() -> BTreeMap<OsString, EnvEntry> {
     let mut env: BTreeMap<OsString, EnvEntry> = std::env::vars_os()
         .map(|(key, value)| {
@@ -103,37 +167,8 @@ fn get_base_env() -> BTreeMap<OsString, EnvEntry> {
 
     #[cfg(windows)]
     {
-        use std::os::windows::ffi::OsStringExt;
-        use winapi::um::processenv::ExpandEnvironmentStringsW;
-        use winreg::enums::{RegType, HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE};
-        use winreg::types::FromRegValue;
-        use winreg::{RegKey, RegValue};
-
-        fn reg_value_to_string(value: &RegValue) -> anyhow::Result<OsString> {
-            match value.vtype {
-                RegType::REG_EXPAND_SZ => {
-                    let src = unsafe {
-                        std::slice::from_raw_parts(
-                            value.bytes.as_ptr() as *const u16,
-                            value.bytes.len() / 2,
-                        )
-                    };
-                    let size =
-                        unsafe { ExpandEnvironmentStringsW(src.as_ptr(), std::ptr::null_mut(), 0) };
-                    let mut buf = vec![0u16; size as usize + 1];
-                    unsafe {
-                        ExpandEnvironmentStringsW(src.as_ptr(), buf.as_mut_ptr(), buf.len() as u32)
-                    };
-
-                    let mut buf = buf.as_slice();
-                    while let Some(0) = buf.last() {
-                        buf = &buf[0..buf.len() - 1];
-                    }
-                    Ok(OsString::from_wide(buf))
-                }
-                _ => Ok(OsString::from_reg_value(value)?),
-            }
-        }
+        use winreg::enums::{HKEY_CURRENT_USER, HKEY_LOCAL_MACHINE};
+        use winreg::RegKey;
 
         if let Ok(sys_env) = RegKey::predef(HKEY_LOCAL_MACHINE)
             .open_subkey("System\\CurrentControlSet\\Control\\Session Manager\\Environment")
@@ -662,12 +697,24 @@ impl CommandBuilder {
             value,
         } in self.envs.values()
         {
-            block.extend(preferred_key.encode_wide());
+            let key: Vec<u16> = preferred_key.encode_wide().collect();
+            if key.is_empty()
+                || key.contains(&0)
+                || key[1..].contains(&(b'=' as u16))
+                || value.encode_wide().any(|unit| unit == 0)
+            {
+                continue;
+            }
+
+            block.extend(key);
             block.push(b'=' as u16);
             block.extend(value.encode_wide());
             block.push(0);
         }
         // and a final terminator for CreateProcessW
+        if block.is_empty() {
+            block.push(0);
+        }
         block.push(0);
 
         block
@@ -869,4 +916,65 @@ mod tests {
         assert!(command.ends_with(r#"/d /c echo "hi""#), "{}", command);
         assert!(!command.contains(r#"\"hi\""#), "{}", command);
     }
+
+    #[cfg(windows)]
+    #[test]
+    fn windows_environment_rejects_malformed_entries() {
+        use std::os::windows::ffi::{OsStrExt, OsStringExt};
+        use winreg::enums::RegType;
+        use winreg::RegValue;
+
+        let multi_string = RegValue {
+            bytes: vec![b'a', 0, 0, 0],
+            vtype: RegType::REG_MULTI_SZ,
+        };
+        assert!(reg_value_to_string(&multi_string).is_err());
+        let malformed_expand_string = RegValue {
+            bytes: vec![b'%', 0, b'P'],
+            vtype: RegType::REG_EXPAND_SZ,
+        };
+        assert!(reg_value_to_string(&malformed_expand_string).is_err());
+        let expandable_string = RegValue {
+            bytes: "literal\0"
+                .encode_utf16()
+                .flat_map(u16::to_le_bytes)
+                .collect(),
+            vtype: RegType::REG_EXPAND_SZ,
+        };
+        assert_eq!(
+            reg_value_to_string(&expandable_string).unwrap(),
+            OsString::from("literal")
+        );
+
+        let mut cmd = CommandBuilder::new("dummy");
+        cmd.env_clear();
+        cmd.env("", "ignored");
+        cmd.env(
+            OsString::from_wide(&[b'B' as u16, 0, b'A' as u16, b'D' as u16]),
+            "ignored",
+        );
+        cmd.env(
+            "MULTI",
+            OsString::from_wide(&[b'a' as u16, 0, b'b' as u16]),
+        );
+        cmd.env("BAD=KEY", "ignored");
+        cmd.env("=::", "colon");
+        cmd.env("=C:", r"C:\valid");
+        cmd.env("=ExitCode", "hidden");
+        cmd.env("EMPTY", "");
+        cmd.env("VALID", "ok");
+
+        let expected: Vec<u16> =
+            OsStr::new("=::=colon\0=C:=C:\\valid\0=ExitCode=hidden\0EMPTY=\0VALID=ok\0\0")
+                .encode_wide()
+                .collect();
+        assert_eq!(cmd.environment_block(), expected);
+
+        cmd.env_remove("=::");
+        cmd.env_remove("=C:");
+        cmd.env_remove("=ExitCode");
+        cmd.env_remove("EMPTY");
+        cmd.env_remove("VALID");
+        assert_eq!(cmd.environment_block(), vec![0, 0]);
+    }
 }