From 6cc2ba8a9567e3083531283f923127b86a6c5908 Mon Sep 17 00:00:00 2001 From: "Adam Perry @ OpenAI" Date: Wed, 19 Aug 2026 03:57:32 +0000 Subject: [PATCH] Support FD mounts with older system Bubblewrap versions (#39404) ## Why System Bubblewrap installations that lack `--ro-bind-fd` cannot directly create the descriptor-backed read-only mounts used by the Linux sandbox. ## What changed - Detect `--ro-bind-fd` support when probing system Bubblewrap. - On older versions, translate descriptor-backed mounts to `/proc/self/fd` read-only binds and pass their descriptors and destinations to the trusted inner sandbox stage for verification. - Reject malformed, duplicate, mismatched, or symlink-substituted mounts, and close inherited descriptors before running sandboxed code. ## Testing Added unit coverage for capability detection, legacy argument translation, invalid mount rejection, inode verification, and descriptor closure. GitOrigin-RevId: dfc0a457b572cdd9aed093fb5abf57360e9724a2 --- codex-rs/linux-sandbox/src/fd_mount.rs | 91 +++++++ codex-rs/linux-sandbox/src/fd_mount_tests.rs | 108 +++++++++ codex-rs/linux-sandbox/src/launcher.rs | 235 +++++++++++++++++++ codex-rs/linux-sandbox/src/lib.rs | 2 + codex-rs/linux-sandbox/src/linux_run_main.rs | 12 + 5 files changed, 448 insertions(+) create mode 100644 codex-rs/linux-sandbox/src/fd_mount.rs create mode 100644 codex-rs/linux-sandbox/src/fd_mount_tests.rs diff --git a/codex-rs/linux-sandbox/src/fd_mount.rs b/codex-rs/linux-sandbox/src/fd_mount.rs new file mode 100644 index 0000000000..c4a8a638a6 --- /dev/null +++ b/codex-rs/linux-sandbox/src/fd_mount.rs @@ -0,0 +1,91 @@ +//! Authenticate descriptor-backed mounts before sandboxed code can inherit them. + +use std::collections::HashSet; +use std::fs; +use std::fs::File; +use std::io; +use std::os::fd::AsRawFd; +use std::os::fd::FromRawFd; +use std::os::unix::fs::MetadataExt; +use std::path::Path; + +pub(crate) fn verify_fd_mounts(mounts: &[String]) -> io::Result<()> { + let mut claimed_descriptors = HashSet::with_capacity(mounts.len()); + + for mount in mounts { + let (descriptor, destination) = mount.split_once(':').ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidInput, + format!("descriptor-backed mount must contain FD:DEST: {mount}"), + ) + })?; + let descriptor = descriptor.parse::().map_err(|_| { + io::Error::new( + io::ErrorKind::InvalidInput, + format!("descriptor-backed mount has an invalid descriptor: {mount}"), + ) + })?; + if descriptor <= libc::STDERR_FILENO { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!("descriptor-backed mount cannot use a standard descriptor: {descriptor}"), + )); + } + if !claimed_descriptors.insert(descriptor) { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!("descriptor-backed mount reuses descriptor {descriptor}"), + )); + } + + // The launcher transfers each distinct descriptor exactly once across + // bubblewrap, so first confirm it is live before assuming ownership. + let flags = unsafe { libc::fcntl(descriptor, libc::F_GETFD) }; + if flags < 0 { + return Err(io::Error::last_os_error()); + } + // SAFETY: the launcher transferred this live descriptor to this stage, + // and the set above prevents claiming the same descriptor twice. + let file = unsafe { File::from_raw_fd(descriptor) }; + let result = + unsafe { libc::fcntl(file.as_raw_fd(), libc::F_SETFD, flags | libc::FD_CLOEXEC) }; + if result < 0 { + return Err(io::Error::last_os_error()); + } + + let destination = Path::new(destination); + if !destination.is_absolute() { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!( + "descriptor-backed mount destination must be absolute: {}", + destination.display() + ), + )); + } + + let descriptor_metadata = file.metadata()?; + let destination_metadata = fs::symlink_metadata(destination)?; + if (descriptor_metadata.dev(), descriptor_metadata.ino()) + != (destination_metadata.dev(), destination_metadata.ino()) + { + return Err(io::Error::new( + io::ErrorKind::PermissionDenied, + format!( + "descriptor-backed mount does not match its destination: {}", + destination.display() + ), + )); + } + + // Closing immediately prevents a writable host directory descriptor + // from reaching bridge workers or the sandboxed command. + drop(file); + } + + Ok(()) +} + +#[cfg(test)] +#[path = "fd_mount_tests.rs"] +mod tests; diff --git a/codex-rs/linux-sandbox/src/fd_mount_tests.rs b/codex-rs/linux-sandbox/src/fd_mount_tests.rs new file mode 100644 index 0000000000..fe0bf7d7d5 --- /dev/null +++ b/codex-rs/linux-sandbox/src/fd_mount_tests.rs @@ -0,0 +1,108 @@ +use super::verify_fd_mounts; +use pretty_assertions::assert_eq; +use std::fs::File; +use std::os::fd::IntoRawFd; + +/// A matching mount authenticates its original inode and closes the inherited descriptor. +#[test] +fn matching_mount_closes_inherited_descriptor() { + let root = tempfile::tempdir().expect("temporary directory should be created"); + let descriptor = File::open(root.path()) + .expect("directory descriptor should open") + .into_raw_fd(); + let marker = format!("{descriptor}:{}", root.path().display()); + + verify_fd_mounts(&[marker]).expect("matching mount should verify"); + + assert_eq!(unsafe { libc::fcntl(descriptor, libc::F_GETFD) }, -1); + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::EBADF) + ); +} + +/// A swapped destination fails closed without leaking the original descriptor. +#[test] +fn mismatched_mount_closes_inherited_descriptor() { + let source = tempfile::tempdir().expect("source directory should be created"); + let destination = tempfile::tempdir().expect("destination directory should be created"); + let descriptor = File::open(source.path()) + .expect("source directory descriptor should open") + .into_raw_fd(); + let marker = format!("{descriptor}:{}", destination.path().display()); + + let error = verify_fd_mounts(&[marker]).expect_err("different inodes must be rejected"); + + assert_eq!(error.kind(), std::io::ErrorKind::PermissionDenied); + assert_eq!(unsafe { libc::fcntl(descriptor, libc::F_GETFD) }, -1); + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::EBADF) + ); +} + +/// A symlink to the original inode is not itself the authenticated mount. +#[test] +fn symlinked_mount_destination_closes_inherited_descriptor() { + let root = tempfile::tempdir().expect("temporary directory should be created"); + let source = root.path().join("source"); + let destination = root.path().join("destination"); + std::fs::create_dir(&source).expect("source directory should be created"); + std::os::unix::fs::symlink(&source, &destination) + .expect("mount destination symlink should be created"); + let descriptor = File::open(&source) + .expect("source directory descriptor should open") + .into_raw_fd(); + let marker = format!("{descriptor}:{}", destination.display()); + + let error = verify_fd_mounts(&[marker]).expect_err("symlinked mounts must be rejected"); + + assert_eq!(error.kind(), std::io::ErrorKind::PermissionDenied); + assert_eq!(unsafe { libc::fcntl(descriptor, libc::F_GETFD) }, -1); + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::EBADF) + ); +} + +/// Malformed mount markers cannot claim standard streams or relative destinations. +#[test] +fn malformed_mount_markers_are_rejected() { + for marker in [ + "missing-separator", + "invalid:/tmp", + "0:/tmp", + "1:/tmp", + "2:/tmp", + ] { + let error = verify_fd_mounts(&[marker.to_string()]) + .expect_err("malformed mount marker must be rejected"); + assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput); + } + + let root = tempfile::tempdir().expect("temporary directory should be created"); + let descriptor = File::open(root.path()) + .expect("directory descriptor should open") + .into_raw_fd(); + let error = verify_fd_mounts(&[format!("{descriptor}:relative")]) + .expect_err("relative mount destinations must be rejected"); + + assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!(unsafe { libc::fcntl(descriptor, libc::F_GETFD) }, -1); +} + +/// A transferred descriptor can be consumed only once even if a marker is repeated. +#[test] +fn duplicate_mount_descriptors_are_rejected() { + let root = tempfile::tempdir().expect("temporary directory should be created"); + let descriptor = File::open(root.path()) + .expect("directory descriptor should open") + .into_raw_fd(); + let marker = format!("{descriptor}:{}", root.path().display()); + + let error = verify_fd_mounts(&[marker.clone(), marker]) + .expect_err("a mount descriptor must not be consumed twice"); + + assert_eq!(error.kind(), std::io::ErrorKind::InvalidInput); + assert_eq!(unsafe { libc::fcntl(descriptor, libc::F_GETFD) }, -1); +} diff --git a/codex-rs/linux-sandbox/src/launcher.rs b/codex-rs/linux-sandbox/src/launcher.rs index 8f491a37c6..d4e9b521bf 100644 --- a/codex-rs/linux-sandbox/src/launcher.rs +++ b/codex-rs/linux-sandbox/src/launcher.rs @@ -25,12 +25,14 @@ enum BubblewrapLauncher { struct SystemBwrapLauncher { program: AbsolutePathBuf, supports_argv0: bool, + supports_ro_bind_fd: bool, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] struct SystemBwrapCapabilities { supports_argv0: bool, supports_perms: bool, + supports_ro_bind_fd: bool, } pub(crate) fn exec_bwrap(mut argv: Vec, preserved_files: Vec) -> ! { @@ -38,6 +40,10 @@ pub(crate) fn exec_bwrap(mut argv: Vec, preserved_files: Vec) -> ! match preferred_bwrap_launcher() { BubblewrapLauncher::System(launcher) => { + if !launcher.supports_ro_bind_fd { + translate_legacy_bwrap_fd_mounts(&mut argv) + .unwrap_or_else(|error| panic!("invalid legacy bubblewrap fd mount: {error}")); + } exec_system_bwrap(&launcher.program, argv, preserved_files) } BubblewrapLauncher::Bundled(launcher) => launcher.exec(argv, preserved_files), @@ -50,6 +56,73 @@ pub(crate) fn exec_bwrap(mut argv: Vec, preserved_files: Vec) -> ! } } +fn translate_legacy_bwrap_fd_mounts(argv: &mut Vec) -> Result<(), String> { + let command_separator = argv + .iter() + .position(|argument| argument == "--") + .ok_or_else(|| "bubblewrap argv is missing the command separator '--'".to_string())?; + let mut verification_args = Vec::new(); + let mut verified_fds = Vec::new(); + let mut argument_index = 0; + + while argument_index < command_separator { + if argv[argument_index] != "--ro-bind-fd" { + argument_index += 1; + continue; + } + + let fd_argument = argv + .get(argument_index + 1) + .filter(|_| argument_index + 1 < command_separator) + .ok_or_else(|| "--ro-bind-fd is missing its file descriptor".to_string())?; + let fd = fd_argument + .parse::() + .map_err(|_| format!("invalid --ro-bind-fd file descriptor: {fd_argument}"))?; + if fd <= libc::STDERR_FILENO { + return Err(format!( + "--ro-bind-fd file descriptor must not use standard descriptors: {fd}" + )); + } + if verified_fds.contains(&fd) { + return Err(format!("duplicate --ro-bind-fd file descriptor: {fd}")); + } + + let destination = argv + .get(argument_index + 2) + .filter(|_| argument_index + 2 < command_separator) + .ok_or_else(|| "--ro-bind-fd is missing its mount destination".to_string())?; + if !Path::new(destination).is_absolute() { + return Err(format!( + "--ro-bind-fd mount destination must be absolute: {destination}" + )); + } + + verification_args.push("--verify-fd-mount".to_string()); + verification_args.push(format!("{fd}:{destination}")); + verified_fds.push(fd); + argv[argument_index] = "--ro-bind".to_string(); + argv[argument_index + 1] = format!("/proc/self/fd/{fd}"); + argument_index += 3; + } + + if verification_args.is_empty() { + return Ok(()); + } + let inner_command = command_separator + 1; + if argv.get(inner_command).is_none() { + return Err("bubblewrap argv is missing the inner command after '--'".to_string()); + } + if !argv[inner_command + 1..] + .iter() + .take_while(|argument| argument.as_str() != "--") + .any(|argument| argument == "--apply-seccomp-then-exec") + { + return Err("descriptor-backed mounts require the trusted inner sandbox stage".to_string()); + } + argv.splice(inner_command + 1..inner_command + 1, verification_args); + Ok(()) +} + fn preferred_bwrap_launcher() -> BubblewrapLauncher { static LAUNCHER: OnceLock = OnceLock::new(); LAUNCHER @@ -83,6 +156,7 @@ fn system_bwrap_launcher_for_path_with_probe( let Some(SystemBwrapCapabilities { supports_argv0, supports_perms: true, + supports_ro_bind_fd, }) = system_bwrap_capabilities(system_bwrap_path) else { return None; @@ -97,6 +171,7 @@ fn system_bwrap_launcher_for_path_with_probe( Some(SystemBwrapLauncher { program: system_bwrap_path, supports_argv0, + supports_ro_bind_fd, }) } @@ -125,6 +200,7 @@ fn system_bwrap_capabilities(system_bwrap_path: &Path) -> Option, + /// Inherited fallback mounts that must be authenticated before sandboxed code runs. + #[arg(long = "verify-fd-mount", hide = true)] + pub verify_fd_mounts: Vec, + /// When set, skip mounting a fresh `/proc` even though PID isolation is /// still enabled. This is primarily intended for restrictive container /// environments that deny `--proc /proc`. @@ -158,6 +162,7 @@ pub fn run_main() -> ! { apply_seccomp_then_exec, allow_network_for_proxy, proxy_route_spec, + verify_fd_mounts, no_proc, command, } = LandlockCommand::parse(); @@ -165,6 +170,9 @@ pub fn run_main() -> ! { if command.is_empty() { panic!("No command specified to execute."); } + if !apply_seccomp_then_exec && !verify_fd_mounts.is_empty() { + panic!("--verify-fd-mount is only supported in the inner sandbox stage"); + } ensure_inner_stage_mode_is_valid(apply_seccomp_then_exec, use_legacy_landlock); let EffectivePermissions { permission_profile, @@ -181,6 +189,10 @@ pub fn run_main() -> ! { // Inner stage: apply seccomp/no_new_privs after bubblewrap has already // established the filesystem view. if apply_seccomp_then_exec { + if let Err(err) = crate::fd_mount::verify_fd_mounts(&verify_fd_mounts) { + panic!("failed to verify descriptor-backed bubblewrap mount: {err}"); + } + let mut capability_header = [LINUX_CAPABILITY_VERSION_3, 0]; let mut capability_sets = [[0_u32; 3]; 2]; // SAFETY: capability ABI version 3 uses a [version, pid] header and