From 826ea82dc79c1f1580923352c7ae04113d093741 Mon Sep 17 00:00:00 2001 From: Eva Wong Date: Tue, 5 May 2026 15:39:11 -0700 Subject: [PATCH] Terminate Windows sandbox on metadata monitor violation --- codex-rs/cli/src/debug_sandbox.rs | 54 +++++- .../windows-sandbox-rs/src/elevated_impl.rs | 23 ++- codex-rs/windows-sandbox-rs/src/lib.rs | 10 + .../src/protected_metadata.rs | 178 +++++++++++++++++- .../src/unified_exec/backends/elevated.rs | 11 ++ .../src/unified_exec/backends/legacy.rs | 12 ++ 6 files changed, 284 insertions(+), 4 deletions(-) diff --git a/codex-rs/cli/src/debug_sandbox.rs b/codex-rs/cli/src/debug_sandbox.rs index 35c1a4fafe..a9a4df0e2b 100644 --- a/codex-rs/cli/src/debug_sandbox.rs +++ b/codex-rs/cli/src/debug_sandbox.rs @@ -478,7 +478,7 @@ fn windows_debug_protected_metadata_targets( let mode = if std::fs::symlink_metadata(path.as_path()).is_ok() { codex_windows_sandbox::ProtectedMetadataMode::ExistingDeny } else { - codex_windows_sandbox::ProtectedMetadataMode::MissingDenySentinel + codex_windows_sandbox::ProtectedMetadataMode::MissingCreationMonitor }; targets.push(codex_windows_sandbox::ProtectedMetadataTarget { path: path.as_path().to_path_buf(), @@ -794,6 +794,58 @@ mod tests { Ok(()) } + #[cfg(target_os = "windows")] + #[test] + fn windows_debug_sandbox_uses_creation_monitor_for_missing_metadata() { + let temp_dir = TempDir::new().expect("tempdir"); + let policy = codex_protocol::permissions::FileSystemSandboxPolicy::restricted(vec![ + codex_protocol::permissions::FileSystemSandboxEntry { + path: codex_protocol::permissions::FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::Root, + }, + access: codex_protocol::permissions::FileSystemAccessMode::Read, + }, + codex_protocol::permissions::FileSystemSandboxEntry { + path: codex_protocol::permissions::FileSystemPath::Special { + value: codex_protocol::permissions::FileSystemSpecialPath::project_roots(None), + }, + access: codex_protocol::permissions::FileSystemAccessMode::Write, + }, + ]); + + let expected_root = policy + .get_writable_roots_with_cwd(temp_dir.path()) + .into_iter() + .find(|root| { + root.protected_metadata_names + .iter() + .any(|name| name == ".agents") + }) + .expect("protected metadata writable root") + .root + .as_path() + .to_path_buf(); + let targets = windows_debug_protected_metadata_targets(&policy, temp_dir.path()); + + assert_eq!( + targets, + vec![ + codex_windows_sandbox::ProtectedMetadataTarget { + path: expected_root.join(".agents"), + mode: codex_windows_sandbox::ProtectedMetadataMode::MissingCreationMonitor, + }, + codex_windows_sandbox::ProtectedMetadataTarget { + path: expected_root.join(".codex"), + mode: codex_windows_sandbox::ProtectedMetadataMode::MissingCreationMonitor, + }, + codex_windows_sandbox::ProtectedMetadataTarget { + path: expected_root.join(".git"), + mode: codex_windows_sandbox::ProtectedMetadataMode::MissingCreationMonitor, + }, + ] + ); + } + #[tokio::test] async fn debug_sandbox_honors_active_permission_profiles() -> anyhow::Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs index 3237a75e56..b1e9d0ff6e 100644 --- a/codex-rs/windows-sandbox-rs/src/elevated_impl.rs +++ b/codex-rs/windows-sandbox-rs/src/elevated_impl.rs @@ -30,11 +30,14 @@ mod windows_impl { use crate::env::inherit_path_env; use crate::env::normalize_null_device_env; use crate::identity::require_logon_sandbox_creds; + use crate::ipc_framed::EmptyPayload; + use crate::ipc_framed::FramedMessage; use crate::ipc_framed::Message; use crate::ipc_framed::OutputStream; use crate::ipc_framed::SpawnRequest; use crate::ipc_framed::decode_bytes; use crate::ipc_framed::read_frame; + use crate::ipc_framed::write_frame; use crate::logging::log_failure; use crate::logging::log_start; use crate::logging::log_success; @@ -47,6 +50,8 @@ mod windows_impl { use crate::token::convert_string_sid_to_sid; use anyhow::Result; use std::path::Path; + use std::sync::Arc; + use std::sync::Mutex; pub use crate::windows_impl::CaptureResult; @@ -157,7 +162,23 @@ mod windows_impl { spawn_request, )?; let (pipe_write, mut pipe_read) = transport.into_files(); - drop(pipe_write); + let pipe_write = Arc::new(Mutex::new(pipe_write)); + { + let pipe_write = Arc::clone(&pipe_write); + protected_metadata_runtime.set_violation_handler(Box::new(move || { + if let Ok(mut pipe_write) = pipe_write.lock() { + let _ = write_frame( + &mut *pipe_write, + &FramedMessage { + version: 1, + message: Message::Terminate { + payload: EmptyPayload::default(), + }, + }, + ); + } + }))?; + } let mut stdout = Vec::new(); let mut stderr = Vec::new(); diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 48abbc6543..19d4b13b83 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -304,6 +304,7 @@ mod windows_impl { use windows_sys::Win32::Foundation::GetLastError; use windows_sys::Win32::Foundation::HANDLE; use windows_sys::Win32::Foundation::HANDLE_FLAG_INHERIT; + use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; use windows_sys::Win32::Foundation::SetHandleInformation; use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_EXECUTE; use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_READ; @@ -570,6 +571,15 @@ mod windows_impl { } }; let pi = created.process_info; + let child_process_handle = pi.hProcess; + protected_metadata_runtime.set_violation_handler(Box::new(move || unsafe { + if child_process_handle != 0 && child_process_handle != INVALID_HANDLE_VALUE { + let _ = windows_sys::Win32::System::Threading::TerminateProcess( + child_process_handle, + 1, + ); + } + }))?; let _desktop = created; unsafe { diff --git a/codex-rs/windows-sandbox-rs/src/protected_metadata.rs b/codex-rs/windows-sandbox-rs/src/protected_metadata.rs index f71658f948..13e108c9a2 100644 --- a/codex-rs/windows-sandbox-rs/src/protected_metadata.rs +++ b/codex-rs/windows-sandbox-rs/src/protected_metadata.rs @@ -15,6 +15,8 @@ use std::path::PathBuf; use std::process::Command; use std::process::Stdio; use std::sync::Arc; +use std::sync::atomic::AtomicBool; +use std::sync::atomic::Ordering; use std::sync::Mutex; use std::thread; use std::time::SystemTime; @@ -117,6 +119,10 @@ pub(crate) struct ProtectedMetadataRuntime { } impl ProtectedMetadataRuntime { + pub(crate) fn set_violation_handler(&self, handler: ViolationHandler) -> Result<()> { + self.monitor.set_violation_handler(handler) + } + pub(crate) fn finish(mut self) -> Result> { let monitor_result = self.monitor.finish(); let cleanup_result = self.guard.cleanup_created_paths(); @@ -153,18 +159,28 @@ impl Drop for SentinelHandle { struct MissingCreationMonitor { stop_event: HANDLE, listeners: Vec>, + monitored_paths: Vec, removed_paths: Arc>>, errors: Arc>>, + armed: Arc, + violation_seen: Arc, + violation_handler: Arc>>, } +type ViolationHandler = Box; + impl MissingCreationMonitor { fn start(paths: &[PathBuf]) -> Result { if paths.is_empty() { return Ok(Self { stop_event: 0, listeners: Vec::new(), + monitored_paths: Vec::new(), removed_paths: Arc::new(Mutex::new(Vec::new())), errors: Arc::new(Mutex::new(Vec::new())), + armed: Arc::new(AtomicBool::new(false)), + violation_seen: Arc::new(AtomicBool::new(false)), + violation_handler: Arc::new(Mutex::new(None)), }); } @@ -179,8 +195,12 @@ impl MissingCreationMonitor { let mut monitor = Self { stop_event, listeners: Vec::new(), + monitored_paths: paths.to_vec(), removed_paths: Arc::new(Mutex::new(Vec::new())), errors: Arc::new(Mutex::new(Vec::new())), + armed: Arc::new(AtomicBool::new(false)), + violation_seen: Arc::new(AtomicBool::new(false)), + violation_handler: Arc::new(Mutex::new(None)), }; for (parent, watched_paths) in monitored_paths_by_parent(paths) { @@ -222,12 +242,22 @@ impl MissingCreationMonitor { let stop_event = self.stop_event; let removed_paths = Arc::clone(&self.removed_paths); let errors = Arc::clone(&self.errors); + let armed = Arc::clone(&self.armed); + let violation_seen = Arc::clone(&self.violation_seen); + let violation_handler = Arc::clone(&self.violation_handler); let parent_display = parent.display().to_string(); let parent_display_for_listener = parent_display.clone(); thread::Builder::new() .name("codex-protected-metadata-monitor".to_string()) .spawn(move || { - enforce_monitored_paths(&watched_paths, &removed_paths, &errors); + enforce_monitored_paths( + &watched_paths, + &removed_paths, + &errors, + armed.load(Ordering::SeqCst), + &violation_seen, + &violation_handler, + ); loop { let handles = [change_handle, stop_event]; let wait_result = unsafe { @@ -240,7 +270,14 @@ impl MissingCreationMonitor { }; if wait_result == WAIT_OBJECT_0 { - enforce_monitored_paths(&watched_paths, &removed_paths, &errors); + enforce_monitored_paths( + &watched_paths, + &removed_paths, + &errors, + armed.load(Ordering::SeqCst), + &violation_seen, + &violation_handler, + ); if unsafe { FindNextChangeNotification(change_handle) } == 0 { record_monitor_error( &errors, @@ -288,6 +325,28 @@ impl MissingCreationMonitor { }) } + fn set_violation_handler(&self, handler: ViolationHandler) -> Result<()> { + { + let mut guard = self + .violation_handler + .lock() + .map_err(|_| anyhow!("protected metadata monitor handler state is poisoned"))?; + *guard = Some(handler); + } + + self.armed.store(true, Ordering::SeqCst); + enforce_monitored_paths( + &self.monitored_paths, + &self.removed_paths, + &self.errors, + /*record_violation*/ true, + &self.violation_seen, + &self.violation_handler, + ); + + Ok(()) + } + fn finish(&mut self) -> Result> { self.stop_listeners(); let errors = self @@ -361,10 +420,16 @@ fn enforce_monitored_paths( paths: &[PathBuf], removed_paths: &Arc>>, errors: &Arc>>, + record_violation: bool, + violation_seen: &Arc, + violation_handler: &Arc>>, ) { for path in paths { match existing_metadata_path(path) { Ok(Some(existing_path)) => { + if record_violation { + record_metadata_violation(violation_seen, violation_handler); + } if let Err(err) = remove_metadata_path(&existing_path) { record_monitor_error( errors, @@ -395,6 +460,23 @@ fn enforce_monitored_paths( } } +fn record_metadata_violation( + violation_seen: &Arc, + violation_handler: &Arc>>, +) { + if !violation_seen.swap(true, Ordering::SeqCst) { + invoke_violation_handler(violation_handler); + } +} + +fn invoke_violation_handler(violation_handler: &Arc>>) { + if let Ok(handler) = violation_handler.lock() + && let Some(handler) = handler.as_ref() + { + handler(); + } +} + fn record_monitor_error(errors: &Arc>>, message: String) { if let Ok(mut errors) = errors.lock() { errors.push(message); @@ -639,6 +721,9 @@ mod tests { use super::*; use crate::setup::ProtectedMetadataMode; use crate::setup::ProtectedMetadataTarget; + use std::sync::Arc; + use std::sync::atomic::AtomicBool; + use std::sync::atomic::Ordering; use std::time::Duration; use std::time::Instant; @@ -699,6 +784,95 @@ mod tests { ); } + #[test] + fn missing_creation_monitor_invokes_violation_handler_when_path_is_created() { + let temp_dir = tempfile::TempDir::new().expect("tempdir"); + let target = temp_dir.path().join(".codex"); + let created = temp_dir.path().join(".CODEX"); + let runtime = prepare_protected_metadata_targets(&[ProtectedMetadataTarget { + path: target, + mode: ProtectedMetadataMode::MissingCreationMonitor, + }]) + .expect("guard") + .into_runtime() + .expect("runtime"); + let violation_seen = Arc::new(AtomicBool::new(false)); + runtime + .set_violation_handler(Box::new({ + let violation_seen = Arc::clone(&violation_seen); + move || violation_seen.store(true, Ordering::SeqCst) + })) + .expect("set violation handler"); + + std::fs::create_dir_all(&created).expect("create metadata"); + let deadline = Instant::now() + Duration::from_secs(2); + while (!violation_seen.load(Ordering::SeqCst) || created.exists()) + && Instant::now() < deadline + { + std::thread::sleep(Duration::from_millis(20)); + } + + assert!( + violation_seen.load(Ordering::SeqCst), + "monitor should invoke the violation handler" + ); + assert!( + !created.exists(), + "monitor should remove protected metadata before final cleanup" + ); + let removed = runtime.finish().expect("finish"); + assert!( + removed + .iter() + .any(|path| path.file_name().and_then(std::ffi::OsStr::to_str) == Some(".CODEX")), + "removed paths should include the created case variant: {removed:?}" + ); + } + + #[test] + fn missing_creation_monitor_cleans_pre_arm_path_without_violation() { + let temp_dir = tempfile::TempDir::new().expect("tempdir"); + let target = temp_dir.path().join(".codex"); + let created = temp_dir.path().join(".CODEX"); + let runtime = prepare_protected_metadata_targets(&[ProtectedMetadataTarget { + path: target, + mode: ProtectedMetadataMode::MissingCreationMonitor, + }]) + .expect("guard") + .into_runtime() + .expect("runtime"); + + std::fs::create_dir_all(&created).expect("create metadata before arm"); + let deadline = Instant::now() + Duration::from_secs(2); + while created.exists() && Instant::now() < deadline { + std::thread::sleep(Duration::from_millis(20)); + } + assert!( + !created.exists(), + "pre-arm cleanup should remove setup-created metadata" + ); + + let violation_seen = Arc::new(AtomicBool::new(false)); + runtime + .set_violation_handler(Box::new({ + let violation_seen = Arc::clone(&violation_seen); + move || violation_seen.store(true, Ordering::SeqCst) + })) + .expect("set violation handler"); + assert!( + !violation_seen.load(Ordering::SeqCst), + "pre-arm cleanup should not terminate the command" + ); + + let removed = runtime.finish().expect("finish"); + assert!( + removed + .iter() + .any(|path| path.file_name().and_then(std::ffi::OsStr::to_str) == Some(".CODEX")), + "removed paths should include the pre-arm case variant: {removed:?}" + ); + } + #[test] fn existing_deny_paths_include_symlink_target() { let temp_dir = tempfile::TempDir::new().expect("tempdir"); diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs index d2496eadee..255f3b17e1 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/elevated.rs @@ -95,6 +95,17 @@ pub(crate) async fn spawn_windows_sandbox_session_elevated( let outbound_tx = start_runner_pipe_writer(pipe_write); let writer_handle = start_runner_stdin_writer(writer_rx, outbound_tx.clone(), tty, stdin_open); + { + let outbound_tx = outbound_tx.clone(); + protected_metadata_runtime.set_violation_handler(Box::new(move || { + let _ = outbound_tx.send(FramedMessage { + version: 1, + message: Message::Terminate { + payload: EmptyPayload::default(), + }, + }); + }))?; + } let terminator = { let outbound_tx = outbound_tx.clone(); Some(Box::new(move || { diff --git a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs index d3850b4713..9958ee02a3 100644 --- a/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs +++ b/codex-rs/windows-sandbox-rs/src/unified_exec/backends/legacy.rs @@ -412,6 +412,18 @@ pub(crate) async fn spawn_windows_sandbox_session_legacy( let hpc_handle = hpc.map(|hpc| Arc::new(StdMutex::new(Some(hpc)))); let process_handle = Arc::new(StdMutex::new(Some(pi.hProcess))); + { + let process_handle = Arc::clone(&process_handle); + protected_metadata_runtime.set_violation_handler(Box::new(move || { + if let Ok(guard) = process_handle.lock() + && let Some(handle) = guard.as_ref() + { + unsafe { + let _ = TerminateProcess(*handle, 1); + } + } + }))?; + } let wait_handle = Arc::clone(&process_handle); let command_for_wait = command.clone(); let guards_for_wait = if persist_aces { Vec::new() } else { guards };