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.
This commit is contained in:
iceweasel-oai
2025-11-07 21:28:55 -08:00
committed by GitHub
parent a2fdfce02a
commit 917f39ec12
7 changed files with 66 additions and 20 deletions

1
codex-rs/Cargo.lock generated
View File

@@ -1566,6 +1566,7 @@ version = "0.1.0"
dependencies = [ dependencies = [
"anyhow", "anyhow",
"dirs-next", "dirs-next",
"dunce",
"rand 0.8.5", "rand 0.8.5",
"serde", "serde",
"serde_json", "serde_json",

View File

@@ -136,6 +136,8 @@ async fn run_command_under_sandbox(
let env_map = env.clone(); let env_map = env.clone();
let command_vec = command.clone(); let command_vec = command.clone();
let base_dir = config.codex_home.clone(); let base_dir = config.codex_home.clone();
// Preflight audit is invoked elsewhere at the appropriate times.
let res = tokio::task::spawn_blocking(move || { let res = tokio::task::spawn_blocking(move || {
run_windows_sandbox_capture( run_windows_sandbox_capture(
policy_str, policy_str,

View File

@@ -191,7 +191,8 @@ impl App {
let cwd = app.config.cwd.clone(); let cwd = app.config.cwd.clone();
let env_map: std::collections::HashMap<String, String> = std::env::vars().collect(); let env_map: std::collections::HashMap<String, String> = std::env::vars().collect();
let tx = app.app_event_tx.clone(); 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<String, String> = let env_map: std::collections::HashMap<String, String> =
std::env::vars().collect(); std::env::vars().collect();
let tx = self.app_event_tx.clone(); 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( fn spawn_world_writable_scan(
cwd: PathBuf, cwd: PathBuf,
env_map: std::collections::HashMap<String, String>, env_map: std::collections::HashMap<String, String>,
logs_base_dir: PathBuf,
tx: AppEventSender, tx: AppEventSender,
apply_preset_on_continue: bool, apply_preset_on_continue: bool,
) { ) {
tokio::task::spawn_blocking(move || { 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 apply_preset_on_continue {
if let Some(preset) = codex_common::approval_presets::builtin_approval_presets() if let Some(preset) = codex_common::approval_presets::builtin_approval_presets()
.into_iter() .into_iter()

View File

@@ -2106,7 +2106,11 @@ impl ChatWidget {
for (k, v) in std::env::vars() { for (k, v) in std::env::vars() {
env_map.insert(k, v); 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, Ok(()) => false,
Err(_) => true, Err(_) => true,
} }

View File

@@ -11,6 +11,7 @@ path = "src/lib.rs"
anyhow = "1.0" anyhow = "1.0"
serde = { version = "1.0", features = ["derive"] } serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0" serde_json = "1.0"
dunce = "1.0"
[dependencies.rand] [dependencies.rand]
version = "0.8" version = "0.8"
default-features = false default-features = false

View File

@@ -38,6 +38,22 @@ use windows_sys::Win32::Security::ACCESS_ALLOWED_ACE;
use windows_sys::Win32::Security::ACE_HEADER; use windows_sys::Win32::Security::ACE_HEADER;
use windows_sys::Win32::Security::EqualSid; 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<PathBuf>, out: &mut Vec<PathBuf>, p: PathBuf) { fn unique_push(set: &mut HashSet<PathBuf>, out: &mut Vec<PathBuf>, p: PathBuf) {
if let Ok(abs) = p.canonicalize() { if let Ok(abs) = p.canonicalize() {
if set.insert(abs.clone()) { if set.insert(abs.clone()) {
@@ -77,11 +93,7 @@ fn gather_candidates(cwd: &Path, env: &std::collections::HashMap<String, String>
} }
} }
// 5) Core system roots last // 5) Core system roots last
for p in [ for p in [PathBuf::from("C:/"), PathBuf::from("C:/Windows")] {
PathBuf::from("C:/"),
PathBuf::from("C:/Windows"),
PathBuf::from("C:/ProgramData"),
] {
unique_push(&mut set, &mut out, p); unique_push(&mut set, &mut out, p);
} }
out out
@@ -164,14 +176,18 @@ unsafe fn path_has_world_write_allow(path: &Path) -> Result<bool> {
pub fn audit_everyone_writable( pub fn audit_everyone_writable(
cwd: &Path, cwd: &Path,
env: &std::collections::HashMap<String, String>, env: &std::collections::HashMap<String, String>,
logs_base_dir: Option<&Path>,
) -> Result<()> { ) -> Result<()> {
let start = Instant::now(); let start = Instant::now();
let mut flagged: Vec<PathBuf> = Vec::new(); let mut flagged: Vec<PathBuf> = Vec::new();
let mut seen: HashSet<String> = HashSet::new();
let mut checked = 0usize; let mut checked = 0usize;
// Fast path: check CWD immediate children first so workspace issues are caught early. // Fast path: check CWD immediate children first so workspace issues are caught early.
if let Ok(read) = std::fs::read_dir(cwd) { if let Ok(read) = std::fs::read_dir(cwd) {
for ent in read.flatten().take(250) { for ent in read.flatten().take(MAX_ITEMS_PER_DIR as usize) {
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; break;
} }
let ft = match ent.file_type() { let ft = match ent.file_type() {
@@ -185,26 +201,32 @@ pub fn audit_everyone_writable(
checked += 1; checked += 1;
let has = unsafe { path_has_world_write_allow(&p)? }; let has = unsafe { path_has_world_write_allow(&p)? };
if has { if has {
flagged.push(p); let key = normalize_path_key(&p);
if seen.insert(key) { flagged.push(p); }
} }
} }
} }
// Continue with broader candidate sweep // Continue with broader candidate sweep
let candidates = gather_candidates(cwd, env); let candidates = gather_candidates(cwd, env);
for root in candidates { 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; break;
} }
checked += 1; checked += 1;
let has_root = unsafe { path_has_world_write_allow(&root)? }; let has_root = unsafe { path_has_world_write_allow(&root)? };
if has_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 // one level down best-effort
if let Ok(read) = std::fs::read_dir(&root) { 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(); 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; break;
} }
// Skip reparse points (symlinks/junctions) to avoid auditing link ACLs // Skip reparse points (symlinks/junctions) to avoid auditing link ACLs
@@ -215,11 +237,16 @@ pub fn audit_everyone_writable(
if ft.is_symlink() { if ft.is_symlink() {
continue; 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() { if ft.is_dir() {
checked += 1; checked += 1;
let has_child = unsafe { path_has_world_write_allow(&p)? }; let has_child = unsafe { path_has_world_write_allow(&p)? };
if has_child { 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:{}", "AUDIT: world-writable scan FAILED; checked={checked}; duration_ms={elapsed_ms}; flagged:{}",
list list
), ),
Some(cwd), logs_base_dir,
); );
let mut list_err = String::new(); let mut list_err = String::new();
for p in flagged { for p in flagged {
@@ -252,7 +279,7 @@ pub fn audit_everyone_writable(
&format!( &format!(
"AUDIT: world-writable scan OK; checked={checked}; duration_ms={elapsed_ms}" "AUDIT: world-writable scan OK; checked={checked}; duration_ms={elapsed_ms}"
), ),
Some(cwd), logs_base_dir,
); );
Ok(()) Ok(())
} }

View File

@@ -171,8 +171,9 @@ mod windows_impl {
pub fn preflight_audit_everyone_writable( pub fn preflight_audit_everyone_writable(
cwd: &Path, cwd: &Path,
env_map: &HashMap<String, String>, env_map: &HashMap<String, String>,
logs_base_dir: Option<&Path>,
) -> Result<()> { ) -> Result<()> {
audit::audit_everyone_writable(cwd, env_map) audit::audit_everyone_writable(cwd, env_map, logs_base_dir)
} }
pub fn run_windows_sandbox_capture( pub fn run_windows_sandbox_capture(
@@ -436,6 +437,7 @@ mod stub {
pub fn preflight_audit_everyone_writable( pub fn preflight_audit_everyone_writable(
_cwd: &Path, _cwd: &Path,
_env_map: &HashMap<String, String>, _env_map: &HashMap<String, String>,
_logs_base_dir: Option<&Path>,
) -> Result<()> { ) -> Result<()> {
bail!("Windows sandbox is only available on Windows") bail!("Windows sandbox is only available on Windows")
} }