From 917f39ec12ddc30e5cd093364c4532901fdc604c Mon Sep 17 00:00:00 2001 From: iceweasel-oai Date: Fri, 7 Nov 2025 21:28:55 -0800 Subject: [PATCH] Improve world-writable scan (#6381) 1. scan many more directories since it's much faster than the original implementation 2. limit overall scan time to 2s 3. skip some directories that are noisy - ApplicationData, Installer, etc. --- codex-rs/Cargo.lock | 1 + codex-rs/cli/src/debug_sandbox.rs | 2 + codex-rs/tui/src/app.rs | 15 +++++-- codex-rs/tui/src/chatwidget.rs | 6 ++- codex-rs/windows-sandbox-rs/Cargo.toml | 1 + codex-rs/windows-sandbox-rs/src/audit.rs | 57 +++++++++++++++++------- codex-rs/windows-sandbox-rs/src/lib.rs | 4 +- 7 files changed, 66 insertions(+), 20 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index a7f73e9c..98ff97cd 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1566,6 +1566,7 @@ version = "0.1.0" dependencies = [ "anyhow", "dirs-next", + "dunce", "rand 0.8.5", "serde", "serde_json", diff --git a/codex-rs/cli/src/debug_sandbox.rs b/codex-rs/cli/src/debug_sandbox.rs index 6e7cd077..cbb66af8 100644 --- a/codex-rs/cli/src/debug_sandbox.rs +++ b/codex-rs/cli/src/debug_sandbox.rs @@ -136,6 +136,8 @@ async fn run_command_under_sandbox( let env_map = env.clone(); let command_vec = command.clone(); let base_dir = config.codex_home.clone(); + + // Preflight audit is invoked elsewhere at the appropriate times. let res = tokio::task::spawn_blocking(move || { run_windows_sandbox_capture( policy_str, diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index fbf3d2a5..d992fccc 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -191,7 +191,8 @@ impl App { let cwd = app.config.cwd.clone(); let env_map: std::collections::HashMap = std::env::vars().collect(); let tx = app.app_event_tx.clone(); - Self::spawn_world_writable_scan(cwd, env_map, tx, false); + let logs_base_dir = app.config.codex_home.clone(); + Self::spawn_world_writable_scan(cwd, env_map, logs_base_dir, tx, false); } } @@ -472,7 +473,8 @@ impl App { let env_map: std::collections::HashMap = std::env::vars().collect(); let tx = self.app_event_tx.clone(); - Self::spawn_world_writable_scan(cwd, env_map, tx, false); + let logs_base_dir = self.config.codex_home.clone(); + Self::spawn_world_writable_scan(cwd, env_map, logs_base_dir, tx, false); } } } @@ -624,11 +626,18 @@ impl App { fn spawn_world_writable_scan( cwd: PathBuf, env_map: std::collections::HashMap, + logs_base_dir: PathBuf, tx: AppEventSender, apply_preset_on_continue: bool, ) { tokio::task::spawn_blocking(move || { - if codex_windows_sandbox::preflight_audit_everyone_writable(&cwd, &env_map).is_err() { + if codex_windows_sandbox::preflight_audit_everyone_writable( + &cwd, + &env_map, + Some(logs_base_dir.as_path()), + ) + .is_err() + { if apply_preset_on_continue { if let Some(preset) = codex_common::approval_presets::builtin_approval_presets() .into_iter() diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 30f9b66f..0e0ff4c5 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -2106,7 +2106,11 @@ impl ChatWidget { for (k, v) in std::env::vars() { env_map.insert(k, v); } - match codex_windows_sandbox::preflight_audit_everyone_writable(&self.config.cwd, &env_map) { + match codex_windows_sandbox::preflight_audit_everyone_writable( + &self.config.cwd, + &env_map, + Some(self.config.codex_home.as_path()), + ) { Ok(()) => false, Err(_) => true, } diff --git a/codex-rs/windows-sandbox-rs/Cargo.toml b/codex-rs/windows-sandbox-rs/Cargo.toml index ab231530..ba39bbe0 100644 --- a/codex-rs/windows-sandbox-rs/Cargo.toml +++ b/codex-rs/windows-sandbox-rs/Cargo.toml @@ -11,6 +11,7 @@ path = "src/lib.rs" anyhow = "1.0" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" +dunce = "1.0" [dependencies.rand] version = "0.8" default-features = false diff --git a/codex-rs/windows-sandbox-rs/src/audit.rs b/codex-rs/windows-sandbox-rs/src/audit.rs index 71dceaa5..b0a55da0 100644 --- a/codex-rs/windows-sandbox-rs/src/audit.rs +++ b/codex-rs/windows-sandbox-rs/src/audit.rs @@ -38,6 +38,22 @@ use windows_sys::Win32::Security::ACCESS_ALLOWED_ACE; use windows_sys::Win32::Security::ACE_HEADER; use windows_sys::Win32::Security::EqualSid; +// Preflight scan limits +const MAX_ITEMS_PER_DIR: i32 = 1000; +const AUDIT_TIME_LIMIT_SECS: i64 = 2; +const MAX_CHECKED_LIMIT: i32 = 50000; +// Case-insensitive suffixes (normalized to forward slashes) to skip during one-level child scan +const SKIP_DIR_SUFFIXES: &[&str] = &[ + "/windows/installer", + "/windows/registration", + "/programdata", +]; + +fn normalize_path_key(p: &Path) -> String { + let n = dunce::canonicalize(p).unwrap_or_else(|_| p.to_path_buf()); + n.to_string_lossy().replace('\\', "/").to_ascii_lowercase() +} + fn unique_push(set: &mut HashSet, out: &mut Vec, p: PathBuf) { if let Ok(abs) = p.canonicalize() { if set.insert(abs.clone()) { @@ -77,11 +93,7 @@ fn gather_candidates(cwd: &Path, env: &std::collections::HashMap } } // 5) Core system roots last - for p in [ - PathBuf::from("C:/"), - PathBuf::from("C:/Windows"), - PathBuf::from("C:/ProgramData"), - ] { + for p in [PathBuf::from("C:/"), PathBuf::from("C:/Windows")] { unique_push(&mut set, &mut out, p); } out @@ -164,14 +176,18 @@ unsafe fn path_has_world_write_allow(path: &Path) -> Result { pub fn audit_everyone_writable( cwd: &Path, env: &std::collections::HashMap, + logs_base_dir: Option<&Path>, ) -> Result<()> { let start = Instant::now(); let mut flagged: Vec = Vec::new(); + let mut seen: HashSet = HashSet::new(); let mut checked = 0usize; // Fast path: check CWD immediate children first so workspace issues are caught early. if let Ok(read) = std::fs::read_dir(cwd) { - for ent in read.flatten().take(250) { - if start.elapsed() > Duration::from_secs(5) || checked > 5000 { + for ent in read.flatten().take(MAX_ITEMS_PER_DIR as usize) { + if start.elapsed() > Duration::from_secs(AUDIT_TIME_LIMIT_SECS as u64) + || checked > MAX_CHECKED_LIMIT as usize + { break; } let ft = match ent.file_type() { @@ -185,26 +201,32 @@ pub fn audit_everyone_writable( checked += 1; let has = unsafe { path_has_world_write_allow(&p)? }; if has { - flagged.push(p); + let key = normalize_path_key(&p); + if seen.insert(key) { flagged.push(p); } } } } // Continue with broader candidate sweep let candidates = gather_candidates(cwd, env); for root in candidates { - if start.elapsed() > Duration::from_secs(5) || checked > 5000 { + if start.elapsed() > Duration::from_secs(AUDIT_TIME_LIMIT_SECS as u64) + || checked > MAX_CHECKED_LIMIT as usize + { break; } checked += 1; let has_root = unsafe { path_has_world_write_allow(&root)? }; if has_root { - flagged.push(root.clone()); + let key = normalize_path_key(&root); + if seen.insert(key) { flagged.push(root.clone()); } } // one level down best-effort if let Ok(read) = std::fs::read_dir(&root) { - for ent in read.flatten().take(250) { + for ent in read.flatten().take(MAX_ITEMS_PER_DIR as usize) { let p = ent.path(); - if start.elapsed() > Duration::from_secs(5) || checked > 5000 { + if start.elapsed() > Duration::from_secs(AUDIT_TIME_LIMIT_SECS as u64) + || checked > MAX_CHECKED_LIMIT as usize + { break; } // Skip reparse points (symlinks/junctions) to avoid auditing link ACLs @@ -215,11 +237,16 @@ pub fn audit_everyone_writable( if ft.is_symlink() { continue; } + // Skip noisy/irrelevant Windows system subdirectories + let pl = p.to_string_lossy().to_ascii_lowercase(); + let norm = pl.replace('\\', "/"); + if SKIP_DIR_SUFFIXES.iter().any(|s| norm.ends_with(s)) { continue; } if ft.is_dir() { checked += 1; let has_child = unsafe { path_has_world_write_allow(&p)? }; if has_child { - flagged.push(p); + let key = normalize_path_key(&p); + if seen.insert(key) { flagged.push(p); } } } } @@ -236,7 +263,7 @@ pub fn audit_everyone_writable( "AUDIT: world-writable scan FAILED; checked={checked}; duration_ms={elapsed_ms}; flagged:{}", list ), - Some(cwd), + logs_base_dir, ); let mut list_err = String::new(); for p in flagged { @@ -252,7 +279,7 @@ pub fn audit_everyone_writable( &format!( "AUDIT: world-writable scan OK; checked={checked}; duration_ms={elapsed_ms}" ), - Some(cwd), + logs_base_dir, ); Ok(()) } diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 5e165f42..561ae2c3 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -171,8 +171,9 @@ mod windows_impl { pub fn preflight_audit_everyone_writable( cwd: &Path, env_map: &HashMap, + logs_base_dir: Option<&Path>, ) -> Result<()> { - audit::audit_everyone_writable(cwd, env_map) + audit::audit_everyone_writable(cwd, env_map, logs_base_dir) } pub fn run_windows_sandbox_capture( @@ -436,6 +437,7 @@ mod stub { pub fn preflight_audit_everyone_writable( _cwd: &Path, _env_map: &HashMap, + _logs_base_dir: Option<&Path>, ) -> Result<()> { bail!("Windows sandbox is only available on Windows") }