From b34ed2ab831d9b11d77ce493371112fd7d67eccb Mon Sep 17 00:00:00 2001 From: oai-ragona <144164704+oai-ragona@users.noreply.github.com> Date: Thu, 24 Apr 2025 15:33:45 -0700 Subject: [PATCH] [codex-rs] More fine-grained sandbox flag support on Linux (#632) ##### What/Why This PR makes it so that in Linux we actually respect the different types of `--sandbox` flag, such that users can apply network and filesystem restrictions in combination (currently the only supported behavior), or just pick one or the other. We should add similar support for OSX in a future PR. ##### Testing From Linux devbox, updated tests to use more specific flags: ``` test linux::tests_linux::sandbox_blocks_ping ... ok test linux::tests_linux::sandbox_blocks_getent ... ok test linux::tests_linux::test_root_read ... ok test linux::tests_linux::test_dev_null_write ... ok test linux::tests_linux::sandbox_blocks_dev_tcp_redirection ... ok test linux::tests_linux::sandbox_blocks_ssh ... ok test linux::tests_linux::test_writable_root ... ok test linux::tests_linux::sandbox_blocks_curl ... ok test linux::tests_linux::sandbox_blocks_wget ... ok test linux::tests_linux::sandbox_blocks_nc ... ok test linux::tests_linux::test_root_write - should panic ... ok ``` ##### Todo - [ ] Add negative tests (e.g. confirm you can hit the network if you configure filesystem only restrictions) --- codex-rs/core/src/codex.rs | 4 +++ codex-rs/core/src/exec.rs | 11 ++++-- codex-rs/core/src/linux.rs | 63 ++++++++++++++++++++--------------- codex-rs/core/src/protocol.rs | 24 +++++++++++++ 4 files changed, 73 insertions(+), 29 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 74e46678..e57d3bbf 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -868,6 +868,7 @@ async fn handle_function_call( sandbox_type, &roots_snapshot, sess.ctrl_c.clone(), + sess.sandbox_policy, ) .await; @@ -952,11 +953,14 @@ async fn handle_function_call( 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, ) .await; diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 1a92c8ad..fe6bad54 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -15,8 +15,10 @@ use tokio::sync::Notify; use crate::error::CodexErr; use crate::error::Result; use crate::error::SandboxErr; +use crate::protocol::SandboxPolicy; /// Maximum we keep for each stream (100 KiB). +/// TODO(ragona) this should be reduced const MAX_STREAM_OUTPUT: usize = 100 * 1024; const DEFAULT_TIMEOUT_MS: u64 = 10_000; @@ -55,8 +57,9 @@ 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).await + crate::linux::exec_linux(params, writable_roots, ctrl_c, sandbox_policy).await } #[cfg(not(target_os = "linux"))] @@ -64,6 +67,7 @@ async fn exec_linux( _params: ExecParams, _writable_roots: &[PathBuf], _ctrl_c: Arc, + _sandbox_policy: SandboxPolicy, ) -> Result { Err(CodexErr::Io(io::Error::new( io::ErrorKind::InvalidInput, @@ -76,6 +80,7 @@ pub async fn process_exec_tool_call( sandbox_type: SandboxType, writable_roots: &[PathBuf], ctrl_c: Arc, + sandbox_policy: SandboxPolicy, ) -> Result { let start = Instant::now(); @@ -98,7 +103,9 @@ pub async fn process_exec_tool_call( ) .await } - SandboxType::LinuxSeccomp => exec_linux(params, writable_roots, ctrl_c).await, + SandboxType::LinuxSeccomp => { + exec_linux(params, writable_roots, ctrl_c, sandbox_policy).await + } }; let duration = start.elapsed(); match raw_output_result { diff --git a/codex-rs/core/src/linux.rs b/codex-rs/core/src/linux.rs index f2dd9e6b..61711c46 100644 --- a/codex-rs/core/src/linux.rs +++ b/codex-rs/core/src/linux.rs @@ -9,6 +9,7 @@ use crate::error::SandboxErr; use crate::exec::exec; use crate::exec::ExecParams; use crate::exec::RawExecToolCallOutput; +use crate::protocol::SandboxPolicy; use landlock::Access; use landlock::AccessFs; @@ -33,6 +34,7 @@ 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 @@ -47,34 +49,12 @@ pub async fn exec_linux( .expect("Failed to create runtime"); rt.block_on(async { - let abi = ABI::V5; - let access_rw = AccessFs::from_all(abi); - let access_ro = AccessFs::from_read(abi); - - let mut ruleset = Ruleset::default() - .set_compatibility(CompatLevel::BestEffort) - .handle_access(access_rw)? - .create()? - .add_rules(landlock::path_beneath_rules(&["/"], access_ro))? - .add_rules(landlock::path_beneath_rules(&["/dev/null"], access_rw))? - .set_no_new_privs(true); - - if !writable_roots_copy.is_empty() { - ruleset = ruleset.add_rules(landlock::path_beneath_rules( - &writable_roots_copy, - access_rw, - ))?; + if sandbox_policy.is_network_restricted() { + install_network_seccomp_filter_on_current_thread()?; } - let status = ruleset.restrict_self()?; - - // TODO(wpt): Probably wanna expand this more generically and not warn every time. - if status.ruleset == landlock::RulesetStatus::NotEnforced { - return Err(CodexErr::Sandbox(SandboxErr::LandlockRestrict)); - } - - if let Err(e) = install_network_seccomp_filter() { - return Err(CodexErr::Sandbox(e)); + if sandbox_policy.is_file_write_restricted() { + install_filesystem_landlock_rules_on_current_thread(writable_roots_copy)?; } exec(params, ctrl_c_copy).await @@ -92,7 +72,33 @@ pub async fn exec_linux( } } -fn install_network_seccomp_filter() -> std::result::Result<(), SandboxErr> { +fn install_filesystem_landlock_rules_on_current_thread(writable_roots: Vec) -> Result<()> { + let abi = ABI::V5; + let access_rw = AccessFs::from_all(abi); + let access_ro = AccessFs::from_read(abi); + + let mut ruleset = Ruleset::default() + .set_compatibility(CompatLevel::BestEffort) + .handle_access(access_rw)? + .create()? + .add_rules(landlock::path_beneath_rules(&["/"], access_ro))? + .add_rules(landlock::path_beneath_rules(&["/dev/null"], access_rw))? + .set_no_new_privs(true); + + if !writable_roots.is_empty() { + ruleset = ruleset.add_rules(landlock::path_beneath_rules(&writable_roots, access_rw))?; + } + + let status = ruleset.restrict_self()?; + + if status.ruleset == landlock::RulesetStatus::NotEnforced { + return Err(CodexErr::Sandbox(SandboxErr::LandlockRestrict)); + } + + Ok(()) +} + +fn install_network_seccomp_filter_on_current_thread() -> std::result::Result<(), SandboxErr> { // Build rule map. let mut rules: BTreeMap> = BTreeMap::new(); @@ -156,6 +162,7 @@ mod tests_linux { use crate::exec::process_exec_tool_call; use crate::exec::ExecParams; use crate::exec::SandboxType; + use crate::protocol::SandboxPolicy; use std::sync::Arc; use tempfile::NamedTempFile; use tokio::sync::Notify; @@ -172,6 +179,7 @@ mod tests_linux { SandboxType::LinuxSeccomp, writable_roots, Arc::new(Notify::new()), + SandboxPolicy::NetworkAndFileWriteRestricted, ) .await .unwrap(); @@ -238,6 +246,7 @@ mod tests_linux { SandboxType::LinuxSeccomp, &[], Arc::new(Notify::new()), + SandboxPolicy::NetworkRestricted, ) .await; diff --git a/codex-rs/core/src/protocol.rs b/codex-rs/core/src/protocol.rs index d1975ae8..42c8478e 100644 --- a/codex-rs/core/src/protocol.rs +++ b/codex-rs/core/src/protocol.rs @@ -100,6 +100,30 @@ pub enum SandboxPolicy { DangerousNoRestrictions, } +impl SandboxPolicy { + pub fn is_dangerous(&self) -> bool { + match self { + SandboxPolicy::NetworkRestricted => false, + SandboxPolicy::FileWriteRestricted => false, + SandboxPolicy::NetworkAndFileWriteRestricted => false, + SandboxPolicy::DangerousNoRestrictions => true, + } + } + + pub fn is_network_restricted(&self) -> bool { + matches!( + self, + SandboxPolicy::NetworkRestricted | SandboxPolicy::NetworkAndFileWriteRestricted + ) + } + + pub fn is_file_write_restricted(&self) -> bool { + matches!( + self, + SandboxPolicy::FileWriteRestricted | SandboxPolicy::NetworkAndFileWriteRestricted + ) + } +} /// User input #[non_exhaustive] #[derive(Debug, Clone, Deserialize, Serialize)]