From 3ebbcbca316fe4cf48c71306b90e4b12dd03c6b8 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Fri, 8 May 2026 14:21:30 -0700 Subject: [PATCH] apply-patch: avoid sandbox link write-through --- codex-rs/core/tests/suite/apply_patch_cli.rs | 137 +++++++++++++++++++ codex-rs/exec-server/src/fs_helper.rs | 92 ++++++++++++- codex-rs/exec-server/tests/file_system.rs | 49 +++++++ 3 files changed, 276 insertions(+), 2 deletions(-) diff --git a/codex-rs/core/tests/suite/apply_patch_cli.rs b/codex-rs/core/tests/suite/apply_patch_cli.rs index bc51fdd460..2ed468f931 100644 --- a/codex-rs/core/tests/suite/apply_patch_cli.rs +++ b/codex-rs/core/tests/suite/apply_patch_cli.rs @@ -15,6 +15,11 @@ use std::time::Duration; use codex_exec_server::CreateDirectoryOptions; use codex_features::Feature; use codex_protocol::models::PermissionProfile; +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::AskForApproval; use codex_protocol::protocol::EventMsg; @@ -23,6 +28,7 @@ use codex_protocol::protocol::SandboxPolicy; use codex_protocol::user_input::UserInput; #[cfg(target_os = "linux")] use codex_sandboxing::landlock::CODEX_LINUX_SANDBOX_ARG0; +use codex_utils_absolute_path::AbsolutePathBuf; use core_test_support::assert_regex_match; use core_test_support::responses::ev_apply_patch_function_call; use core_test_support::responses::ev_assistant_message; @@ -113,6 +119,67 @@ fn restrictive_workspace_write_profile() -> PermissionProfile { ) } +fn workspace_write_with_read_only_root(read_only_root: AbsolutePathBuf) -> PermissionProfile { + let file_system_sandbox_policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry { + path: FileSystemPath::Path { + path: read_only_root, + }, + access: FileSystemAccessMode::Read, + }, + FileSystemSandboxEntry { + path: FileSystemPath::Special { + value: FileSystemSpecialPath::project_roots(/*subpath*/ None), + }, + access: FileSystemAccessMode::Write, + }, + ]); + PermissionProfile::from_runtime_permissions( + &file_system_sandbox_policy, + NetworkSandboxPolicy::Restricted, + ) +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum WorkspaceLinkKind { + Soft, + Hard, +} + +impl WorkspaceLinkKind { + fn name(self) -> &'static str { + match self { + WorkspaceLinkKind::Soft => "soft", + WorkspaceLinkKind::Hard => "hard", + } + } + + fn create(self, source: &std::path::Path, link: &std::path::Path) -> std::io::Result<()> { + match self { + WorkspaceLinkKind::Soft => create_file_symlink(source, link), + WorkspaceLinkKind::Hard => std::fs::hard_link(source, link), + } + } +} + +#[cfg(unix)] +fn create_file_symlink(source: &std::path::Path, link: &std::path::Path) -> std::io::Result<()> { + std::os::unix::fs::symlink(source, link) +} + +#[cfg(windows)] +fn create_file_symlink(source: &std::path::Path, link: &std::path::Path) -> std::io::Result<()> { + std::os::windows::fs::symlink_file(source, link) +} + +#[cfg(not(any(unix, windows)))] +fn create_file_symlink(_source: &std::path::Path, _link: &std::path::Path) -> std::io::Result<()> { + Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "file symlinks are unsupported on this platform", + )) +} + pub async fn mount_apply_patch( harness: &TestCodexHarness, call_id: &str, @@ -691,6 +758,76 @@ async fn apply_patch_cli_rejects_path_traversal_outside_workspace( Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +#[test_case(ApplyPatchModelOutput::Freeform, WorkspaceLinkKind::Soft ; "freeform_soft_link")] +#[test_case(ApplyPatchModelOutput::Function, WorkspaceLinkKind::Soft ; "function_soft_link")] +#[test_case(ApplyPatchModelOutput::Shell, WorkspaceLinkKind::Soft ; "shell_soft_link")] +#[test_case(ApplyPatchModelOutput::ShellViaHeredoc, WorkspaceLinkKind::Soft ; "shell_heredoc_soft_link")] +#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc, WorkspaceLinkKind::Soft ; "shell_command_heredoc_soft_link")] +#[test_case(ApplyPatchModelOutput::Freeform, WorkspaceLinkKind::Hard ; "freeform_hard_link")] +#[test_case(ApplyPatchModelOutput::Function, WorkspaceLinkKind::Hard ; "function_hard_link")] +#[test_case(ApplyPatchModelOutput::Shell, WorkspaceLinkKind::Hard ; "shell_hard_link")] +#[test_case(ApplyPatchModelOutput::ShellViaHeredoc, WorkspaceLinkKind::Hard ; "shell_heredoc_hard_link")] +#[test_case(ApplyPatchModelOutput::ShellCommandViaHeredoc, WorkspaceLinkKind::Hard ; "shell_command_heredoc_hard_link")] +async fn apply_patch_cli_does_not_write_through_link_escape_outside_workspace( + model_output: ApplyPatchModelOutput, + link_kind: WorkspaceLinkKind, +) -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_remote!( + Ok(()), + "link escape setup needs local filesystem link creation" + ); + + let test_root = tempfile::tempdir_in(std::env::current_dir()?)?; + let work_dir = AbsolutePathBuf::try_from(test_root.path().join("work"))?; + let outside_dir = AbsolutePathBuf::try_from(test_root.path().join("outside"))?; + std::fs::create_dir_all(work_dir.as_path())?; + std::fs::create_dir_all(outside_dir.as_path())?; + + let harness_work_dir = work_dir.clone(); + let harness = apply_patch_harness_with(move |builder| { + builder.with_config(move |config| { + config.cwd = harness_work_dir; + }) + }) + .await?; + let original_contents = "original outside content\n"; + let outside_file = outside_dir.join("victim.txt"); + std::fs::write(&outside_file, original_contents)?; + + let link_rel = format!("{}-link.txt", link_kind.name()); + let link_path = harness.path(&link_rel); + match link_kind.create(&outside_file, &link_path) { + Ok(()) => {} + Err(error) if link_kind == WorkspaceLinkKind::Soft && cfg!(windows) => { + eprintln!("Skipping Windows symlink apply_patch sandbox test: {error}"); + return Ok(()); + } + Err(error) => return Err(error.into()), + } + + let patch = format!("*** Begin Patch\n*** Add File: {link_rel}\n+pwned\n*** End Patch"); + let call_id = format!("apply-{}-link-escape", link_kind.name()); + mount_apply_patch(&harness, &call_id, &patch, "fail", model_output).await; + + harness + .submit_with_permission_profile( + "attempt to escape workspace via apply_patch link", + workspace_write_with_read_only_root(outside_dir.clone()), + ) + .await?; + + let out = harness.apply_patch_output(&call_id, model_output).await; + assert_eq!( + std::fs::read_to_string(&outside_file)?, + original_contents, + "{:?} link escape should not modify the outside victim; tool output: {out}", + link_kind + ); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[test_case(ApplyPatchModelOutput::Freeform)] #[test_case(ApplyPatchModelOutput::Function)] diff --git a/codex-rs/exec-server/src/fs_helper.rs b/codex-rs/exec-server/src/fs_helper.rs index 8d91224803..0be69e9c2c 100644 --- a/codex-rs/exec-server/src/fs_helper.rs +++ b/codex-rs/exec-server/src/fs_helper.rs @@ -3,7 +3,11 @@ use base64::engine::general_purpose::STANDARD; use codex_app_server_protocol::JSONRPCErrorError; use serde::Deserialize; use serde::Serialize; +use std::path::PathBuf; +use std::sync::atomic::AtomicU64; +use std::sync::atomic::Ordering; use tokio::io; +use tokio::io::AsyncWriteExt; use crate::CopyOptions; use crate::CreateDirectoryOptions; @@ -37,6 +41,7 @@ use crate::rpc::invalid_request; use crate::rpc::not_found; pub const CODEX_FS_HELPER_ARG1: &str = "--codex-run-as-fs-helper"; +static TEMP_FILE_COUNTER: AtomicU64 = AtomicU64::new(0); #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(tag = "operation", content = "params")] @@ -185,8 +190,7 @@ pub(crate) async fn run_direct_request( "{FS_WRITE_FILE_METHOD} requires valid base64 dataBase64: {err}" )) })?; - file_system - .write_file(¶ms.path, bytes, /*sandbox*/ None) + write_file_by_replacing_path(¶ms.path, bytes) .await .map_err(map_fs_error)?; Ok(FsHelperPayload::WriteFile(FsWriteFileResponse {})) @@ -266,6 +270,90 @@ pub(crate) async fn run_direct_request( } } +/// The helper is already running inside the platform sandbox, but the sandbox +/// checks the path being opened. If a writable workspace path is itself a +/// symlink or a hard link to a file outside the sandbox, opening that path for +/// writing can either follow the symlink or mutate the shared hard-link inode. +/// +/// Create a fresh file in the destination directory and rename it into place +/// instead. The sandbox still has to allow creating and renaming inside the +/// writable directory, but any existing final-path link is replaced rather than +/// used as the write target. +async fn write_file_by_replacing_path( + path: &codex_utils_absolute_path::AbsolutePathBuf, + bytes: Vec, +) -> io::Result<()> { + let parent = path + .parent() + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "path has no parent"))?; + if path.as_path().file_name().is_none() { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "path has no file name", + )); + } + + let (temp_path, mut temp_file) = loop { + let counter = TEMP_FILE_COUNTER.fetch_add(1, Ordering::Relaxed); + let temp_name = format!(".codex-write-{}-{counter}.tmp", std::process::id()); + let temp_path = parent.join(PathBuf::from(temp_name)); + match tokio::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(temp_path.as_path()) + .await + { + Ok(file) => break (temp_path, file), + Err(err) if err.kind() == io::ErrorKind::AlreadyExists => continue, + Err(err) => return Err(err), + } + }; + + let write_result = async { + temp_file.write_all(&bytes).await?; + temp_file.flush().await + } + .await; + drop(temp_file); + if let Err(err) = write_result { + let _ = tokio::fs::remove_file(temp_path.as_path()).await; + return Err(err); + } + + if let Ok(metadata) = tokio::fs::metadata(path.as_path()).await { + let _ = tokio::fs::set_permissions(temp_path.as_path(), metadata.permissions()).await; + } + + if let Err(err) = replace_with_temp_file(&temp_path, path).await { + let _ = tokio::fs::remove_file(temp_path.as_path()).await; + return Err(err); + } + Ok(()) +} + +#[cfg(not(windows))] +async fn replace_with_temp_file( + temp_path: &codex_utils_absolute_path::AbsolutePathBuf, + path: &codex_utils_absolute_path::AbsolutePathBuf, +) -> io::Result<()> { + tokio::fs::rename(temp_path.as_path(), path.as_path()).await +} + +#[cfg(windows)] +async fn replace_with_temp_file( + temp_path: &codex_utils_absolute_path::AbsolutePathBuf, + path: &codex_utils_absolute_path::AbsolutePathBuf, +) -> io::Result<()> { + match tokio::fs::rename(temp_path.as_path(), path.as_path()).await { + Ok(()) => Ok(()), + Err(err) if err.kind() == io::ErrorKind::AlreadyExists => { + tokio::fs::remove_file(path.as_path()).await?; + tokio::fs::rename(temp_path.as_path(), path.as_path()).await + } + Err(err) => Err(err), + } +} + fn map_fs_error(err: io::Error) -> JSONRPCErrorError { match err.kind() { io::ErrorKind::NotFound => not_found(err.to_string()), diff --git a/codex-rs/exec-server/tests/file_system.rs b/codex-rs/exec-server/tests/file_system.rs index 0840b2a909..19ec3eba50 100644 --- a/codex-rs/exec-server/tests/file_system.rs +++ b/codex-rs/exec-server/tests/file_system.rs @@ -732,6 +732,55 @@ async fn file_system_sandboxed_write_rejects_symlink_escape(use_remote: bool) -> Ok(()) } +#[test_case(false ; "local")] +#[test_case(true ; "remote")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn file_system_sandboxed_write_replaces_final_path_links(use_remote: bool) -> Result<()> { + let context = create_file_system_context(use_remote).await?; + let file_system = context.file_system; + + let tmp = TempDir::new()?; + let allowed_dir = tmp.path().join("allowed"); + let outside_dir = tmp.path().join("outside"); + std::fs::create_dir_all(&allowed_dir)?; + std::fs::create_dir_all(&outside_dir)?; + + let original = "outside\n"; + let sandbox = workspace_write_sandbox(allowed_dir.clone()); + let soft_target = outside_dir.join("soft-target.txt"); + let soft_link = allowed_dir.join("soft-link.txt"); + let hard_target = outside_dir.join("hard-target.txt"); + let hard_link = allowed_dir.join("hard-link.txt"); + std::fs::write(&soft_target, original)?; + std::fs::write(&hard_target, original)?; + symlink(&soft_target, &soft_link)?; + std::fs::hard_link(&hard_target, &hard_link)?; + + for (target, link, contents) in [ + (&soft_target, &soft_link, "soft replacement\n"), + (&hard_target, &hard_link, "hard replacement\n"), + ] { + file_system + .write_file( + &absolute_path(link.clone()), + contents.as_bytes().to_vec(), + Some(&sandbox), + ) + .await + .with_context(|| { + format!("sandboxed write should replace final path link mode={use_remote}") + })?; + + assert_eq!(std::fs::read_to_string(target)?, original); + assert_eq!(std::fs::read_to_string(link)?, contents); + let metadata = std::fs::symlink_metadata(link)?; + assert!(metadata.file_type().is_file()); + assert!(!metadata.file_type().is_symlink()); + } + + Ok(()) +} + #[test_case(false ; "local")] #[test_case(true ; "remote")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)]