From 101e4b3c61ef3335cf16a4e3e1a77c08d5c7b52c Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 21 May 2026 13:46:43 -0700 Subject: [PATCH] fix: preserve deny-read sandboxing for safe commands --- codex-rs/core/src/tools/orchestrator.rs | 7 ++ .../tools/runtimes/shell/unix_escalation.rs | 38 +++++-- .../runtimes/shell/unix_escalation_tests.rs | 98 +++++++++++++++++++ codex-rs/core/src/tools/sandboxing.rs | 19 ++-- codex-rs/core/src/tools/sandboxing_tests.rs | 7 +- 5 files changed, 154 insertions(+), 15 deletions(-) diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index deb9ae596e..fa847ba753 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -28,6 +28,7 @@ use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::ToolRuntime; use crate::tools::sandboxing::default_exec_approval_requirement; use crate::tools::sandboxing::sandbox_override_for_first_attempt; +use crate::tools::sandboxing::unsandboxed_execution_allowed; use codex_hooks::PermissionRequestDecision; use codex_otel::ToolDecisionSource; use codex_protocol::error::CodexErr; @@ -291,6 +292,12 @@ impl ToolOrchestrator { network_policy_decision, }))); } + if !unsandboxed_execution_allowed(&file_system_sandbox_policy) { + return Err(ToolError::Codex(CodexErr::Sandbox(SandboxErr::Denied { + output, + network_policy_decision, + }))); + } // Under `Never` or `OnRequest`, do not retry without sandbox; // surface a concise sandbox denial that preserves the // original output. diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index fef8db5ca9..03ce6e4eb9 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -21,6 +21,7 @@ use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; use crate::tools::sandboxing::managed_network_for_sandbox_permissions; +use crate::tools::sandboxing::unsandboxed_execution_allowed; use codex_execpolicy::Decision; use codex_execpolicy::Evaluation; use codex_execpolicy::MatchOptions; @@ -97,6 +98,18 @@ fn approval_sandbox_permissions( } } +fn approval_sandbox_permissions_for_policy( + sandbox_permissions: SandboxPermissions, + additional_permissions_preapproved: bool, + file_system_sandbox_policy: &FileSystemSandboxPolicy, +) -> SandboxPermissions { + if unsandboxed_execution_allowed(file_system_sandbox_policy) { + approval_sandbox_permissions(sandbox_permissions, additional_permissions_preapproved) + } else { + SandboxPermissions::UseDefault + } +} + pub(super) async fn try_run_zsh_fork( req: &ShellRequest, attempt: &SandboxAttempt<'_>, @@ -196,9 +209,10 @@ pub(super) async fn try_run_zsh_fork( if let Some(cancellation) = attempt.network_denial_cancellation_token.clone() { cancel_token = cancel_when_either(cancel_token, cancellation); } - let approval_sandbox_permissions = approval_sandbox_permissions( + let approval_sandbox_permissions = approval_sandbox_permissions_for_policy( req.sandbox_permissions, req.additional_permissions_preapproved, + &command_executor.file_system_sandbox_policy, ); let escalation_policy = CoreShellActionProvider { policy: Arc::clone(&exec_policy), @@ -283,9 +297,10 @@ pub(crate) async fn prepare_unified_exec_zsh_fork( file_system_sandbox_policy: exec_request.file_system_sandbox_policy.clone(), sandbox_policy_cwd: exec_request.windows_sandbox_policy_cwd.clone(), sandbox_permissions: req.sandbox_permissions, - approval_sandbox_permissions: approval_sandbox_permissions( + approval_sandbox_permissions: approval_sandbox_permissions_for_policy( req.sandbox_permissions, req.additional_permissions_preapproved, + &exec_request.file_system_sandbox_policy, ), prompt_permissions: req.additional_permissions.clone(), stopwatch: Stopwatch::unlimited(), @@ -367,11 +382,18 @@ impl CoreShellActionProvider { fn shell_request_escalation_execution( sandbox_permissions: SandboxPermissions, permission_profile: &PermissionProfile, + file_system_sandbox_policy: &FileSystemSandboxPolicy, additional_permissions: Option<&AdditionalPermissionProfile>, ) -> EscalationExecution { match sandbox_permissions { SandboxPermissions::UseDefault => EscalationExecution::TurnDefault, - SandboxPermissions::RequireEscalated => EscalationExecution::Unsandboxed, + SandboxPermissions::RequireEscalated => { + if unsandboxed_execution_allowed(file_system_sandbox_policy) { + EscalationExecution::Unsandboxed + } else { + EscalationExecution::TurnDefault + } + } SandboxPermissions::WithAdditionalPermissions => additional_permissions .map(|_| { // Shell request additional permissions were already normalized and @@ -611,8 +633,10 @@ impl EscalationPolicy for CoreShellActionProvider { // fallback function. let decision_driven_by_policy = Self::decision_driven_by_policy(&evaluation.matched_rules, evaluation.decision); - let needs_escalation = - self.sandbox_permissions.requires_escalated_permissions() || decision_driven_by_policy; + let unsandboxed_allowed = unsandboxed_execution_allowed(&self.file_system_sandbox_policy); + let needs_escalation = unsandboxed_allowed + && (self.sandbox_permissions.requires_escalated_permissions() + || decision_driven_by_policy); let decision_source = if decision_driven_by_policy { DecisionSource::PrefixRule @@ -620,10 +644,12 @@ impl EscalationPolicy for CoreShellActionProvider { DecisionSource::UnmatchedCommandFallback }; let escalation_execution = match decision_source { - DecisionSource::PrefixRule => EscalationExecution::Unsandboxed, + DecisionSource::PrefixRule if unsandboxed_allowed => EscalationExecution::Unsandboxed, + DecisionSource::PrefixRule => EscalationExecution::TurnDefault, DecisionSource::UnmatchedCommandFallback => Self::shell_request_escalation_execution( self.sandbox_permissions, &self.permission_profile, + &self.file_system_sandbox_policy, self.prompt_permissions.as_ref(), ), }; diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs index 43d8e46952..84dbfa4e33 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs @@ -66,6 +66,23 @@ fn read_only_file_system_sandbox_policy() -> FileSystemSandboxPolicy { }]) } +fn denied_read_file_system_sandbox_policy() -> FileSystemSandboxPolicy { + FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: "**/*.env".to_string(), + }, + access: FileSystemAccessMode::Deny, + }, + ]) +} + fn test_sandbox_cwd() -> AbsolutePathBuf { AbsolutePathBuf::try_from(host_absolute_path(&["workspace"])).unwrap() } @@ -129,6 +146,26 @@ fn approval_sandbox_permissions_only_downgrades_preapproved_additional_permissio ); } +#[test] +fn approval_sandbox_permissions_for_policy_suppresses_no_sandbox_with_deny_reads() { + let file_system_sandbox_policy = + FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::GlobPattern { + pattern: "**/*.env".to_string(), + }, + access: FileSystemAccessMode::Deny, + }]); + + assert_eq!( + super::approval_sandbox_permissions_for_policy( + SandboxPermissions::RequireEscalated, + /*additional_permissions_preapproved*/ false, + &file_system_sandbox_policy, + ), + SandboxPermissions::UseDefault, + ); +} + #[test] fn extract_shell_script_preserves_login_flag() { assert_eq!( @@ -289,11 +326,13 @@ fn shell_request_escalation_execution_is_explicit() { &file_system_sandbox_policy, network_sandbox_policy, ); + let read_only_file_system_policy = read_only_file_system_sandbox_policy(); assert_eq!( CoreShellActionProvider::shell_request_escalation_execution( crate::sandboxing::SandboxPermissions::UseDefault, &permission_profile, + &file_system_sandbox_policy, /*additional_permissions*/ None, ), EscalationExecution::TurnDefault, @@ -302,14 +341,25 @@ fn shell_request_escalation_execution_is_explicit() { CoreShellActionProvider::shell_request_escalation_execution( crate::sandboxing::SandboxPermissions::RequireEscalated, &permission_profile, + &read_only_file_system_policy, /*additional_permissions*/ None, ), EscalationExecution::Unsandboxed, ); + assert_eq!( + CoreShellActionProvider::shell_request_escalation_execution( + crate::sandboxing::SandboxPermissions::RequireEscalated, + &permission_profile, + &file_system_sandbox_policy, + /*additional_permissions*/ None, + ), + EscalationExecution::TurnDefault, + ); assert_eq!( CoreShellActionProvider::shell_request_escalation_execution( crate::sandboxing::SandboxPermissions::WithAdditionalPermissions, &permission_profile, + &file_system_sandbox_policy, Some(&requested_permissions), ), EscalationExecution::Permissions(EscalationPermissions::ResolvedPermissionProfile( @@ -614,6 +664,54 @@ host_executable(name = "git", paths = ["{git_path_literal}"]) )); } +#[tokio::test(flavor = "current_thread")] +async fn denied_reads_keep_prefix_rule_allow_inside_sandbox() -> anyhow::Result<()> { + let cat_path = host_absolute_path(&["usr", "bin", "cat"]); + let cat_path_literal = starlark_string(&cat_path); + let policy_src = format!( + r#" +prefix_rule(pattern = ["{cat_path_literal}"], decision = "allow") +"# + ); + let mut parser = PolicyParser::new(); + parser.parse("test.rules", &policy_src).unwrap(); + let policy = parser.build(); + + let (session, turn_context) = make_session_and_context().await; + let file_system_sandbox_policy = denied_read_file_system_sandbox_policy(); + let permission_profile = PermissionProfile::from_runtime_permissions( + &file_system_sandbox_policy, + NetworkSandboxPolicy::Restricted, + ); + let workdir = test_sandbox_cwd(); + let provider = CoreShellActionProvider { + policy: Arc::new(RwLock::new(policy)), + session: Arc::new(session), + turn: Arc::new(turn_context), + call_id: "deny-read-prefix-allow".to_string(), + tool_name: GuardianCommandSource::Shell, + approval_policy: AskForApproval::OnRequest, + permission_profile, + file_system_sandbox_policy, + sandbox_policy_cwd: workdir.clone(), + sandbox_permissions: SandboxPermissions::UseDefault, + approval_sandbox_permissions: SandboxPermissions::UseDefault, + prompt_permissions: None, + stopwatch: codex_shell_escalation::Stopwatch::new(Duration::from_secs(1)), + }; + + let action = codex_shell_escalation::EscalationPolicy::determine_action( + &provider, + &AbsolutePathBuf::try_from(cat_path).unwrap(), + &["cat".to_string(), "/tmp/visible.txt".to_string()], + &workdir, + ) + .await?; + + assert_eq!(action, codex_shell_escalation::EscalationDecision::Run); + Ok(()) +} + #[test] fn intercepted_exec_policy_treats_preapproved_additional_permissions_as_default() { let policy = PolicyParser::new().build(); diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 1ca589a5f9..0eda3ccebf 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -248,6 +248,13 @@ pub(crate) fn sandbox_override_for_first_attempt( exec_approval_requirement: &ExecApprovalRequirement, file_system_sandbox_policy: &FileSystemSandboxPolicy, ) -> SandboxOverride { + // Deny-read restrictions are part of the active permission policy. Running + // without a filesystem sandbox would discard them, even if the command was + // otherwise approved by rules or explicit escalation. + if !unsandboxed_execution_allowed(file_system_sandbox_policy) { + return SandboxOverride::NoOverride; + } + // ExecPolicy `Allow` can intentionally imply full trust (Skip + bypass_sandbox=true), // which supersedes `with_additional_permissions` sandboxed execution hints. if matches!( @@ -260,12 +267,6 @@ pub(crate) fn sandbox_override_for_first_attempt( return SandboxOverride::BypassSandboxFirstAttempt; } - // Deny-read restrictions suppress explicit escalation because that path - // would otherwise discard the filesystem policy entirely. - if file_system_sandbox_policy.has_denied_read_restrictions() { - return SandboxOverride::NoOverride; - } - if sandbox_permissions.requires_escalated_permissions() { SandboxOverride::BypassSandboxFirstAttempt } else { @@ -273,6 +274,12 @@ pub(crate) fn sandbox_override_for_first_attempt( } } +pub(crate) fn unsandboxed_execution_allowed( + file_system_sandbox_policy: &FileSystemSandboxPolicy, +) -> bool { + !file_system_sandbox_policy.has_denied_read_restrictions() +} + pub(crate) fn managed_network_for_sandbox_permissions( network: Option<&NetworkProxy>, sandbox_permissions: SandboxPermissions, diff --git a/codex-rs/core/src/tools/sandboxing_tests.rs b/codex-rs/core/src/tools/sandboxing_tests.rs index f2e4c4cd64..3aaac52bf2 100644 --- a/codex-rs/core/src/tools/sandboxing_tests.rs +++ b/codex-rs/core/src/tools/sandboxing_tests.rs @@ -138,7 +138,7 @@ fn guardian_bypasses_sandbox_for_explicit_escalation_on_first_attempt() { } #[test] -fn deny_read_blocks_explicit_escalation_but_preserves_policy_bypass() { +fn deny_read_blocks_explicit_escalation_and_policy_bypass() { let file_system_policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { path: FileSystemPath::GlobPattern { pattern: "**/*.env".to_string(), @@ -158,6 +158,7 @@ fn deny_read_blocks_explicit_escalation_but_preserves_policy_bypass() { SandboxOverride::NoOverride, "explicit escalation would drop deny-read filesystem policy, so keep the first attempt sandboxed", ); + assert!(!unsandboxed_execution_allowed(&file_system_policy)); assert_eq!( sandbox_override_for_first_attempt( SandboxPermissions::WithAdditionalPermissions, @@ -167,7 +168,7 @@ fn deny_read_blocks_explicit_escalation_but_preserves_policy_bypass() { }, &file_system_policy, ), - SandboxOverride::BypassSandboxFirstAttempt, - "exec-policy allow rules intentionally bypass sandbox even when deny-read entries exist", + SandboxOverride::NoOverride, + "exec-policy allow rules would drop deny-read filesystem policy, so keep the first attempt sandboxed", ); }