diff --git a/codex-rs/windows-sandbox-rs/BUILD.bazel b/codex-rs/windows-sandbox-rs/BUILD.bazel index cb701594cccf..4a4253472d8a 100644 --- a/codex-rs/windows-sandbox-rs/BUILD.bazel +++ b/codex-rs/windows-sandbox-rs/BUILD.bazel @@ -6,4 +6,8 @@ codex_rust_crate( "codex-windows-sandbox-setup.manifest", ], crate_name = "codex_windows_sandbox", + test_data_extra = [ + ":codex-command-runner", + ":codex-windows-sandbox-setup", + ], ) diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index 073452d354ab..97e5726edb08 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -204,21 +204,6 @@ pub fn path_mask_allows( ) } -/// Returns whether an explicit allow ACE for one of the provided SIDs grants any bit in `desired_mask`. -pub fn path_mask_has_explicit_allow_ace( - path: &Path, - psids: &[*mut c_void], - desired_mask: u32, -) -> Result { - path_mask_allows_with_scope( - path, - psids, - desired_mask, - /*require_all_bits*/ false, - AceScope::Explicit, - ) -} - fn path_mask_allows_with_scope( path: &Path, psids: &[*mut c_void], @@ -366,6 +351,36 @@ pub unsafe fn dacl_has_read_deny_for_sid(p_dacl: *mut ACL, psid: *mut c_void) -> const WRITE_ALLOW_MASK: u32 = FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE; +unsafe fn dacl_allow_mask_needs_refresh( + p_dacl: *mut ACL, + psid: *mut c_void, + allow_mask: u32, + disallow_mask: u32, +) -> bool { + !dacl_mask_allows(p_dacl, &[psid], allow_mask, /*require_all_bits*/ true) + || dacl_mask_allows_with_scope( + p_dacl, + &[psid], + disallow_mask, + /*require_all_bits*/ false, + AceScope::Explicit, + ) +} + +/// Returns whether any provided SID needs its writable-root allow ACE refreshed. +pub fn path_write_aces_need_refresh(path: &Path, psids: &[*mut c_void]) -> Result { + unsafe { + let (p_dacl, p_sd) = fetch_dacl_handle(path)?; + let needs_refresh = psids.iter().any(|psid| { + dacl_allow_mask_needs_refresh(p_dacl, *psid, WRITE_ALLOW_MASK, FILE_DELETE_CHILD) + }); + if !p_sd.is_null() { + LocalFree(p_sd as HLOCAL); + } + Ok(needs_refresh) + } +} + unsafe fn ensure_allow_mask_aces_with_inheritance_impl( path: &Path, sids: &[*mut c_void], @@ -376,15 +391,7 @@ unsafe fn ensure_allow_mask_aces_with_inheritance_impl( let (p_dacl, p_sd) = fetch_dacl_handle(path)?; let mut entries: Vec = Vec::new(); for sid in sids { - if dacl_mask_allows(p_dacl, &[*sid], allow_mask, /*require_all_bits*/ true) - && !dacl_mask_allows_with_scope( - p_dacl, - &[*sid], - disallow_mask, - /*require_all_bits*/ false, - AceScope::Explicit, - ) - { + if !dacl_allow_mask_needs_refresh(p_dacl, *sid, allow_mask, disallow_mask) { continue; } entries.push(EXPLICIT_ACCESS_W { diff --git a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs index 3904785afaa7..6dd372375e88 100644 --- a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs +++ b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs @@ -11,18 +11,16 @@ use codex_windows_sandbox::SetupErrorCode; use codex_windows_sandbox::SetupErrorReport; use codex_windows_sandbox::SetupFailure; use codex_windows_sandbox::add_deny_write_ace; -use codex_windows_sandbox::canonicalize_path; use codex_windows_sandbox::convert_string_sid_to_sid; use codex_windows_sandbox::ensure_allow_mask_aces_with_inheritance; use codex_windows_sandbox::ensure_allow_write_aces; use codex_windows_sandbox::extract_setup_failure; use codex_windows_sandbox::hide_newly_created_users; use codex_windows_sandbox::install_wfp_filters; -use codex_windows_sandbox::is_command_cwd_root; use codex_windows_sandbox::log_note; use codex_windows_sandbox::log_writer; use codex_windows_sandbox::path_mask_allows; -use codex_windows_sandbox::path_mask_has_explicit_allow_ace; +use codex_windows_sandbox::path_write_aces_need_refresh; use codex_windows_sandbox::sandbox_bin_dir; use codex_windows_sandbox::sandbox_dir; use codex_windows_sandbox::sandbox_secrets_dir; @@ -60,12 +58,12 @@ use windows_sys::Win32::Security::CONTAINER_INHERIT_ACE; use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION; use windows_sys::Win32::Security::OBJECT_INHERIT_ACE; use windows_sys::Win32::Storage::FileSystem::DELETE; -use windows_sys::Win32::Storage::FileSystem::FILE_DELETE_CHILD; use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_EXECUTE; use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_READ; use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_WRITE; const DENY_ACCESS: i32 = 3; +#[cfg(test)] const WRITE_ROOT_ALLOW_MASK: u32 = FILE_GENERIC_READ | FILE_GENERIC_WRITE | FILE_GENERIC_EXECUTE | DELETE; @@ -161,18 +159,6 @@ fn workspace_write_cap_sids_for_path( Ok(sid_strs) } -fn write_root_needs_refresh(root: &Path, psid: *mut c_void) -> Result { - if !path_mask_allows( - root, - &[psid], - WRITE_ROOT_ALLOW_MASK, - /*require_all_bits*/ true, - )? { - return Ok(true); - } - path_mask_has_explicit_allow_ace(root, &[psid], FILE_DELETE_CHILD) -} - fn spawn_read_acl_helper(payload: &Payload, _log: &mut dyn Write) -> Result<()> { let mut read_payload = payload.clone(); read_payload.mode = SetupMode::ReadAclsOnly; @@ -845,8 +831,6 @@ fn run_setup_full(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Res let mut seen_deny_paths: HashSet = HashSet::new(); let mut seen_write_roots: HashSet = HashSet::new(); - let canonical_command_cwd = canonicalize_path(&payload.command_cwd); - for root in &payload.write_roots { if !seen_write_roots.insert(root.clone()) { continue; @@ -858,35 +842,25 @@ fn run_setup_full(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Res )?; continue; } - let mut need_grant = false; - let is_command_cwd = is_command_cwd_root(root, &canonical_command_cwd); - let cap_label = if is_command_cwd { - "workspace_cap" - } else { - "root_cap" - }; let root_cap_sid_str = workspace_write_cap_sid_for_root(&payload.codex_home, &payload.command_cwd, root)?; let root_cap_psid = unsafe { convert_string_sid_to_sid(&root_cap_sid_str) .ok_or_else(|| anyhow::anyhow!("convert write root capability SID failed"))? }; - for (label, psid) in [ - ("sandbox_group", sandbox_group_psid), - (cap_label, root_cap_psid), - ] { - let needs_refresh = match write_root_needs_refresh(root, psid) { + let need_grant = + match path_write_aces_need_refresh(root, &[sandbox_group_psid, root_cap_psid]) { Ok(needs_refresh) => needs_refresh, Err(e) => { refresh_errors.push(format!( - "write ACE check failed on {} for {label}: {}", + "write ACE check failed on {}: {}", root.display(), e )); log_line( log, &format!( - "write ACE check failed on {} for {label}: {}; continuing", + "write ACE check failed on {}: {}; continuing", root.display(), e ), @@ -894,10 +868,6 @@ fn run_setup_full(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Res true } }; - if needs_refresh { - need_grant = true; - } - } unsafe { LocalFree(root_cap_psid as HLOCAL); } @@ -1051,12 +1021,12 @@ mod tests { use super::WRITE_ROOT_ALLOW_MASK; use super::convert_string_sid_to_sid; use super::workspace_write_cap_sids_for_path; - use super::write_root_needs_refresh; use codex_otel::StatsigMetricsSettings; use codex_windows_sandbox::ensure_allow_mask_aces; use codex_windows_sandbox::ensure_allow_write_aces; use codex_windows_sandbox::load_or_create_cap_sids; use codex_windows_sandbox::path_mask_allows; + use codex_windows_sandbox::path_write_aces_need_refresh; use codex_windows_sandbox::workspace_write_cap_sid_for_root; use pretty_assertions::assert_eq; use serde_json::json; @@ -1126,11 +1096,11 @@ mod tests { let seeded = unsafe { ensure_allow_mask_aces(&workspace, &[psid], stale_write_mask) } .expect("seed stale write ACE"); let needs_refresh_before = - write_root_needs_refresh(&workspace, psid).expect("check stale write ACE"); + path_write_aces_need_refresh(&workspace, &[psid]).expect("check stale write ACE"); let replaced = unsafe { ensure_allow_write_aces(&workspace, &[psid]) } .expect("replace stale write ACE"); let needs_refresh_after = - write_root_needs_refresh(&workspace, psid).expect("check refreshed write ACE"); + path_write_aces_need_refresh(&workspace, &[psid]).expect("check refreshed write ACE"); unsafe { LocalFree(psid as HLOCAL); } @@ -1141,6 +1111,47 @@ mod tests { ); } + #[test] + fn write_root_refresh_checks_each_sid() { + let temp = tempfile::tempdir().expect("tempdir"); + let codex_home = temp.path().join("codex-home"); + let workspace = temp.path().join("workspace"); + let other_root = temp.path().join("other-root"); + fs::create_dir_all(&codex_home).expect("create codex home"); + fs::create_dir_all(&workspace).expect("create workspace"); + fs::create_dir_all(&other_root).expect("create other root"); + + let workspace_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &workspace) + .expect("workspace sid"); + let other_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &other_root) + .expect("other root sid"); + let workspace_psid = + unsafe { convert_string_sid_to_sid(&workspace_sid).expect("convert workspace sid") }; + let other_psid = + unsafe { convert_string_sid_to_sid(&other_sid).expect("convert other root sid") }; + + let seeded = unsafe { ensure_allow_write_aces(&workspace, &[workspace_psid]) } + .expect("seed workspace SID"); + let needs_refresh_before = + path_write_aces_need_refresh(&workspace, &[workspace_psid, other_psid]) + .expect("check both SIDs"); + let refreshed = + unsafe { ensure_allow_write_aces(&workspace, &[workspace_psid, other_psid]) } + .expect("refresh both SIDs"); + let needs_refresh_after = + path_write_aces_need_refresh(&workspace, &[workspace_psid, other_psid]) + .expect("recheck both SIDs"); + unsafe { + LocalFree(workspace_psid as HLOCAL); + LocalFree(other_psid as HLOCAL); + } + + assert_eq!( + (seeded, needs_refresh_before, refreshed, needs_refresh_after,), + (true, true, true, false) + ); + } + #[test] fn write_root_refresh_ignores_inherited_delete_child_grant() { let temp = tempfile::tempdir().expect("tempdir"); @@ -1167,8 +1178,8 @@ mod tests { /*require_all_bits*/ false, ) .expect("check inherited stale write ACE"); - let needs_refresh = - write_root_needs_refresh(&workspace, psid).expect("check inherited stale write ACE"); + let needs_refresh = path_write_aces_need_refresh(&workspace, &[psid]) + .expect("check inherited stale write ACE"); let first_refresh = unsafe { ensure_allow_write_aces(&workspace, &[psid]) } .expect("first inherited write ACE refresh"); let second_refresh = unsafe { ensure_allow_write_aces(&workspace, &[psid]) } 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 29fe0d7a0f12..90ad2dbc93ea 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated/runner_client.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated/runner_client.rs @@ -41,7 +41,6 @@ use windows_sys::Win32::System::Pipes::PeekNamedPipe; use windows_sys::Win32::System::Threading::CreateProcessWithLogonW; use windows_sys::Win32::System::Threading::GetCurrentProcess; use windows_sys::Win32::System::Threading::GetCurrentThread; -use windows_sys::Win32::System::Threading::LOGON_WITH_PROFILE; use windows_sys::Win32::System::Threading::PROCESS_INFORMATION; use windows_sys::Win32::System::Threading::STARTUPINFOW; use windows_sys::Win32::System::Threading::TerminateProcess; @@ -49,7 +48,7 @@ use windows_sys::Win32::System::Threading::WaitForSingleObject; const RUNNER_SPAWN_READY_TIMEOUT: Duration = Duration::from_secs(15); const RUNNER_PIPE_CONNECT_TIMEOUT: Duration = Duration::from_secs(15); -const RUNNER_SPAWN_READY_POLL_INTERVAL: Duration = Duration::from_millis(50); +const RUNNER_SPAWN_READY_POLL_INTERVAL: Duration = Duration::from_millis(5); const RUNNER_ERROR_MODE_FLAGS: u32 = 0x0001 | 0x0002; const WAIT_OBJECT_0: u32 = 0; @@ -344,12 +343,13 @@ pub(crate) fn spawn_runner_transport( let env_block: Option> = None; let previous_error_mode = unsafe { SetErrorMode(RUNNER_ERROR_MODE_FLAGS) }; + // Sandbox users have no profile state that commands should inherit. let spawn_res = unsafe { CreateProcessWithLogonW( user_w.as_ptr(), domain_w.as_ptr(), password_w.as_ptr(), - LOGON_WITH_PROFILE, + /*dwlogonflags*/ 0, exe_w.as_ptr(), cmdline_vec.as_mut_ptr(), windows_sys::Win32::System::Threading::CREATE_NO_WINDOW diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 3f68aeab9ea5..d4db186b9c24 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -157,7 +157,7 @@ pub use acl::fetch_dacl_handle; #[cfg(target_os = "windows")] pub use acl::path_mask_allows; #[cfg(target_os = "windows")] -pub use acl::path_mask_has_explicit_allow_ace; +pub use acl::path_write_aces_need_refresh; #[cfg(target_os = "windows")] pub use audit::apply_world_writable_scan_and_denies_for_permissions; #[cfg(target_os = "windows")] diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs index 7a9a988656ca..02e41fd59046 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/tests.rs @@ -1,5 +1,6 @@ #![cfg(target_os = "windows")] +use super::spawn_windows_sandbox_session_elevated_for_permission_profile; use super::spawn_windows_sandbox_session_legacy; use crate::WindowsSandboxCancellationToken; use crate::ipc_framed::Message; @@ -258,6 +259,52 @@ fn legacy_non_tty_cmd_emits_output() { }); } +#[test] +fn elevated_non_tty_cmd_forwards_env_output_and_exit() { + let _guard = legacy_process_test_guard(); + let runtime = current_thread_runtime(); + runtime.block_on(async move { + let cwd = sandbox_cwd(); + let codex_home = sandbox_home("elevated-non-tty-cmd"); + let permission_profile = PermissionProfile::workspace_write(); + let env_map = HashMap::from([( + "CODEX_ELEVATED_TEST".to_string(), + "ELEVATED-ENV-OK".to_string(), + )]); + let spawned = spawn_windows_sandbox_session_elevated_for_permission_profile( + &permission_profile, + workspace_roots_for(cwd.as_path()).as_slice(), + codex_home.path(), + vec![ + "C:\\Windows\\System32\\cmd.exe".to_string(), + "/d".to_string(), + "/c".to_string(), + "echo %CODEX_ELEVATED_TEST% & exit /b 23".to_string(), + ], + cwd.as_path(), + env_map, + /*proxy_enforced*/ false, + /*network_proxy_restricting_sid*/ None, + Some(5_000), + /*read_roots_override*/ None, + /*read_roots_include_platform_defaults*/ true, + /*write_roots_override*/ None, + &[], + &[], + /*tty*/ false, + /*stdin_open*/ false, + /*use_private_desktop*/ true, + ) + .await + .expect("spawn elevated non-tty cmd session"); + let (stdout, exit_code) = + collect_stdout_and_exit(spawned, codex_home.path(), Duration::from_secs(10)).await; + let stdout = String::from_utf8_lossy(&stdout); + assert_eq!(exit_code, 23, "stdout={stdout:?}"); + assert!(stdout.contains("ELEVATED-ENV-OK"), "stdout={stdout:?}"); + }); +} + #[test] fn legacy_non_tty_cmd_rejects_deny_read_overrides() { let _guard = legacy_process_test_guard();