From e7eccd035affed5767e502099970ce0be9d8fb26 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Sat, 7 Mar 2026 22:14:56 -0800 Subject: [PATCH] fix(linux-sandbox): enforce root read carveouts in bwrap --- codex-rs/linux-sandbox/src/bwrap.rs | 182 +++++++++++++++--- codex-rs/linux-sandbox/src/linux_run_main.rs | 28 +-- .../linux-sandbox/src/linux_run_main_tests.rs | 12 +- codex-rs/linux-sandbox/src/vendored_bwrap.rs | 21 +- .../linux-sandbox/tests/suite/landlock.rs | 53 +++++ 5 files changed, 250 insertions(+), 46 deletions(-) diff --git a/codex-rs/linux-sandbox/src/bwrap.rs b/codex-rs/linux-sandbox/src/bwrap.rs index 8abf150056..f96dd34918 100644 --- a/codex-rs/linux-sandbox/src/bwrap.rs +++ b/codex-rs/linux-sandbox/src/bwrap.rs @@ -10,6 +10,8 @@ //! - seccomp + `PR_SET_NO_NEW_PRIVS` applied in-process, and //! - bubblewrap used to construct the filesystem view before exec. use std::collections::BTreeSet; +use std::fs::File; +use std::os::fd::AsRawFd; use std::path::Path; use std::path::PathBuf; @@ -77,6 +79,12 @@ impl BwrapNetworkMode { } } +#[derive(Debug)] +pub(crate) struct BwrapArgs { + pub args: Vec, + pub preserved_files: Vec, +} + /// Wrap a command with bubblewrap so the filesystem is read-only by default, /// with explicit writable roots and read-only subpaths layered afterward. /// @@ -84,15 +92,18 @@ impl BwrapNetworkMode { /// returns `command` unchanged so we avoid unnecessary sandboxing overhead. /// If network isolation is requested, we still wrap with bubblewrap so network /// namespace restrictions apply while preserving full filesystem access. -pub(crate) fn create_bwrap_command_args_for_policy( +pub(crate) fn create_bwrap_command_args( command: Vec, file_system_sandbox_policy: &FileSystemSandboxPolicy, cwd: &Path, options: BwrapOptions, -) -> Result> { +) -> Result { if file_system_sandbox_policy.has_full_disk_write_access() { return if options.network_mode == BwrapNetworkMode::FullAccess { - Ok(command) + Ok(BwrapArgs { + args: command, + preserved_files: Vec::new(), + }) } else { Ok(create_bwrap_flags_full_filesystem(command, options)) }; @@ -101,7 +112,7 @@ pub(crate) fn create_bwrap_command_args_for_policy( create_bwrap_flags(command, file_system_sandbox_policy, cwd, options) } -fn create_bwrap_flags_full_filesystem(command: Vec, options: BwrapOptions) -> Vec { +fn create_bwrap_flags_full_filesystem(command: Vec, options: BwrapOptions) -> BwrapArgs { let mut args = vec![ "--new-session".to_string(), "--die-with-parent".to_string(), @@ -122,7 +133,10 @@ fn create_bwrap_flags_full_filesystem(command: Vec, options: BwrapOption } args.push("--".to_string()); args.extend(command); - args + BwrapArgs { + args, + preserved_files: Vec::new(), + } } /// Build the bubblewrap flags (everything after `argv[0]`). @@ -131,11 +145,15 @@ fn create_bwrap_flags( file_system_sandbox_policy: &FileSystemSandboxPolicy, cwd: &Path, options: BwrapOptions, -) -> Result> { +) -> Result { + let BwrapArgs { + args: filesystem_args, + preserved_files, + } = create_filesystem_args(file_system_sandbox_policy, cwd)?; let mut args = Vec::new(); args.push("--new-session".to_string()); args.push("--die-with-parent".to_string()); - args.extend(create_filesystem_args(file_system_sandbox_policy, cwd)?); + args.extend(filesystem_args); // Request a user namespace explicitly rather than relying on bubblewrap's // auto-enable behavior, which is skipped when the caller runs as uid 0. args.push("--unshare-user".to_string()); @@ -151,25 +169,32 @@ fn create_bwrap_flags( } args.push("--".to_string()); args.extend(command); - Ok(args) + Ok(BwrapArgs { + args, + preserved_files, + }) } /// Build the bubblewrap filesystem mounts for a given filesystem policy. /// /// The mount order is important: -/// 1. Full-read policies use `--ro-bind / /`; restricted-read policies start -/// from `--tmpfs /` and layer scoped `--ro-bind` mounts. +/// 1. Full-read policies, and restricted policies that explicitly read `/`, +/// use `--ro-bind / /`; other restricted-read policies start from +/// `--tmpfs /` and layer scoped `--ro-bind` mounts. /// 2. `--dev /dev` mounts a minimal writable `/dev` with standard device nodes /// (including `/dev/urandom`) even under a read-only root. /// 3. `--bind ` re-enables writes for allowed roots, including /// writable subpaths under `/dev` (for example, `/dev/shm`). /// 4. `--ro-bind ` re-applies read-only protections under /// those writable roots so protected subpaths win. +/// 5. Explicit unreadable roots are masked last so deny carveouts still win +/// even when the readable baseline includes `/`. fn create_filesystem_args( file_system_sandbox_policy: &FileSystemSandboxPolicy, cwd: &Path, -) -> Result> { +) -> Result { let writable_roots = file_system_sandbox_policy.get_writable_roots_with_cwd(cwd); + let unreadable_roots = file_system_sandbox_policy.get_unreadable_roots_with_cwd(cwd); ensure_mount_targets_exist(&writable_roots)?; let mut args = if file_system_sandbox_policy.has_full_disk_read_access() { @@ -210,7 +235,8 @@ fn create_filesystem_args( } // A restricted policy can still explicitly request `/`, which is - // semantically equivalent to broad read access. + // the broad read baseline. Explicit unreadable carveouts are + // re-applied later. if readable_roots.iter().any(|root| root == Path::new("/")) { args = vec![ "--ro-bind".to_string(), @@ -232,6 +258,7 @@ fn create_filesystem_args( args }; + let mut preserved_files = Vec::new(); for writable_root in &writable_roots { let root = writable_root.root.as_path(); @@ -275,7 +302,34 @@ fn create_filesystem_args( } } - Ok(args) + if !unreadable_roots.is_empty() { + let null_file = File::open("/dev/null")?; + let null_fd = null_file.as_raw_fd().to_string(); + for unreadable_root in unreadable_roots { + let unreadable_root = unreadable_root.as_path(); + if unreadable_root.is_dir() { + args.push("--perms".to_string()); + args.push("000".to_string()); + args.push("--tmpfs".to_string()); + args.push(path_to_string(unreadable_root)); + args.push("--remount-ro".to_string()); + args.push(path_to_string(unreadable_root)); + continue; + } + + args.push("--perms".to_string()); + args.push("000".to_string()); + args.push("--ro-bind-data".to_string()); + args.push(null_fd.clone()); + args.push(path_to_string(unreadable_root)); + } + preserved_files.push(null_file); + } + + Ok(BwrapArgs { + args, + preserved_files, + }) } /// Collect unique read-only subpaths across all writable roots. @@ -394,6 +448,7 @@ mod tests { use codex_protocol::protocol::FileSystemPath; use codex_protocol::protocol::FileSystemSandboxEntry; use codex_protocol::protocol::FileSystemSandboxPolicy; + use codex_protocol::protocol::FileSystemSpecialPath; use codex_protocol::protocol::ReadOnlyAccess; use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; @@ -403,7 +458,7 @@ mod tests { #[test] fn full_disk_write_full_network_returns_unwrapped_command() { let command = vec!["/bin/true".to_string()]; - let args = create_bwrap_command_args_for_policy( + let args = create_bwrap_command_args( command.clone(), &FileSystemSandboxPolicy::from(&SandboxPolicy::DangerFullAccess), Path::new("/"), @@ -414,13 +469,13 @@ mod tests { ) .expect("create bwrap args"); - assert_eq!(args, command); + assert_eq!(args.args, command); } #[test] fn full_disk_write_proxy_only_keeps_full_filesystem_but_unshares_network() { let command = vec!["/bin/true".to_string()]; - let args = create_bwrap_command_args_for_policy( + let args = create_bwrap_command_args( command, &FileSystemSandboxPolicy::from(&SandboxPolicy::DangerFullAccess), Path::new("/"), @@ -432,7 +487,7 @@ mod tests { .expect("create bwrap args"); assert_eq!( - args, + args.args, vec![ "--new-session".to_string(), "--die-with-parent".to_string(), @@ -466,7 +521,7 @@ mod tests { ) .expect("bwrap fs args"); assert_eq!( - args, + args.args, vec![ "--ro-bind".to_string(), "/".to_string(), @@ -503,10 +558,10 @@ mod tests { let args = create_filesystem_args(&FileSystemSandboxPolicy::from(&policy), temp_dir.path()) .expect("filesystem args"); - assert_eq!(args[0..4], ["--tmpfs", "/", "--dev", "/dev"]); + assert_eq!(args.args[0..4], ["--tmpfs", "/", "--dev", "/dev"]); let readable_root_str = path_to_string(&readable_root); - assert!(args.windows(3).any(|window| { + assert!(args.args.windows(3).any(|window| { window == [ "--ro-bind", @@ -533,11 +588,15 @@ mod tests { let args = create_filesystem_args(&FileSystemSandboxPolicy::from(&policy), temp_dir.path()) .expect("filesystem args"); - assert!(args.starts_with(&["--tmpfs".to_string(), "/".to_string()])); + assert!( + args.args + .starts_with(&["--tmpfs".to_string(), "/".to_string()]) + ); if Path::new("/usr").exists() { assert!( - args.windows(3) + args.args + .windows(3) .any(|window| window == ["--ro-bind", "/usr", "/usr"]) ); } @@ -571,7 +630,7 @@ mod tests { let writable_root_str = path_to_string(writable_root.as_path()); let blocked_str = path_to_string(blocked.as_path()); - assert!(args.windows(3).any(|window| { + assert!(args.args.windows(3).any(|window| { window == [ "--bind", @@ -580,9 +639,84 @@ mod tests { ] })); assert!( - args.windows(3).any(|window| { + args.args.windows(3).any(|window| { window == ["--ro-bind", blocked_str.as_str(), blocked_str.as_str()] }) ); } + + #[test] + fn split_policy_masks_root_read_directory_carveouts() { + let temp_dir = TempDir::new().expect("temp dir"); + let blocked = temp_dir.path().join("blocked"); + std::fs::create_dir_all(&blocked).expect("create blocked dir"); + let blocked = AbsolutePathBuf::from_absolute_path(&blocked).expect("absolute blocked dir"); + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: blocked.clone(), + }, + access: FileSystemAccessMode::None, + }, + ]); + + let args = create_filesystem_args(&policy, temp_dir.path()).expect("filesystem args"); + let blocked_str = path_to_string(blocked.as_path()); + + assert!( + args.args + .windows(3) + .any(|window| window == ["--ro-bind", "/", "/"]) + ); + assert!( + args.args + .windows(4) + .any(|window| { window == ["--perms", "000", "--tmpfs", blocked_str.as_str()] }) + ); + assert!( + args.args + .windows(2) + .any(|window| window == ["--remount-ro", blocked_str.as_str()]) + ); + } + + #[test] + fn split_policy_masks_root_read_file_carveouts() { + let temp_dir = TempDir::new().expect("temp dir"); + let blocked_file = temp_dir.path().join("blocked.txt"); + std::fs::write(&blocked_file, "secret").expect("create blocked file"); + let blocked_file = + AbsolutePathBuf::from_absolute_path(&blocked_file).expect("absolute blocked file"); + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: blocked_file.clone(), + }, + access: FileSystemAccessMode::None, + }, + ]); + + let args = create_filesystem_args(&policy, temp_dir.path()).expect("filesystem args"); + let blocked_file_str = path_to_string(blocked_file.as_path()); + + assert_eq!(args.preserved_files.len(), 1); + assert!(args.args.windows(5).any(|window| { + window[0] == "--perms" + && window[1] == "000" + && window[2] == "--ro-bind-data" + && window[4] == blocked_file_str + })); + } } diff --git a/codex-rs/linux-sandbox/src/linux_run_main.rs b/codex-rs/linux-sandbox/src/linux_run_main.rs index e4569dfdbf..2f1b1b6b3c 100644 --- a/codex-rs/linux-sandbox/src/linux_run_main.rs +++ b/codex-rs/linux-sandbox/src/linux_run_main.rs @@ -8,7 +8,7 @@ use std::path::PathBuf; use crate::bwrap::BwrapNetworkMode; use crate::bwrap::BwrapOptions; -use crate::bwrap::create_bwrap_command_args_for_policy; +use crate::bwrap::create_bwrap_command_args; use crate::landlock::apply_sandbox_policy_to_current_thread; use crate::proxy_routing::activate_proxy_routes_in_netns; use crate::proxy_routing::prepare_host_proxy_route_spec; @@ -285,13 +285,13 @@ fn run_bwrap_with_proc_fallback( mount_proc, network_mode, }; - let argv = build_bwrap_argv( + let bwrap_args = build_bwrap_argv( inner, file_system_sandbox_policy, sandbox_policy_cwd, options, ); - exec_vendored_bwrap(argv); + exec_vendored_bwrap(bwrap_args.args, bwrap_args.preserved_files); } fn bwrap_network_mode( @@ -312,8 +312,8 @@ fn build_bwrap_argv( file_system_sandbox_policy: &FileSystemSandboxPolicy, sandbox_policy_cwd: &Path, options: BwrapOptions, -) -> Vec { - let mut args = create_bwrap_command_args_for_policy( +) -> crate::bwrap::BwrapArgs { + let mut bwrap_args = create_bwrap_command_args( inner, file_system_sandbox_policy, sandbox_policy_cwd, @@ -321,18 +321,22 @@ fn build_bwrap_argv( ) .unwrap_or_else(|err| panic!("error building bubblewrap command: {err:?}")); - let command_separator_index = args + let command_separator_index = bwrap_args + .args .iter() .position(|arg| arg == "--") .unwrap_or_else(|| panic!("bubblewrap argv is missing command separator '--'")); - args.splice( + bwrap_args.args.splice( command_separator_index..command_separator_index, ["--argv0".to_string(), "codex-linux-sandbox".to_string()], ); let mut argv = vec!["bwrap".to_string()]; - argv.extend(args); - argv + argv.extend(bwrap_args.args); + crate::bwrap::BwrapArgs { + args: argv, + preserved_files: bwrap_args.preserved_files, + } } fn preflight_proc_mount_support( @@ -350,7 +354,7 @@ fn build_preflight_bwrap_argv( sandbox_policy_cwd: &Path, file_system_sandbox_policy: &FileSystemSandboxPolicy, network_mode: BwrapNetworkMode, -) -> Vec { +) -> crate::bwrap::BwrapArgs { let preflight_command = vec![resolve_true_command()]; build_bwrap_argv( preflight_command, @@ -383,7 +387,7 @@ fn resolve_true_command() -> String { /// - We capture stderr from that preflight to match known mount-failure text. /// We do not stream it because this is a one-shot probe with a trivial /// command, and reads are bounded to a fixed max size. -fn run_bwrap_in_child_capture_stderr(argv: Vec) -> String { +fn run_bwrap_in_child_capture_stderr(bwrap_args: crate::bwrap::BwrapArgs) -> String { const MAX_PREFLIGHT_STDERR_BYTES: u64 = 64 * 1024; let mut pipe_fds = [0; 2]; @@ -412,7 +416,7 @@ fn run_bwrap_in_child_capture_stderr(argv: Vec) -> String { close_fd_or_panic(write_fd, "close write end in bubblewrap child"); } - let exit_code = run_vendored_bwrap_main(&argv); + let exit_code = run_vendored_bwrap_main(&bwrap_args.args, &bwrap_args.preserved_files); std::process::exit(exit_code); } diff --git a/codex-rs/linux-sandbox/src/linux_run_main_tests.rs b/codex-rs/linux-sandbox/src/linux_run_main_tests.rs index 12817a5470..a1adf65b1d 100644 --- a/codex-rs/linux-sandbox/src/linux_run_main_tests.rs +++ b/codex-rs/linux-sandbox/src/linux_run_main_tests.rs @@ -44,7 +44,8 @@ fn inserts_bwrap_argv0_before_command_separator() { mount_proc: true, network_mode: BwrapNetworkMode::FullAccess, }, - ); + ) + .args; assert_eq!( argv, vec![ @@ -79,7 +80,8 @@ fn inserts_unshare_net_when_network_isolation_requested() { mount_proc: true, network_mode: BwrapNetworkMode::Isolated, }, - ); + ) + .args; assert!(argv.contains(&"--unshare-net".to_string())); } @@ -94,7 +96,8 @@ fn inserts_unshare_net_when_proxy_only_network_mode_requested() { mount_proc: true, network_mode: BwrapNetworkMode::ProxyOnly, }, - ); + ) + .args; assert!(argv.contains(&"--unshare-net".to_string())); } @@ -111,7 +114,8 @@ fn managed_proxy_preflight_argv_is_wrapped_for_full_access_policy() { Path::new("/"), &FileSystemSandboxPolicy::from(&SandboxPolicy::DangerFullAccess), mode, - ); + ) + .args; assert!(argv.iter().any(|arg| arg == "--")); } diff --git a/codex-rs/linux-sandbox/src/vendored_bwrap.rs b/codex-rs/linux-sandbox/src/vendored_bwrap.rs index 3150a1d861..5385522687 100644 --- a/codex-rs/linux-sandbox/src/vendored_bwrap.rs +++ b/codex-rs/linux-sandbox/src/vendored_bwrap.rs @@ -6,6 +6,7 @@ #[cfg(vendored_bwrap_available)] mod imp { use std::ffi::CString; + use std::fs::File; use std::os::raw::c_char; unsafe extern "C" { @@ -27,7 +28,10 @@ mod imp { /// /// On success, bubblewrap will `execve` into the target program and this /// function will never return. A return value therefore implies failure. - pub(crate) fn run_vendored_bwrap_main(argv: &[String]) -> libc::c_int { + pub(crate) fn run_vendored_bwrap_main( + argv: &[String], + _preserved_files: &[File], + ) -> libc::c_int { let cstrings = argv_to_cstrings(argv); let mut argv_ptrs: Vec<*const c_char> = cstrings.iter().map(|arg| arg.as_ptr()).collect(); @@ -39,16 +43,21 @@ mod imp { } /// Execute the build-time bubblewrap `main` function with the given argv. - pub(crate) fn exec_vendored_bwrap(argv: Vec) -> ! { - let exit_code = run_vendored_bwrap_main(&argv); + pub(crate) fn exec_vendored_bwrap(argv: Vec, preserved_files: Vec) -> ! { + let exit_code = run_vendored_bwrap_main(&argv, &preserved_files); std::process::exit(exit_code); } } #[cfg(not(vendored_bwrap_available))] mod imp { + use std::fs::File; + /// Panics with a clear error when the build-time bwrap path is not enabled. - pub(crate) fn run_vendored_bwrap_main(_argv: &[String]) -> libc::c_int { + pub(crate) fn run_vendored_bwrap_main( + _argv: &[String], + _preserved_files: &[File], + ) -> libc::c_int { panic!( r#"build-time bubblewrap is not available in this build. codex-linux-sandbox should always compile vendored bubblewrap on Linux targets. @@ -60,8 +69,8 @@ Notes: } /// Panics with a clear error when the build-time bwrap path is not enabled. - pub(crate) fn exec_vendored_bwrap(_argv: Vec) -> ! { - let _ = run_vendored_bwrap_main(&[]); + pub(crate) fn exec_vendored_bwrap(_argv: Vec, _preserved_files: Vec) -> ! { + let _ = run_vendored_bwrap_main(&[], &[]); unreachable!("run_vendored_bwrap_main should always panic in this configuration") } } diff --git a/codex-rs/linux-sandbox/tests/suite/landlock.rs b/codex-rs/linux-sandbox/tests/suite/landlock.rs index 153efc3128..8a6eac6c12 100644 --- a/codex-rs/linux-sandbox/tests/suite/landlock.rs +++ b/codex-rs/linux-sandbox/tests/suite/landlock.rs @@ -13,7 +13,9 @@ use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; use codex_protocol::permissions::FileSystemSandboxEntry; use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_protocol::permissions::FileSystemSpecialPath; use codex_protocol::permissions::NetworkSandboxPolicy; +use codex_protocol::protocol::ReadOnlyAccess; use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; @@ -556,6 +558,57 @@ async fn sandbox_blocks_explicit_split_policy_carveouts_under_bwrap() { assert_ne!(output.exit_code, 0); } +#[tokio::test] +async fn sandbox_blocks_root_read_carveouts_under_bwrap() { + if should_skip_bwrap_tests().await { + eprintln!("skipping bwrap test: bwrap sandbox prerequisites are unavailable"); + return; + } + + let tmpdir = tempfile::tempdir().expect("tempdir"); + let blocked = tmpdir.path().join("blocked"); + std::fs::create_dir_all(&blocked).expect("create blocked dir"); + let blocked_target = blocked.join("secret.txt"); + std::fs::write(&blocked_target, "secret").expect("seed blocked file"); + + let sandbox_policy = SandboxPolicy::ReadOnly { + access: ReadOnlyAccess::FullAccess, + network_access: true, + }; + let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: AbsolutePathBuf::try_from(blocked.as_path()).expect("absolute blocked dir"), + }, + access: FileSystemAccessMode::None, + }, + ]); + let output = expect_denied( + run_cmd_result_with_policies( + &[ + "bash", + "-lc", + &format!("cat {}", blocked_target.to_string_lossy()), + ], + sandbox_policy, + file_system_sandbox_policy, + NetworkSandboxPolicy::Enabled, + LONG_TIMEOUT_MS, + true, + ) + .await, + "root-read carveout should be denied under bubblewrap", + ); + + assert_ne!(output.exit_code, 0); +} + #[tokio::test] async fn sandbox_blocks_ssh() { // Force ssh to attempt a real TCP connection but fail quickly. `BatchMode`