From febc95ce1de15676ad1474461114bf95e3356bbc Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 29 Apr 2025 10:09:36 -0700 Subject: [PATCH] feat: flip the sense of the --sandbox option --- codex-rs/core/src/approval_mode_cli_arg.rs | 32 ++--- codex-rs/core/src/codex.rs | 12 +- codex-rs/core/src/exec.rs | 94 +++++++------ codex-rs/core/src/linux.rs | 3 +- codex-rs/core/src/protocol.rs | 125 ++++++++++++++---- codex-rs/core/src/safety.rs | 13 +- ..._policy.sbpl => seatbelt_base_policy.sbpl} | 3 - codex-rs/core/tests/live_agent.rs | 2 +- codex-rs/core/tests/previous_response_id.rs | 2 +- codex-rs/core/tests/stream_no_completed.rs | 2 +- codex-rs/tui/src/cli.rs | 7 +- codex-rs/tui/src/lib.rs | 3 +- 12 files changed, 183 insertions(+), 115 deletions(-) rename codex-rs/core/src/{seatbelt_readonly_policy.sbpl => seatbelt_base_policy.sbpl} (97%) diff --git a/codex-rs/core/src/approval_mode_cli_arg.rs b/codex-rs/core/src/approval_mode_cli_arg.rs index 0da6a89efc..4addca4b30 100644 --- a/codex-rs/core/src/approval_mode_cli_arg.rs +++ b/codex-rs/core/src/approval_mode_cli_arg.rs @@ -23,6 +23,15 @@ pub enum ApprovalModeCliArg { /// Execution failures are immediately returned to the model. Never, } +impl From for AskForApproval { + fn from(value: ApprovalModeCliArg) -> Self { + match value { + ApprovalModeCliArg::OnFailure => AskForApproval::OnFailure, + ApprovalModeCliArg::UnlessAllowListed => AskForApproval::UnlessAllowListed, + ApprovalModeCliArg::Never => AskForApproval::Never, + } + } +} #[derive(Clone, Copy, Debug, ValueEnum)] #[value(rename_all = "kebab-case")] @@ -37,25 +46,8 @@ pub enum SandboxModeCliArg { DangerousNoRestrictions, } -impl From for AskForApproval { - fn from(value: ApprovalModeCliArg) -> Self { - match value { - ApprovalModeCliArg::OnFailure => AskForApproval::OnFailure, - ApprovalModeCliArg::UnlessAllowListed => AskForApproval::UnlessAllowListed, - ApprovalModeCliArg::Never => AskForApproval::Never, - } - } -} - -impl From for SandboxPolicy { - fn from(value: SandboxModeCliArg) -> Self { - match value { - SandboxModeCliArg::NetworkRestricted => SandboxPolicy::NetworkRestricted, - SandboxModeCliArg::FileWriteRestricted => SandboxPolicy::FileWriteRestricted, - SandboxModeCliArg::NetworkAndFileWriteRestricted => { - SandboxPolicy::NetworkAndFileWriteRestricted - } - SandboxModeCliArg::DangerousNoRestrictions => SandboxPolicy::DangerousNoRestrictions, - } +impl From> for SandboxPolicy { + fn from(value: Vec) -> Self { + unimplemented!("need to convert {value:?}"); } } diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index edeaef9932..384011e302 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -861,7 +861,7 @@ async fn handle_function_call( assess_command_safety( ¶ms.command, sess.approval_policy, - sess.sandbox_policy, + &sess.sandbox_policy, &state.approved_commands, ) }; @@ -916,14 +916,11 @@ async fn handle_function_call( ) .await; - let roots_snapshot = { sess.writable_roots.lock().unwrap().clone() }; - let output_result = process_exec_tool_call( params.clone(), sandbox_type, - &roots_snapshot, sess.ctrl_c.clone(), - sess.sandbox_policy, + &sess.sandbox_policy, ) .await; @@ -1006,16 +1003,13 @@ async fn handle_function_call( ) .await; - let retry_roots = { sess.writable_roots.lock().unwrap().clone() }; - // This is an escalated retry; the policy will not be // examined and the sandbox has been set to `None`. let retry_output_result = process_exec_tool_call( params.clone(), SandboxType::None, - &retry_roots, sess.ctrl_c.clone(), - sess.sandbox_policy, + &sess.sandbox_policy, ) .await; diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 952b4453df..c8e92ba221 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -1,7 +1,6 @@ use std::io; #[cfg(target_family = "unix")] use std::os::unix::process::ExitStatusExt; -use std::path::PathBuf; use std::process::ExitStatus; use std::process::Stdio; use std::sync::Arc; @@ -33,7 +32,7 @@ const DEFAULT_TIMEOUT_MS: u64 = 10_000; const SIGKILL_CODE: i32 = 9; const TIMEOUT_CODE: i32 = 64; -const MACOS_SEATBELT_READONLY_POLICY: &str = include_str!("seatbelt_readonly_policy.sbpl"); +const MACOS_SEATBELT_BASE_POLICY: &str = include_str!("seatbelt_base_policy.sbpl"); /// When working with `sandbox-exec`, only consider `sandbox-exec` in `/usr/bin` /// to defend against an attacker trying to inject a malicious version on the @@ -67,19 +66,17 @@ pub enum SandboxType { #[cfg(target_os = "linux")] async fn exec_linux( params: ExecParams, - writable_roots: &[PathBuf], ctrl_c: Arc, sandbox_policy: SandboxPolicy, ) -> Result { - crate::linux::exec_linux(params, writable_roots, ctrl_c, sandbox_policy).await + crate::linux::exec_linux(params, ctrl_c, sandbox_policy).await } #[cfg(not(target_os = "linux"))] async fn exec_linux( _params: ExecParams, - _writable_roots: &[PathBuf], _ctrl_c: Arc, - _sandbox_policy: SandboxPolicy, + _sandbox_policy: &SandboxPolicy, ) -> Result { Err(CodexErr::Io(io::Error::new( io::ErrorKind::InvalidInput, @@ -90,9 +87,8 @@ async fn exec_linux( pub async fn process_exec_tool_call( params: ExecParams, sandbox_type: SandboxType, - writable_roots: &[PathBuf], ctrl_c: Arc, - sandbox_policy: SandboxPolicy, + sandbox_policy: &SandboxPolicy, ) -> Result { let start = Instant::now(); @@ -104,7 +100,7 @@ pub async fn process_exec_tool_call( workdir, timeout_ms, } = params; - let seatbelt_command = create_seatbelt_command(command, sandbox_policy, writable_roots); + let seatbelt_command = create_seatbelt_command(command, sandbox_policy); exec( ExecParams { command: seatbelt_command, @@ -115,9 +111,7 @@ pub async fn process_exec_tool_call( ) .await } - SandboxType::LinuxSeccomp => { - exec_linux(params, writable_roots, ctrl_c, sandbox_policy).await - } + SandboxType::LinuxSeccomp => exec_linux(params, ctrl_c, sandbox_policy).await, }; let duration = start.elapsed(); match raw_output_result { @@ -162,41 +156,61 @@ pub async fn process_exec_tool_call( pub fn create_seatbelt_command( command: Vec, - sandbox_policy: SandboxPolicy, - writable_roots: &[PathBuf], + sandbox_policy: &SandboxPolicy, ) -> Vec { - let (policies, cli_args): (Vec, Vec) = writable_roots - .iter() - .enumerate() - .map(|(index, root)| { - let param_name = format!("WRITABLE_ROOT_{index}"); - let policy: String = format!("(subpath (param \"{param_name}\"))"); - let cli_arg = format!("-D{param_name}={}", root.to_string_lossy()); - (policy, cli_arg) - }) - .unzip(); - - // TODO(ragona): The seatbelt policy should reflect the SandboxPolicy that - // is passed, but everything is currently hardcoded to use - // MACOS_SEATBELT_READONLY_POLICY. - // TODO(mbolin): apply_patch calls must also honor the SandboxPolicy. - if !matches!(sandbox_policy, SandboxPolicy::NetworkRestricted) { - tracing::error!("specified sandbox policy {sandbox_policy:?} will not be honroed"); - } - - let full_policy = if policies.is_empty() { - MACOS_SEATBELT_READONLY_POLICY.to_string() - } else { - let scoped_write_policy = format!("(allow file-write*\n{}\n)", policies.join(" ")); - format!("{MACOS_SEATBELT_READONLY_POLICY}\n{scoped_write_policy}") + let (file_write_policy, extra_cli_args) = { + if sandbox_policy.has_full_disk_write_access() { + // Allegedly, this is more permissive than `(allow file-write*)`. + ( + r#"(allow file-write* (regex #"^/"))"#.to_string(), + Vec::::new(), + ) + } else { + let writable_roots = sandbox_policy.get_writable_roots(); + let (writable_folder_policies, cli_args): (Vec, Vec) = writable_roots + .iter() + .enumerate() + .map(|(index, root)| { + let param_name = format!("WRITABLE_ROOT_{index}"); + let policy: String = format!("(subpath (param \"{param_name}\"))"); + let cli_arg = format!("-D{param_name}={}", root.to_string_lossy()); + (policy, cli_arg) + }) + .unzip(); + if writable_folder_policies.is_empty() { + ("".to_string(), Vec::::new()) + } else { + let file_write_policy = format!( + "(allow file-write*\n{}\n)", + writable_folder_policies.join(" ") + ); + (file_write_policy, cli_args) + } + } }; + let file_read_policy = if sandbox_policy.has_full_disk_read_access() { + "; allow read-only file operations\n(allow file-read*)" + } else { + "" + }; + + // TODO(mbolin): apply_patch calls must also honor the SandboxPolicy. + let network_policy = if sandbox_policy.has_full_network_access() { + "(allow network-outbound)\n(allow network-inbound)\n(allow system-socket)" + } else { + "" + }; + + let full_policy = format!( + "{MACOS_SEATBELT_BASE_POLICY}\n{file_read_policy}\n{file_write_policy}\n{network_policy}" + ); let mut seatbelt_command: Vec = vec![ MACOS_PATH_TO_SEATBELT_EXECUTABLE.to_string(), "-p".to_string(), - full_policy.to_string(), + full_policy, ]; - seatbelt_command.extend(cli_args); + seatbelt_command.extend(extra_cli_args); seatbelt_command.push("--".to_string()); seatbelt_command.extend(command); seatbelt_command diff --git a/codex-rs/core/src/linux.rs b/codex-rs/core/src/linux.rs index 9f9d44b04f..52857fec6d 100644 --- a/codex-rs/core/src/linux.rs +++ b/codex-rs/core/src/linux.rs @@ -32,14 +32,13 @@ use tokio::sync::Notify; pub async fn exec_linux( params: ExecParams, - writable_roots: &[PathBuf], ctrl_c: Arc, sandbox_policy: SandboxPolicy, ) -> Result { // Allow READ on / // Allow WRITE on /dev/null let ctrl_c_copy = ctrl_c.clone(); - let writable_roots_copy = writable_roots.to_vec(); + let writable_roots = sandbox_policy.get_writable_roots(); // Isolate thread to run the sandbox from let tool_call_output = std::thread::spawn(move || { diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index 139e2f2fc2..7f3c7f577e 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -93,44 +93,115 @@ pub enum AskForApproval { } /// Determines execution restrictions for model shell commands -#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[derive(Debug, Clone, Default, PartialEq, Eq, Hash, Serialize, Deserialize)] #[serde(rename_all = "kebab-case")] -pub enum SandboxPolicy { - /// Network syscalls will be blocked - NetworkRestricted, - /// Filesystem writes will be restricted - FileWriteRestricted, - /// Network and filesystem writes will be restricted - #[default] - NetworkAndFileWriteRestricted, - /// No restrictions; full "unsandboxed" mode - DangerousNoRestrictions, +pub struct SandboxPolicy { + pub permissions: Vec, } impl SandboxPolicy { - pub fn is_dangerous(&self) -> bool { - match self { - SandboxPolicy::NetworkRestricted => false, - SandboxPolicy::FileWriteRestricted => false, - SandboxPolicy::NetworkAndFileWriteRestricted => false, - SandboxPolicy::DangerousNoRestrictions => true, + pub fn new_read_only_policy() -> Self { + Self { + permissions: vec![SandboxPermission::DiskFullReadAccess], } } - pub fn is_network_restricted(&self) -> bool { - matches!( - self, - SandboxPolicy::NetworkRestricted | SandboxPolicy::NetworkAndFileWriteRestricted - ) + pub fn has_full_disk_read_access(&self) -> bool { + self.permissions + .iter() + .any(|perm| matches!(perm, SandboxPermission::DiskFullReadAccess)) } - pub fn is_file_write_restricted(&self) -> bool { - matches!( - self, - SandboxPolicy::FileWriteRestricted | SandboxPolicy::NetworkAndFileWriteRestricted - ) + pub fn has_full_disk_write_access(&self) -> bool { + self.permissions + .iter() + .any(|perm| matches!(perm, SandboxPermission::DiskFullWriteAccess)) + } + + pub fn has_full_network_access(&self) -> bool { + self.permissions + .iter() + .any(|perm| matches!(perm, SandboxPermission::NetworkFullAccess)) + } + + pub fn get_writable_roots(&self) -> Vec { + let mut writable_roots = Vec::::new(); + for perm in &self.permissions { + use SandboxPermission::*; + match perm { + DiskWritePlatformUserTempFolder => { + if cfg!(target_os = "macos") { + if let Some(temp_dir) = std::env::var_os("TMPDIR") { + // Add temp_dir and the canonicalized version of temp_dir. + } + } + + // For Linux, should this be XDG_RUNTIME_DIR, /run/user/, or something else? + } + DiskWritePlatformGlobalTempFolder => { + if cfg!(unix) { + writable_roots.push(PathBuf::from("/tmp")); + } + } + DiskWriteCwd => match std::env::current_dir() { + Ok(cwd) => writable_roots.push(cwd), + Err(err) => { + tracing::error!("Failed to get current working directory: {err}"); + } + }, + DiskWriteFolder { folder } => { + writable_roots.push(folder.clone()); + } + DiskFullReadAccess | NetworkFullAccess => {} + DiskFullWriteAccess => { + // Currently, we expect callers to only invoke this method + // after verifying has_full_disk_write_access() is false. + } + } + } + writable_roots + } + + pub fn is_unrestricted(&self) -> bool { + self.has_full_disk_read_access() + && self.has_full_disk_write_access() + && self.has_full_network_access() } } + +/// Permissions that should be granted to the sandbox in which the agent +/// operates. +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +pub enum SandboxPermission { + /// Is allowed to read all files on disk. + DiskFullReadAccess, + + /// Is allowed to write to the operating system's temp dir that + /// is restricted to the user the agent is running as. For + /// example, on macOS, this is generally something under + /// `/var/folders` as opposed to `/tmp`. + DiskWritePlatformUserTempFolder, + + /// Is allowed to write to the operating system's shared temp + /// dir. On UNIX, this is generally `/tmp`. + DiskWritePlatformGlobalTempFolder, + + /// Is allowed to write to the current working directory (in practice, this + /// is the `cwd` where `codex` was spawned). + DiskWriteCwd, + + /// Is allowed to the specified folder. `PathBuf` must be an + /// absolute path, though it is up to the caller to canonicalize + /// it if the path contains symlinks. + DiskWriteFolder { folder: PathBuf }, + + /// Is allowed to write to any file on disk. + DiskFullWriteAccess, + + /// Can make arbitrary network requests. + NetworkFullAccess, +} + /// User input #[non_exhaustive] #[derive(Debug, Clone, Deserialize, Serialize)] diff --git a/codex-rs/core/src/safety.rs b/codex-rs/core/src/safety.rs index e7841b2a85..50ed3573df 100644 --- a/codex-rs/core/src/safety.rs +++ b/codex-rs/core/src/safety.rs @@ -65,7 +65,7 @@ pub fn assess_patch_safety( pub fn assess_command_safety( command: &[String], approval_policy: AskForApproval, - sandbox_policy: SandboxPolicy, + sandbox_policy: &SandboxPolicy, approved: &HashSet>, ) -> SafetyCheck { let approve_without_sandbox = || SafetyCheck::AutoApprove { @@ -81,11 +81,10 @@ pub fn assess_command_safety( } // Command was not known-safe or allow-listed - match sandbox_policy { - // Only the dangerous sandbox policy will run arbitrary commands outside a sandbox - SandboxPolicy::DangerousNoRestrictions => approve_without_sandbox(), - // All other policies try to run the command in a sandbox if it is available - _ => match get_platform_sandbox() { + if sandbox_policy.is_unrestricted() { + approve_without_sandbox() + } else { + match get_platform_sandbox() { // We have a sandbox, so we can approve the command in all modes Some(sandbox_type) => SafetyCheck::AutoApprove { sandbox_type }, None => { @@ -99,7 +98,7 @@ pub fn assess_command_safety( _ => SafetyCheck::AskUser, } } - }, + } } } diff --git a/codex-rs/core/src/seatbelt_readonly_policy.sbpl b/codex-rs/core/src/seatbelt_base_policy.sbpl similarity index 97% rename from codex-rs/core/src/seatbelt_readonly_policy.sbpl rename to codex-rs/core/src/seatbelt_base_policy.sbpl index c06326583a..c9664651c2 100644 --- a/codex-rs/core/src/seatbelt_readonly_policy.sbpl +++ b/codex-rs/core/src/seatbelt_base_policy.sbpl @@ -6,9 +6,6 @@ ; start with closed-by-default (deny default) -; allow read-only file operations -(allow file-read*) - ; child processes inherit the policy of their parent (allow process-exec) (allow process-fork) diff --git a/codex-rs/core/tests/live_agent.rs b/codex-rs/core/tests/live_agent.rs index 2387649873..7d2be33d17 100644 --- a/codex-rs/core/tests/live_agent.rs +++ b/codex-rs/core/tests/live_agent.rs @@ -55,7 +55,7 @@ async fn spawn_codex() -> Codex { model: config.model, instructions: None, approval_policy: config.approval_policy, - sandbox_policy: SandboxPolicy::NetworkAndFileWriteRestricted, + sandbox_policy: SandboxPolicy::new_read_only_policy(), disable_response_storage: false, }, }) diff --git a/codex-rs/core/tests/previous_response_id.rs b/codex-rs/core/tests/previous_response_id.rs index 24c8691630..c83d49eec7 100644 --- a/codex-rs/core/tests/previous_response_id.rs +++ b/codex-rs/core/tests/previous_response_id.rs @@ -95,7 +95,7 @@ async fn keeps_previous_response_id_between_tasks() { model: config.model, instructions: None, approval_policy: config.approval_policy, - sandbox_policy: SandboxPolicy::NetworkAndFileWriteRestricted, + sandbox_policy: SandboxPolicy::new_read_only_policy(), disable_response_storage: false, }, }) diff --git a/codex-rs/core/tests/stream_no_completed.rs b/codex-rs/core/tests/stream_no_completed.rs index e696ea97ae..e64281e377 100644 --- a/codex-rs/core/tests/stream_no_completed.rs +++ b/codex-rs/core/tests/stream_no_completed.rs @@ -78,7 +78,7 @@ async fn retries_on_early_close() { model: config.model, instructions: None, approval_policy: config.approval_policy, - sandbox_policy: SandboxPolicy::NetworkAndFileWriteRestricted, + sandbox_policy: SandboxPolicy::new_read_only_policy(), disable_response_storage: false, }, }) diff --git a/codex-rs/tui/src/cli.rs b/codex-rs/tui/src/cli.rs index f336b0c34c..5087021fff 100644 --- a/codex-rs/tui/src/cli.rs +++ b/codex-rs/tui/src/cli.rs @@ -1,3 +1,4 @@ +use clap::ArgAction; use clap::Parser; use codex_core::ApprovalModeCliArg; use codex_core::SandboxModeCliArg; @@ -21,11 +22,11 @@ pub struct Cli { #[arg(long = "ask-for-approval", short = 'a')] pub approval_policy: Option, - /// Configure the process restrictions when a command is executed. + /// Configure the sandbox permissions when executed an untrusted command. /// /// Uses OS-specific sandboxing tools; Seatbelt on OSX, landlock+seccomp on Linux. - #[arg(long = "sandbox", short = 's')] - pub sandbox_policy: Option, + #[arg(long = "sandbox", short = 's', action = ArgAction::Append)] + pub sandbox_policy: Vec, /// Allow running Codex outside a Git repository. #[arg(long = "skip-git-repo-check", default_value_t = false)] diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index bf4ebec43c..179bcd4d68 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -38,7 +38,8 @@ pub fn run_main(cli: Cli) -> std::io::Result<()> { let overrides = ConfigOverrides { model: cli.model.clone(), approval_policy: cli.approval_policy.map(Into::into), - sandbox_policy: cli.sandbox_policy.map(Into::into), + // FIXME(mbolin): How should this work? + sandbox_policy: None, disable_response_storage: if cli.disable_response_storage { Some(true) } else {