From 0ed2735d195d149766b242d972f95f3173ebc984 Mon Sep 17 00:00:00 2001 From: Abhinav Date: Wed, 3 Jun 2026 15:33:34 -0700 Subject: [PATCH] Use Windows setup marker as completion signal (#26074) # Why When an organization requires the elevated Windows sandbox, Codex launches an elevated helper to provision users, configure firewall and ACL rules, and lock persistent sandbox directories. We observed that closing the helper after setup started could leave the machine partially initialized while the TUI still announced **Sandbox ready**. Model-only turns continued to work, but the first shell command retried setup and failed with Windows cancellation error `1223`. This was not an enforcement bypass; command execution continued to fail closed. The issue was a false readiness signal: `setup_marker.json` was written during user provisioning, before the remaining setup stages had completed. # What Treat `setup_marker.json` as the commit record for Windows sandbox setup: 1. Before full or provisioning setup begins, remove the existing marker and create the final marker path with a protected ACL. 2. Keep the marker empty and therefore invalid while setup is in progress. Sandbox users cannot read, modify, or replace it. 3. Run every synchronous setup stage. 4. After setup succeeds, write the valid marker contents without changing its ACL. 5. After the helper exits successfully, verify the existing readiness check before enabling the sandbox. If setup is canceled or fails, the marker remains invalid and Codex reports setup as incomplete instead of announcing readiness. Refresh-only and read-ACL-only helper runs continue to leave the marker untouched. The setup version remains `5` to avoid forcing all existing Windows users through elevated setup again. # Verification - Added coverage confirming sandbox users cannot read or modify the setup marker after elevated setup. - Added coverage confirming a successful helper exit without complete setup artifacts is rejected. - Ran `just test -p codex-windows-sandbox`. --- codex-rs/core/tests/suite/windows_sandbox.rs | 32 +++- .../src/bin/setup_main/win.rs | 18 ++- .../src/bin/setup_main/win/sandbox_users.rs | 145 ++++++++++++++---- codex-rs/windows-sandbox-rs/src/setup.rs | 29 ++++ .../windows-sandbox-rs/src/setup_error.rs | 5 +- 5 files changed, 195 insertions(+), 34 deletions(-) diff --git a/codex-rs/core/tests/suite/windows_sandbox.rs b/codex-rs/core/tests/suite/windows_sandbox.rs index 120381e67..74d7f6a23 100644 --- a/codex-rs/core/tests/suite/windows_sandbox.rs +++ b/codex-rs/core/tests/suite/windows_sandbox.rs @@ -3,6 +3,7 @@ use codex_core::exec::ExecCapturePolicy; use codex_core::exec::ExecParams; use codex_core::exec::process_exec_tool_call; use codex_core::sandboxing::SandboxPermissions; +use codex_core::windows_sandbox::sandbox_setup_is_complete; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::exec_output::ExecToolCallOutput; use codex_protocol::models::PermissionProfile; @@ -197,7 +198,7 @@ async fn windows_restricted_token_rejects_exact_and_glob_deny_read_policy() -> a #[tokio::test] #[serial(codex_home)] -async fn windows_elevated_enforces_exact_and_glob_deny_read_policy() -> anyhow::Result<()> { +async fn windows_elevated_enforces_deny_read_and_protects_setup_marker() -> anyhow::Result<()> { let codex_home = codex_home_for_windows_sandbox_test("windows-elevated-deny-read-codex-home")?; let _codex_home_guard = EnvVarGuard::set("CODEX_HOME", codex_home.path().as_os_str()); stage_windows_sandbox_helpers()?; @@ -206,6 +207,7 @@ async fn windows_elevated_enforces_exact_and_glob_deny_read_policy() -> anyhow:: let glob_secret = cwd.join("secret.env"); let exact_secret = cwd.join("exact-secret.txt"); let public = cwd.join("public.txt"); + let setup_marker = codex_home.path().join(".sandbox").join("setup_marker.json"); std::fs::write(&glob_secret, "glob secret\n")?; std::fs::write(&exact_secret, "exact secret\n")?; std::fs::write(&public, "public ok\n")?; @@ -242,7 +244,6 @@ async fn windows_elevated_enforces_exact_and_glob_deny_read_policy() -> anyhow:: let ExecToolCallOutput { exit_code, stdout, - stderr, .. } = process_exec_tool_call( ExecParams { @@ -250,7 +251,11 @@ async fn windows_elevated_enforces_exact_and_glob_deny_read_policy() -> anyhow:: "cmd.exe".to_string(), "/D".to_string(), "/C".to_string(), - "(type secret.env 1>NUL 2>NUL && echo GLOB-READ || echo GLOB-DENIED) & (type exact-secret.txt 1>NUL 2>NUL && echo EXACT-READ || echo EXACT-DENIED) & type public.txt".to_string(), + format!( + "(type secret.env 1>NUL 2>NUL && echo GLOB-READ || echo GLOB-DENIED) & (type exact-secret.txt 1>NUL 2>NUL && echo EXACT-READ || echo EXACT-DENIED) & (type \"{}\" 1>NUL 2>NUL && echo MARKER-READ-ALLOWED || echo MARKER-READ-DENIED) & (echo tampered > \"{}\" 2>NUL && echo MARKER-WRITE-ALLOWED || echo MARKER-WRITE-DENIED) & type public.txt", + setup_marker.display(), + setup_marker.display() + ), ], cwd: cwd.clone(), expiration: 10_000.into(), @@ -293,6 +298,25 @@ async fn windows_elevated_enforces_exact_and_glob_deny_read_policy() -> anyhow:: stdout.text.contains("public ok"), "allowed reads should still work: {stdout:?}" ); - assert_eq!(stderr.text, ""); + assert!( + stdout.text.contains("MARKER-READ-DENIED"), + "sandboxed command should not read setup readiness: {stdout:?}" + ); + assert!( + stdout.text.contains("MARKER-WRITE-DENIED"), + "sandboxed command should not modify setup readiness: {stdout:?}" + ); + assert!( + !stdout.text.contains("MARKER-READ-ALLOWED"), + "sandboxed command must not read setup readiness: {stdout:?}" + ); + assert!( + !stdout.text.contains("MARKER-WRITE-ALLOWED"), + "sandboxed command must not modify setup readiness: {stdout:?}" + ); + assert!( + sandbox_setup_is_complete(codex_home.path()), + "setup should remain ready after the tamper attempt" + ); Ok(()) } diff --git a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs index 41fa2f720..e3376f405 100644 --- a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs +++ b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win.rs @@ -70,6 +70,8 @@ mod sandbox_users; mod setup_runtime_bin; use read_acl_mutex::acquire_read_acl_mutex; use read_acl_mutex::read_acl_mutex_exists; +use sandbox_users::commit_setup_marker; +use sandbox_users::prepare_setup_marker; use sandbox_users::provision_sandbox_users; use sandbox_users::resolve_sandbox_users_group_sid; use sandbox_users::resolve_sid; @@ -475,11 +477,25 @@ fn real_main() -> Result<()> { } fn run_setup(payload: &Payload, log: &mut dyn Write, sbx_dir: &Path) -> Result<()> { + let writes_setup_marker = !payload.refresh_only && payload.mode != SetupMode::ReadAclsOnly; + if writes_setup_marker { + prepare_setup_marker(&payload.codex_home, &payload.real_user)?; + } match payload.mode { SetupMode::ReadAclsOnly => run_read_acl_only(payload, log), SetupMode::ProvisionOnly => run_provision_only(payload, log, sbx_dir), SetupMode::Full => run_setup_full(payload, log, sbx_dir), + }?; + if writes_setup_marker { + commit_setup_marker( + &payload.codex_home, + &payload.offline_username, + &payload.online_username, + &payload.proxy_ports, + payload.allow_local_binding, + )?; } + Ok(()) } fn run_read_acl_only(payload: &Payload, log: &mut dyn Write) -> Result<()> { @@ -554,8 +570,6 @@ fn provision_and_hide_sandbox_users( &payload.codex_home, &payload.offline_username, &payload.online_username, - &payload.proxy_ports, - payload.allow_local_binding, log, ); if let Err(err) = provision_result { diff --git a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/sandbox_users.rs b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/sandbox_users.rs index 7f21f185e..0c7aa2198 100644 --- a/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/sandbox_users.rs +++ b/codex-rs/windows-sandbox-rs/src/bin/setup_main/win/sandbox_users.rs @@ -10,8 +10,11 @@ use std::ffi::c_void; use std::io::Write; use std::path::Path; use std::path::PathBuf; +use windows_sys::Win32::Foundation::CloseHandle; use windows_sys::Win32::Foundation::ERROR_INSUFFICIENT_BUFFER; +use windows_sys::Win32::Foundation::GENERIC_WRITE; use windows_sys::Win32::Foundation::GetLastError; +use windows_sys::Win32::Foundation::INVALID_HANDLE_VALUE; use windows_sys::Win32::Foundation::LocalFree; use windows_sys::Win32::NetworkManagement::NetManagement::LOCALGROUP_INFO_1; use windows_sys::Win32::NetworkManagement::NetManagement::LOCALGROUP_MEMBERS_INFO_3; @@ -25,12 +28,19 @@ use windows_sys::Win32::NetworkManagement::NetManagement::UF_SCRIPT; use windows_sys::Win32::NetworkManagement::NetManagement::USER_INFO_1; use windows_sys::Win32::NetworkManagement::NetManagement::USER_INFO_1003; use windows_sys::Win32::NetworkManagement::NetManagement::USER_PRIV_USER; +use windows_sys::Win32::Security::Authorization::ConvertStringSecurityDescriptorToSecurityDescriptorW; use windows_sys::Win32::Security::Authorization::ConvertStringSidToSidW; +use windows_sys::Win32::Security::Authorization::SDDL_REVISION_1; use windows_sys::Win32::Security::CopySid; use windows_sys::Win32::Security::GetLengthSid; use windows_sys::Win32::Security::LookupAccountNameW; use windows_sys::Win32::Security::LookupAccountSidW; +use windows_sys::Win32::Security::PSECURITY_DESCRIPTOR; +use windows_sys::Win32::Security::SECURITY_ATTRIBUTES; use windows_sys::Win32::Security::SID_NAME_USE; +use windows_sys::Win32::Storage::FileSystem::CREATE_NEW; +use windows_sys::Win32::Storage::FileSystem::CreateFileW; +use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_NORMAL; use codex_windows_sandbox::SETUP_VERSION; use codex_windows_sandbox::SetupErrorCode; @@ -61,8 +71,6 @@ pub fn provision_sandbox_users( codex_home: &Path, offline_username: &str, online_username: &str, - proxy_ports: &[u16], - allow_local_binding: bool, log: &mut dyn Write, ) -> Result<()> { ensure_sandbox_users_group(log)?; @@ -80,8 +88,6 @@ pub fn provision_sandbox_users( &offline_password, online_username, &online_password, - proxy_ports, - allow_local_binding, )?; Ok(()) } @@ -402,19 +408,7 @@ fn write_secrets( offline_pwd: &str, online_user: &str, online_pwd: &str, - proxy_ports: &[u16], - allow_local_binding: bool, ) -> Result<()> { - let sandbox_dir = sandbox_dir(codex_home); - std::fs::create_dir_all(&sandbox_dir).map_err(|err| { - anyhow::Error::new(SetupFailure::new( - SetupErrorCode::HelperUsersFileWriteFailed, - format!( - "failed to create sandbox dir {}: {err}", - sandbox_dir.display() - ), - )) - })?; let secrets_dir = sandbox_secrets_dir(codex_home); std::fs::create_dir_all(&secrets_dir).map_err(|err| { anyhow::Error::new(SetupFailure::new( @@ -448,18 +442,7 @@ fn write_secrets( password: BASE64.encode(online_blob), }, }; - let marker = SetupMarker { - version: SETUP_VERSION, - offline_username: offline_user.to_string(), - online_username: online_user.to_string(), - created_at: chrono::Utc::now().to_rfc3339(), - proxy_ports: proxy_ports.to_vec(), - allow_local_binding, - read_roots: Vec::new(), - write_roots: Vec::new(), - }; let users_path = secrets_dir.join("sandbox_users.json"); - let marker_path = sandbox_dir.join("setup_marker.json"); let users_json = serde_json::to_vec_pretty(&users).map_err(|err| { anyhow::Error::new(SetupFailure::new( SetupErrorCode::HelperUsersFileWriteFailed, @@ -475,6 +458,114 @@ fn write_secrets( ), )) })?; + Ok(()) +} + +// Create the final marker path with its protected ACL before provisioning begins. The empty file +// intentionally fails readiness checks while setup is in progress, and sandbox users cannot read, +// modify, or replace it. Once every setup step succeeds, `commit_setup_marker` writes the valid +// marker contents without changing the file's ACL. +pub(super) fn prepare_setup_marker(codex_home: &Path, real_user: &str) -> Result<()> { + let marker_path = sandbox_dir(codex_home).join("setup_marker.json"); + match std::fs::remove_file(&marker_path) { + Ok(()) => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => { + return Err(anyhow::Error::new(SetupFailure::new( + SetupErrorCode::HelperSetupMarkerWriteFailed, + format!( + "remove setup marker file {} failed: {err}", + marker_path.display() + ), + ))); + } + } + + let real_user_sid = resolve_sid(real_user) + .and_then(|sid| string_from_sid_bytes(&sid).map_err(anyhow::Error::msg)) + .map_err(|err| { + anyhow::Error::new(SetupFailure::new( + SetupErrorCode::HelperSetupMarkerWriteFailed, + format!("resolve real user SID for setup marker failed: {err}"), + )) + })?; + let sddl = to_wide(format!( + "D:P(A;;GA;;;SY)(A;;GA;;;BA)(A;;GA;;;{real_user_sid})" + )); + let mut security_descriptor: PSECURITY_DESCRIPTOR = std::ptr::null_mut(); + let converted = unsafe { + ConvertStringSecurityDescriptorToSecurityDescriptorW( + sddl.as_ptr(), + SDDL_REVISION_1, + &mut security_descriptor, + std::ptr::null_mut(), + ) + }; + if converted == 0 { + return Err(anyhow::Error::new(SetupFailure::new( + SetupErrorCode::HelperSetupMarkerWriteFailed, + format!( + "create setup marker security descriptor failed: {}", + unsafe { GetLastError() } + ), + ))); + } + + let security_attributes = SECURITY_ATTRIBUTES { + nLength: std::mem::size_of::() as u32, + lpSecurityDescriptor: security_descriptor, + bInheritHandle: 0, + }; + let marker_path_wide = to_wide(marker_path.as_os_str()); + let marker_handle = unsafe { + CreateFileW( + marker_path_wide.as_ptr(), + GENERIC_WRITE, + /*dwsharemode*/ 0, + &security_attributes, + CREATE_NEW, + FILE_ATTRIBUTE_NORMAL, + /*htemplatefile*/ 0, + ) + }; + let create_error = unsafe { GetLastError() }; + unsafe { + LocalFree(security_descriptor as _); + } + if marker_handle == INVALID_HANDLE_VALUE { + return Err(anyhow::Error::new(SetupFailure::new( + SetupErrorCode::HelperSetupMarkerWriteFailed, + format!( + "create protected setup marker file {} failed: {}", + marker_path.display(), + create_error + ), + ))); + } + unsafe { + CloseHandle(marker_handle); + } + Ok(()) +} + +pub(super) fn commit_setup_marker( + codex_home: &Path, + offline_user: &str, + online_user: &str, + proxy_ports: &[u16], + allow_local_binding: bool, +) -> Result<()> { + let marker = SetupMarker { + version: SETUP_VERSION, + offline_username: offline_user.to_string(), + online_username: online_user.to_string(), + created_at: chrono::Utc::now().to_rfc3339(), + proxy_ports: proxy_ports.to_vec(), + allow_local_binding, + read_roots: Vec::new(), + write_roots: Vec::new(), + }; + let marker_path = sandbox_dir(codex_home).join("setup_marker.json"); let marker_json = serde_json::to_vec_pretty(&marker).map_err(|err| { anyhow::Error::new(SetupFailure::new( SetupErrorCode::HelperSetupMarkerWriteFailed, diff --git a/codex-rs/windows-sandbox-rs/src/setup.rs b/codex-rs/windows-sandbox-rs/src/setup.rs index a0f46fe3f..c1881b55c 100644 --- a/codex-rs/windows-sandbox-rs/src/setup.rs +++ b/codex-rs/windows-sandbox-rs/src/setup.rs @@ -14,6 +14,7 @@ use crate::allow::AllowDenyPaths; use crate::allow::compute_allow_paths_for_permissions; use crate::helper_materialization::bundled_executable_path_for_exe; use crate::helper_materialization::helper_bin_dir; +use crate::identity::sandbox_setup_is_complete; use crate::logging::log_note; use crate::path_normalization::canonical_path_key; use crate::path_normalization::canonicalize_path; @@ -671,6 +672,17 @@ fn report_helper_failure( } } +fn verify_setup_completed(codex_home: &Path) -> Result<()> { + if sandbox_setup_is_complete(codex_home) { + Ok(()) + } else { + Err(failure( + SetupErrorCode::OrchestratorHelperIncomplete, + "setup helper exited successfully before setup completed", + )) + } +} + fn run_setup_exe( payload: &ElevationPayload, needs_elevation: bool, @@ -724,6 +736,7 @@ fn run_setup_exe( status.code(), )); } + verify_setup_completed(codex_home)?; if let Err(err) = clear_setup_error_report(codex_home) { log_note( &format!( @@ -773,6 +786,7 @@ fn run_setup_exe( )); } } + verify_setup_completed(codex_home)?; if let Err(err) = clear_setup_error_report(codex_home) { log_note( &format!("setup orchestrator: failed to clear setup_error.json after success: {err}"), @@ -1067,10 +1081,13 @@ mod tests { use super::offline_proxy_settings_from_env; use super::profile_read_roots; use super::proxy_ports_from_env; + use super::verify_setup_completed; use crate::helper_materialization::BIN_DIRNAME; use crate::helper_materialization::RESOURCES_DIRNAME; use crate::helper_materialization::helper_bin_dir; use crate::resolved_permissions::ResolvedWindowsSandboxPermissions; + use crate::setup_error::SetupErrorCode; + use crate::setup_error::extract_failure; use codex_protocol::models::PermissionProfile; use codex_protocol::permissions::NetworkSandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; @@ -1089,6 +1106,18 @@ mod tests { .collect() } + #[test] + fn setup_completion_requires_ready_artifacts() { + let codex_home = TempDir::new().expect("tempdir"); + let err = verify_setup_completed(codex_home.path()) + .expect_err("missing setup artifacts should fail"); + + assert_eq!( + extract_failure(&err).map(|failure| failure.code), + Some(SetupErrorCode::OrchestratorHelperIncomplete) + ); + } + fn permissions_for( permission_profile: &PermissionProfile, workspace_roots: &[AbsolutePathBuf], diff --git a/codex-rs/windows-sandbox-rs/src/setup_error.rs b/codex-rs/windows-sandbox-rs/src/setup_error.rs index 4d2a9d57d..0f759ef87 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_error.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_error.rs @@ -31,6 +31,8 @@ pub enum SetupErrorCode { OrchestratorHelperExitNonzero, /// Helper exited non-zero and reading `setup_error.json` failed. OrchestratorHelperReportReadFailed, + /// Helper exited successfully before setup completed. + OrchestratorHelperIncomplete, // Helper (elevated process) failures. /// Helper failed while validating or decoding the request payload. HelperRequestArgsFailed, @@ -48,7 +50,7 @@ pub enum SetupErrorCode { HelperDpapiProtectFailed, /// Helper failed to write the sandbox users secrets file. HelperUsersFileWriteFailed, - /// Helper failed to write the setup marker file. + /// Helper failed to write or protect the setup marker file. HelperSetupMarkerWriteFailed, /// Helper failed to resolve a SID or convert it to a PSID. HelperSidResolveFailed, @@ -83,6 +85,7 @@ impl SetupErrorCode { Self::OrchestratorHelperLaunchCanceled => "orchestrator_helper_launch_canceled", Self::OrchestratorHelperExitNonzero => "orchestrator_helper_exit_nonzero", Self::OrchestratorHelperReportReadFailed => "orchestrator_helper_report_read_failed", + Self::OrchestratorHelperIncomplete => "orchestrator_helper_incomplete", Self::HelperRequestArgsFailed => "helper_request_args_failed", Self::HelperSandboxDirCreateFailed => "helper_sandbox_dir_create_failed", Self::HelperLogFailed => "helper_log_failed",