mirror of
https://github.com/openai/codex.git
synced 2026-09-20 12:47:38 +00:00
apply-patch: reject sandbox hard-link writes
This commit is contained in:
@@ -393,6 +393,9 @@ async fn apply_hunks_to_files(
|
||||
let path_abs = hunk.resolve_path(cwd);
|
||||
match hunk {
|
||||
Hunk::AddFile { contents, .. } => {
|
||||
ensure_sandbox_write_target_is_not_hard_link(&path_abs, fs, sandbox)
|
||||
.await
|
||||
.with_context(|| format!("Failed to write file {}", path_abs.display()))?;
|
||||
let overwritten_content =
|
||||
read_optional_file_text_for_delta(&path_abs, fs, sandbox, &mut delta.exact)
|
||||
.await;
|
||||
@@ -455,6 +458,9 @@ async fn apply_hunks_to_files(
|
||||
Hunk::UpdateFile {
|
||||
move_path, chunks, ..
|
||||
} => {
|
||||
ensure_sandbox_write_target_is_not_hard_link(&path_abs, fs, sandbox)
|
||||
.await
|
||||
.with_context(|| format!("Failed to write file {}", path_abs.display()))?;
|
||||
note_existing_path_delta_support(&path_abs, fs, sandbox, &mut delta.exact).await;
|
||||
let AppliedPatch {
|
||||
original_contents,
|
||||
@@ -462,6 +468,9 @@ async fn apply_hunks_to_files(
|
||||
} = derive_new_contents_from_chunks(&path_abs, chunks, fs, sandbox).await?;
|
||||
if let Some(dest) = move_path {
|
||||
let dest_abs = AbsolutePathBuf::resolve_path_against_base(dest, cwd);
|
||||
ensure_sandbox_write_target_is_not_hard_link(&dest_abs, fs, sandbox)
|
||||
.await
|
||||
.with_context(|| format!("Failed to write file {}", dest_abs.display()))?;
|
||||
let overwritten_move_content =
|
||||
read_optional_file_text_for_delta(&dest_abs, fs, sandbox, &mut delta.exact)
|
||||
.await;
|
||||
@@ -550,6 +559,26 @@ async fn apply_hunks_to_files(
|
||||
})
|
||||
}
|
||||
|
||||
async fn ensure_sandbox_write_target_is_not_hard_link(
|
||||
path: &AbsolutePathBuf,
|
||||
fs: &dyn ExecutorFileSystem,
|
||||
sandbox: Option<&FileSystemSandboxContext>,
|
||||
) -> io::Result<()> {
|
||||
let Some(sandbox) = sandbox.filter(|sandbox| sandbox.should_run_in_sandbox()) else {
|
||||
return Ok(());
|
||||
};
|
||||
|
||||
match fs.get_metadata(path, Some(sandbox)).await {
|
||||
Ok(metadata) if metadata.is_file && metadata.link_count > 1 => Err(io::Error::new(
|
||||
io::ErrorKind::PermissionDenied,
|
||||
"refusing to write through hard link",
|
||||
)),
|
||||
Ok(_) => Ok(()),
|
||||
Err(source) if source.kind() == io::ErrorKind::NotFound => Ok(()),
|
||||
Err(source) => Err(source),
|
||||
}
|
||||
}
|
||||
|
||||
async fn ensure_not_directory(
|
||||
path: &AbsolutePathBuf,
|
||||
fs: &dyn ExecutorFileSystem,
|
||||
|
||||
@@ -113,6 +113,46 @@ fn restrictive_workspace_write_profile() -> PermissionProfile {
|
||||
)
|
||||
}
|
||||
|
||||
#[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 +731,86 @@ 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_rejects_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 harness = apply_patch_harness().await?;
|
||||
let original_contents = "original outside content\n";
|
||||
let outside_dir = harness
|
||||
.test()
|
||||
.config
|
||||
.cwd
|
||||
.parent()
|
||||
.expect("cwd should have parent")
|
||||
.join(format!(
|
||||
"{}-{}-outside",
|
||||
harness
|
||||
.test()
|
||||
.config
|
||||
.cwd
|
||||
.file_name()
|
||||
.expect("cwd should have a file name")
|
||||
.to_string_lossy(),
|
||||
link_kind.name()
|
||||
));
|
||||
std::fs::create_dir_all(&outside_dir)?;
|
||||
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",
|
||||
restrictive_workspace_write_profile(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
let out = harness.apply_patch_output(&call_id, model_output).await;
|
||||
assert!(
|
||||
!out.contains("Success. Updated the following files"),
|
||||
"link escape should not report success: {out}"
|
||||
);
|
||||
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)]
|
||||
|
||||
@@ -215,6 +215,7 @@ pub(crate) async fn run_direct_request(
|
||||
is_directory: metadata.is_directory,
|
||||
is_file: metadata.is_file,
|
||||
is_symlink: metadata.is_symlink,
|
||||
link_count: metadata.link_count,
|
||||
created_at_ms: metadata.created_at_ms,
|
||||
modified_at_ms: metadata.modified_at_ms,
|
||||
}))
|
||||
|
||||
@@ -291,6 +291,7 @@ impl ExecutorFileSystem for DirectFileSystem {
|
||||
is_directory: metadata.is_dir(),
|
||||
is_file: metadata.is_file(),
|
||||
is_symlink: symlink_metadata.file_type().is_symlink(),
|
||||
link_count: metadata_link_count(&metadata),
|
||||
created_at_ms: metadata.created().ok().map_or(0, system_time_to_unix_ms),
|
||||
modified_at_ms: metadata.modified().ok().map_or(0, system_time_to_unix_ms),
|
||||
})
|
||||
@@ -513,6 +514,25 @@ fn system_time_to_unix_ms(time: SystemTime) -> i64 {
|
||||
.unwrap_or(0)
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
fn metadata_link_count(metadata: &std::fs::Metadata) -> u64 {
|
||||
use std::os::unix::fs::MetadataExt;
|
||||
|
||||
metadata.nlink()
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
fn metadata_link_count(metadata: &std::fs::Metadata) -> u64 {
|
||||
use std::os::windows::fs::MetadataExt;
|
||||
|
||||
metadata.number_of_links().map(u64::from).unwrap_or(1)
|
||||
}
|
||||
|
||||
#[cfg(not(any(unix, windows)))]
|
||||
fn metadata_link_count(_metadata: &std::fs::Metadata) -> u64 {
|
||||
1
|
||||
}
|
||||
|
||||
#[cfg(all(test, unix))]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
@@ -207,6 +207,7 @@ pub struct FsGetMetadataResponse {
|
||||
pub is_directory: bool,
|
||||
pub is_file: bool,
|
||||
pub is_symlink: bool,
|
||||
pub link_count: u64,
|
||||
pub created_at_ms: i64,
|
||||
pub modified_at_ms: i64,
|
||||
}
|
||||
|
||||
@@ -118,6 +118,7 @@ impl ExecutorFileSystem for RemoteFileSystem {
|
||||
is_directory: response.is_directory,
|
||||
is_file: response.is_file,
|
||||
is_symlink: response.is_symlink,
|
||||
link_count: response.link_count,
|
||||
created_at_ms: response.created_at_ms,
|
||||
modified_at_ms: response.modified_at_ms,
|
||||
})
|
||||
|
||||
@@ -139,6 +139,7 @@ impl ExecutorFileSystem for SandboxedFileSystem {
|
||||
is_directory: response.is_directory,
|
||||
is_file: response.is_file,
|
||||
is_symlink: response.is_symlink,
|
||||
link_count: response.link_count,
|
||||
created_at_ms: response.created_at_ms,
|
||||
modified_at_ms: response.modified_at_ms,
|
||||
})
|
||||
|
||||
@@ -101,6 +101,7 @@ impl FileSystemHandler {
|
||||
is_directory: metadata.is_directory,
|
||||
is_file: metadata.is_file,
|
||||
is_symlink: metadata.is_symlink,
|
||||
link_count: metadata.link_count,
|
||||
created_at_ms: metadata.created_at_ms,
|
||||
modified_at_ms: metadata.modified_at_ms,
|
||||
})
|
||||
|
||||
@@ -295,8 +295,20 @@ async fn file_system_get_metadata_returns_expected_fields(use_remote: bool) -> R
|
||||
assert_eq!(metadata.is_directory, false);
|
||||
assert_eq!(metadata.is_file, true);
|
||||
assert_eq!(metadata.is_symlink, false);
|
||||
assert_eq!(metadata.link_count, 1);
|
||||
assert!(metadata.modified_at_ms > 0);
|
||||
|
||||
let hard_link_path = tmp.path().join("note-hard-link.txt");
|
||||
std::fs::hard_link(&file_path, &hard_link_path)?;
|
||||
let hard_link_metadata = file_system
|
||||
.get_metadata(&absolute_path(hard_link_path), /*sandbox*/ None)
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
assert_eq!(hard_link_metadata.is_directory, false);
|
||||
assert_eq!(hard_link_metadata.is_file, true);
|
||||
assert_eq!(hard_link_metadata.is_symlink, false);
|
||||
assert_eq!(hard_link_metadata.link_count, 2);
|
||||
|
||||
let symlink_path = tmp.path().join("note-link.txt");
|
||||
symlink(&file_path, &symlink_path)?;
|
||||
let symlink_metadata = file_system
|
||||
|
||||
@@ -33,6 +33,7 @@ pub struct FileMetadata {
|
||||
pub is_directory: bool,
|
||||
pub is_file: bool,
|
||||
pub is_symlink: bool,
|
||||
pub link_count: u64,
|
||||
pub created_at_ms: i64,
|
||||
pub modified_at_ms: i64,
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user