From fb0781b9eee6d2da741b984bed9dde95834d909d Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 24 Aug 2026 00:44:13 +0000 Subject: [PATCH] Harden bubblewrap synthetic mount registry isolation (#40302) ## Why The synthetic mount registry must remain private to the sandbox helper. A writable bind overlapping its temporary directory could expose the registry, and a pre-existing registry symlink could redirect bookkeeping into a writable location. ## What changed - Add the registry's read-only bind while constructing filesystem mounts, but only when a writable bind exposes it and before applying deny masks. - Reject a symlinked registry path before registering mounts or starting the sandboxed command. ## Testing Add coverage for symlink rejection and for denied temporary directories under both full-root and minimal filesystem policies. GitOrigin-RevId: 3adff28998f0ef8c8ec26a73e5a1183b4931c0e8 --- codex-rs/linux-sandbox/src/bwrap.rs | 61 ++++++++ codex-rs/linux-sandbox/src/linux_run_main.rs | 30 ++-- .../linux-sandbox/tests/suite/landlock.rs | 131 ++++++++++++++++++ 3 files changed, 204 insertions(+), 18 deletions(-) diff --git a/codex-rs/linux-sandbox/src/bwrap.rs b/codex-rs/linux-sandbox/src/bwrap.rs index 097e689e84..11e8e612af 100644 --- a/codex-rs/linux-sandbox/src/bwrap.rs +++ b/codex-rs/linux-sandbox/src/bwrap.rs @@ -24,6 +24,7 @@ use std::path::Path; use std::path::PathBuf; use std::process::Command; +use crate::linux_run_main::synthetic_mount_registry_root; use codex_protocol::error::CodexErr; use codex_protocol::error::Result; use codex_protocol::permissions::is_protected_metadata_name; @@ -608,6 +609,23 @@ fn create_filesystem_args( for subpath in read_only_subpaths { append_read_only_subpath_args(&mut bwrap_args, &subpath, &allowed_write_paths)?; } + // Protect the registry only where a writable bind exposes it. Apply + // this before deny masks so a denied parent stays hidden, rather than + // trying to recreate the registry beneath an already read-only tmpfs. + let registry_root = synthetic_mount_registry_root(); + let registry_mount = if registry_root.starts_with(mount_root) { + Some(registry_root.as_path()) + } else if mount_root.starts_with(®istry_root) { + Some(mount_root) + } else { + None + }; + if let Some(registry_mount) = registry_mount { + fs::create_dir_all(®istry_root)?; + bwrap_args.args.push("--ro-bind".to_string()); + bwrap_args.args.push(path_to_string(registry_mount)); + bwrap_args.args.push(path_to_string(registry_mount)); + } let mut nested_unreadable_roots: Vec = unreadable_roots .iter() .filter(|path| path.starts_with(root)) @@ -1687,6 +1705,46 @@ mod tests { assert!(message.contains(&real_linked_private_str), "{message}"); } + #[test] + fn registry_protection_precedes_denied_parent_mask() { + let registry_root = synthetic_mount_registry_root(); + let temp_root = registry_root.parent().expect("registry parent"); + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::Root, + }, + access: FileSystemAccessMode::Write, + missing_path_behavior: None, + }, + FileSystemSandboxEntry { + path: AbsolutePathBuf::try_from(temp_root) + .expect("absolute temp root") + .into(), + access: FileSystemAccessMode::Deny, + missing_path_behavior: None, + }, + ]); + + let args = + create_filesystem_args(&policy, Path::new("/"), NO_UNREADABLE_GLOB_SCAN_MAX_DEPTH) + .expect("filesystem args"); + let registry_path = path_to_string(®istry_root); + let temp_path = path_to_string(temp_root); + let registry_bind = args + .args + .windows(3) + .position(|args| args == ["--ro-bind", registry_path.as_str(), registry_path.as_str()]); + let temp_mask = args + .args + .windows(2) + .position(|args| args == ["--tmpfs", temp_path.as_str()]); + assert!( + registry_bind.expect("registry read-only bind") < temp_mask.expect("temp deny mask"), + "registry protection must not reopen denied temp paths" + ); + } + #[test] fn missing_read_only_subpath_uses_empty_file_bind_data() { let temp_dir = TempDir::new().expect("temp dir"); @@ -2053,6 +2111,9 @@ mod tests { "/.codex".to_string(), "--remount-ro".to_string(), "/.codex".to_string(), + "--ro-bind".to_string(), + path_to_string(&synthetic_mount_registry_root()), + path_to_string(&synthetic_mount_registry_root()), // Rebind /dev after the root bind so device nodes remain // writable/usable inside the writable root. "--bind".to_string(), diff --git a/codex-rs/linux-sandbox/src/linux_run_main.rs b/codex-rs/linux-sandbox/src/linux_run_main.rs index abedd97671..b50f19732b 100644 --- a/codex-rs/linux-sandbox/src/linux_run_main.rs +++ b/codex-rs/linux-sandbox/src/linux_run_main.rs @@ -567,7 +567,7 @@ fn run_or_exec_bwrap(bwrap_args: crate::bwrap::BwrapArgs) -> ! { fn run_bwrap_in_child_with_synthetic_mount_cleanup(bwrap_args: crate::bwrap::BwrapArgs) -> ! { let crate::bwrap::BwrapArgs { - mut args, + args, preserved_files, synthetic_mount_targets, protected_create_targets, @@ -576,20 +576,6 @@ fn run_bwrap_in_child_with_synthetic_mount_cleanup(bwrap_args: crate::bwrap::Bwr let synthetic_mount_registrations = register_synthetic_mount_targets(&synthetic_mount_targets); let protected_create_registrations = register_protected_create_targets(&protected_create_targets); - let registry_root = synthetic_mount_registry_root() - .to_string_lossy() - .into_owned(); - let Some(command_separator) = args.iter().position(|arg| arg == "--") else { - panic!("bubblewrap argv is missing command separator '--'"); - }; - args.splice( - command_separator..command_separator, - [ - "--ro-bind".to_string(), - registry_root.clone(), - registry_root, - ], - ); let exec_start_pipe = create_exec_start_pipe(!protected_create_targets.is_empty()); let parent_pid = unsafe { libc::getpid() }; let pid = unsafe { libc::fork() }; @@ -1347,7 +1333,7 @@ fn synthetic_mount_marker_dir(path: &Path) -> PathBuf { synthetic_mount_registry_root().join(format!("{:016x}", hash_path(path))) } -fn synthetic_mount_registry_root() -> PathBuf { +pub(crate) fn synthetic_mount_registry_root() -> PathBuf { static REGISTRY_ROOT: OnceLock = OnceLock::new(); REGISTRY_ROOT @@ -1360,9 +1346,17 @@ fn synthetic_mount_registry_root() -> PathBuf { temp_dir.display() ) }); - temp_dir.join(format!( + let registry_root = temp_dir.join(format!( "codex-bwrap-synthetic-mount-targets-{effective_uid}" - )) + )); + // A registry symlink can redirect bookkeeping into a writable root + // that does not overlap TMPDIR, bypassing its read-only mount. + assert!( + !registry_root.is_symlink(), + "synthetic mount registry must not be a symlink: {}", + registry_root.display() + ); + registry_root }) .clone() } diff --git a/codex-rs/linux-sandbox/tests/suite/landlock.rs b/codex-rs/linux-sandbox/tests/suite/landlock.rs index 794b1927eb..a82022754d 100644 --- a/codex-rs/linux-sandbox/tests/suite/landlock.rs +++ b/codex-rs/linux-sandbox/tests/suite/landlock.rs @@ -730,6 +730,61 @@ async fn sandbox_reports_codex_symlink_build_failure_without_panicking() { ); } +#[tokio::test] +async fn sandbox_rejects_symlinked_synthetic_mount_registry() { + if should_skip_bwrap_tests().await { + eprintln!("skipping bwrap test: bwrap sandbox prerequisites are unavailable"); + return; + } + + let temp = tempfile::tempdir().expect("tempdir"); + let workspace = temp.path().join("workspace"); + let registry_target = workspace.join("registry"); + std::fs::create_dir_all(®istry_target).expect("create registry target"); + let effective_uid = unsafe { libc::geteuid() }; + let registry = temp.path().join(format!( + "codex-bwrap-synthetic-mount-targets-{effective_uid}" + )); + std::os::unix::fs::symlink(®istry_target, ®istry).expect("symlink registry"); + + let cwd = AbsolutePathBuf::try_from(workspace).expect("absolute workspace"); + let permission_profile = PermissionProfile::workspace_write_with( + std::slice::from_ref(&cwd), + NetworkSandboxPolicy::Enabled, + /*exclude_tmpdir_env_var*/ true, + /*exclude_slash_tmp*/ true, + ); + let mut env = create_env_from_core_vars(); + env.insert("TMPDIR".to_string(), temp.path().display().to_string()); + let output = expect_denied( + run_cmd_result_with_permission_profile_for_cwd( + &["sh", "-c", "touch registry/forged-marker"], + cwd, + permission_profile, + env, + LONG_TIMEOUT_MS, + /*use_legacy_landlock*/ false, + ) + .await, + "a symlinked registry must not expose writable bookkeeping", + ); + assert!( + output + .stderr + .text + .contains("synthetic mount registry must not be a symlink"), + "stderr: {}", + output.stderr.text + ); + assert_eq!( + std::fs::read_dir(registry_target) + .expect("read registry target") + .count(), + 0, + "the registry symlink must be rejected before registration or command execution" + ); +} + #[tokio::test] async fn sandbox_keeps_parent_repo_discovery_while_blocking_child_metadata() { if should_skip_bwrap_tests().await { @@ -958,6 +1013,82 @@ async fn sandbox_blocks_explicit_split_policy_carveouts_under_bwrap() { assert_ne!(output.exit_code, 0); } +#[tokio::test] +async fn sandbox_starts_with_denied_tmp_without_exposing_registry() { + if should_skip_bwrap_tests().await { + eprintln!("skipping bwrap test: bwrap sandbox prerequisites are unavailable"); + return; + } + + let temp = tempfile::tempdir().expect("tempdir"); + std::fs::write(temp.path().join("AGENTS.md"), "project instructions\n") + .expect("write instructions"); + let cwd = AbsolutePathBuf::try_from(temp.path()).expect("absolute workspace"); + let sandbox_helper = codex_linux_sandbox_exe(); + let helper_dir = AbsolutePathBuf::try_from(sandbox_helper.parent().expect("helper parent")) + .expect("absolute helper directory"); + + for tmp_root in [PathBuf::from("/tmp"), temp.path().join("denied-tmp")] { + std::fs::create_dir_all(&tmp_root).expect("create temp root"); + let secret = NamedTempFile::new_in(&tmp_root).expect("denied file"); + std::fs::write(secret.path(), "private").expect("write denied file"); + for read_root in [FileSystemSpecialPath::Root, FileSystemSpecialPath::Minimal] { + let denied_path = if tmp_root == std::path::Path::new("/tmp") { + FileSystemPath::Special { + value: FileSystemSpecialPath::SlashTmp, + } + } else { + AbsolutePathBuf::try_from(tmp_root.as_path()) + .expect("absolute temp root") + .into() + }; + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry::new( + FileSystemPath::Special { value: read_root }, + FileSystemAccessMode::Read, + ), + FileSystemSandboxEntry::new(helper_dir.clone().into(), FileSystemAccessMode::Read), + FileSystemSandboxEntry::new(cwd.clone().into(), FileSystemAccessMode::Write), + FileSystemSandboxEntry::new(denied_path, FileSystemAccessMode::Deny), + ]); + let mut env = create_env_from_core_vars(); + env.insert("TMPDIR".to_string(), tmp_root.display().to_string()); + env.insert( + "DENIED_SECRET".to_string(), + secret.path().display().to_string(), + ); + let output = run_cmd_result_with_permission_profile_for_cwd( + &[ + "sh", + "-c", + r#"set -e +cat AGENTS.md +test ! -r "$DENIED_SECRET" +if printf modified > "$DENIED_SECRET" 2>/dev/null; then exit 1; fi +registry="$TMPDIR/codex-bwrap-synthetic-mount-targets-$(id -u)" +test ! -e "$registry" +"#, + ], + cwd.clone(), + PermissionProfile::from_runtime_permissions(&policy, NetworkSandboxPolicy::Enabled), + env, + LONG_TIMEOUT_MS, + /*use_legacy_landlock*/ false, + ) + .await + .expect("sandbox should start with denied temp directory"); + assert_eq!( + (output.exit_code, output.stdout.text.as_str()), + (0, "project instructions\n"), + "stderr: {}", + output.stderr.text + ); + assert_eq!(std::fs::read_to_string(secret.path()).unwrap(), "private"); + assert!(!temp.path().join(".git").exists()); + } + } +} + #[tokio::test] async fn sandbox_reenables_writable_subpaths_under_unreadable_parents() { if should_skip_bwrap_tests().await {