diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index c1f3d0724b..abbfc0b19f 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -232,8 +232,39 @@ impl ReadDenyMatcher { /// can skip read-deny checks without allocating matcher state. The `cwd` /// resolves cwd-relative policy paths and special paths before matching. pub fn new(file_system_sandbox_policy: &FileSystemSandboxPolicy, cwd: &Path) -> Option { + match Self::build( + file_system_sandbox_policy, + cwd, + InvalidDenyReadGlobBehavior::FailClosed, + ) { + Ok(matcher) => matcher, + Err(_) => unreachable!("fail-closed glob handling does not return errors"), + } + } + + /// Builds a matcher for callers that must reject malformed glob patterns. + /// + /// Runtime read checks intentionally fail closed on malformed deny patterns. + /// Host-side expansion work should use this constructor instead so a typo + /// cannot broaden the set of paths it mutates before execution starts. + pub fn try_new( + file_system_sandbox_policy: &FileSystemSandboxPolicy, + cwd: &Path, + ) -> Result, String> { + Self::build( + file_system_sandbox_policy, + cwd, + InvalidDenyReadGlobBehavior::ReturnError, + ) + } + + fn build( + file_system_sandbox_policy: &FileSystemSandboxPolicy, + cwd: &Path, + invalid_glob_behavior: InvalidDenyReadGlobBehavior, + ) -> Result, String> { if !file_system_sandbox_policy.has_denied_read_restrictions() { - return None; + return Ok(None); } // Exact roots are stored as all meaningful path spellings we can derive @@ -247,22 +278,23 @@ impl ReadDenyMatcher { // Pattern entries stay as policy-level globs. They are matched at read // time here instead of being snapshotted to startup filesystem state. let mut invalid_pattern = false; - let deny_read_matchers = file_system_sandbox_policy - .get_unreadable_globs_with_cwd(cwd) - .into_iter() - .filter_map(|pattern| match build_glob_matcher(&pattern) { - Some(matcher) => Some(matcher), - None => { - invalid_pattern = true; - None - } - }) - .collect(); - Some(Self { + let mut deny_read_matchers = Vec::new(); + for pattern in file_system_sandbox_policy.get_unreadable_globs_with_cwd(cwd) { + match build_glob_matcher(&pattern) { + Ok(matcher) => deny_read_matchers.push(matcher), + Err(err) => match invalid_glob_behavior { + InvalidDenyReadGlobBehavior::FailClosed => invalid_pattern = true, + InvalidDenyReadGlobBehavior::ReturnError => { + return Err(format!("invalid deny-read glob pattern `{pattern}`: {err}")); + } + }, + } + } + Ok(Some(Self { denied_candidates, deny_read_matchers, invalid_pattern, - }) + })) } /// Returns whether `path` is denied by the policy used to build this matcher. @@ -295,6 +327,12 @@ impl ReadDenyMatcher { } } +#[derive(Clone, Copy)] +enum InvalidDenyReadGlobBehavior { + FailClosed, + ReturnError, +} + #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, JsonSchema, TS)] #[serde(tag = "type", rename_all = "snake_case")] #[ts(tag = "type")] @@ -1291,15 +1329,15 @@ fn push_unique(candidates: &mut Vec, candidate: PathBuf) { } } -fn build_glob_matcher(pattern: &str) -> Option { +fn build_glob_matcher(pattern: &str) -> Result { // Keep `*` and `?` within a single path component and preserve an unclosed // `[` as a literal so matcher behavior stays aligned with config parsing. GlobBuilder::new(pattern) .literal_separator(true) .allow_unclosed_class(true) .build() - .ok() .map(|glob| glob.compile_matcher()) + .map_err(|err| err.to_string()) } fn resolve_file_system_special_path( diff --git a/codex-rs/windows-sandbox-rs/src/acl.rs b/codex-rs/windows-sandbox-rs/src/acl.rs index 371cf132f7..a368371dc4 100644 --- a/codex-rs/windows-sandbox-rs/src/acl.rs +++ b/codex-rs/windows-sandbox-rs/src/acl.rs @@ -487,6 +487,41 @@ pub unsafe fn add_allow_ace(path: &Path, psid: *mut c_void) -> Result { /// # Safety /// Caller must ensure `psid` points to a valid SID and `path` refers to an existing file or directory. pub unsafe fn add_deny_write_ace(path: &Path, psid: *mut c_void) -> Result { + add_deny_ace(path, psid, DenyAceKind::Write) +} + +#[derive(Clone, Copy)] +enum DenyAceKind { + Read, + Write, +} + +impl DenyAceKind { + fn mask(self) -> u32 { + match self { + Self::Read => FILE_GENERIC_READ | GENERIC_READ_MASK, + Self::Write => { + FILE_GENERIC_WRITE + | FILE_WRITE_DATA + | FILE_APPEND_DATA + | FILE_WRITE_EA + | FILE_WRITE_ATTRIBUTES + | GENERIC_WRITE_MASK + | DELETE + | FILE_DELETE_CHILD + } + } + } + + unsafe fn already_present(self, p_dacl: *mut ACL, psid: *mut c_void) -> bool { + match self { + Self::Read => dacl_has_read_deny_for_sid(p_dacl, psid), + Self::Write => dacl_has_write_deny_for_sid(p_dacl, psid), + } + } +} + +unsafe fn add_deny_ace(path: &Path, psid: *mut c_void, kind: DenyAceKind) -> Result { let mut p_sd: *mut c_void = std::ptr::null_mut(); let mut p_dacl: *mut ACL = std::ptr::null_mut(); let code = GetNamedSecurityInfoW( @@ -503,7 +538,7 @@ pub unsafe fn add_deny_write_ace(path: &Path, psid: *mut c_void) -> Result return Err(anyhow!("GetNamedSecurityInfoW failed: {code}")); } let mut added = false; - if !dacl_has_write_deny_for_sid(p_dacl, psid) { + if !kind.already_present(p_dacl, psid) { let trustee = TRUSTEE_W { pMultipleTrustee: std::ptr::null_mut(), MultipleTrusteeOperation: 0, @@ -512,14 +547,7 @@ pub unsafe fn add_deny_write_ace(path: &Path, psid: *mut c_void) -> Result ptstrName: psid as *mut u16, }; let mut explicit: EXPLICIT_ACCESS_W = std::mem::zeroed(); - explicit.grfAccessPermissions = FILE_GENERIC_WRITE - | FILE_WRITE_DATA - | FILE_APPEND_DATA - | FILE_WRITE_EA - | FILE_WRITE_ATTRIBUTES - | GENERIC_WRITE_MASK - | DELETE - | FILE_DELETE_CHILD; + explicit.grfAccessPermissions = kind.mask(); explicit.grfAccessMode = DENY_ACCESS; explicit.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE; explicit.Trustee = trustee; @@ -559,73 +587,7 @@ pub unsafe fn add_deny_write_ace(path: &Path, psid: *mut c_void) -> Result /// # Safety /// Caller must ensure `psid` points to a valid SID and `path` refers to an existing file or directory. pub unsafe fn add_deny_read_ace(path: &Path, psid: *mut c_void) -> Result { - let mut p_sd: *mut c_void = std::ptr::null_mut(); - let mut p_dacl: *mut ACL = std::ptr::null_mut(); - let code = GetNamedSecurityInfoW( - to_wide(path).as_ptr(), - 1, - DACL_SECURITY_INFORMATION, - std::ptr::null_mut(), - std::ptr::null_mut(), - &mut p_dacl, - std::ptr::null_mut(), - &mut p_sd, - ); - if code != ERROR_SUCCESS { - return Err(anyhow!("GetNamedSecurityInfoW failed: {code}")); - } - let mut added = false; - if !dacl_has_read_deny_for_sid(p_dacl, psid) { - let trustee = TRUSTEE_W { - pMultipleTrustee: std::ptr::null_mut(), - MultipleTrusteeOperation: 0, - TrusteeForm: TRUSTEE_IS_SID, - TrusteeType: TRUSTEE_IS_UNKNOWN, - ptstrName: psid as *mut u16, - }; - let mut explicit: EXPLICIT_ACCESS_W = std::mem::zeroed(); - explicit.grfAccessPermissions = FILE_GENERIC_READ | GENERIC_READ_MASK; - explicit.grfAccessMode = DENY_ACCESS; - explicit.grfInheritance = CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE; - explicit.Trustee = trustee; - let mut p_new_dacl: *mut ACL = std::ptr::null_mut(); - let code2 = SetEntriesInAclW(1, &explicit, p_dacl, &mut p_new_dacl); - if code2 != ERROR_SUCCESS { - if !p_sd.is_null() { - LocalFree(p_sd as HLOCAL); - } - if !p_new_dacl.is_null() { - LocalFree(p_new_dacl as HLOCAL); - } - return Err(anyhow!("SetEntriesInAclW failed: {code2}")); - } - let code3 = SetNamedSecurityInfoW( - to_wide(path).as_ptr() as *mut u16, - 1, - DACL_SECURITY_INFORMATION, - std::ptr::null_mut(), - std::ptr::null_mut(), - p_new_dacl, - std::ptr::null_mut(), - ); - if code3 != ERROR_SUCCESS { - if !p_sd.is_null() { - LocalFree(p_sd as HLOCAL); - } - if !p_new_dacl.is_null() { - LocalFree(p_new_dacl as HLOCAL); - } - return Err(anyhow!("SetNamedSecurityInfoW failed: {code3}")); - } - added = true; - if !p_new_dacl.is_null() { - LocalFree(p_new_dacl as HLOCAL); - } - } - if !p_sd.is_null() { - LocalFree(p_sd as HLOCAL); - } - Ok(added) + add_deny_ace(path, psid, DenyAceKind::Read) } pub unsafe fn revoke_ace(path: &Path, psid: *mut c_void) { 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 a98ea8959c..0ee822f404 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 @@ -6,7 +6,6 @@ use anyhow::Result; use base64::Engine; use base64::engine::general_purpose::STANDARD as BASE64; use codex_otel::StatsigMetricsSettings; -use codex_windows_sandbox::DenyReadAclRecordKind; use codex_windows_sandbox::LOG_FILE_NAME; use codex_windows_sandbox::SETUP_VERSION; use codex_windows_sandbox::SetupErrorCode; @@ -15,7 +14,6 @@ use codex_windows_sandbox::SetupFailure; use codex_windows_sandbox::add_deny_write_ace; use codex_windows_sandbox::apply_deny_read_acls; use codex_windows_sandbox::canonicalize_path; -use codex_windows_sandbox::cleanup_stale_persistent_deny_read_acls; 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; @@ -32,7 +30,6 @@ use codex_windows_sandbox::sandbox_secrets_dir; use codex_windows_sandbox::string_from_sid_bytes; use codex_windows_sandbox::to_wide; use codex_windows_sandbox::workspace_cap_sid_for_cwd; -use codex_windows_sandbox::write_persistent_deny_read_acl_record; use codex_windows_sandbox::write_setup_error_report; use serde::Deserialize; use serde::Serialize; @@ -466,29 +463,6 @@ fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> { let sandbox_group_sid = resolve_sandbox_users_group_sid()?; let sandbox_group_psid = sid_bytes_to_psid(&sandbox_group_sid)?; let mut refresh_errors: Vec = Vec::new(); - // Stale cleanup must happen before the helper re-grants read ACLs because - // the cleanup primitive revokes all ACEs for the sandbox group SID. - match unsafe { - cleanup_stale_persistent_deny_read_acls( - &payload.codex_home, - DenyReadAclRecordKind::SandboxGroup, - &payload.deny_read_paths, - sandbox_group_psid, - ) - } { - Ok(cleaned) => { - if !cleaned.is_empty() { - log_line( - log, - &format!("cleaned {} stale deny-read ACLs", cleaned.len()), - )?; - } - } - Err(err) => { - refresh_errors.push(format!("cleanup stale deny-read ACLs failed: {err}")); - log_line(log, &format!("cleanup stale deny-read ACLs failed: {err}"))?; - } - } if !payload.read_roots.is_empty() { let users_sid = resolve_sid("Users")?; let users_psid = sid_bytes_to_psid(&users_sid)?; @@ -526,11 +500,6 @@ fn run_read_acl_only(payload: &Payload, log: &mut File) -> Result<()> { // explicit deny entries that take precedence over the broad read allowlist. match unsafe { apply_deny_read_acls(&payload.deny_read_paths, sandbox_group_psid) } { Ok(applied) => { - write_persistent_deny_read_acl_record( - &payload.codex_home, - DenyReadAclRecordKind::SandboxGroup, - &applied, - )?; if !applied.is_empty() { log_line(log, &format!("applied {} deny-read ACLs", applied.len()))?; } @@ -664,9 +633,8 @@ fn run_setup_full(payload: &Payload, log: &mut File, sbx_dir: &Path) -> Result<( ); } - // The read ACL helper is also responsible for persistent deny-read cleanup, - // so it must run whenever deny-read paths are present even if no new read - // roots need to be granted. + // The read ACL helper also applies deny-read ACEs, so it must run whenever + // deny-read paths are present even if no new read roots need to be granted. if payload.read_roots.is_empty() && payload.deny_read_paths.is_empty() { log_line( log, diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs b/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs index 368ee58acc..ed4d0bc32c 100644 --- a/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs +++ b/codex-rs/windows-sandbox-rs/src/deny_read_acl.rs @@ -1,46 +1,13 @@ use crate::acl::add_deny_read_ace; use crate::acl::revoke_ace; use crate::path_normalization::canonicalize_path; -use crate::setup::sandbox_dir; use anyhow::Context; use anyhow::Result; -use serde::Deserialize; -use serde::Serialize; use std::collections::HashSet; use std::ffi::c_void; use std::path::Path; use std::path::PathBuf; -/// Identifies which Windows sandbox principal owns a persistent deny-read ACL. -/// -/// The elevated backend applies deny ACEs to the shared sandbox users group, -/// while the restricted-token backend applies them to capability SIDs. Keeping -/// separate records prevents stale cleanup for one backend from revoking entries -/// that are still owned by the other backend. -#[derive(Debug, Clone, Copy)] -pub enum DenyReadAclRecordKind { - SandboxGroup, - RestrictedToken, -} - -impl DenyReadAclRecordKind { - fn file_name(self) -> &'static str { - match self { - Self::SandboxGroup => "deny_read_acls_sandbox_group.json", - Self::RestrictedToken => "deny_read_acls_restricted_token.json", - } - } -} - -#[derive(Debug, Default, Deserialize, Serialize)] -struct DenyReadAclRecord { - paths: Vec, -} - -pub fn deny_read_acl_record_path(codex_home: &Path, kind: DenyReadAclRecordKind) -> PathBuf { - sandbox_dir(codex_home).join(kind.file_name()) -} - /// Build the exact ACL paths that should receive a deny-read ACE. /// /// We keep both the lexical policy path and, when it already exists, the @@ -73,80 +40,6 @@ fn lexical_path_key(path: &Path) -> String { .to_ascii_lowercase() } -fn read_record(path: &Path) -> Result { - match std::fs::read_to_string(path) { - Ok(contents) => serde_json::from_str(&contents) - .with_context(|| format!("parse deny-read ACL record {}", path.display())), - Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(DenyReadAclRecord::default()), - Err(err) => { - Err(err).with_context(|| format!("read deny-read ACL record {}", path.display())) - } - } -} - -pub fn write_persistent_deny_read_acl_record( - codex_home: &Path, - kind: DenyReadAclRecordKind, - paths: &[PathBuf], -) -> Result<()> { - let record_path = deny_read_acl_record_path(codex_home, kind); - let planned_paths = plan_deny_read_acl_paths(paths); - if planned_paths.is_empty() { - match std::fs::remove_file(&record_path) { - Ok(()) => return Ok(()), - Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(()), - Err(err) => { - return Err(err).with_context(|| { - format!("remove deny-read ACL record {}", record_path.display()) - }); - } - } - } - if let Some(parent) = record_path.parent() { - std::fs::create_dir_all(parent) - .with_context(|| format!("create deny-read ACL record dir {}", parent.display()))?; - } - let record = DenyReadAclRecord { - paths: planned_paths, - }; - let contents = serde_json::to_vec_pretty(&record) - .with_context(|| format!("serialize deny-read ACL record {}", record_path.display()))?; - std::fs::write(&record_path, contents) - .with_context(|| format!("write deny-read ACL record {}", record_path.display())) -} - -/// Removes deny-read ACEs that were applied for a previous policy but are not -/// present in the current policy. This uses the same broad revoke primitive as -/// the rest of the Windows sandbox ACL guard path, so callers should run stale -/// cleanup before re-granting any read ACLs for the same SID in this refresh. -/// That ordering matters because `revoke_ace` removes all ACEs for the SID, not -/// only the deny-read ACEs recorded here. -/// -/// # Safety -/// Caller must pass a valid SID pointer for the ACEs recorded under `kind`. -pub unsafe fn cleanup_stale_persistent_deny_read_acls( - codex_home: &Path, - kind: DenyReadAclRecordKind, - desired_paths: &[PathBuf], - psid: *mut c_void, -) -> Result> { - let record_path = deny_read_acl_record_path(codex_home, kind); - let record = read_record(&record_path)?; - let desired_keys = plan_deny_read_acl_paths(desired_paths) - .into_iter() - .map(|path| lexical_path_key(&path)) - .collect::>(); - let mut cleaned = Vec::new(); - for path in record.paths { - if desired_keys.contains(&lexical_path_key(&path)) || !path.exists() { - continue; - } - revoke_ace(&path, psid); - cleaned.push(path); - } - Ok(cleaned) -} - /// Applies deny-read ACEs to explicit paths. Missing paths are materialized as /// directories before the ACE is applied so a sandboxed command cannot create a /// previously absent denied path and then read from it in the same run. diff --git a/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs b/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs index b94dd2db7f..13eb5a8141 100644 --- a/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs +++ b/codex-rs/windows-sandbox-rs/src/deny_read_resolver.rs @@ -47,7 +47,7 @@ pub fn resolve_windows_deny_read_paths( }) .collect(), ); - let Some(matcher) = ReadDenyMatcher::new(&glob_policy, cwd.as_path()) else { + let Some(matcher) = ReadDenyMatcher::try_new(&glob_policy, cwd.as_path())? else { return Ok(paths); }; @@ -276,6 +276,23 @@ mod tests { assert_eq!(actual, expected); } + #[test] + fn invalid_glob_patterns_fail_before_expansion() { + let tmp = TempDir::new().expect("tempdir"); + let cwd = AbsolutePathBuf::from_absolute_path(tmp.path()).expect("absolute cwd"); + let policy = FileSystemSandboxPolicy::restricted(vec![unreadable_glob_entry(format!( + "{}/**/[z-a]", + tmp.path().display() + ))]); + + let err = resolve_windows_deny_read_paths(&policy, &cwd).expect_err("invalid glob"); + assert!( + err.contains("invalid deny-read glob pattern"), + "unexpected error: {err}" + ); + assert!(err.contains("invalid range"), "unexpected error: {err}"); + } + #[test] fn non_recursive_globs_do_not_expand_nested_matches() { let tmp = TempDir::new().expect("tempdir"); diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 31297690e9..0615f427e7 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -112,15 +112,9 @@ pub use conpty::ConptyInstance; #[cfg(target_os = "windows")] pub use conpty::spawn_conpty_process_as_user; #[cfg(target_os = "windows")] -pub use deny_read_acl::DenyReadAclRecordKind; -#[cfg(target_os = "windows")] pub use deny_read_acl::apply_deny_read_acls; #[cfg(target_os = "windows")] -pub use deny_read_acl::cleanup_stale_persistent_deny_read_acls; -#[cfg(target_os = "windows")] pub use deny_read_acl::plan_deny_read_acl_paths; -#[cfg(target_os = "windows")] -pub use deny_read_acl::write_persistent_deny_read_acl_record; pub use deny_read_resolver::resolve_windows_deny_read_paths; #[cfg(target_os = "windows")] pub use desktop::LaunchDesktop; @@ -283,10 +277,7 @@ mod windows_impl { use super::allow::compute_allow_paths; use super::cap::load_or_create_cap_sids; use super::cap::workspace_cap_sid_for_cwd; - use super::deny_read_acl::DenyReadAclRecordKind; use super::deny_read_acl::apply_deny_read_acls; - use super::deny_read_acl::cleanup_stale_persistent_deny_read_acls; - use super::deny_read_acl::write_persistent_deny_read_acl_record; use super::logging::log_failure; use super::logging::log_success; use super::path_normalization::canonicalize_path; @@ -297,6 +288,7 @@ mod windows_impl { use super::token::convert_string_sid_to_sid; use super::token::create_workspace_write_token_with_caps_from; use super::workspace_acl::is_command_cwd_root; + use anyhow::Context; use anyhow::Result; use std::collections::HashMap; use std::ffi::c_void; @@ -457,26 +449,17 @@ mod windows_impl { let AllowDenyPaths { allow, mut deny } = compute_allow_paths(&policy, sandbox_policy_cwd, ¤t_dir, &env_map); for path in additional_deny_write_paths { - if path.exists() { - deny.insert(path.clone()); + // Explicit deny-write carveouts must already exist when the process + // starts, otherwise it could create a missing path under a writable + // parent before the deny-write ACE exists. + if !path.exists() { + std::fs::create_dir_all(path) + .with_context(|| format!("create deny-write path {}", path.display()))?; } + deny.insert(path.clone()); } let canonical_cwd = canonicalize_path(¤t_dir); let mut guards: Vec<(PathBuf, *mut c_void)> = Vec::new(); - if persist_aces { - // Persistent workspace-write ACEs survive between commands, so drop - // deny-read ACLs from the previous policy before applying the new - // overlay. Non-persistent runs use guards and clean up at process - // exit instead. - unsafe { - cleanup_stale_persistent_deny_read_acls( - codex_home, - DenyReadAclRecordKind::RestrictedToken, - additional_deny_read_paths, - psid_generic, - )?; - } - } unsafe { for p in &allow { let psid = if is_workspace_write && is_command_cwd_root(p, &canonical_cwd) { @@ -517,13 +500,7 @@ mod windows_impl { return Err(err); } }; - if persist_aces { - write_persistent_deny_read_acl_record( - codex_home, - DenyReadAclRecordKind::RestrictedToken, - &applied_deny_read_paths, - )?; - } else { + if !persist_aces { for path in applied_deny_read_paths { guards.push((path, psid_generic)); }