From e787358f709d52b1233a9c866df0fde7cfa2bb82 Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Fri, 24 Apr 2026 10:41:08 -0700 Subject: [PATCH] check PID of named pipe consumer (#19283) ## Why The elevated Windows command runner currently trusts the first process that connects to its parent-created named pipes. Tightening the pipe ACL already narrows who can reach that boundary, but verifying the connected client PID gives the parent one more fail-closed check: it only accepts the exact runner process it just spawned. ## What changed - validate `GetNamedPipeClientProcessId` after `ConnectNamedPipe` and reject clients whose PID does not match the spawned runner - also did some code de-duplication to route the one-shot elevated capture flow in `windows-sandbox-rs/src/elevated_impl.rs` through `spawn_runner_transport()` so both elevated codepaths use the same pipe bootstrap and PID validation Using the transport unification here also reduces duplication in the elevated Windows IPC bootstrap, so future hardening to the runner handshake only needs to land in one place. ## Validation - `cargo test -p codex-windows-sandbox` - manual testing: one-shot elevated path via `target/debug/codex.exe exec` running a randomized shell command and confirming captured output - manual testing: elevated session path via `target/debug/codex.exe -c 'windows.sandbox="elevated"' sandbox windows -- python -u -c ...` with stdin/stdout round-trips (`READY`, then `GOT:...` for two input lines) --------- Co-authored-by: viyatb-oai --- .../src/elevated/runner_client.rs | 11 +- .../src/elevated/runner_pipe.rs | 26 +- .../windows-sandbox-rs/src/elevated_impl.rs | 282 ++---------------- .../src/unified_exec/backends/elevated.rs | 2 +- 4 files changed, 52 insertions(+), 269 deletions(-) diff --git a/codex-rs/windows-sandbox-rs/src/elevated/runner_client.rs b/codex-rs/windows-sandbox-rs/src/elevated/runner_client.rs index bbcaec330..f296e6b6e 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated/runner_client.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated/runner_client.rs @@ -52,7 +52,6 @@ impl RunnerTransport { } pub(crate) fn read_spawn_ready(&mut self) -> Result<()> { - wait_for_complete_frame(&self.pipe_read, RUNNER_SPAWN_READY_TIMEOUT)?; let msg = read_frame(&mut self.pipe_read)? .ok_or_else(|| anyhow::anyhow!("runner pipe closed before spawn_ready"))?; match msg.message { @@ -64,6 +63,11 @@ impl RunnerTransport { } } + pub(crate) fn read_spawn_ready_with_timeout(&mut self) -> Result<()> { + wait_for_complete_frame(&self.pipe_read, RUNNER_SPAWN_READY_TIMEOUT)?; + self.read_spawn_ready() + } + pub(crate) fn into_files(self) -> (File, File) { (self.pipe_write, self.pipe_read) } @@ -134,10 +138,11 @@ pub(crate) fn spawn_runner_transport( } return Err(anyhow::anyhow!("CreateProcessWithLogonW failed: {err}")); } + let expected_runner_pid = pi.dwProcessId; let connect_result = (|| -> Result<()> { - connect_pipe(h_pipe_in)?; - connect_pipe(h_pipe_out)?; + connect_pipe(h_pipe_in, expected_runner_pid)?; + connect_pipe(h_pipe_out, expected_runner_pid)?; Ok(()) })(); diff --git a/codex-rs/windows-sandbox-rs/src/elevated/runner_pipe.rs b/codex-rs/windows-sandbox-rs/src/elevated/runner_pipe.rs index 904c5102a..c8ae092c4 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated/runner_pipe.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated/runner_pipe.rs @@ -1,7 +1,8 @@ //! Named pipe helpers for the elevated Windows sandbox runner. //! -//! This module generates paired pipe names, creates server‑side pipes with permissive -//! ACLs, and waits for the runner to connect. It is **elevated-path only** and is +//! This module generates paired pipe names, creates server‑side pipes with +//! sandbox-user-scoped ACLs, and waits for the runner to connect. It is +//! **elevated-path only** and is //! used by the parent to establish the IPC channel for both unified_exec sessions //! and elevated capture. The legacy restricted‑token path spawns the child directly //! and does not use these helpers. @@ -27,6 +28,7 @@ use windows_sys::Win32::Security::PSECURITY_DESCRIPTOR; use windows_sys::Win32::Security::SECURITY_ATTRIBUTES; use windows_sys::Win32::System::Pipes::ConnectNamedPipe; use windows_sys::Win32::System::Pipes::CreateNamedPipeW; +use windows_sys::Win32::System::Pipes::GetNamedPipeClientProcessId; use windows_sys::Win32::System::Pipes::PIPE_READMODE_BYTE; use windows_sys::Win32::System::Pipes::PIPE_TYPE_BYTE; use windows_sys::Win32::System::Pipes::PIPE_WAIT; @@ -103,8 +105,9 @@ pub fn create_named_pipe(name: &str, access: u32, sandbox_username: &str) -> io: /// Waits for the runner to connect to a parent-created server pipe. /// /// This is parent-side only: the runner opens the pipe with `CreateFileW`, while the -/// parent calls `ConnectNamedPipe` and tolerates the already-connected case. -pub fn connect_pipe(h: HANDLE) -> io::Result<()> { +/// parent calls `ConnectNamedPipe`, tolerates the already-connected case, and +/// verifies that the connected client is the runner process we just spawned. +pub fn connect_pipe(h: HANDLE, expected_runner_pid: u32) -> io::Result<()> { let ok = unsafe { ConnectNamedPipe(h, ptr::null_mut()) }; if ok == 0 { let err = unsafe { GetLastError() }; @@ -113,5 +116,20 @@ pub fn connect_pipe(h: HANDLE) -> io::Result<()> { return Err(io::Error::from_raw_os_error(err as i32)); } } + let mut client_pid = 0; + let ok = unsafe { GetNamedPipeClientProcessId(h, &mut client_pid) }; + if ok == 0 { + return Err(io::Error::from_raw_os_error(unsafe { + GetLastError() as i32 + })); + } + if client_pid != expected_runner_pid { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + format!( + "named pipe client pid {client_pid} did not match runner pid {expected_runner_pid}" + ), + )); + } Ok(()) } diff --git a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs index 327425bd0..77e4b3a89 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -24,57 +24,23 @@ mod windows_impl { use crate::env::ensure_non_interactive_pager; use crate::env::inherit_path_env; use crate::env::normalize_null_device_env; - use crate::helper_materialization::HelperExecutable; - use crate::helper_materialization::resolve_helper_for_launch; use crate::identity::require_logon_sandbox_creds; - use crate::ipc_framed::FramedMessage; use crate::ipc_framed::Message; use crate::ipc_framed::OutputStream; use crate::ipc_framed::SpawnRequest; use crate::ipc_framed::decode_bytes; use crate::ipc_framed::read_frame; - use crate::ipc_framed::write_frame; use crate::logging::log_failure; - use crate::logging::log_note; use crate::logging::log_start; use crate::logging::log_success; use crate::policy::SandboxPolicy; use crate::policy::parse_policy; + use crate::runner_client::spawn_runner_transport; use crate::token::convert_string_sid_to_sid; - use crate::winutil::quote_windows_arg; - use crate::winutil::resolve_sid; - use crate::winutil::string_from_sid_bytes; - use crate::winutil::to_wide; use anyhow::Result; - use rand::Rng; - use rand::SeedableRng; - use rand::rngs::SmallRng; use std::collections::HashMap; - use std::ffi::c_void; - use std::fs::File; - use std::io; - use std::os::windows::io::FromRawHandle; use std::path::Path; use std::path::PathBuf; - use std::ptr; - use windows_sys::Win32::Foundation::CloseHandle; - use windows_sys::Win32::Foundation::GetLastError; - use windows_sys::Win32::Foundation::HANDLE; - use windows_sys::Win32::Security::Authorization::ConvertStringSecurityDescriptorToSecurityDescriptorW; - use windows_sys::Win32::Security::PSECURITY_DESCRIPTOR; - use windows_sys::Win32::Security::SECURITY_ATTRIBUTES; - use windows_sys::Win32::System::Diagnostics::Debug::SetErrorMode; - use windows_sys::Win32::System::Pipes::ConnectNamedPipe; - use windows_sys::Win32::System::Pipes::CreateNamedPipeW; - const PIPE_ACCESS_INBOUND: u32 = 0x0000_0001; - const PIPE_ACCESS_OUTBOUND: u32 = 0x0000_0002; - use windows_sys::Win32::System::Pipes::PIPE_READMODE_BYTE; - use windows_sys::Win32::System::Pipes::PIPE_TYPE_BYTE; - use windows_sys::Win32::System::Pipes::PIPE_WAIT; - use windows_sys::Win32::System::Threading::CreateProcessWithLogonW; - use windows_sys::Win32::System::Threading::LOGON_WITH_PROFILE; - use windows_sys::Win32::System::Threading::PROCESS_INFORMATION; - use windows_sys::Win32::System::Threading::STARTUPINFOW; /// Ensures the parent directory of a path exists before writing to it. /// Walks upward from `start` to locate the git worktree root, following gitfile redirects. @@ -137,91 +103,8 @@ mod windows_impl { } } - /// Resolves the command runner path, preferring CODEX_HOME/.sandbox/bin. - fn find_runner_exe(codex_home: &Path, log_dir: Option<&Path>) -> PathBuf { - resolve_helper_for_launch(HelperExecutable::CommandRunner, codex_home, log_dir) - } - - /// Generates a unique named-pipe path used to communicate with the runner process. - fn pipe_name(suffix: &str) -> String { - let mut rng = SmallRng::from_entropy(); - format!( - r"\\.\pipe\codex-runner-{:x}-{}", - rng.r#gen::(), - suffix - ) - } - - /// Creates a named pipe whose DACL only allows the sandbox user to connect. - fn create_named_pipe(name: &str, access: u32, sandbox_sid: &str) -> io::Result { - let sddl = to_wide(format!("D:(A;;GA;;;{sandbox_sid})")); - let mut sd: PSECURITY_DESCRIPTOR = ptr::null_mut(); - let ok = unsafe { - ConvertStringSecurityDescriptorToSecurityDescriptorW( - sddl.as_ptr(), - 1, // SDDL_REVISION_1 - &mut sd, - ptr::null_mut(), - ) - }; - if ok == 0 { - return Err(io::Error::from_raw_os_error(unsafe { - GetLastError() as i32 - })); - } - let mut sa = SECURITY_ATTRIBUTES { - nLength: std::mem::size_of::() as u32, - lpSecurityDescriptor: sd, - bInheritHandle: 0, - }; - let wide = to_wide(name); - let h = unsafe { - CreateNamedPipeW( - wide.as_ptr(), - access, - PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT, - 1, - 65536, - 65536, - 0, - &mut sa as *mut SECURITY_ATTRIBUTES, - ) - }; - if h == 0 || h == windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE { - return Err(io::Error::from_raw_os_error(unsafe { - GetLastError() as i32 - })); - } - Ok(h) - } - - /// Waits for a client connection on the named pipe, tolerating an existing connection. - fn connect_pipe(h: HANDLE) -> io::Result<()> { - let ok = unsafe { ConnectNamedPipe(h, ptr::null_mut()) }; - if ok == 0 { - let err = unsafe { GetLastError() }; - const ERROR_PIPE_CONNECTED: u32 = 535; - if err != ERROR_PIPE_CONNECTED { - return Err(io::Error::from_raw_os_error(err as i32)); - } - } - Ok(()) - } - pub use crate::windows_impl::CaptureResult; - fn read_spawn_ready(pipe_read: &mut File) -> Result<()> { - let msg = read_frame(pipe_read)? - .ok_or_else(|| anyhow::anyhow!("runner pipe closed before spawn_ready"))?; - match msg.message { - Message::SpawnReady { .. } => Ok(()), - Message::Error { payload } => Err(anyhow::anyhow!("runner error: {}", payload.message)), - other => Err(anyhow::anyhow!( - "expected spawn_ready from runner, got {other:?}" - )), - } - } - /// Launches the command runner under the sandbox user and captures its output. #[allow(clippy::too_many_arguments)] pub fn run_windows_sandbox_capture( @@ -263,11 +146,6 @@ mod windows_impl { deny_write_paths_override, proxy_enforced, )?; - let sandbox_sid = resolve_sid(&sandbox_creds.username).map_err(|err: anyhow::Error| { - io::Error::new(io::ErrorKind::PermissionDenied, err.to_string()) - })?; - let sandbox_sid = string_from_sid_bytes(&sandbox_sid) - .map_err(|err| io::Error::new(io::ErrorKind::PermissionDenied, err))?; // Build capability SID for ACL grants. if matches!( &policy, @@ -302,133 +180,26 @@ mod windows_impl { allow_null_device(psid_to_use); } - let pipe_in_name = pipe_name("in"); - let pipe_out_name = pipe_name("out"); - let h_pipe_in = create_named_pipe(&pipe_in_name, PIPE_ACCESS_OUTBOUND, &sandbox_sid)?; - let h_pipe_out = create_named_pipe(&pipe_out_name, PIPE_ACCESS_INBOUND, &sandbox_sid)?; - - // Launch runner as sandbox user via CreateProcessWithLogonW. - let runner_exe = find_runner_exe(codex_home, logs_base_dir); - let runner_cmdline = runner_exe - .to_str() - .map(ToString::to_string) - .unwrap_or_else(|| "codex-command-runner.exe".to_string()); - let runner_full_cmd = format!( - "{} {} {}", - quote_windows_arg(&runner_cmdline), - quote_windows_arg(&format!("--pipe-in={pipe_in_name}")), - quote_windows_arg(&format!("--pipe-out={pipe_out_name}")) - ); - let mut cmdline_vec: Vec = to_wide(&runner_full_cmd); - let exe_w: Vec = to_wide(&runner_cmdline); - let cwd_w: Vec = to_wide(cwd); - - // Minimal CPWL launch: inherit env, no desktop override, no handle inheritance. - let env_block: Option> = None; - let mut si: STARTUPINFOW = unsafe { std::mem::zeroed() }; - si.cb = std::mem::size_of::() as u32; - let mut pi: PROCESS_INFORMATION = unsafe { std::mem::zeroed() }; - let user_w = to_wide(&sandbox_creds.username); - let domain_w = to_wide("."); - let password_w = to_wide(&sandbox_creds.password); - // Suppress WER/UI popups from the runner process so we can collect exit codes. - let _ = unsafe { SetErrorMode(0x0001 | 0x0002) }; // SEM_FAILCRITICALERRORS | SEM_NOGPFAULTERRORBOX - - log_note( - &format!( - "runner launch: exe={} cmdline={} cwd={}", - runner_exe.display(), - runner_full_cmd, - cwd.display() - ), - logs_base_dir, - ); - - // Ensure command line buffer is mutable and includes the exe as argv[0]. - let spawn_res = unsafe { - CreateProcessWithLogonW( - user_w.as_ptr(), - domain_w.as_ptr(), - password_w.as_ptr(), - LOGON_WITH_PROFILE, - exe_w.as_ptr(), - cmdline_vec.as_mut_ptr(), - windows_sys::Win32::System::Threading::CREATE_NO_WINDOW - | windows_sys::Win32::System::Threading::CREATE_UNICODE_ENVIRONMENT, - env_block - .as_ref() - .map(|b| b.as_ptr() as *const c_void) - .unwrap_or(ptr::null()), - cwd_w.as_ptr(), - &si, - &mut pi, - ) - }; - if spawn_res == 0 { - let err = unsafe { GetLastError() } as i32; - log_note( - &format!( - "runner launch failed before process start: exe={} cmdline={} error={err}", - runner_exe.display(), - runner_full_cmd - ), - logs_base_dir, - ); - return Err(anyhow::anyhow!("CreateProcessWithLogonW failed: {err}")); - } - - if let Err(err) = connect_pipe(h_pipe_in) { - unsafe { - CloseHandle(h_pipe_in); - CloseHandle(h_pipe_out); - if pi.hThread != 0 { - CloseHandle(pi.hThread); - } - if pi.hProcess != 0 { - CloseHandle(pi.hProcess); - } - } - return Err(err.into()); - } - if let Err(err) = connect_pipe(h_pipe_out) { - unsafe { - CloseHandle(h_pipe_in); - CloseHandle(h_pipe_out); - if pi.hThread != 0 { - CloseHandle(pi.hThread); - } - if pi.hProcess != 0 { - CloseHandle(pi.hProcess); - } - } - return Err(err.into()); - } - - let result = (|| -> Result { - let mut pipe_write = unsafe { File::from_raw_handle(h_pipe_in as _) }; - let mut pipe_read = unsafe { File::from_raw_handle(h_pipe_out as _) }; - - let spawn_request = FramedMessage { - version: 1, - message: Message::SpawnRequest { - payload: Box::new(SpawnRequest { - command: command.clone(), - cwd: cwd.to_path_buf(), - env: env_map.clone(), - policy_json_or_preset: policy_json_or_preset.to_string(), - sandbox_policy_cwd: sandbox_policy_cwd.to_path_buf(), - codex_home: sandbox_base.clone(), - real_codex_home: codex_home.to_path_buf(), - cap_sids, - timeout_ms, - tty: false, - stdin_open: false, - use_private_desktop, - }), - }, + (|| -> Result { + let spawn_request = SpawnRequest { + command: command.clone(), + cwd: cwd.to_path_buf(), + env: env_map.clone(), + policy_json_or_preset: policy_json_or_preset.to_string(), + sandbox_policy_cwd: sandbox_policy_cwd.to_path_buf(), + codex_home: sandbox_base.clone(), + real_codex_home: codex_home.to_path_buf(), + cap_sids, + timeout_ms, + tty: false, + stdin_open: false, + use_private_desktop, }; - write_frame(&mut pipe_write, &spawn_request)?; - read_spawn_ready(&mut pipe_read)?; + let mut transport = + spawn_runner_transport(codex_home, cwd, &sandbox_creds, logs_base_dir)?; + transport.send_spawn_request(spawn_request)?; + transport.read_spawn_ready()?; + let (pipe_write, mut pipe_read) = transport.into_files(); drop(pipe_write); let mut stdout = Vec::new(); @@ -469,18 +240,7 @@ mod windows_impl { stderr, timed_out, }) - })(); - - unsafe { - if pi.hThread != 0 { - CloseHandle(pi.hThread); - } - if pi.hProcess != 0 { - CloseHandle(pi.hProcess); - } - } - - result + })() } #[cfg(test)] diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs index 0ed408fbf..fd46ff09c 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs @@ -62,7 +62,7 @@ pub(crate) async fn spawn_windows_sandbox_session_elevated( let mut transport = spawn_runner_transport(&codex_home, &cwd, &sandbox_creds, logs_base_dir.as_deref())?; transport.send_spawn_request(spawn_request)?; - transport.read_spawn_ready()?; + transport.read_spawn_ready_with_timeout()?; Ok(transport) }) .await