From 7e45bdb5fd0e6cae3aeb14330deed7861bb516da Mon Sep 17 00:00:00 2001 From: johnl-oai Date: Wed, 2 Sep 2026 18:40:49 +0000 Subject: [PATCH] Enable authenticated Windows sandbox provisioning (#42351) ## Why The Windows sandbox service had provisioning policy and client authentication in place, but its IPC listener was still disabled. ## What changed - Serve framed provisioning requests over a local named pipe, authenticate packaged clients, validate requests and machine policy, and report provisioning outcomes through bounded responses and Windows event logging. - Return `unavailable` for configuration parse failures so clients can fall back to the elevated setup helper. - Keep validated directory handles alive in the setup helper so path protections survive an unexpected service exit. - Make connection recovery and shutdown wakeups tolerate clients that disconnect before the listener accepts them. ## Testing Added Windows tests for response framing, configuration-error classification, pipe security and reconnect behavior, shutdown wakeups, and retained helper handles. GitOrigin-RevId: 68e9d546dcada88ff162ea4f98c0f4d748706b34 --- codex-rs/windows-sandbox-rs/src/lib.rs | 5 + .../src/provisioning_client.rs | 3 + .../src/provisioning_protocol.rs | 6 +- codex-rs/windows-sandbox-rs/src/setup.rs | 64 ++- .../windows-sandbox-rs/src/setup_launch.rs | 75 ++++ .../src/setup_launch_tests.rs | 136 +++++++ codex-rs/windows-sandbox-service/Cargo.toml | 2 - codex-rs/windows-sandbox-service/src/ipc.rs | 370 +++++++++++++++++- .../src/ipc/authentication.rs | 6 +- .../windows-sandbox-service/src/ipc_tests.rs | 237 ++++++++++- codex-rs/windows-sandbox-service/src/lib.rs | 3 - .../windows-sandbox-service/src/service.rs | 9 +- 12 files changed, 857 insertions(+), 59 deletions(-) create mode 100644 codex-rs/windows-sandbox-rs/src/setup_launch.rs create mode 100644 codex-rs/windows-sandbox-rs/src/setup_launch_tests.rs diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index d4c23b2dab..9c6dd779e3 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -126,6 +126,9 @@ mod setup; #[cfg(target_os = "windows")] mod setup_error; +#[cfg(target_os = "windows")] +mod setup_launch; + #[cfg(target_os = "windows")] mod spawn_prep; @@ -321,6 +324,8 @@ pub use setup::SetupRootOverrides; #[cfg(target_os = "windows")] pub use setup::run_elevated_provisioning_setup; #[cfg(target_os = "windows")] +pub use setup::run_elevated_provisioning_setup_with_retained_handles; +#[cfg(target_os = "windows")] pub use setup::run_elevated_setup; #[cfg(target_os = "windows")] pub use setup::run_setup_refresh; diff --git a/codex-rs/windows-sandbox-rs/src/provisioning_client.rs b/codex-rs/windows-sandbox-rs/src/provisioning_client.rs index edc58e045b..d02072b054 100644 --- a/codex-rs/windows-sandbox-rs/src/provisioning_client.rs +++ b/codex-rs/windows-sandbox-rs/src/provisioning_client.rs @@ -171,6 +171,9 @@ pub fn provision_windows_sandbox_via_service( crate::SandboxProvisioningResponse::Ok => { Ok(WindowsSandboxProvisioningOutcome::Provisioned) } + crate::SandboxProvisioningResponse::Unavailable => { + Ok(WindowsSandboxProvisioningOutcome::Unavailable) + } crate::SandboxProvisioningResponse::Error { message } => Err(anyhow!(message)), } } diff --git a/codex-rs/windows-sandbox-rs/src/provisioning_protocol.rs b/codex-rs/windows-sandbox-rs/src/provisioning_protocol.rs index ca05a73ef1..f2524faf79 100644 --- a/codex-rs/windows-sandbox-rs/src/provisioning_protocol.rs +++ b/codex-rs/windows-sandbox-rs/src/provisioning_protocol.rs @@ -55,7 +55,11 @@ pub struct WindowsSandboxProxyListeners { #[serde(tag = "status", rename_all = "snake_case")] pub enum SandboxProvisioningResponse { Ok, - Error { message: String }, + /// The client should fall back to its elevated setup helper. + Unavailable, + Error { + message: String, + }, } /// Write a length-prefixed provisioning-service message. diff --git a/codex-rs/windows-sandbox-rs/src/setup.rs b/codex-rs/windows-sandbox-rs/src/setup.rs index 86611a0164..7926c7e7fd 100644 --- a/codex-rs/windows-sandbox-rs/src/setup.rs +++ b/codex-rs/windows-sandbox-rs/src/setup.rs @@ -4,6 +4,7 @@ use std::collections::BTreeSet; use std::collections::HashMap; use std::collections::HashSet; use std::ffi::c_void; +use std::os::windows::io::BorrowedHandle; use std::os::windows::process::CommandExt; use std::path::Path; use std::path::PathBuf; @@ -897,6 +898,7 @@ fn run_setup_exe( payload: &ElevationPayload, needs_elevation: bool, codex_home: &Path, + retained_handles: &[BorrowedHandle<'_>], ) -> Result<()> { let payload_json = serde_json::to_string(payload).map_err(|err| { failure( @@ -905,8 +907,13 @@ fn run_setup_exe( ) })?; let payload_b64 = BASE64_STANDARD.encode(payload_json.as_bytes()); + if !retained_handles.is_empty() { + // Service requests are serialized and must not join a bare setup flight + // whose helper was started without these directory protections. + return run_setup_exe_payload(&payload_b64, needs_elevation, codex_home, retained_handles); + } run_setup_singleflight(payload_b64.clone(), || { - run_setup_exe_payload(&payload_b64, needs_elevation, codex_home) + run_setup_exe_payload(&payload_b64, needs_elevation, codex_home, retained_handles) }) } @@ -914,6 +921,7 @@ fn run_setup_exe_payload( payload_b64: &str, needs_elevation: bool, codex_home: &Path, + retained_handles: &[BorrowedHandle<'_>], ) -> Result<()> { use windows_sys::Win32::System::Threading::GetExitCodeProcess; use windows_sys::Win32::System::Threading::INFINITE; @@ -937,19 +945,27 @@ fn run_setup_exe_payload( }; if !needs_elevation { - let status = Command::new(&exe) - .arg(payload_b64) - .creation_flags(0x08000000) // CREATE_NO_WINDOW - .stdin(Stdio::null()) - .stdout(Stdio::null()) - .stderr(Stdio::null()) - .status() - .map_err(|err| { - failure( - SetupErrorCode::OrchestratorHelperLaunchFailed, - format!("failed to launch setup helper (non-elevated): {err}"), - ) - })?; + let status = if retained_handles.is_empty() { + Command::new(&exe) + .arg(payload_b64) + .creation_flags(0x08000000) // CREATE_NO_WINDOW + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status() + } else { + crate::setup_launch::spawn_with_retained_handles( + Command::new(&exe).arg(payload_b64), + retained_handles, + ) + .and_then(|mut child| child.wait()) + } + .map_err(|err| { + failure( + SetupErrorCode::OrchestratorHelperLaunchFailed, + format!("failed to launch setup helper (non-elevated): {err}"), + ) + })?; if !status.success() { return Err(report_helper_failure( codex_home, @@ -1052,7 +1068,7 @@ fn run_elevated_setup_inner( format!("failed to determine elevation state: {err}"), ) })?; - run_setup_exe(&payload, needs_elevation, request.codex_home) + run_setup_exe(&payload, needs_elevation, request.codex_home, &[]) } fn elevated_provisioning_payload( @@ -1084,6 +1100,17 @@ pub fn run_elevated_provisioning_setup( codex_home: &Path, real_user: &str, settings: crate::WindowsSandboxProvisioningSettings, +) -> Result<()> { + run_elevated_provisioning_setup_with_retained_handles(codex_home, real_user, settings, &[]) +} + +/// Runs service provisioning with directory protections retained by the helper +/// itself, so they survive an unexpected exit of the provisioning service. +pub fn run_elevated_provisioning_setup_with_retained_handles( + codex_home: &Path, + real_user: &str, + settings: crate::WindowsSandboxProvisioningSettings, + retained_handles: &[BorrowedHandle<'_>], ) -> Result<()> { if !codex_home.is_absolute() || !matches!( @@ -1138,7 +1165,12 @@ pub fn run_elevated_provisioning_setup( mode: SetupMode::ProvisionOnly, refresh_only: false, }; - run_setup_exe(&payload, /*needs_elevation*/ false, codex_home) + run_setup_exe( + &payload, + /*needs_elevation*/ false, + codex_home, + retained_handles, + ) } pub(crate) fn build_payload_roots( diff --git a/codex-rs/windows-sandbox-rs/src/setup_launch.rs b/codex-rs/windows-sandbox-rs/src/setup_launch.rs new file mode 100644 index 0000000000..a811b6acca --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/setup_launch.rs @@ -0,0 +1,75 @@ +//! Starts provisioning helpers only after their retained directory handles are installed. +//! The child owns its duplicates until exit, independently of the service lifetime. + +use std::io; +use std::os::windows::io::AsRawHandle; +use std::os::windows::io::BorrowedHandle; +use std::os::windows::process::CommandExt; +use std::process::Child; +use std::process::Command; +use std::process::Stdio; +use windows_sys::Win32::Foundation::DUPLICATE_SAME_ACCESS; +use windows_sys::Win32::Foundation::DuplicateHandle; +use windows_sys::Win32::Foundation::HANDLE; +use windows_sys::Win32::Foundation::NTSTATUS; +use windows_sys::Win32::Foundation::RtlNtStatusToDosError; +use windows_sys::Win32::System::Threading::CREATE_NO_WINDOW; +use windows_sys::Win32::System::Threading::CREATE_SUSPENDED; +use windows_sys::Win32::System::Threading::GetCurrentProcess; + +#[link(name = "ntdll")] +unsafe extern "system" { + fn NtResumeProcess(process_handle: HANDLE) -> NTSTATUS; +} + +pub(super) fn spawn_with_retained_handles( + command: &mut Command, + retained_handles: &[BorrowedHandle<'_>], +) -> io::Result { + let mut child = command + .creation_flags(CREATE_NO_WINDOW | CREATE_SUSPENDED) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn()?; + let initialized = (|| { + for handle in retained_handles { + let mut child_handle = 0; + if unsafe { + DuplicateHandle( + GetCurrentProcess(), + handle.as_raw_handle() as HANDLE, + child.as_raw_handle() as HANDLE, + &mut child_handle, + /*dwdesiredaccess*/ 0, + /*binherithandle*/ 0, + DUPLICATE_SAME_ACCESS, + ) + } == 0 + { + return Err(io::Error::last_os_error()); + } + // This value belongs to the child's handle table. Do not close it here + // or make it inheritable by unrelated processes or helper descendants. + } + let status = unsafe { NtResumeProcess(child.as_raw_handle() as HANDLE) }; + if status < 0 { + return Err(io::Error::from_raw_os_error( + unsafe { RtlNtStatusToDosError(status) } as i32, + )); + } + Ok(()) + })(); + if let Err(error) = initialized { + // A partially initialized helper must never run after its parent drops + // the directory protections. Reap it, including any installed duplicates. + child.kill()?; + child.wait()?; + return Err(error); + } + Ok(child) +} + +#[cfg(test)] +#[path = "setup_launch_tests.rs"] +mod tests; diff --git a/codex-rs/windows-sandbox-rs/src/setup_launch_tests.rs b/codex-rs/windows-sandbox-rs/src/setup_launch_tests.rs new file mode 100644 index 0000000000..21968f1364 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/setup_launch_tests.rs @@ -0,0 +1,136 @@ +//! Exercises retained setup pins using bounded, disposable child processes. + +use std::fs; +use std::io; +use std::io::Write; +use std::os::windows::fs::OpenOptionsExt; +use std::os::windows::io::AsHandle; +use std::path::PathBuf; +use std::process::Child; +use std::process::Command; +use std::time::Duration; +use std::time::Instant; + +use pretty_assertions::assert_eq; +use windows_sys::Win32::Storage::FileSystem::DELETE; +use windows_sys::Win32::Storage::FileSystem::FILE_FLAG_BACKUP_SEMANTICS; +use windows_sys::Win32::Storage::FileSystem::FILE_FLAG_DELETE_ON_CLOSE; +use windows_sys::Win32::Storage::FileSystem::FILE_READ_DATA; +use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ; +use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE; + +use super::spawn_with_retained_handles; + +const CHILD_TEST: &str = "setup_launch::tests::retained_handles_child"; +const CHILD_DIRECTORY_ENV: &str = "CODEX_TEST_SETUP_LAUNCH_DIRECTORY"; +const CHILD_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 10); +const POLL_INTERVAL: Duration = Duration::from_millis(/*millis*/ 10); + +struct ChildGuard(Child); + +impl Drop for ChildGuard { + fn drop(&mut self) { + let _ = self.0.kill(); + let _ = self.0.wait(); + } +} + +#[test] +fn child_retains_directory_pin_until_exit() -> io::Result<()> { + let temporary = tempfile::tempdir()?; + let directory = temporary.path().join("pinned"); + let renamed_directory = temporary.path().join("renamed"); + fs::create_dir(&directory)?; + let pin = fs::OpenOptions::new() + .read(true) + .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE) + .custom_flags(FILE_FLAG_BACKUP_SEMANTICS) + .open(&directory)?; + let guard_path = directory.join(".guard"); + let guard = fs::OpenOptions::new() + .write(true) + .create_new(true) + .access_mode(FILE_READ_DATA | DELETE) + .share_mode(FILE_SHARE_READ | FILE_SHARE_WRITE) + .custom_flags(FILE_FLAG_DELETE_ON_CLOSE) + .open(&guard_path)?; + let mut command = Command::new(std::env::current_exe()?); + command + .args(["--exact", CHILD_TEST, "--ignored"]) + .env(CHILD_DIRECTORY_ENV, temporary.path()); + let mut child = ChildGuard(spawn_with_retained_handles( + &mut command, + &[pin.as_handle(), guard.as_handle()], + )?); + drop(pin); + drop(guard); + + let started = Instant::now(); + while !temporary.path().join("ready").exists() { + assert_eq!( + child.0.try_wait()?, + None, + "child exited before becoming ready" + ); + assert!( + started.elapsed() < CHILD_TIMEOUT, + "child did not become ready" + ); + std::thread::sleep(POLL_INTERVAL); + } + assert!( + fs::rename(&directory, &renamed_directory).is_err(), + "the child's duplicate must keep the directory pinned after the parent drops its handle" + ); + assert_eq!(child.0.try_wait()?, None); + assert!(guard_path.exists(), "the child must retain the guard file"); + + fs::write(temporary.path().join("release"), b"")?; + let started = Instant::now(); + let status = loop { + if let Some(status) = child.0.try_wait()? { + break status; + } + assert!(started.elapsed() < CHILD_TIMEOUT, "child did not exit"); + std::thread::sleep(POLL_INTERVAL); + }; + assert!(status.success(), "child failed: {status}"); + assert!( + !guard_path.exists(), + "the guard must be deleted on child exit" + ); + fs::rename(&directory, &renamed_directory)?; + Ok(()) +} + +#[test] +fn missing_executable_returns_spawn_error_without_closing_retained_handles() -> io::Result<()> { + let temporary = tempfile::tempdir()?; + let mut retained_file = tempfile::tempfile()?; + let mut command = Command::new(temporary.path().join("missing.exe")); + + let error = spawn_with_retained_handles(&mut command, &[retained_file.as_handle()]) + .expect_err("missing executable must fail to spawn"); + + assert_eq!(error.kind(), io::ErrorKind::NotFound); + retained_file.write_all(b"the caller still owns its handle")?; + Ok(()) +} + +#[test] +#[ignore = "child process for child_retains_directory_pin_until_exit"] +fn retained_handles_child() -> io::Result<()> { + let Some(directory) = std::env::var_os(CHILD_DIRECTORY_ENV).map(PathBuf::from) else { + return Ok(()); + }; + fs::write(directory.join("ready"), b"")?; + let started = Instant::now(); + while !directory.join("release").exists() { + assert!( + started.elapsed() < CHILD_TIMEOUT, + "parent did not release child" + ); + std::thread::sleep(POLL_INTERVAL); + } + Ok(()) +} diff --git a/codex-rs/windows-sandbox-service/Cargo.toml b/codex-rs/windows-sandbox-service/Cargo.toml index 5d6baa76b7..87793e0309 100644 --- a/codex-rs/windows-sandbox-service/Cargo.toml +++ b/codex-rs/windows-sandbox-service/Cargo.toml @@ -23,8 +23,6 @@ codex-config = { workspace = true } codex-core = { workspace = true } codex-windows-sandbox = { workspace = true } tokio = { workspace = true, features = ["macros", "rt", "sync", "time"] } - -[dev-dependencies] toml = { workspace = true } [target.'cfg(windows)'.dependencies.windows-sys] diff --git a/codex-rs/windows-sandbox-service/src/ipc.rs b/codex-rs/windows-sandbox-service/src/ipc.rs index 7fc32e7b4b..ebc639628a 100644 --- a/codex-rs/windows-sandbox-service/src/ipc.rs +++ b/codex-rs/windows-sandbox-service/src/ipc.rs @@ -1,36 +1,378 @@ -//! Disabled provisioning IPC until authenticated request handling is installed. +//! Authenticated local IPC for the Windows sandbox provisioning service. +//! Configuration parse failures defer provisioning to the client's elevated helper. +//! Shutdown wakeups are retried until the listener connects or stops. -// Keep these private until the authenticated transport is connected. -#[allow(dead_code)] mod authentication; -#[allow(dead_code)] mod home; -#[allow(dead_code)] mod request; +use anyhow::Context; use anyhow::Result; -#[cfg(test)] +use anyhow::bail; +use authentication::authenticate_client; +use codex_windows_sandbox::FramedProvisioningMessage; +use codex_windows_sandbox::PROVISIONING_PROTOCOL_VERSION; +use codex_windows_sandbox::ProvisioningMessage; +use codex_windows_sandbox::SandboxProvisioningResponse; +use codex_windows_sandbox::ensure_sandbox_users_group; +use codex_windows_sandbox::run_elevated_provisioning_setup_with_retained_handles; +use codex_windows_sandbox::sandbox_setup_is_complete_with_settings; +use codex_windows_sandbox::string_from_sid_bytes; +use codex_windows_sandbox::to_wide; +use codex_windows_sandbox::write_provisioning_frame; use home::OwnedHandle; #[cfg(test)] use home::pin_existing_ancestors; #[cfg(test)] use request::ProvisioningRequest; -#[cfg(test)] use request::validate_request; +use std::mem::size_of; +use std::os::windows::fs::MetadataExt; +use std::os::windows::io::BorrowedHandle; +use std::ptr; use std::sync::Arc; use std::sync::atomic::AtomicBool; +use std::sync::atomic::Ordering; +use std::time::Duration; +use std::time::Instant; +use windows_sys::Win32::Foundation as foundation; +use windows_sys::Win32::Foundation::HANDLE; +use windows_sys::Win32::Security as security; +use windows_sys::Win32::Security::Authorization as authorization; +use windows_sys::Win32::Storage::FileSystem as filesystem; +use windows_sys::Win32::System::Pipes as pipes; + +pub(crate) const PIPE_NAME: &str = codex_windows_sandbox::SANDBOX_PROVISIONING_PIPE_NAME; -pub(crate) const PIPE_NAME: &str = r"\\.\pipe\OpenAI.CodexSandbox"; const MAX_REQUEST_BYTES: usize = 4096; +const MAX_RESPONSE_MESSAGE_BYTES: usize = 512; +const REQUEST_IDLE_TIMEOUT: Duration = Duration::from_secs(5); +const PIPE_USER_ACCESS: &str = "0x0012019b"; -pub(crate) fn run( - _shutdown: Arc, - _on_ready: impl FnOnce() -> Result<()>, -) -> Result<()> { - anyhow::bail!("sandbox provisioning IPC is disabled until authenticated handling is installed") +struct SecurityDescriptor(security::PSECURITY_DESCRIPTOR); + +impl Drop for SecurityDescriptor { + fn drop(&mut self) { + unsafe { foundation::LocalFree(self.0 as foundation::HLOCAL) }; + } } -pub(crate) fn wake() {} +#[derive(Debug, Eq, PartialEq)] +enum PipeConnection { + Connected, + Disconnected, +} + +pub(crate) fn run(shutdown: Arc, on_ready: impl FnOnce() -> Result<()>) -> Result<()> { + let sandbox_sid = ensure_sandbox_users_group()?; + let sid_string = string_from_sid_bytes(&sandbox_sid).map_err(anyhow::Error::msg)?; + let sddl = pipe_security_descriptor(&sid_string); + let mut descriptor: security::PSECURITY_DESCRIPTOR = ptr::null_mut(); + if unsafe { + authorization::ConvertStringSecurityDescriptorToSecurityDescriptorW( + to_wide(sddl).as_ptr(), + authorization::SDDL_REVISION_1, + &mut descriptor, + ptr::null_mut(), + ) + } == 0 + { + return Err(std::io::Error::last_os_error()).context("create provisioning pipe DACL"); + } + let descriptor = SecurityDescriptor(descriptor); + let attributes = security::SECURITY_ATTRIBUTES { + nLength: size_of::() as u32, + lpSecurityDescriptor: descriptor.0, + bInheritHandle: 0, + }; + + let pipe = unsafe { + pipes::CreateNamedPipeW( + to_wide(PIPE_NAME).as_ptr(), + filesystem::PIPE_ACCESS_DUPLEX | filesystem::FILE_FLAG_FIRST_PIPE_INSTANCE, + pipes::PIPE_TYPE_BYTE + | pipes::PIPE_READMODE_BYTE + | pipes::PIPE_WAIT + | pipes::PIPE_REJECT_REMOTE_CLIENTS, + 1, + 1024, + MAX_REQUEST_BYTES as u32, + 0, + &attributes, + ) + }; + if pipe == foundation::INVALID_HANDLE_VALUE { + return Err(std::io::Error::last_os_error()).context("create provisioning pipe"); + } + let pipe = OwnedHandle(pipe); + on_ready().context("publish provisioning listener readiness")?; + + while !shutdown.load(Ordering::Acquire) { + if accept_pipe_connection(pipe.0)? == PipeConnection::Disconnected { + continue; + } + if shutdown.load(Ordering::Acquire) { + break; + } + + let authorized_process = match crate::package_identity::authorize_client_process(pipe.0) { + Ok(process) => process, + Err(_) => { + unsafe { pipes::DisconnectNamedPipe(pipe.0) }; + continue; + } + }; + let result = handle_request(pipe.0, &authorized_process, &sandbox_sid, &shutdown); + let response = match result { + Ok(response) => response, + Err(error) => { + eprintln!("sandbox provisioning request failed: {error}"); + let mut message = String::new(); + for character in error.to_string().chars() { + let character = if character.is_control() { + ' ' + } else { + character + }; + if message.len() + character.len_utf8() > MAX_RESPONSE_MESSAGE_BYTES { + break; + } + message.push(character); + } + SandboxProvisioningResponse::Error { message } + } + }; + let response = FramedProvisioningMessage { + version: PROVISIONING_PROTOCOL_VERSION, + message: ProvisioningMessage::ProvisionSandboxResponse { payload: response }, + }; + let mut frame = Vec::new(); + write_provisioning_frame(&mut frame, &response) + .context("serialize sandbox provisioning response")?; + let mut written = 0; + let sent = unsafe { + filesystem::WriteFile( + pipe.0, + frame.as_ptr(), + frame.len() as u32, + &mut written, + ptr::null_mut(), + ) + }; + if sent != 0 { + let deadline = Instant::now() + Duration::from_secs(1); + while !shutdown.load(Ordering::Acquire) && Instant::now() < deadline { + if unsafe { + pipes::PeekNamedPipe( + pipe.0, + ptr::null_mut(), + 0, + ptr::null_mut(), + ptr::null_mut(), + ptr::null_mut(), + ) + } == 0 + { + break; + } + std::thread::sleep(Duration::from_millis(10)); + } + } + unsafe { pipes::DisconnectNamedPipe(pipe.0) }; + } + Ok(()) +} + +fn accept_pipe_connection(pipe: HANDLE) -> Result { + if unsafe { pipes::ConnectNamedPipe(pipe, ptr::null_mut()) } != 0 { + return Ok(PipeConnection::Connected); + } + + let error = unsafe { foundation::GetLastError() }; + match error { + foundation::ERROR_PIPE_CONNECTED => Ok(PipeConnection::Connected), + foundation::ERROR_NO_DATA | foundation::ERROR_BROKEN_PIPE => { + if unsafe { pipes::DisconnectNamedPipe(pipe) } == 0 { + let reset_error = std::io::Error::last_os_error(); + if reset_error.raw_os_error() != Some(foundation::ERROR_PIPE_NOT_CONNECTED as i32) { + return Err(reset_error).context("reset disconnected provisioning client"); + } + } + Ok(PipeConnection::Disconnected) + } + _ => Err(std::io::Error::from_raw_os_error(error as i32)) + .context("accept provisioning client"), + } +} + +pub(crate) fn wake(pipe_name: &str, is_stopped: impl Fn() -> bool) { + let pipe_name = to_wide(pipe_name); + while !is_stopped() { + let handle = unsafe { + filesystem::CreateFileW( + pipe_name.as_ptr(), + foundation::GENERIC_WRITE, + /*dwsharemode*/ 0, + ptr::null(), + filesystem::OPEN_EXISTING, + /*dwflagsandattributes*/ 0, + /*htemplatefile*/ 0, + ) + }; + if handle != foundation::INVALID_HANDLE_VALUE { + unsafe { foundation::CloseHandle(handle) }; + return; + } + // A disconnected instance cannot accept the wakeup until ConnectNamedPipe runs. + std::thread::sleep(Duration::from_millis(25)); + } +} + +fn pipe_security_descriptor(sandbox_sid: &str) -> String { + format!("D:P(D;;GA;;;{sandbox_sid})(A;;GA;;;SY)(A;;GA;;;BA)(A;;{PIPE_USER_ACCESS};;;IU)") +} + +fn handle_request( + pipe: HANDLE, + authorized_process: &crate::package_identity::AuthorizedClientProcess, + sandbox_sid: &[u8], + shutdown: &AtomicBool, +) -> Result { + let deadline = Instant::now() + REQUEST_IDLE_TIMEOUT; + let mut request = [0_u8; MAX_REQUEST_BYTES]; + let mut request_length = 0; + loop { + if shutdown.load(Ordering::Acquire) { + bail!("service is stopping"); + } + if Instant::now() >= deadline { + bail!("provisioning request timed out"); + } + let mut available = 0; + if unsafe { + pipes::PeekNamedPipe( + pipe, + ptr::null_mut(), + 0, + ptr::null_mut(), + &mut available, + ptr::null_mut(), + ) + } == 0 + { + return Err(std::io::Error::last_os_error()).context("inspect provisioning request"); + } + if available as usize > MAX_REQUEST_BYTES - request_length { + bail!("provisioning request exceeds size limit"); + } + if available != 0 { + let mut read = 0; + if unsafe { + filesystem::ReadFile( + pipe, + request[request_length..].as_mut_ptr(), + available, + &mut read, + ptr::null_mut(), + ) + } == 0 + { + return Err(std::io::Error::last_os_error()).context("read provisioning request"); + } + if read == 0 { + bail!("provisioning client sent an empty request"); + } + request_length += read as usize; + let received = &request[..request_length]; + if received.len() >= size_of::() { + let payload_length = + u32::from_le_bytes([received[0], received[1], received[2], received[3]]) + as usize; + if payload_length > MAX_REQUEST_BYTES - size_of::() { + bail!("provisioning request exceeds size limit"); + } + let frame_length = size_of::() + payload_length; + if request_length > frame_length { + bail!("provisioning requests must contain exactly one IPC frame"); + } + if request_length == frame_length { + break; + } + } + continue; + } + std::thread::sleep(Duration::from_millis(25)); + } + + let request = validate_request(&request[..request_length])?; + let (identity, policy_result) = + authenticate_client(pipe, authorized_process, sandbox_sid, &request)?; + if let Err(error) = policy_result { + if is_config_parse_error(&error) { + return Ok(SandboxProvisioningResponse::Unavailable); + } + crate::service::log_error( + crate::service::EVENT_REQUEST_REJECTED, + &format!("Codex sandbox provisioning was rejected by administrator policy: {error}"), + ); + return Err(error) + .context("requested sandbox settings violate administrator-controlled machine policy"); + } + if sandbox_setup_is_complete_with_settings(&identity.codex_home, &request.settings) { + return Ok(SandboxProvisioningResponse::Ok); + } + let helper = std::env::current_exe() + .context("locate the provisioning service executable")? + .with_file_name("codex-windows-sandbox-setup.exe"); + let helper_metadata = helper + .symlink_metadata() + .with_context(|| format!("inspect packaged setup helper {}", helper.display()))?; + if !helper_metadata.is_file() + || helper_metadata.file_attributes() & filesystem::FILE_ATTRIBUTE_REPARSE_POINT != 0 + { + bail!( + "refusing invalid packaged setup helper {}", + helper.display() + ); + } + let retained_handles = identity + .directory_handles + .iter() + // The identity owns these handles through the synchronous helper launch and wait. + .map(|handle| unsafe { BorrowedHandle::borrow_raw(handle.0 as _) }) + .collect::>(); + match run_elevated_provisioning_setup_with_retained_handles( + &identity.codex_home, + &identity.account, + request.settings, + &retained_handles, + ) { + Ok(()) => { + crate::service::log_information( + crate::service::EVENT_PROVISIONING_SUCCEEDED, + "Codex sandbox provisioning completed successfully.", + ); + Ok(SandboxProvisioningResponse::Ok) + } + Err(error) => { + crate::service::log_error( + crate::service::EVENT_PROVISIONING_FAILED, + &format!("Codex sandbox provisioning failed: {error}"), + ); + Err(error).context("sandbox provisioning failed") + } + } +} + +fn is_config_parse_error(error: &anyhow::Error) -> bool { + error.chain().any(|cause| { + cause.is::() + || cause + .downcast_ref::() + .and_then(std::io::Error::get_ref) + .is_some_and(::is::) + }) +} #[cfg(test)] #[path = "ipc_tests.rs"] diff --git a/codex-rs/windows-sandbox-service/src/ipc/authentication.rs b/codex-rs/windows-sandbox-service/src/ipc/authentication.rs index 64d48f7279..a94451fafc 100644 --- a/codex-rs/windows-sandbox-service/src/ipc/authentication.rs +++ b/codex-rs/windows-sandbox-service/src/ipc/authentication.rs @@ -20,8 +20,8 @@ use super::request::ProvisioningRequest; pub(super) struct ClientIdentity { pub(super) account: String, pub(super) codex_home: PathBuf, - // Denying FILE_SHARE_DELETE pins every checked directory through provisioning. - _directory_handles: Vec, + // Retained by both the service and helper throughout provisioning. + pub(super) directory_handles: Vec, } pub(super) fn authenticate_client( @@ -144,7 +144,7 @@ fn authenticate_impersonated_client( ClientIdentity { account, codex_home, - _directory_handles: handles, + directory_handles: handles, }, token, )) diff --git a/codex-rs/windows-sandbox-service/src/ipc_tests.rs b/codex-rs/windows-sandbox-service/src/ipc_tests.rs index a30c11253a..43e77e827f 100644 --- a/codex-rs/windows-sandbox-service/src/ipc_tests.rs +++ b/codex-rs/windows-sandbox-service/src/ipc_tests.rs @@ -1,7 +1,12 @@ use super::OwnedHandle; +use super::PipeConnection; use super::ProvisioningRequest; +use super::accept_pipe_connection; +use super::is_config_parse_error; use super::pin_existing_ancestors; +use super::pipe_security_descriptor; use super::validate_request; +use super::wake; use codex_windows_sandbox::FramedProvisioningMessage; use codex_windows_sandbox::PROVISIONING_PROTOCOL_VERSION; use codex_windows_sandbox::ProvisioningMessage; @@ -9,12 +14,17 @@ use codex_windows_sandbox::SandboxProvisioningRequest; use codex_windows_sandbox::SandboxProvisioningResponse; use codex_windows_sandbox::WindowsSandboxProvisioningSettings; use codex_windows_sandbox::WindowsSandboxProxyListeners; +use codex_windows_sandbox::read_provisioning_frame; use codex_windows_sandbox::to_wide; use codex_windows_sandbox::write_provisioning_frame; +use std::path::Path; use std::path::PathBuf; use std::ptr; use std::sync::atomic::AtomicU64; use std::sync::atomic::Ordering; +use std::sync::mpsc; +use std::time::Duration; +use std::time::Instant; use windows_sys::Win32::Foundation as foundation; use windows_sys::Win32::Storage::FileSystem as filesystem; use windows_sys::Win32::Storage::Packaging::Appx; @@ -33,6 +43,67 @@ fn framed_request(request: SandboxProvisioningRequest) -> Vec { frame } +#[test] +fn unavailable_response_round_trips_through_provisioning_frame() { + let mut frame = Vec::new(); + write_provisioning_frame( + &mut frame, + &FramedProvisioningMessage { + version: PROVISIONING_PROTOCOL_VERSION, + message: ProvisioningMessage::ProvisionSandboxResponse { + payload: SandboxProvisioningResponse::Unavailable, + }, + }, + ) + .unwrap(); + + assert!(matches!( + read_provisioning_frame(frame.as_slice()).unwrap(), + Some(FramedProvisioningMessage { + version: PROVISIONING_PROTOCOL_VERSION, + message: ProvisioningMessage::ProvisionSandboxResponse { + payload: SandboxProvisioningResponse::Unavailable, + }, + }) + )); +} + +#[test] +fn config_parse_errors_are_distinguished_from_policy_and_io_failures() { + let schema_error = codex_core::config::deserialize_config_toml_with_base( + toml::from_str(r#"model_verbosity = "future-verbosity""#).unwrap(), + Path::new(r"C:\CodexTest"), + ) + .unwrap_err(); + let contents = "["; + let parse_error = toml::from_str::(contents).unwrap_err(); + let syntax_error = codex_config::io_error_from_config_error( + std::io::ErrorKind::InvalidData, + codex_config::config_error_from_toml("config.toml", contents, parse_error.clone()), + Some(parse_error), + ); + for error in [schema_error, syntax_error] { + assert!(is_config_parse_error( + &anyhow::Error::new(error).context("load managed configuration"), + )); + } + + for error in [ + anyhow::anyhow!("managed policy does not permit the elevated Windows sandbox"), + anyhow::Error::new(std::io::Error::from_raw_os_error( + foundation::ERROR_ACCESS_DENIED as i32, + )), + anyhow::Error::new(std::io::Error::new( + std::io::ErrorKind::InvalidData, + "invalid credentials", + )), + ] { + assert!(!is_config_parse_error( + &error.context("load managed configuration"), + )); + } +} + #[test] fn provisioning_request_preserves_home_spaces_and_unicode() { let request = framed_request(SandboxProvisioningRequest { @@ -214,25 +285,6 @@ fn provisioning_request_rejects_empty_control_characters_and_invalid_utf8() { assert!(validate_request(&invalid_utf8).is_err()); } -#[test] -fn pin_existing_ancestors_accepts_drive_and_verbatim_drive_roots() { - let executable = std::env::current_exe().unwrap().canonicalize().unwrap(); - let verbatim_root = executable.ancestors().last().unwrap(); - let ordinary_root = PathBuf::from( - verbatim_root - .to_str() - .unwrap() - .strip_prefix(r"\\?\") - .unwrap(), - ); - - for root in [ordinary_root.as_path(), verbatim_root] { - let mut handles = Vec::new(); - pin_existing_ancestors(root, &mut handles).unwrap(); - assert_eq!(handles.len(), 1); - } -} - #[test] fn unpackaged_pipe_clients_are_rejected_before_sending_a_request() { let mut family_length = 0; @@ -297,3 +349,150 @@ fn unpackaged_pipe_clients_are_rejected_before_sending_a_request() { "unexpected package authorization failure: {error:#}" ); } + +#[test] +fn disconnected_pipe_clients_do_not_prevent_subsequent_connections() { + let pipe_name = format!( + r"\\.\pipe\OpenAI.CodexSandbox.DisconnectTests.{}", + std::process::id() + ); + let name = to_wide(&pipe_name); + let server = unsafe { + pipes::CreateNamedPipeW( + name.as_ptr(), + filesystem::PIPE_ACCESS_DUPLEX | filesystem::FILE_FLAG_FIRST_PIPE_INSTANCE, + pipes::PIPE_TYPE_BYTE + | pipes::PIPE_READMODE_BYTE + | pipes::PIPE_WAIT + | pipes::PIPE_REJECT_REMOTE_CLIENTS, + 1, + 1024, + 1024, + 0, + ptr::null(), + ) + }; + assert_ne!(server, foundation::INVALID_HANDLE_VALUE); + let server = OwnedHandle(server); + + let first_client = unsafe { + filesystem::CreateFileW( + name.as_ptr(), + foundation::GENERIC_READ | foundation::GENERIC_WRITE, + 0, + ptr::null(), + filesystem::OPEN_EXISTING, + 0, + 0, + ) + }; + assert_ne!(first_client, foundation::INVALID_HANDLE_VALUE); + drop(OwnedHandle(first_client)); + + assert_eq!( + accept_pipe_connection(server.0).unwrap(), + PipeConnection::Disconnected + ); + + let server_handle = server.0; + let listener = std::thread::spawn(move || accept_pipe_connection(server_handle)); + let available = unsafe { pipes::WaitNamedPipeW(name.as_ptr(), 5_000) }; + assert_ne!( + available, + 0, + "wait for replacement named-pipe listener: {}", + std::io::Error::last_os_error() + ); + let next_client = unsafe { + filesystem::CreateFileW( + name.as_ptr(), + foundation::GENERIC_READ | foundation::GENERIC_WRITE, + 0, + ptr::null(), + filesystem::OPEN_EXISTING, + 0, + 0, + ) + }; + assert_ne!( + next_client, + foundation::INVALID_HANDLE_VALUE, + "connect replacement named-pipe client: {}", + std::io::Error::last_os_error() + ); + let next_client = OwnedHandle(next_client); + assert_eq!( + listener.join().expect("join named-pipe listener").unwrap(), + PipeConnection::Connected + ); + + drop(next_client); + assert_ne!(unsafe { pipes::DisconnectNamedPipe(server.0) }, 0); + + let timeout = Duration::from_secs(5); + let deadline = Instant::now() + timeout; + let (checks_tx, checks_rx) = mpsc::channel(); + let waker = std::thread::spawn(move || { + wake(&pipe_name, || { + let _ = checks_tx.send(()); + Instant::now() >= deadline + }); + }); + checks_rx.recv_timeout(timeout).unwrap(); + // The second check proves a wake failed before the listener started reconnecting. + checks_rx.recv_timeout(timeout).unwrap(); + + let (accepted_tx, accepted_rx) = mpsc::channel(); + let listener = std::thread::spawn(move || { + let _ = accepted_tx.send(accept_pipe_connection(server.0)); + drop(server); + }); + let accepted = accepted_rx.recv_timeout(timeout); + waker.join().expect("join shutdown waker"); + // The wake handle may close before ConnectNamedPipe observes the connection. + assert!( + accepted + .expect("shutdown wake should unblock the listener") + .is_ok() + ); + listener.join().expect("join shutdown listener"); +} + +#[test] +fn unexpected_pipe_connection_errors_remain_fatal() { + let error = accept_pipe_connection(foundation::INVALID_HANDLE_VALUE).unwrap_err(); + + assert_eq!( + error + .downcast_ref::() + .and_then(std::io::Error::raw_os_error), + Some(foundation::ERROR_INVALID_HANDLE as i32) + ); +} + +#[test] +fn pin_existing_ancestors_accepts_drive_and_verbatim_drive_roots() { + let executable = std::env::current_exe().unwrap().canonicalize().unwrap(); + let verbatim_root = executable.ancestors().last().unwrap(); + let ordinary_root = PathBuf::from( + verbatim_root + .to_str() + .unwrap() + .strip_prefix(r"\\?\") + .unwrap(), + ); + + for root in [ordinary_root.as_path(), verbatim_root] { + let mut handles = Vec::new(); + pin_existing_ancestors(root, &mut handles).unwrap(); + assert_eq!(handles.len(), 1); + } +} + +#[test] +fn pipe_descriptor_denies_sandbox_group_before_interactive_users() { + let descriptor = pipe_security_descriptor("S-1-5-21-11-12-13-14"); + let deny = descriptor.find("(D;;GA;;;S-1-5-21-11-12-13-14)").unwrap(); + let interactive = descriptor.find("(A;;0x0012019b;;;IU)").unwrap(); + assert!(deny < interactive); +} diff --git a/codex-rs/windows-sandbox-service/src/lib.rs b/codex-rs/windows-sandbox-service/src/lib.rs index 557d9826ca..aa89704ee0 100644 --- a/codex-rs/windows-sandbox-service/src/lib.rs +++ b/codex-rs/windows-sandbox-service/src/lib.rs @@ -2,12 +2,9 @@ use anyhow::Result; #[cfg(windows)] mod ipc; -// Provisioning remains disabled until authenticated transport connects this policy. #[cfg(windows)] -#[allow(dead_code)] mod machine_policy; #[cfg(windows)] -#[allow(dead_code)] mod package_identity; #[cfg(windows)] mod service; diff --git a/codex-rs/windows-sandbox-service/src/service.rs b/codex-rs/windows-sandbox-service/src/service.rs index 29a1d118c0..929b9bb717 100644 --- a/codex-rs/windows-sandbox-service/src/service.rs +++ b/codex-rs/windows-sandbox-service/src/service.rs @@ -41,6 +41,9 @@ const EVENT_SERVICE_STARTED: u32 = 1000; const EVENT_SERVICE_STOP_REQUESTED: u32 = 1001; const EVENT_SERVICE_STOPPED: u32 = 1002; const EVENT_SERVICE_FAILED: u32 = 1003; +pub(crate) const EVENT_PROVISIONING_SUCCEEDED: u32 = 2000; +pub(crate) const EVENT_PROVISIONING_FAILED: u32 = 2001; +pub(crate) const EVENT_REQUEST_REJECTED: u32 = 2002; const MAX_EVENT_MESSAGE_UNITS: usize = 1024; static SERVICE_STATE: OnceLock = OnceLock::new(); @@ -167,7 +170,11 @@ unsafe extern "system" fn service_control_handler( if let Err(error) = state.report_status(SERVICE_STOP_PENDING, NO_ERROR) { eprintln!("unable to report service shutdown: {error:#}"); } - crate::ipc::wake(); + std::thread::spawn(move || { + crate::ipc::wake(crate::ipc::PIPE_NAME, || { + state.current_status.load(Ordering::Acquire) == SERVICE_STOPPED + }); + }); log_information( EVENT_SERVICE_STOP_REQUESTED, "The Codex sandbox service was asked to stop.",