Harden Windows elevated sandbox startup (#34629)

## What changed

- Check writable-root permissions for the sandbox group and root capability SID from a single DACL snapshot, and refresh the ACL when either SID is missing the required access or has a stale explicit delete-child grant.
- Start the command runner without loading a user profile and poll more frequently while waiting for it to become ready.
- Make the Windows sandbox binaries available to Bazel tests.

## Testing

- Add coverage that verifies ACL refresh checks every requested SID.
- Add an elevated non-TTY command test that verifies environment forwarding, stdout, and exit-code propagation.

GitOrigin-RevId: 657ed9dc906039299f9ffa4772e0926c47066a73

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",
+    ],
 )

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<bool> {
-    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<bool> {
+    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<EXPLICIT_ACCESS_W> = Vec::new();
     for sid in sids {
-        if dacl_mask_allows(p_dacl, &[*sid], allow_mask, /*require_all_bits*/ true)
+        if !dacl_allow_mask_needs_refresh(p_dacl, *sid, allow_mask, disallow_mask) {
-            && !dacl_mask_allows_with_scope(
-                p_dacl,
-                &[*sid],
-                disallow_mask,
-                /*require_all_bits*/ false,
-                AceScope::Explicit,
-            )
-        {
             continue;
         }
         entries.push(EXPLICIT_ACCESS_W {

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<bool> {
-    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<PathBuf> = HashSet::new();
     let mut seen_write_roots: HashSet<PathBuf> = 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 [
+        let need_grant =
-            ("sandbox_group", sandbox_group_psid),
+            match path_write_aces_need_refresh(root, &[sandbox_group_psid, root_cap_psid]) {
-            (cap_label, root_cap_psid),
-        ] {
-            let needs_refresh = match write_root_needs_refresh(root, 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 =
+        let needs_refresh = path_write_aces_need_refresh(&workspace, &[psid])
-            write_root_needs_refresh(&workspace, psid).expect("check inherited stale write ACE");
+            .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]) }

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<Vec<u16>> = 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

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")]

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();