From b0b2e20b5328282e9ae8212fd3bbc2342bd330d8 Mon Sep 17 00:00:00 2001 From: David Wiesen Date: Mon, 11 May 2026 09:16:28 -0700 Subject: [PATCH] windows sandbox: stamp workspace SID on extra roots --- .../src/bin/setup_main/win.rs | 109 +++++++++++------- codex-rs/windows-sandbox-rs/src/spawn_prep.rs | 44 +++++-- 2 files changed, 104 insertions(+), 49 deletions(-) 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 549eb2426d..4d9bbf7df3 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 @@ -117,6 +117,32 @@ fn log_line(log: &mut File, msg: &str) -> Result<()> { Ok(()) } +fn write_capability_labels_and_psids_for_root( + root: &Path, + canonical_command_cwd: &Path, + cap_psid: *mut c_void, + workspace_psid: *mut c_void, +) -> Vec<(&'static str, *mut c_void)> { + if is_command_cwd_root(root, canonical_command_cwd) { + vec![("workspace_cap", workspace_psid)] + } else { + vec![("cap", cap_psid), ("workspace_cap", workspace_psid)] + } +} + +fn write_capability_sid_strings_for_root( + root: &Path, + canonical_command_cwd: &Path, + cap_sid_str: &str, + workspace_sid_str: &str, +) -> Vec { + if is_command_cwd_root(root, canonical_command_cwd) { + vec![workspace_sid_str.to_string()] + } else { + vec![cap_sid_str.to_string(), workspace_sid_str.to_string()] + } +} + fn spawn_read_acl_helper(payload: &Payload, _log: &mut File) -> Result<()> { let mut read_payload = payload.clone(); read_payload.mode = SetupMode::ReadAclsOnly; @@ -676,21 +702,15 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( 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 { - "cap" - }; - let cap_psid_for_root = if is_command_cwd { - workspace_psid - } else { - cap_psid - }; - for (label, psid) in [ - ("sandbox_group", sandbox_group_psid), - (cap_label, cap_psid_for_root), - ] { + for (label, psid) in [("sandbox_group", sandbox_group_psid)].into_iter().chain( + write_capability_labels_and_psids_for_root( + root, + &canonical_command_cwd, + cap_psid, + workspace_psid, + ) + .into_iter(), + ) { let has = match path_mask_allows(root, &[psid], write_mask, /*require_all_bits*/ true) { Ok(h) => h, @@ -719,7 +739,7 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( log_line( log, &format!( - "granting write ACE to {} for sandbox group and capability SID", + "granting write ACE to {} for sandbox group and capability SID set", root.display() ), )?; @@ -730,12 +750,17 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( let (tx, rx) = mpsc::channel::<(PathBuf, Result)>(); std::thread::scope(|scope| { for root in grant_tasks { - let is_command_cwd = is_command_cwd_root(&root, &canonical_command_cwd); - let sid_strings = if is_command_cwd { - vec![sandbox_group_sid_str.clone(), workspace_sid_str.clone()] - } else { - vec![sandbox_group_sid_str.clone(), cap_sid_str.clone()] - }; + let sid_strings = std::iter::once(sandbox_group_sid_str.clone()) + .chain( + write_capability_sid_strings_for_root( + &root, + &canonical_command_cwd, + &cap_sid_str, + &workspace_sid_str, + ) + .into_iter(), + ) + .collect::>(); let tx = tx.clone(); scope.spawn(move || { // Convert SID strings to psids locally in this thread. @@ -798,28 +823,34 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( } let canonical_path = canonicalize_path(path); - let deny_psid = if canonical_path.starts_with(&canonical_command_cwd) { - workspace_psid + let deny_psids = if canonical_path.starts_with(&canonical_command_cwd) { + vec![workspace_psid] } else { - cap_psid + vec![cap_psid, workspace_psid] }; - match unsafe { add_deny_write_ace(path, deny_psid) } { - Ok(true) => { - log_line( - log, - &format!("applied deny ACE to protect {}", path.display()), - )?; - } - Ok(false) => {} - Err(err) => { - refresh_errors.push(format!("deny ACE failed on {}: {err}", path.display())); - log_line( - log, - &format!("deny ACE failed on {}: {err}", path.display()), - )?; + let mut applied = false; + for deny_psid in deny_psids { + match unsafe { add_deny_write_ace(path, deny_psid) } { + Ok(true) => { + applied = true; + } + Ok(false) => {} + Err(err) => { + refresh_errors.push(format!("deny ACE failed on {}: {err}", path.display())); + log_line( + log, + &format!("deny ACE failed on {}: {err}", path.display()), + )?; + } } } + if applied { + log_line( + log, + &format!("applied deny ACE to protect {}", path.display()), + )?; + } } lock_sandbox_dir( diff --git a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs index 1b3704d6a2..5576c35c87 100644 --- a/codex-rs/windows-sandbox-rs/src/spawn_prep.rs +++ b/codex-rs/windows-sandbox-rs/src/spawn_prep.rs @@ -56,6 +56,24 @@ pub(crate) struct LegacySessionSecurity { pub(crate) cap_sid_str: String, } +fn allow_psids_for_path<'a>( + policy: &SandboxPolicy, + path: &Path, + canonical_cwd: &Path, + psid_generic: &'a LocalSid, + psid_workspace: Option<&'a LocalSid>, +) -> Vec<&'a LocalSid> { + if !matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) { + return vec![psid_generic]; + } + + match psid_workspace { + Some(workspace_sid) if is_command_cwd_root(path, canonical_cwd) => vec![workspace_sid], + Some(workspace_sid) => vec![psid_generic, workspace_sid], + None => vec![psid_generic], + } +} + /// Owns a SID allocated by `ConvertStringSidToSidW` and releases it with `LocalFree`. pub struct LocalSid { psid: *mut c_void, @@ -226,22 +244,28 @@ pub(crate) fn apply_legacy_session_acl_rules( let canonical_cwd = canonicalize_path(current_dir); unsafe { for p in &allow { - let psid = if matches!(policy, SandboxPolicy::WorkspaceWrite { .. }) - && is_command_cwd_root(p, &canonical_cwd) + let mut added_any = false; + for sid in allow_psids_for_path(policy, p, &canonical_cwd, psid_generic, psid_workspace) { - psid_workspace.unwrap_or(psid_generic).as_ptr() - } else { - psid_generic.as_ptr() - }; - if matches!(add_allow_ace(p, psid), Ok(true)) && !persist_aces { + if matches!(add_allow_ace(p, sid.as_ptr()), Ok(true)) { + added_any = true; + } + } + if added_any && !persist_aces { guards.push(p.clone()); } } for p in &deny { - if let Ok(added) = add_deny_write_ace(p, psid_generic.as_ptr()) - && added - && !persist_aces + let mut added_any = false; + for sid in allow_psids_for_path(policy, p, &canonical_cwd, psid_generic, psid_workspace) { + if let Ok(added) = add_deny_write_ace(p, sid.as_ptr()) + && added + { + added_any = true; + } + } + if added_any && !persist_aces { guards.push(p.clone()); } }