[codex] Scope Windows sandbox write-root capability SIDs

codex-rs/windows-sandbox-rs/src/audit.rs

@@ -2,12 +2,14 @@ use crate::acl::add_deny_write_ace;
 use crate::acl::path_mask_allows;
 use crate::cap::cap_sid_file;
 use crate::cap::load_or_create_cap_sids;
+use crate::cap::workspace_write_cap_sid_for_root;
+use crate::cap::workspace_write_root_contains_path;
 use crate::logging::{debug_log, log_note};
 use crate::path_normalization::canonical_path_key;
 use crate::policy::SandboxPolicy;
-use crate::token::convert_string_sid_to_sid;
+use crate::setup::effective_write_roots_for_setup;
+use crate::token::LocalSid;
 use crate::token::world_sid;
-use anyhow::anyhow;
 use anyhow::Result;
 use std::collections::HashSet;
 use std::ffi::c_void;
@@ -225,6 +227,7 @@ pub fn apply_world_writable_scan_and_denies(
         &flagged,
         sandbox_policy,
         cwd,
+        env_map,
         logs_base_dir,
     ) {
         log_note(
@@ -240,6 +243,7 @@ pub fn apply_capability_denies_for_world_writable(
     flagged: &[PathBuf],
     sandbox_policy: &SandboxPolicy,
     cwd: &Path,
+    env_map: &std::collections::HashMap<String, String>,
     logs_base_dir: Option<&Path>,
 ) -> Result<()> {
     if flagged.is_empty() {
@@ -249,47 +253,58 @@ pub fn apply_capability_denies_for_world_writable(
     let cap_path = cap_sid_file(codex_home);
     let caps = load_or_create_cap_sids(codex_home)?;
     std::fs::write(&cap_path, serde_json::to_string(&caps)?)?;
-    let (active_sid, workspace_roots): (*mut c_void, Vec<PathBuf>) = match sandbox_policy {
-        SandboxPolicy::WorkspaceWrite { writable_roots, .. } => {
-            let sid = unsafe { convert_string_sid_to_sid(&caps.workspace) }
-                .ok_or_else(|| anyhow!("ConvertStringSidToSidW failed for workspace capability"))?;
-            let mut roots: Vec<PathBuf> =
-                vec![dunce::canonicalize(cwd).unwrap_or_else(|_| cwd.to_path_buf())];
-            for root in writable_roots {
-                let candidate = root.as_path();
-                roots.push(dunce::canonicalize(candidate).unwrap_or_else(|_| root.to_path_buf()));
-            }
-            (sid, roots)
+    let (active_sids, workspace_roots): (Vec<LocalSid>, Vec<PathBuf>) = match sandbox_policy {
+        SandboxPolicy::WorkspaceWrite { .. } => {
+            let roots =
+                effective_write_roots_for_setup(
+                    sandbox_policy,
+                    cwd,
+                    cwd,
+                    env_map,
+                    codex_home,
+                    /*write_roots_override*/ None,
+                );
+            let active_sids = roots
+                .iter()
+                .map(|root| {
+                    workspace_write_cap_sid_for_root(codex_home, cwd, root)
+                        .and_then(|sid| LocalSid::from_string(&sid))
+                })
+                .collect::<Result<Vec<_>>>()?;
+            (active_sids, roots)
         }
         SandboxPolicy::ReadOnly { .. } => (
-            unsafe { convert_string_sid_to_sid(&caps.readonly) }.ok_or_else(|| {
-                anyhow!("ConvertStringSidToSidW failed for readonly capability")
-            })?,
+            vec![LocalSid::from_string(&caps.readonly)?],
             Vec::new(),
         ),
         SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => {
             return Ok(());
         }
     };
     for path in flagged {
-        if workspace_roots.iter().any(|root| path.starts_with(root)) {
+        if workspace_roots
+            .iter()
+            .any(|root| workspace_write_root_contains_path(root, path))
+        {
             continue;
         }
-        let res = unsafe { add_deny_write_ace(path, active_sid) };
-        match res {
-            Ok(true) => log_note(
-                &format!("AUDIT: applied capability deny ACE to {}", path.display()),
-                logs_base_dir,
-            ),
-            Ok(false) => {}
-            Err(err) => log_note(
-                &format!(
-                    "AUDIT: failed to apply capability deny ACE to {}: {}",
-                    path.display(),
-                    err
+        for active_sid in &active_sids {
+            let res = unsafe { add_deny_write_ace(path, active_sid.as_ptr()) };
+            match res {
+                Ok(true) => log_note(
+                    &format!("AUDIT: applied capability deny ACE to {}", path.display()),
+                    logs_base_dir,
                 ),
-                logs_base_dir,
-            ),
+                Ok(false) => {}
+                Err(err) => log_note(
+                    &format!(
+                        "AUDIT: failed to apply capability deny ACE to {}: {}",
+                        path.display(),
+                        err
+                    ),
+                    logs_base_dir,
+                ),
+            }
         }
     }
     Ok(())

codex-rs/windows-sandbox-rs/src/cap.rs

@@ -10,6 +10,7 @@ use std::fs;
 use std::path::Path;
 use std::path::PathBuf;
 use crate::path_normalization::canonical_path_key;
+use crate::path_normalization::canonicalize_path;
 
 #[derive(Serialize, Deserialize, Clone, Debug)]
 pub struct CapSids {
@@ -22,6 +23,13 @@ pub struct CapSids {
     /// without permanently affecting other workspaces.
     #[serde(default)]
     pub workspace_by_cwd: HashMap<String, String>,
+    /// Per-write-root capability SIDs keyed by canonicalized write-root path.
+    ///
+    /// These are included in a workspace-write token only when the root is
+    /// currently allowed, so stale ACLs from earlier extra roots do not expand
+    /// later workspace sandboxes.
+    #[serde(default)]
+    pub writable_root_by_path: HashMap<String, String>,
 }
 
 pub fn cap_sid_file(codex_home: &Path) -> PathBuf {
@@ -61,6 +69,7 @@ pub fn load_or_create_cap_sids(codex_home: &Path) -> Result<CapSids> {
                 workspace: t.to_string(),
                 readonly: make_random_cap_sid_string(),
                 workspace_by_cwd: HashMap::new(),
+                writable_root_by_path: HashMap::new(),
             };
             persist_caps(&path, &caps)?;
             return Ok(caps);
@@ -70,6 +79,7 @@ pub fn load_or_create_cap_sids(codex_home: &Path) -> Result<CapSids> {
         workspace: make_random_cap_sid_string(),
         readonly: make_random_cap_sid_string(),
         workspace_by_cwd: HashMap::new(),
+        writable_root_by_path: HashMap::new(),
     };
     persist_caps(&path, &caps)?;
     Ok(caps)
@@ -89,10 +99,46 @@ pub fn workspace_cap_sid_for_cwd(codex_home: &Path, cwd: &Path) -> Result<String
     Ok(sid)
 }
 
+/// Returns the capability SID for an additional writable root, creating and persisting it if missing.
+pub fn writable_root_cap_sid_for_path(codex_home: &Path, root: &Path) -> Result<String> {
+    let path = cap_sid_file(codex_home);
+    let mut caps = load_or_create_cap_sids(codex_home)?;
+    let key = canonical_path_key(root);
+    if let Some(sid) = caps.writable_root_by_path.get(&key) {
+        return Ok(sid.clone());
+    }
+    let sid = make_random_cap_sid_string();
+    caps.writable_root_by_path.insert(key, sid.clone());
+    persist_caps(&path, &caps)?;
+    Ok(sid)
+}
+
+pub fn workspace_write_cap_sid_for_root(
+    codex_home: &Path,
+    cwd: &Path,
+    root: &Path,
+) -> Result<String> {
+    if canonical_path_key(root) == canonical_path_key(cwd) {
+        workspace_cap_sid_for_cwd(codex_home, cwd)
+    } else {
+        writable_root_cap_sid_for_path(codex_home, root)
+    }
+}
+
+pub fn workspace_write_root_contains_path(root: &Path, path: &Path) -> bool {
+    canonicalize_path(path).starts_with(canonicalize_path(root))
+}
+
+pub fn workspace_write_root_specificity(root: &Path) -> usize {
+    canonicalize_path(root).components().count()
+}
+
 #[cfg(test)]
 mod tests {
     use super::load_or_create_cap_sids;
+    use super::writable_root_cap_sid_for_path;
     use super::workspace_cap_sid_for_cwd;
+    use super::workspace_write_cap_sid_for_root;
     use pretty_assertions::assert_eq;
     use std::path::PathBuf;
 
@@ -118,4 +164,32 @@ mod tests {
         let caps = load_or_create_cap_sids(&codex_home).expect("load caps");
         assert_eq!(caps.workspace_by_cwd.len(), 1);
     }
+
+    #[test]
+    fn write_roots_get_path_scoped_sids() {
+        let temp = tempfile::tempdir().expect("tempdir");
+        let codex_home = temp.path().join("codex-home");
+        std::fs::create_dir_all(&codex_home).expect("create codex home");
+
+        let workspace = temp.path().join("workspace");
+        let extra_root = temp.path().join("extra-root");
+        std::fs::create_dir_all(&workspace).expect("create workspace");
+        std::fs::create_dir_all(&extra_root).expect("create extra root");
+
+        let workspace_sid =
+            workspace_write_cap_sid_for_root(&codex_home, &workspace, &workspace)
+                .expect("workspace sid");
+        let extra_sid = workspace_write_cap_sid_for_root(&codex_home, &workspace, &extra_root)
+            .expect("extra root sid");
+
+        assert_ne!(workspace_sid, extra_sid);
+        assert_eq!(
+            extra_sid,
+            writable_root_cap_sid_for_path(&codex_home, &extra_root).expect("extra root sid again")
+        );
+
+        let caps = load_or_create_cap_sids(&codex_home).expect("load caps");
+        assert_eq!(caps.workspace_by_cwd.len(), 1);
+        assert_eq!(caps.writable_root_by_path.len(), 1);
+    }
 }

codex-rs/windows-sandbox-rs/src/elevated_impl.rs

@@ -22,6 +22,7 @@ mod windows_impl {
     use super::ElevatedSandboxCaptureRequest;
     use crate::acl::allow_null_device;
     use crate::cap::load_or_create_cap_sids;
+    use crate::cap::workspace_write_cap_sid_for_root;
     use crate::env::ensure_non_interactive_pager;
     use crate::env::inherit_path_env;
     use crate::env::normalize_null_device_env;
@@ -39,7 +40,8 @@ mod windows_impl {
     use crate::runner_client::spawn_runner_transport;
     use crate::sandbox_utils::ensure_codex_home_exists;
     use crate::sandbox_utils::inject_git_safe_directory;
-    use crate::token::convert_string_sid_to_sid;
+    use crate::setup::effective_write_roots_for_setup;
+    use crate::token::LocalSid;
     use anyhow::Result;
     use std::path::Path;
 
@@ -96,30 +98,36 @@ mod windows_impl {
             anyhow::bail!("DangerFullAccess and ExternalSandbox are not supported for sandboxing")
         }
         let caps = load_or_create_cap_sids(codex_home)?;
-        let (psid_to_use, cap_sids) = match &policy {
+        let (sid_for_null, cap_sids) = match &policy {
             SandboxPolicy::ReadOnly { .. } => {
-                #[allow(clippy::unwrap_used)]
-                let psid = unsafe { convert_string_sid_to_sid(&caps.readonly).unwrap() };
-                (psid, vec![caps.readonly])
+                let sid = LocalSid::from_string(&caps.readonly)?;
+                (sid, vec![caps.readonly])
             }
             SandboxPolicy::WorkspaceWrite { .. } => {
-                #[allow(clippy::unwrap_used)]
-                let psid = unsafe { convert_string_sid_to_sid(&caps.workspace).unwrap() };
-                (
-                    psid,
-                    vec![
-                        caps.workspace,
-                        crate::cap::workspace_cap_sid_for_cwd(codex_home, cwd)?,
-                    ],
-                )
+                let write_roots = effective_write_roots_for_setup(
+                    &policy,
+                    sandbox_policy_cwd,
+                    cwd,
+                    &env_map,
+                    codex_home,
+                    write_roots_override,
+                );
+                let cap_sids = write_roots
+                    .iter()
+                    .map(|root| workspace_write_cap_sid_for_root(codex_home, cwd, root))
+                    .collect::<Result<Vec<_>>>()?;
+                if cap_sids.is_empty() {
+                    anyhow::bail!("workspace-write sandbox has no writable root capability SIDs");
+                }
+                (LocalSid::from_string(&cap_sids[0])?, cap_sids)
             }
             SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } => {
                 unreachable!("DangerFullAccess handled above")
             }
         };
 
         unsafe {
-            allow_null_device(psid_to_use);
+            allow_null_device(sid_for_null.as_ptr());
         }
 
         (|| -> Result<CaptureResult> {