From 8bbb43da52c3503833ceb30fc5c633175f672010 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Wed, 11 Mar 2026 17:57:05 -0700 Subject: [PATCH] fix(security): require explicit SANDBOX_ALLOW_FULL_ACCESS to enable FullAccess policy (#967) * fix(security): require explicit SANDBOX_ALLOW_FULL_ACCESS to enable FullAccess policy FullAccess policy bypasses Docker entirely and runs commands via sh -c directly on the host. Previously, setting SANDBOX_POLICY=full_access alone was sufficient to enable this, which could be triggered accidentally or via prompt injection if tool approval is bypassed. This adds a double opt-in guard: - New SANDBOX_ALLOW_FULL_ACCESS=true env var must ALSO be set for FullAccess to take effect. Without it, the policy is downgraded to WorkspaceWrite with a tracing::error! log. - At execution time, every FullAccess command emits a tracing::warn! with the command and working directory for audit visibility. - The FullAccess variant now documents its blast radius (host shell, unrestricted filesystem/network/environment). - SandboxConfig and SandboxModeConfig gain an allow_full_access field, wired through from_env() and the builder. Co-Authored-By: Claude Sonnet 4.6 * fix(sandbox): address review feedback on FullAccess double opt-in - Add doc comment on builder .policy() warning that FullAccess requires .allow_full_access(true) or execution will return SandboxError::Config - Sanitize audit log: log only binary name instead of full command to prevent secret leakage; add [FullAccess] prefix for grep-ability - Add test_builder_full_access_without_allow_returns_error test covering the builder path without explicit allow_full_access(true) - Fix doc comment mismatch: config.rs and SandboxPolicy::FullAccess docs said "will downgrade to WorkspaceWrite" but runtime returns SandboxError::Config -- aligned docs with actual behavior Co-Authored-By: Claude Sonnet 4.6 * fix: merge duplicate mod tests; add allow_full_access to struct initializers After upstream merge, src/config/sandbox.rs had two issues: - Duplicate mod tests block (upstream's original tests at line 271 + our new FullAccess guard tests at line 478) caused E0428 compile error - Upstream test struct literals for SandboxModeConfig were missing the new allow_full_access field (E0063) Fixes: merge the two mod tests into one; add allow_full_access: false to the sandbox_mode_config_custom_values and sandbox_mode_to_sandbox_config test struct initializers. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Gabe Hamilton Co-authored-by: Claude Sonnet 4.6 --- .env.example | 12 +++++++ src/config/sandbox.rs | 81 +++++++++++++++++++++++++++++++++++++++++- src/sandbox/config.rs | 19 +++++++++- src/sandbox/manager.rs | 78 +++++++++++++++++++++++++++++++++++++++- 4 files changed, 187 insertions(+), 3 deletions(-) diff --git a/.env.example b/.env.example index 5c21e995..60bb2b95 100644 --- a/.env.example +++ b/.env.example @@ -138,6 +138,18 @@ HEARTBEAT_NOTIFY_USER=default # MEMORY_HYGIENE_CONVERSATION_RETENTION_DAYS=7 # delete conversations/ docs older than this many days # MEMORY_HYGIENE_CADENCE_HOURS=12 # minimum hours between cleanup passes +# Docker Sandbox +# SANDBOX_ENABLED=true +# SANDBOX_POLICY=readonly # readonly, workspace_write, or full_access +# SANDBOX_ALLOW_FULL_ACCESS=false # REQUIRED second opt-in for full_access policy. +# # FullAccess bypasses Docker entirely and runs +# # commands directly on the host. Without this +# # set to "true", full_access is downgraded to +# # workspace_write. +# SANDBOX_IMAGE=ironclaw-worker:latest +# SANDBOX_TIMEOUT_SECS=120 +# SANDBOX_MEMORY_LIMIT_MB=2048 + # Safety settings SAFETY_MAX_OUTPUT_LENGTH=100000 SAFETY_INJECTION_CHECK_ENABLED=true diff --git a/src/config/sandbox.rs b/src/config/sandbox.rs index 35be4393..e9b7ca76 100644 --- a/src/config/sandbox.rs +++ b/src/config/sandbox.rs @@ -8,6 +8,13 @@ pub struct SandboxModeConfig { pub enabled: bool, /// Sandbox policy: "readonly", "workspace_write", or "full_access". pub policy: String, + /// Explicit opt-in for `FullAccess` policy. + /// + /// When `policy` is `full_access` but this is `false`, the policy is + /// downgraded to `workspace_write` with a loud error log. This prevents + /// accidental host-level command execution from a single misconfigured + /// env var. + pub allow_full_access: bool, /// Command timeout in seconds. pub timeout_secs: u64, /// Memory limit in megabytes. @@ -31,6 +38,7 @@ impl Default for SandboxModeConfig { Self { enabled: true, policy: "readonly".to_string(), + allow_full_access: false, timeout_secs: 120, memory_limit_mb: 2048, cpu_shares: 1024, @@ -70,6 +78,7 @@ impl SandboxModeConfig { Ok(Self { enabled: parse_bool_env("SANDBOX_ENABLED", true)?, policy: parse_string_env("SANDBOX_POLICY", "readonly")?, + allow_full_access: parse_bool_env("SANDBOX_ALLOW_FULL_ACCESS", false)?, timeout_secs: parse_optional_env("SANDBOX_TIMEOUT_SECS", 120)?, memory_limit_mb: parse_optional_env("SANDBOX_MEMORY_LIMIT_MB", 2048)?, cpu_shares: parse_optional_env("SANDBOX_CPU_SHARES", 1024)?, @@ -82,11 +91,25 @@ impl SandboxModeConfig { } /// Convert to SandboxConfig for the sandbox module. + /// + /// If `policy` is `FullAccess` but `allow_full_access` is `false`, + /// the policy is downgraded to `WorkspaceWrite` and an error is logged. pub fn to_sandbox_config(&self) -> crate::sandbox::SandboxConfig { use crate::sandbox::SandboxPolicy; use std::time::Duration; - let policy = self.policy.parse().unwrap_or(SandboxPolicy::ReadOnly); + let mut policy = self.policy.parse().unwrap_or(SandboxPolicy::ReadOnly); + + // Double opt-in guard: FullAccess requires SANDBOX_ALLOW_FULL_ACCESS=true + if policy == SandboxPolicy::FullAccess && !self.allow_full_access { + tracing::error!( + "SANDBOX_POLICY=full_access is set but SANDBOX_ALLOW_FULL_ACCESS is not \ + set to 'true'. FullAccess bypasses Docker and runs commands directly on \ + the host. Downgrading to WorkspaceWrite for safety. Set \ + SANDBOX_ALLOW_FULL_ACCESS=true to explicitly enable FullAccess." + ); + policy = SandboxPolicy::WorkspaceWrite; + } let mut allowlist = crate::sandbox::default_allowlist(); allowlist.extend(self.extra_allowed_domains.clone()); @@ -94,6 +117,7 @@ impl SandboxModeConfig { crate::sandbox::SandboxConfig { enabled: self.enabled, policy, + allow_full_access: self.allow_full_access, timeout: Duration::from_secs(self.timeout_secs), memory_limit_mb: self.memory_limit_mb, cpu_shares: self.cpu_shares, @@ -302,6 +326,7 @@ mod tests { extra_allowed_domains: vec!["example.com".to_string()], reaper_interval_secs: 300, orphan_threshold_secs: 600, + allow_full_access: false, }; assert!(!cfg.enabled); assert_eq!(cfg.policy, "full_access"); @@ -326,6 +351,7 @@ mod tests { extra_allowed_domains: vec!["custom.example.com".to_string()], reaper_interval_secs: 300, orphan_threshold_secs: 600, + allow_full_access: false, }; let sc = mode.to_sandbox_config(); assert!(sc.enabled); @@ -485,4 +511,57 @@ mod tests { ); } } + + #[test] + fn test_full_access_downgraded_without_allow() { + let config = SandboxModeConfig { + policy: "full_access".to_string(), + allow_full_access: false, + ..Default::default() + }; + let sandbox = config.to_sandbox_config(); + // Should have been downgraded to WorkspaceWrite + assert_eq!( + sandbox.policy, + crate::sandbox::SandboxPolicy::WorkspaceWrite + ); + assert!(!sandbox.allow_full_access); + } + + #[test] + fn test_full_access_allowed_with_explicit_opt_in() { + let config = SandboxModeConfig { + policy: "full_access".to_string(), + allow_full_access: true, + ..Default::default() + }; + let sandbox = config.to_sandbox_config(); + assert_eq!(sandbox.policy, crate::sandbox::SandboxPolicy::FullAccess); + assert!(sandbox.allow_full_access); + } + + #[test] + fn test_non_full_access_policy_unaffected() { + let config = SandboxModeConfig { + policy: "workspace_write".to_string(), + allow_full_access: false, + ..Default::default() + }; + let sandbox = config.to_sandbox_config(); + assert_eq!( + sandbox.policy, + crate::sandbox::SandboxPolicy::WorkspaceWrite + ); + } + + #[test] + fn test_readonly_policy_unaffected() { + let config = SandboxModeConfig { + policy: "readonly".to_string(), + allow_full_access: false, + ..Default::default() + }; + let sandbox = config.to_sandbox_config(); + assert_eq!(sandbox.policy, crate::sandbox::SandboxPolicy::ReadOnly); + } } diff --git a/src/sandbox/config.rs b/src/sandbox/config.rs index 76356a3c..05594364 100644 --- a/src/sandbox/config.rs +++ b/src/sandbox/config.rs @@ -9,6 +9,13 @@ pub struct SandboxConfig { pub enabled: bool, /// Security policy for sandbox execution. pub policy: SandboxPolicy, + /// Whether `FullAccess` policy is explicitly allowed. + /// + /// When `policy` is `FullAccess` but this field is `false`, the manager + /// will return `SandboxError::Config` and refuse to execute. This is an + /// intentional double opt-in to prevent accidental host execution. + /// Set via `SANDBOX_ALLOW_FULL_ACCESS=true` env var. + pub allow_full_access: bool, /// Default timeout for command execution. pub timeout: Duration, /// Memory limit in megabytes. @@ -30,6 +37,7 @@ impl Default for SandboxConfig { Self { enabled: true, // Startup check disables gracefully if Docker unavailable policy: SandboxPolicy::ReadOnly, + allow_full_access: false, timeout: Duration::from_secs(120), memory_limit_mb: 2048, cpu_shares: 1024, @@ -66,7 +74,16 @@ pub enum SandboxPolicy { WorkspaceWrite, /// Full access (no sandbox). Use with extreme caution. - /// This bypasses all isolation and runs directly on host. + /// + /// **BLAST RADIUS**: This bypasses Docker entirely and executes commands + /// via `sh -c` directly on the host with the agent process's full + /// privileges. If prompt injection bypasses tool approval, arbitrary + /// host shell commands can run. File system, network, and environment + /// are completely unrestricted. + /// + /// Requires `SANDBOX_ALLOW_FULL_ACCESS=true` as a second opt-in. + /// Without it, the sandbox manager will return `SandboxError::Config` + /// and refuse to execute. FullAccess, } diff --git a/src/sandbox/manager.rs b/src/sandbox/manager.rs index 8d042cab..ce709f50 100644 --- a/src/sandbox/manager.rs +++ b/src/sandbox/manager.rs @@ -207,8 +207,27 @@ impl SandboxManager { policy: SandboxPolicy, env: HashMap, ) -> Result { - // FullAccess policy bypasses the sandbox entirely + // FullAccess policy bypasses the sandbox entirely. + // Double-check the allow_full_access guard at execution time as well, + // in case the policy was overridden per-call via execute_with_policy(). if policy == SandboxPolicy::FullAccess { + if !self.config.allow_full_access { + tracing::error!( + "FullAccess execution requested but SANDBOX_ALLOW_FULL_ACCESS is not \ + enabled. Refusing to execute on host. Falling back to error." + ); + return Err(SandboxError::Config { + reason: "FullAccess policy requires SANDBOX_ALLOW_FULL_ACCESS=true".to_string(), + }); + } + // Log only the binary name to avoid leaking secrets embedded in + // command arguments (e.g. tokens in curl headers). + let binary = command.split_whitespace().next().unwrap_or(""); + tracing::warn!( + binary = %binary, + cwd = %cwd.display(), + "[FullAccess] Executing command directly on host (no sandbox isolation)" + ); return self.execute_direct(command, cwd, env).await; } @@ -374,11 +393,22 @@ impl SandboxManagerBuilder { } /// Set the sandbox policy. + /// + /// **Note:** `SandboxPolicy::FullAccess` additionally requires + /// `allow_full_access(true)` to be set, or the manager will return + /// `SandboxError::Config` at execution time. This is an intentional + /// double opt-in to prevent accidental host execution. pub fn policy(mut self, policy: SandboxPolicy) -> Self { self.config.policy = policy; self } + /// Explicitly allow FullAccess policy (double opt-in). + pub fn allow_full_access(mut self, allow: bool) -> Self { + self.config.allow_full_access = allow; + self + } + /// Set the command timeout. pub fn timeout(mut self, timeout: Duration) -> Self { self.config.timeout = timeout; @@ -485,6 +515,7 @@ mod tests { let manager = SandboxManager::new(SandboxConfig { enabled: true, policy: SandboxPolicy::FullAccess, + allow_full_access: true, ..Default::default() }); @@ -498,11 +529,56 @@ mod tests { assert!(output.stdout.contains("hello")); } + #[tokio::test] + async fn test_direct_execution_blocked_without_allow() { + let manager = SandboxManager::new(SandboxConfig { + enabled: true, + policy: SandboxPolicy::FullAccess, + allow_full_access: false, + ..Default::default() + }); + + let result = manager + .execute("echo hello", Path::new("."), HashMap::new()) + .await; + + // Should be rejected because allow_full_access is false + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("SANDBOX_ALLOW_FULL_ACCESS"), + "Error should mention SANDBOX_ALLOW_FULL_ACCESS, got: {}", + err + ); + } + + #[tokio::test] + async fn test_builder_full_access_without_allow_returns_error() { + let manager = SandboxManagerBuilder::new() + .enabled(true) + .policy(SandboxPolicy::FullAccess) + // Deliberately omitting .allow_full_access(true) + .build(); + + let result = manager + .execute("echo hello", Path::new("."), HashMap::new()) + .await; + + assert!(result.is_err()); + let err = result.unwrap_err().to_string(); + assert!( + err.contains("SANDBOX_ALLOW_FULL_ACCESS"), + "Error should mention SANDBOX_ALLOW_FULL_ACCESS, got: {}", + err + ); + } + #[tokio::test] async fn test_direct_execution_truncates_large_output() { let manager = SandboxManager::new(SandboxConfig { enabled: true, policy: SandboxPolicy::FullAccess, + allow_full_access: true, ..Default::default() });