From 93b7196dd6aee11429721469d041debb359518bc Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Mon, 1 Jun 2026 17:05:39 -0700 Subject: [PATCH] fix: reject symlinked config and output parents Co-authored-by: Codex noreply@openai.com --- codex-rs/Cargo.lock | 3 + codex-rs/config/src/loader/mod.rs | 35 +++++-- codex-rs/config/src/loader/tests.rs | 8 ++ .../core/src/config/config_loader_tests.rs | 41 ++++++++ codex-rs/exec-server/Cargo.toml | 1 + codex-rs/exec-server/src/environment_path.rs | 8 ++ codex-rs/exec-server/src/fs_helper.rs | 12 ++- codex-rs/exec-server/src/local_file_system.rs | 49 ++++++++++ codex-rs/exec-server/src/protocol.rs | 2 + .../exec-server/src/remote_file_system.rs | 24 +++++ .../exec-server/src/sandboxed_file_system.rs | 27 ++++++ .../src/server/file_system_handler.rs | 16 +++- codex-rs/exec-server/tests/file_system.rs | 36 +++++++ codex-rs/exec/Cargo.toml | 3 +- codex-rs/exec/src/event_processor.rs | 43 ++++++--- codex-rs/file-system/src/lib.rs | 17 ++++ codex-rs/utils/path-utils/Cargo.toml | 3 + codex-rs/utils/path-utils/src/lib.rs | 96 +++++++++++++++++++ .../utils/path-utils/src/path_utils_tests.rs | 57 +++++++++++ 19 files changed, 451 insertions(+), 30 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 745feb93be..9d5fbda6a4 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2742,6 +2742,7 @@ dependencies = [ "codex-utils-cargo-bin", "codex-utils-cli", "codex-utils-oss", + "codex-utils-path", "codex-utils-sandbox-summary", "core_test_support", "libc", @@ -2782,6 +2783,7 @@ dependencies = [ "codex-sandboxing", "codex-test-binary-support", "codex-utils-absolute-path", + "codex-utils-path", "codex-utils-pty", "codex-utils-rustls-provider", "ctor 0.6.3", @@ -4062,6 +4064,7 @@ version = "0.0.0" dependencies = [ "codex-utils-absolute-path", "dunce", + "libc", "pretty_assertions", "tempfile", ] diff --git a/codex-rs/config/src/loader/mod.rs b/codex-rs/config/src/loader/mod.rs index d751dc9d22..9a173d1add 100644 --- a/codex-rs/config/src/loader/mod.rs +++ b/codex-rs/config/src/loader/mod.rs @@ -1160,12 +1160,17 @@ async fn load_project_layers( let mut startup_warnings = Vec::new(); for dir in dirs { let dot_codex_abs = dir.join(".codex"); - if !fs - .get_metadata(&dot_codex_abs, /*sandbox*/ None) - .await - .map(|metadata| metadata.is_directory) - .unwrap_or(false) - { + let Ok(dot_codex_metadata) = fs.get_metadata(&dot_codex_abs, /*sandbox*/ None).await else { + continue; + }; + if dot_codex_metadata.is_symlink { + let dot_codex_display = dot_codex_abs.as_path().display(); + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!("Project config directory {dot_codex_display} must not be a symlink"), + )); + } + if !dot_codex_metadata.is_directory { continue; } @@ -1178,6 +1183,18 @@ async fn load_project_layers( continue; } let config_file = dot_codex_abs.join(CONFIG_TOML_FILE); + let config_file_for_read = fs + .canonicalize(&dir, /*sandbox*/ None) + .await + .map_err(|err| { + let dir_display = dir.as_path().display(); + io::Error::new( + err.kind(), + format!("Failed to resolve project directory {dir_display}: {err}"), + ) + })? + .join(".codex") + .join(CONFIG_TOML_FILE); match fs.get_metadata(&config_file, /*sandbox*/ None).await { Ok(metadata) => { if metadata.is_symlink { @@ -1194,6 +1211,7 @@ async fn load_project_layers( &dot_codex_abs, TomlValue::Table(toml::map::Map::new()), disabled_reason, + hooks_config_folder_override, )); continue; } @@ -1204,7 +1222,10 @@ async fn load_project_layers( )); } } - match fs.read_file_text(&config_file, /*sandbox*/ None).await { + match fs + .read_file_text_no_follow(&config_file_for_read, /*sandbox*/ None) + .await + { Ok(contents) => { let config: TomlValue = match toml::from_str(&contents) { Ok(config) => config, diff --git a/codex-rs/config/src/loader/tests.rs b/codex-rs/config/src/loader/tests.rs index 9d4511a8a6..454bfd3e17 100644 --- a/codex-rs/config/src/loader/tests.rs +++ b/codex-rs/config/src/loader/tests.rs @@ -43,6 +43,14 @@ impl ExecutorFileSystem for TestFileSystem { tokio::fs::read(path.as_path()).await } + async fn read_file_no_follow( + &self, + path: &AbsolutePathBuf, + _sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult> { + tokio::fs::read(path.as_path()).await + } + async fn write_file( &self, _path: &AbsolutePathBuf, diff --git a/codex-rs/core/src/config/config_loader_tests.rs b/codex-rs/core/src/config/config_loader_tests.rs index 0abe4c65d5..9f85ab290a 100644 --- a/codex-rs/core/src/config/config_loader_tests.rs +++ b/codex-rs/core/src/config/config_loader_tests.rs @@ -2993,6 +2993,47 @@ async fn symlinked_project_config_is_rejected() -> std::io::Result<()> { Ok(()) } +#[cfg(unix)] +#[tokio::test] +async fn symlinked_project_config_directory_is_rejected() -> std::io::Result<()> { + let tmp = tempdir()?; + let project_root = tmp.path().join("project"); + let codex_home = tmp.path().join("home"); + let payload_dir = tmp.path().join("payload"); + tokio::fs::create_dir_all(&project_root).await?; + tokio::fs::create_dir_all(&codex_home).await?; + tokio::fs::create_dir_all(&payload_dir).await?; + tokio::fs::write(project_root.join(".git"), "gitdir: here").await?; + tokio::fs::write( + payload_dir.join(CONFIG_TOML_FILE), + "sandbox_mode = \"danger-full-access\"\n", + ) + .await?; + symlink(&payload_dir, project_root.join(".codex")).expect("create config directory symlink"); + make_config_for_test( + &codex_home, + &project_root, + TrustLevel::Trusted, + /*project_root_markers*/ None, + ) + .await?; + + let err = ConfigBuilder::default() + .codex_home(codex_home) + .fallback_cwd(Some(project_root)) + .build() + .await + .expect_err("symlinked project config directory should fail"); + + assert_eq!(err.kind(), std::io::ErrorKind::InvalidInput); + assert!( + err.to_string().contains("must not be a symlink"), + "unexpected error: {err}" + ); + + Ok(()) +} + #[tokio::test] async fn project_layer_without_config_toml_is_disabled_when_untrusted_or_unknown() -> std::io::Result<()> { diff --git a/codex-rs/exec-server/Cargo.toml b/codex-rs/exec-server/Cargo.toml index d842094a16..bc9f93a86d 100644 --- a/codex-rs/exec-server/Cargo.toml +++ b/codex-rs/exec-server/Cargo.toml @@ -23,6 +23,7 @@ codex-file-system = { workspace = true } codex-protocol = { workspace = true } codex-sandboxing = { workspace = true } codex-utils-absolute-path = { workspace = true } +codex-utils-path = { workspace = true } codex-utils-pty = { workspace = true } codex-utils-rustls-provider = { workspace = true } futures = { workspace = true } diff --git a/codex-rs/exec-server/src/environment_path.rs b/codex-rs/exec-server/src/environment_path.rs index e9e07abef8..2ab9af0536 100644 --- a/codex-rs/exec-server/src/environment_path.rs +++ b/codex-rs/exec-server/src/environment_path.rs @@ -236,6 +236,14 @@ mod tests { Ok(b"skill contents".to_vec()) } + async fn read_file_no_follow( + &self, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> io::Result> { + self.read_file(path, sandbox).await + } + async fn write_file( &self, _path: &AbsolutePathBuf, diff --git a/codex-rs/exec-server/src/fs_helper.rs b/codex-rs/exec-server/src/fs_helper.rs index 0a210175b2..8fe261c05b 100644 --- a/codex-rs/exec-server/src/fs_helper.rs +++ b/codex-rs/exec-server/src/fs_helper.rs @@ -189,10 +189,14 @@ pub(crate) async fn run_direct_request( let file_system = DirectFileSystem; match request { FsHelperRequest::ReadFile(params) => { - let data = file_system - .read_file(¶ms.path, /*sandbox*/ None) - .await - .map_err(map_fs_error)?; + let data = if params.no_follow_symlinks { + file_system + .read_file_no_follow(¶ms.path, /*sandbox*/ None) + .await + } else { + file_system.read_file(¶ms.path, /*sandbox*/ None).await + } + .map_err(map_fs_error)?; Ok(FsHelperPayload::ReadFile(FsReadFileResponse { data_base64: STANDARD.encode(data), })) diff --git a/codex-rs/exec-server/src/local_file_system.rs b/codex-rs/exec-server/src/local_file_system.rs index 3f410f926d..4578e4e553 100644 --- a/codex-rs/exec-server/src/local_file_system.rs +++ b/codex-rs/exec-server/src/local_file_system.rs @@ -7,6 +7,8 @@ use std::sync::LazyLock; use std::time::SystemTime; use std::time::UNIX_EPOCH; use tokio::io; +#[cfg(unix)] +use tokio::io::AsyncReadExt; use crate::CopyOptions; use crate::CreateDirectoryOptions; @@ -109,6 +111,15 @@ impl ExecutorFileSystem for LocalFileSystem { file_system.read_file(path, sandbox).await } + async fn read_file_no_follow( + &self, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult> { + let (file_system, sandbox) = self.file_system_for(sandbox)?; + file_system.read_file_no_follow(path, sandbox).await + } + async fn write_file( &self, path: &AbsolutePathBuf, @@ -203,6 +214,17 @@ impl ExecutorFileSystem for UnsandboxedFileSystem { self.file_system.read_file(path, /*sandbox*/ None).await } + async fn read_file_no_follow( + &self, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult> { + reject_platform_sandbox_context(sandbox)?; + self.file_system + .read_file_no_follow(path, /*sandbox*/ None) + .await + } + async fn write_file( &self, path: &AbsolutePathBuf, @@ -317,6 +339,33 @@ impl ExecutorFileSystem for DirectFileSystem { tokio::fs::read(path.as_path()).await } + async fn read_file_no_follow( + &self, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult> { + reject_sandbox_context(sandbox)?; + #[cfg(unix)] + { + let file = codex_utils_path::open_file_for_read_no_follow(path.as_path())?; + let metadata = file.metadata()?; + if metadata.len() > MAX_READ_FILE_BYTES { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!("file is too large to read: limit is {MAX_READ_FILE_BYTES} bytes"), + )); + } + let mut file = tokio::fs::File::from_std(file); + let mut contents = Vec::new(); + file.read_to_end(&mut contents).await?; + Ok(contents) + } + #[cfg(not(unix))] + { + self.read_file(path, /*sandbox*/ None).await + } + } + async fn write_file( &self, path: &AbsolutePathBuf, diff --git a/codex-rs/exec-server/src/protocol.rs b/codex-rs/exec-server/src/protocol.rs index 9ac80c188e..3e76f8858b 100644 --- a/codex-rs/exec-server/src/protocol.rs +++ b/codex-rs/exec-server/src/protocol.rs @@ -165,6 +165,8 @@ pub struct TerminateResponse { pub struct FsReadFileParams { pub path: AbsolutePathBuf, pub sandbox: Option, + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + pub no_follow_symlinks: bool, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] diff --git a/codex-rs/exec-server/src/remote_file_system.rs b/codex-rs/exec-server/src/remote_file_system.rs index 54198c93ba..3ae40c0e57 100644 --- a/codex-rs/exec-server/src/remote_file_system.rs +++ b/codex-rs/exec-server/src/remote_file_system.rs @@ -98,6 +98,30 @@ impl ExecutorFileSystem for RemoteFileSystem { .fs_read_file(FsReadFileParams { path: path.clone(), sandbox: remote_sandbox_context(sandbox), + no_follow_symlinks: false, + }) + .await + .map_err(map_remote_error)?; + STANDARD.decode(response.data_base64).map_err(|err| { + io::Error::new( + io::ErrorKind::InvalidData, + format!("remote fs/readFile returned invalid base64 dataBase64: {err}"), + ) + }) + } + + async fn read_file_no_follow( + &self, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult> { + trace!("remote fs read_file_no_follow"); + let client = self.client.get().await.map_err(map_remote_error)?; + let response = client + .fs_read_file(FsReadFileParams { + path: path.clone(), + sandbox: remote_sandbox_context(sandbox), + no_follow_symlinks: true, }) .await .map_err(map_remote_error)?; diff --git a/codex-rs/exec-server/src/sandboxed_file_system.rs b/codex-rs/exec-server/src/sandboxed_file_system.rs index 00b6966c3b..d3b173988f 100644 --- a/codex-rs/exec-server/src/sandboxed_file_system.rs +++ b/codex-rs/exec-server/src/sandboxed_file_system.rs @@ -97,6 +97,33 @@ impl ExecutorFileSystem for SandboxedFileSystem { FsHelperRequest::ReadFile(FsReadFileParams { path: path.clone(), sandbox: None, + no_follow_symlinks: false, + }), + ) + .await? + .expect_read_file() + .map_err(map_sandbox_error)?; + STANDARD.decode(response.data_base64).map_err(|err| { + io::Error::new( + io::ErrorKind::InvalidData, + format!("fs/readFile returned invalid base64 dataBase64: {err}"), + ) + }) + } + + async fn read_file_no_follow( + &self, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult> { + let sandbox = require_platform_sandbox(sandbox)?; + let response = self + .run_sandboxed( + sandbox, + FsHelperRequest::ReadFile(FsReadFileParams { + path: path.clone(), + sandbox: None, + no_follow_symlinks: true, }), ) .await? diff --git a/codex-rs/exec-server/src/server/file_system_handler.rs b/codex-rs/exec-server/src/server/file_system_handler.rs index b60589d020..a6e70871e4 100644 --- a/codex-rs/exec-server/src/server/file_system_handler.rs +++ b/codex-rs/exec-server/src/server/file_system_handler.rs @@ -52,11 +52,16 @@ impl FileSystemHandler { &self, params: FsReadFileParams, ) -> Result { - let bytes = self - .file_system - .read_file(¶ms.path, params.sandbox.as_ref()) - .await - .map_err(map_fs_error)?; + let bytes = if params.no_follow_symlinks { + self.file_system + .read_file_no_follow(¶ms.path, params.sandbox.as_ref()) + .await + } else { + self.file_system + .read_file(¶ms.path, params.sandbox.as_ref()) + .await + } + .map_err(map_fs_error)?; Ok(FsReadFileResponse { data_base64: STANDARD.encode(bytes), }) @@ -269,6 +274,7 @@ mod tests { sandbox_policy, sandbox_cwd.clone(), )), + no_follow_symlinks: false, }) .await .expect("read file"); diff --git a/codex-rs/exec-server/tests/file_system.rs b/codex-rs/exec-server/tests/file_system.rs index b8d1c21894..27067fd643 100644 --- a/codex-rs/exec-server/tests/file_system.rs +++ b/codex-rs/exec-server/tests/file_system.rs @@ -735,6 +735,42 @@ async fn file_system_sandboxed_read_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_read_file_no_follow_rejects_symlinked_parent(use_remote: bool) -> Result<()> { + let context = create_file_system_context(use_remote).await?; + let file_system = context.file_system; + + let tmp = TempDir::new()?; + let tmp = tmp.path().canonicalize()?; + let target_dir = tmp.join("target"); + let symlinked_dir = tmp.join("link"); + std::fs::create_dir(&target_dir)?; + let target_path = target_dir.join("payload"); + std::fs::write(&target_path, "original")?; + symlink(&target_dir, &symlinked_dir)?; + + let contents = file_system + .read_file_no_follow(&absolute_path(target_path), /*sandbox*/ None) + .await + .with_context(|| format!("mode={use_remote}"))?; + assert_eq!(contents, b"original"); + + if file_system + .read_file_no_follow( + &absolute_path(symlinked_dir.join("payload")), + /*sandbox*/ None, + ) + .await + .is_ok() + { + anyhow::bail!("symlinked parent should fail"); + } + + Ok(()) +} + #[test_case(false ; "local")] #[test_case(true ; "remote")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] diff --git a/codex-rs/exec/Cargo.toml b/codex-rs/exec/Cargo.toml index 29457ce95e..5745fe4aff 100644 --- a/codex-rs/exec/Cargo.toml +++ b/codex-rs/exec/Cargo.toml @@ -39,8 +39,9 @@ codex-protocol = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-cli = { workspace = true } codex-utils-oss = { workspace = true } -libc = { workspace = true } codex-utils-sandbox-summary = { workspace = true } +codex-utils-path = { workspace = true } +libc = { workspace = true } owo-colors = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } diff --git a/codex-rs/exec/src/event_processor.rs b/codex-rs/exec/src/event_processor.rs index c45c6e91c2..3b2eb9dfb2 100644 --- a/codex-rs/exec/src/event_processor.rs +++ b/codex-rs/exec/src/event_processor.rs @@ -1,9 +1,5 @@ #[cfg(unix)] -use std::fs::OpenOptions; -#[cfg(unix)] use std::io::Write; -#[cfg(unix)] -use std::os::unix::fs::OpenOptionsExt; use std::path::Path; use codex_app_server_protocol::ServerNotification; @@ -55,12 +51,7 @@ fn write_last_message_file(contents: &str, last_message_path: Option<&Path>) { #[cfg(unix)] fn write_last_message_to_path(path: &Path, contents: &str) -> std::io::Result<()> { - let mut file = OpenOptions::new() - .write(true) - .create(true) - .truncate(true) - .custom_flags(libc::O_NOFOLLOW) - .open(path)?; + let mut file = codex_utils_path::open_file_for_write_no_follow(path)?; file.write_all(contents.as_bytes()) } @@ -78,7 +69,11 @@ mod tests { #[test] fn writes_last_message_to_regular_file() { let temp_dir = tempdir().expect("tempdir"); - let output_path = temp_dir.path().join("output.md"); + let output_path = temp_dir + .path() + .canonicalize() + .expect("canonical tempdir") + .join("output.md"); write_last_message_to_path(&output_path, "hello").expect("write output"); @@ -92,8 +87,9 @@ mod tests { #[test] fn rejects_symlinked_last_message_path() { let temp_dir = tempdir().expect("tempdir"); - let target_path = temp_dir.path().join("target.md"); - let output_path = temp_dir.path().join("output.md"); + let temp_dir = temp_dir.path().canonicalize().expect("canonical tempdir"); + let target_path = temp_dir.join("target.md"); + let output_path = temp_dir.join("output.md"); std::fs::write(&target_path, "original").expect("write target"); std::os::unix::fs::symlink(&target_path, &output_path).expect("create symlink"); @@ -106,4 +102,25 @@ mod tests { "original" ); } + + #[cfg(unix)] + #[test] + fn rejects_symlinked_last_message_parent_directory() { + let temp_dir = tempdir().expect("tempdir"); + let temp_dir = temp_dir.path().canonicalize().expect("canonical tempdir"); + let target_dir = temp_dir.join("target"); + let symlinked_dir = temp_dir.join("link"); + std::fs::create_dir(&target_dir).expect("create target directory"); + let target_path = target_dir.join("output.md"); + std::fs::write(&target_path, "original").expect("write target"); + std::os::unix::fs::symlink(&target_dir, &symlinked_dir).expect("create symlink"); + + write_last_message_to_path(&symlinked_dir.join("output.md"), "hello") + .expect_err("symlinked parent should fail"); + + assert_eq!( + std::fs::read_to_string(&target_path).expect("read target"), + "original" + ); + } } diff --git a/codex-rs/file-system/src/lib.rs b/codex-rs/file-system/src/lib.rs index 8fad1f5b62..a76975bb6c 100644 --- a/codex-rs/file-system/src/lib.rs +++ b/codex-rs/file-system/src/lib.rs @@ -156,6 +156,13 @@ pub trait ExecutorFileSystem: Send + Sync { sandbox: Option<&FileSystemSandboxContext>, ) -> FileSystemResult>; + /// Reads a file while rejecting symlinks in any path component. + async fn read_file_no_follow( + &self, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult>; + /// Reads a file and decodes it as UTF-8 text. async fn read_file_text( &self, @@ -166,6 +173,16 @@ pub trait ExecutorFileSystem: Send + Sync { String::from_utf8(bytes).map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err)) } + /// Reads a file without following symlinks and decodes it as UTF-8 text. + async fn read_file_text_no_follow( + &self, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { + let bytes = self.read_file_no_follow(path, sandbox).await?; + String::from_utf8(bytes).map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err)) + } + async fn write_file( &self, path: &AbsolutePathBuf, diff --git a/codex-rs/utils/path-utils/Cargo.toml b/codex-rs/utils/path-utils/Cargo.toml index d1503fd060..287247f877 100644 --- a/codex-rs/utils/path-utils/Cargo.toml +++ b/codex-rs/utils/path-utils/Cargo.toml @@ -12,6 +12,9 @@ codex-utils-absolute-path = { workspace = true } dunce = { workspace = true } tempfile = { workspace = true } +[target.'cfg(unix)'.dependencies] +libc = { workspace = true } + [dev-dependencies] pretty_assertions = { workspace = true } tempfile = { workspace = true } diff --git a/codex-rs/utils/path-utils/src/lib.rs b/codex-rs/utils/path-utils/src/lib.rs index 71b0a6473d..c7179c78db 100644 --- a/codex-rs/utils/path-utils/src/lib.rs +++ b/codex-rs/utils/path-utils/src/lib.rs @@ -5,7 +5,19 @@ pub use env::is_wsl; use codex_utils_absolute_path::AbsolutePathBuf; use std::collections::HashSet; +#[cfg(unix)] +use std::ffi::CString; +#[cfg(unix)] +use std::fs::File; use std::io; +#[cfg(unix)] +use std::os::fd::AsRawFd; +#[cfg(unix)] +use std::os::fd::FromRawFd; +#[cfg(unix)] +use std::os::unix::ffi::OsStrExt; +#[cfg(unix)] +use std::path::Component; use std::path::Path; use std::path::PathBuf; use tempfile::NamedTempFile; @@ -132,6 +144,90 @@ pub fn write_atomically(write_path: &Path, contents: &str) -> io::Result<()> { Ok(()) } +/// Opens a file for reading without following symlinks in any path component. +#[cfg(unix)] +pub fn open_file_for_read_no_follow(path: &Path) -> io::Result { + open_file_no_follow(path, libc::O_RDONLY, /*mode*/ 0) +} + +/// Opens a file for writing without following symlinks in any path component. +#[cfg(unix)] +pub fn open_file_for_write_no_follow(path: &Path) -> io::Result { + open_file_no_follow( + path, + libc::O_WRONLY | libc::O_CREAT | libc::O_TRUNC, + /*mode*/ 0o666, + ) +} + +#[cfg(unix)] +fn open_file_no_follow(path: &Path, flags: libc::c_int, mode: libc::mode_t) -> io::Result { + let components = path + .components() + .filter_map(|component| match component { + Component::RootDir | Component::CurDir => None, + Component::ParentDir => Some(Ok(std::ffi::OsStr::new(".."))), + Component::Normal(component) => Some(Ok(component)), + Component::Prefix(_) => Some(Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!("unsupported path prefix in {}", path.display()), + ))), + }) + .collect::>>()?; + let Some((file_name, parent_components)) = components.split_last() else { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + format!("path {} does not name a file", path.display()), + )); + }; + + let mut current_dir = if path.is_absolute() { + File::open("/")? + } else { + File::open(".")? + }; + for component in parent_components { + current_dir = openat( + ¤t_dir, + component, + libc::O_RDONLY | libc::O_CLOEXEC | libc::O_DIRECTORY | libc::O_NOFOLLOW, + /*mode*/ 0, + )?; + } + openat( + ¤t_dir, + file_name, + flags | libc::O_CLOEXEC | libc::O_NOFOLLOW, + mode, + ) +} + +#[cfg(unix)] +fn openat( + directory: &File, + path: &std::ffi::OsStr, + flags: libc::c_int, + mode: libc::mode_t, +) -> io::Result { + let path = CString::new(path.as_bytes()) + .map_err(|err| io::Error::new(io::ErrorKind::InvalidInput, err))?; + // SAFETY: `path` is NUL-terminated, `directory` owns a valid fd, and the + // returned fd is transferred into a `File` exactly once. + let fd = unsafe { + libc::openat( + directory.as_raw_fd(), + path.as_ptr(), + flags, + libc::c_uint::from(mode), + ) + }; + if fd == -1 { + return Err(io::Error::last_os_error()); + } + // SAFETY: `openat` returned a new owned file descriptor. + Ok(unsafe { File::from_raw_fd(fd) }) +} + fn normalize_for_wsl(path: PathBuf) -> PathBuf { normalize_for_wsl_with_flag(path, env::is_wsl()) } diff --git a/codex-rs/utils/path-utils/src/path_utils_tests.rs b/codex-rs/utils/path-utils/src/path_utils_tests.rs index 94c914fec2..04b24bd5ec 100644 --- a/codex-rs/utils/path-utils/src/path_utils_tests.rs +++ b/codex-rs/utils/path-utils/src/path_utils_tests.rs @@ -1,7 +1,11 @@ #[cfg(unix)] mod symlinks { + use super::super::open_file_for_read_no_follow; + use super::super::open_file_for_write_no_follow; use super::super::resolve_symlink_write_paths; use pretty_assertions::assert_eq; + use std::io::Read; + use std::io::Write; use std::os::unix::fs::symlink; #[test] @@ -19,6 +23,59 @@ mod symlinks { assert_eq!(resolved.write_path, a); Ok(()) } + + #[test] + fn no_follow_read_rejects_symlinked_parent_directory() -> std::io::Result<()> { + let dir = tempfile::tempdir()?; + let dir = dir.path().canonicalize()?; + let target_dir = dir.join("target"); + let symlinked_dir = dir.join("link"); + std::fs::create_dir(&target_dir)?; + std::fs::write(target_dir.join("payload"), "secret")?; + symlink(&target_dir, &symlinked_dir)?; + + let err = open_file_for_read_no_follow(&symlinked_dir.join("payload")) + .expect_err("symlinked parent should fail"); + + assert!(err.raw_os_error().is_some()); + Ok(()) + } + + #[test] + fn no_follow_write_rejects_symlinked_parent_directory() -> std::io::Result<()> { + let dir = tempfile::tempdir()?; + let dir = dir.path().canonicalize()?; + let target_dir = dir.join("target"); + let symlinked_dir = dir.join("link"); + std::fs::create_dir(&target_dir)?; + std::fs::write(target_dir.join("payload"), "original")?; + symlink(&target_dir, &symlinked_dir)?; + + let err = open_file_for_write_no_follow(&symlinked_dir.join("payload")) + .expect_err("symlinked parent should fail"); + + assert!(err.raw_os_error().is_some()); + assert_eq!( + std::fs::read_to_string(target_dir.join("payload"))?, + "original" + ); + Ok(()) + } + + #[test] + fn no_follow_helpers_read_and_write_regular_file() -> std::io::Result<()> { + let dir = tempfile::tempdir()?; + let path = dir.path().canonicalize()?.join("payload"); + + let mut writer = open_file_for_write_no_follow(&path)?; + writer.write_all(b"hello")?; + let mut reader = open_file_for_read_no_follow(&path)?; + let mut contents = String::new(); + reader.read_to_string(&mut contents)?; + + assert_eq!(contents, "hello"); + Ok(()) + } } #[cfg(target_os = "linux")]