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",