mirror of
https://github.com/openai/codex.git
synced 2026-11-08 20:40:33 +00:00
Terminate Windows sandbox on metadata monitor violation
This commit is contained in:
@@ -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()?;
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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<Vec<PathBuf>> {
|
||||
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<thread::JoinHandle<()>>,
|
||||
monitored_paths: Vec<PathBuf>,
|
||||
removed_paths: Arc<Mutex<Vec<PathBuf>>>,
|
||||
errors: Arc<Mutex<Vec<String>>>,
|
||||
armed: Arc<AtomicBool>,
|
||||
violation_seen: Arc<AtomicBool>,
|
||||
violation_handler: Arc<Mutex<Option<ViolationHandler>>>,
|
||||
}
|
||||
|
||||
type ViolationHandler = Box<dyn Fn() + Send + Sync + 'static>;
|
||||
|
||||
impl MissingCreationMonitor {
|
||||
fn start(paths: &[PathBuf]) -> Result<Self> {
|
||||
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<Vec<PathBuf>> {
|
||||
self.stop_listeners();
|
||||
let errors = self
|
||||
@@ -361,10 +420,16 @@ fn enforce_monitored_paths(
|
||||
paths: &[PathBuf],
|
||||
removed_paths: &Arc<Mutex<Vec<PathBuf>>>,
|
||||
errors: &Arc<Mutex<Vec<String>>>,
|
||||
record_violation: bool,
|
||||
violation_seen: &Arc<AtomicBool>,
|
||||
violation_handler: &Arc<Mutex<Option<ViolationHandler>>>,
|
||||
) {
|
||||
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<AtomicBool>,
|
||||
violation_handler: &Arc<Mutex<Option<ViolationHandler>>>,
|
||||
) {
|
||||
if !violation_seen.swap(true, Ordering::SeqCst) {
|
||||
invoke_violation_handler(violation_handler);
|
||||
}
|
||||
}
|
||||
|
||||
fn invoke_violation_handler(violation_handler: &Arc<Mutex<Option<ViolationHandler>>>) {
|
||||
if let Ok(handler) = violation_handler.lock()
|
||||
&& let Some(handler) = handler.as_ref()
|
||||
{
|
||||
handler();
|
||||
}
|
||||
}
|
||||
|
||||
fn record_monitor_error(errors: &Arc<Mutex<Vec<String>>>, 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");
|
||||
|
||||
@@ -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 || {
|
||||
|
||||
@@ -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 };
|
||||
|
||||
Reference in New Issue
Block a user