From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: herdr maintainers Date: Tue, 1 Sep 2026 00:00:00 +0000 Subject: [PATCH] reject malformed Windows process environments Windows registry environment keys can contain empty names, non-string types, or malformed UTF-16. Decode string values into aligned storage, check Win32 expansion failures, preserve valid drive variables, and omit entries that cannot form a valid environment record before calling CreateProcessW. Herdr issue: https://github.com/herdrdev/herdr/issues/3430 Vendored base: portable-pty 0.9.0 --- diff --git a/vendor/portable-pty/src/cmdbuilder.rs b/vendor/portable-pty/src/cmdbuilder.rs index 385fe81c..b214bb6d 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 { + 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 = 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 { let mut env: BTreeMap = std::env::vars_os() .map(|(key, value)| { @@ -103,37 +167,8 @@ fn get_base_env() -> BTreeMap { #[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 { - 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 = 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 { cmd.env_remove("cARGO_pKG_aUTHORS"); assert!(cmd.get_env("CARGO_PKG_AUTHORS").is_none()); } + + #[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 = + 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]); + } }