From 9d1109cc095cbd0943da3ef3a62cb5fcf0ba561b Mon Sep 17 00:00:00 2001 From: David Wiesen Date: Mon, 15 Jun 2026 11:17:17 -0700 Subject: [PATCH] windows-sandbox: retry runner launch without user SID --- .../src/bin/command_runner/win.rs | 218 ++++++++++++------ 1 file changed, 145 insertions(+), 73 deletions(-) diff --git a/codex-rs/windows-sandbox-rs/src/bin/command_runner/win.rs b/codex-rs/windows-sandbox-rs/src/bin/command_runner/win.rs index 682dc06191..0a6fb4aed1 100644 --- a/codex-rs/windows-sandbox-rs/src/bin/command_runner/win.rs +++ b/codex-rs/windows-sandbox-rs/src/bin/command_runner/win.rs @@ -30,7 +30,9 @@ use codex_windows_sandbox::StdinMode; use codex_windows_sandbox::WindowsSandboxTokenMode; use codex_windows_sandbox::allow_null_device; use codex_windows_sandbox::create_readonly_token_with_caps_and_user_from; +use codex_windows_sandbox::create_readonly_token_with_caps_from; use codex_windows_sandbox::create_workspace_write_token_with_caps_and_user_from; +use codex_windows_sandbox::create_workspace_write_token_with_caps_from; use codex_windows_sandbox::decode_bytes; use codex_windows_sandbox::encode_bytes; use codex_windows_sandbox::get_current_token_for_restriction; @@ -233,6 +235,39 @@ fn effective_cwd(req_cwd: &Path, log_dir: Option<&Path>) -> PathBuf { } } +fn create_runner_token( + token_mode: WindowsSandboxTokenMode, + base_token: HANDLE, + cap_psid_ptrs: &[*mut std::ffi::c_void], + include_user_sid: bool, +) -> Result { + let token = unsafe { + match (token_mode, include_user_sid) { + (WindowsSandboxTokenMode::ReadOnlyCapability, true) => { + create_readonly_token_with_caps_and_user_from(base_token, cap_psid_ptrs) + } + (WindowsSandboxTokenMode::ReadOnlyCapability, false) => { + create_readonly_token_with_caps_from(base_token, cap_psid_ptrs) + } + (WindowsSandboxTokenMode::WritableRootsCapability, true) => { + create_workspace_write_token_with_caps_and_user_from(base_token, cap_psid_ptrs) + } + (WindowsSandboxTokenMode::WritableRootsCapability, false) => { + create_workspace_write_token_with_caps_from(base_token, cap_psid_ptrs) + } + } + }?; + Ok(OwnedWinHandle::new(token)) +} + +fn should_retry_without_user_sid(err: &anyhow::Error) -> bool { + err.chain().any(|cause| { + let msg = cause.to_string(); + msg.contains("CreateProcessAsUserW failed: 5") + || msg.contains("CreateProcessAsUserW failed: 1312") + }) +} + fn spawn_ipc_process(req: &SpawnRequest) -> Result { let log_dir = req.codex_home.clone(); hide_current_user_profile_dir(req.codex_home.as_path()); @@ -259,16 +294,6 @@ fn spawn_ipc_process(req: &SpawnRequest) -> Result { // child is fully spawned still releases the backing LocalAlloc memory automatically. let cap_psid_ptrs: Vec<*mut _> = cap_psids.iter().map(LocalSid::as_ptr).collect(); let base = OwnedWinHandle::new(unsafe { get_current_token_for_restriction()? }); - let h_token = OwnedWinHandle::new(unsafe { - match token_mode { - WindowsSandboxTokenMode::ReadOnlyCapability => { - create_readonly_token_with_caps_and_user_from(base.raw(), &cap_psid_ptrs) - } - WindowsSandboxTokenMode::WritableRootsCapability => { - create_workspace_write_token_with_caps_and_user_from(base.raw(), &cap_psid_ptrs) - } - } - }?); unsafe { // These ACL adjustments need the raw SID values, but ownership stays with `cap_psids`. // We do not manually `LocalFree` anything here; the wrappers handle every return path. @@ -280,71 +305,95 @@ fn spawn_ipc_process(req: &SpawnRequest) -> Result { let effective_cwd = effective_cwd(&req.cwd, Some(log_dir.as_path())); - let mut conpty_owner = None; - let mut hpc_handle: Option = None; - let mut pipe_handles = None; - let (pi, stdout_handle, stderr_handle, stdin_handle) = if req.tty { - let (pi, mut conpty) = codex_windows_sandbox::spawn_conpty_process_as_user( - h_token.raw(), - &req.command, - &effective_cwd, - &req.env, - req.use_private_desktop, - Some(log_dir.as_path()), - )?; - hpc_handle = conpty.raw_handle(); - let input_write = conpty.take_input_write(); - let output_read = conpty.take_output_read(); - conpty_owner = Some(conpty); - let stdin_handle = if req.stdin_open { - Some(input_write) + for include_user_sid in [true, false] { + let h_token = + create_runner_token(token_mode, base.raw(), &cap_psid_ptrs, include_user_sid)?; + let mut conpty_owner = None; + let mut hpc_handle: Option = None; + let mut pipe_handles = None; + let spawn_result = if req.tty { + codex_windows_sandbox::spawn_conpty_process_as_user( + h_token.raw(), + &req.command, + &effective_cwd, + &req.env, + req.use_private_desktop, + Some(log_dir.as_path()), + ) + .map(|(pi, mut conpty)| { + hpc_handle = conpty.raw_handle(); + let input_write = conpty.take_input_write(); + let output_read = conpty.take_output_read(); + conpty_owner = Some(conpty); + let stdin_handle = if req.stdin_open { + Some(input_write) + } else { + unsafe { + CloseHandle(input_write); + } + None + }; + ( + pi, + output_read, + windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE, + stdin_handle, + ) + }) } else { - unsafe { - CloseHandle(input_write); + let stdin_mode = if req.stdin_open { + StdinMode::Open + } else { + StdinMode::Closed + }; + spawn_process_with_pipes( + h_token.raw(), + &req.command, + &effective_cwd, + &req.env, + stdin_mode, + StderrMode::Separate, + req.use_private_desktop, + Some(log_dir.as_path()), + ) + .map(|spawned_pipes: PipeSpawnHandles| { + let pi = spawned_pipes.process; + let stdout_handle = spawned_pipes.stdout_read; + let stderr_handle = spawned_pipes + .stderr_read + .unwrap_or(windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE); + let stdin_handle = spawned_pipes.stdin_write; + pipe_handles = Some(spawned_pipes); + (pi, stdout_handle, stderr_handle, stdin_handle) + }) + }; + + match spawn_result { + Ok((pi, stdout_handle, stderr_handle, stdin_handle)) => { + return Ok(IpcSpawnedProcess { + log_dir, + pi, + stdout_handle, + stderr_handle, + stdin_handle, + conpty_owner, + hpc_handle, + _pipe_handles: pipe_handles, + }); } - None - }; - ( - pi, - output_read, - windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE, - stdin_handle, - ) - } else { - let stdin_mode = if req.stdin_open { - StdinMode::Open - } else { - StdinMode::Closed - }; - let spawned_pipes: PipeSpawnHandles = spawn_process_with_pipes( - h_token.raw(), - &req.command, - &effective_cwd, - &req.env, - stdin_mode, - StderrMode::Separate, - req.use_private_desktop, - Some(log_dir.as_path()), - )?; - let pi = spawned_pipes.process; - let stdout_handle = spawned_pipes.stdout_read; - let stderr_handle = spawned_pipes - .stderr_read - .unwrap_or(windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE); - let stdin_handle = spawned_pipes.stdin_write; - pipe_handles = Some(spawned_pipes); - (pi, stdout_handle, stderr_handle, stdin_handle) - }; - Ok(IpcSpawnedProcess { - log_dir, - pi, - stdout_handle, - stderr_handle, - stdin_handle, - conpty_owner, - hpc_handle, - _pipe_handles: pipe_handles, - }) + Err(err) if include_user_sid && should_retry_without_user_sid(&err) => { + log_note( + "CreateProcessAsUserW failed with the sandbox user SID restriction; retrying with capability-only restrictions", + Some(log_dir.as_path()), + ); + } + Err(err) => return Err(err), + } + } + + Err(anyhow::anyhow!( + "runner: exhausted elevated token launch strategies" + )) } /// Stream stdout/stderr from the child into Output frames. @@ -651,3 +700,26 @@ pub fn main() -> Result<()> { std::process::exit(exit_code); } + +#[cfg(test)] +mod tests { + use super::should_retry_without_user_sid; + use anyhow::anyhow; + + #[test] + fn retries_on_access_denied_or_no_mapping() { + assert!(should_retry_without_user_sid(&anyhow!( + "CreateProcessAsUserW failed: 5" + ))); + assert!(should_retry_without_user_sid(&anyhow!( + "CreateProcessAsUserW failed: 1312" + ))); + } + + #[test] + fn ignores_other_failures() { + assert!(!should_retry_without_user_sid(&anyhow!( + "CreateProcessAsUserW failed: 87" + ))); + } +}