openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-20 17:38:45 +00:00
Code Issues Packages Projects Releases Wiki Activity

Reject replaceable symlinks in pre-sandbox executable lookup (#55384)

Browse Source 
## Why

A read-only executable target does not make its lookup path trustworthy: a symlink or ancestor directory may still be replaceable through writes allowed by the filesystem policy.

## What changed

Check the original lookup path, resolved parent aliases, and canonical target before selecting a pre-sandbox executable such as `bwrap`. Account for sticky directory ownership when deciding whether a child can be replaced, while preserving read-only mount carveouts.

Resolve `PATH` entries individually, including relative entries when the working directory contains a colon.

## Testing

Add regression tests for writable symlinks to read-only executables and symlinked search directories into writable roots. Extend search-path coverage to relative entries and a working directory containing a colon, and update the protected system installation test to use a direct executable path.

GitOrigin-RevId: 18d0cb6e809e5abd1313315670d4e2ff91dcfb30
This commit is contained in:
keeley-openai
2026-10-19 21:26:58 +00:00
committed by copyberry
parent 63ba55817a
commit b8f880bc38
2 changed files with 142 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/sandboxing/src/bwrap.rs
+74 -19
View File
@@ -242,31 +242,68 @@ pub fn find_executable_in_search_paths(
.iter()
.any(|root| root.is_path_writable(path))
};
which::which_in_all(program, Some(search_path), &cwd)
.ok()?
let file_is_writable = |path: &Path| {
policy_can_write(path)
&& std::fs::symlink_metadata(path)
.map_or(true, |metadata| !metadata.file_type().is_symlink())
&& current_user_can_modify(path)
};
let path_is_replaceable = |path: &Path| {
path.ancestors().any(|component| {
let Some(parent) = component.parent() else {
return false;
};
let Some(file_name) = component.file_name() else {
return false;
};
let Ok(parent) = std::fs::canonicalize(parent) else {
return true;
};
// Match policy through parent aliases without resolving the child symlink.
let component = parent.join(file_name);
// Read-only carveouts are bind mounts and cannot themselves
// be renamed/unlinked. Continue checking above them, since a
// mutable ancestor could still redirect the installation path.
let is_read_only_mount = writable_roots.iter().any(|root| {
root.read_only_subpaths
.iter()
.any(|path| path.as_path() == component.as_path())
});
policy_can_write(&parent)
&& !is_read_only_mount
&& current_user_can_replace_child(&parent, &component)
})
};
std::env::split_paths(&search_path)
.filter_map(|path| {
// Let which resolve explicit program paths and expand ~/bin entries.
let binary = if Path::new(program).components().count() > 1 || path.starts_with("~") {
PathBuf::from(program)
} else {
cwd.join(&path).join(program)
};
which::which_in(binary, Some(path), &cwd).ok()
})
.find_map(|path| {
if !path.is_absolute() || path_is_replaceable(&path) {
return None;
}
let file_name = path.file_name()?;
let resolved_parent = std::fs::canonicalize(path.parent()?).ok()?;
let resolved_alias = resolved_parent.join(file_name);
if (!cwd_is_root && resolved_alias.starts_with(&cwd))
|| file_is_writable(&resolved_alias)
|| path_is_replaceable(&resolved_alias)
{
return None;
}
let path = std::fs::canonicalize(path).ok()?;
// Preserve read-only carveouts, but also check ancestors: a protected
// file can still be replaced by renaming a writable parent directory.
// Host-protected installations remain usable even with full-disk writes.
if (!cwd_is_root && path.starts_with(&cwd))
|| (policy_can_write(&path) && current_user_can_modify(&path))
|| path.ancestors().any(|component| {
let Some(parent) = component.parent() else {
return false;
};
// Read-only carveouts are bind mounts and cannot themselves
// be renamed/unlinked. Continue checking above them, since a
// mutable ancestor could still redirect the installation path.
let is_read_only_mount = writable_roots.iter().any(|root| {
root.read_only_subpaths
.iter()
.any(|path| path.as_path() == component)
});
policy_can_write(parent)
&& !is_read_only_mount
&& current_user_can_modify(parent)
})
|| file_is_writable(&path)
|| path_is_replaceable(&path)
{
None
} else {
@@ -275,6 +312,24 @@ pub fn find_executable_in_search_paths(
})
}
fn current_user_can_replace_child(parent: &Path, child: &Path) -> bool {
if !current_user_can_modify(parent) {
return false;
}
// SAFETY: geteuid has no preconditions.
let euid = unsafe { libc::geteuid() };
if euid == 0 {
return true;
}
let Ok(parent_metadata) = std::fs::metadata(parent) else {
return true;
};
if parent_metadata.mode() & libc::S_ISVTX == 0 || parent_metadata.uid() == euid {
return true;
}
std::fs::symlink_metadata(child).map_or(true, |metadata| metadata.uid() == euid)
}
fn current_user_can_modify(path: &Path) -> bool {
let Ok(metadata) = std::fs::metadata(path) else {
return true;
codex-rs/sandboxing/src/bwrap_tests.rs
+68 -17
View File
@@ -119,7 +119,7 @@ fn does_not_treat_wsl2_or_native_linux_as_wsl1() {
#[test]
fn finds_first_executable_bwrap_in_joined_search_path() {
let temp_dir = tempdir().expect("temp dir");
let cwd = temp_dir.path().join("cwd");
let cwd = temp_dir.path().join("cwd:with-colon");
let first_dir = temp_dir.path().join("first");
let second_dir = temp_dir.path().join("second");
std::fs::create_dir_all(&cwd).expect("create cwd");
@@ -127,18 +127,22 @@ fn finds_first_executable_bwrap_in_joined_search_path() {
std::fs::create_dir_all(&second_dir).expect("create second dir");
std::fs::write(first_dir.join("bwrap"), "not executable").expect("write non-executable bwrap");
let expected_bwrap = write_named_fake_bwrap_in(&second_dir);
let search_path = std::env::join_paths([first_dir, second_dir]).expect("join search path");
assert_eq!(
find_executable_in_search_paths(
"bwrap",
std::env::split_paths(&search_path),
&cwd,
&PermissionProfile::read_only().file_system_sandbox_policy(),
&cwd,
),
Some(expected_bwrap)
);
for search_paths in [
vec![first_dir, second_dir],
vec![PathBuf::from("../first"), PathBuf::from("../second")],
] {
let search_path = std::env::join_paths(search_paths).expect("join search path");
assert_eq!(
find_executable_in_search_paths(
"bwrap",
std::env::split_paths(&search_path),
&cwd,
&PermissionProfile::read_only().file_system_sandbox_policy(),
&cwd,
),
Some(expected_bwrap.clone())
);
}
}
#[test]
@@ -257,6 +261,53 @@ fn skips_bwrap_in_symlinked_writable_root() {
);
}
#[test]
fn skips_writable_bwrap_symlink_to_read_only_target() {
let temp = tempdir().unwrap();
let bin = temp.path().join("bin");
std::fs::create_dir(&bin).unwrap();
let system_binary = std::fs::canonicalize("/bin/true").unwrap();
std::os::unix::fs::symlink(&system_binary, bin.join("bwrap")).unwrap();
let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry::new(
AbsolutePathBuf::try_from(bin.as_path()).unwrap().into(),
FileSystemAccessMode::Write,
)]);
assert_eq!(
find_executable_in_search_paths("bwrap", [bin], Path::new("/"), &policy, Path::new("/")),
None,
);
}
#[test]
fn skips_bwrap_through_symlinked_path_directory_into_writable_root() {
let temp = tempdir().unwrap();
let workspace = temp.path().join("workspace");
let bin = workspace.join("bin");
let alias = temp.path().join("alias");
std::fs::create_dir_all(&bin).unwrap();
std::os::unix::fs::symlink(&workspace, &alias).unwrap();
let system_binary = std::fs::canonicalize("/bin/true").unwrap();
std::os::unix::fs::symlink(&system_binary, bin.join("bwrap")).unwrap();
let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry::new(
AbsolutePathBuf::try_from(workspace.as_path())
.unwrap()
.into(),
FileSystemAccessMode::Write,
)]);
assert_eq!(
find_executable_in_search_paths(
"bwrap",
[alias.join("bin")],
Path::new("/"),
&policy,
Path::new("/"),
),
None,
);
}
#[test]
fn full_disk_write_rejects_owned_bwrap_even_when_chmod_read_only() {
use std::os::unix::fs::PermissionsExt;
@@ -302,14 +353,14 @@ fn root_write_preserves_a_read_only_system_installation() {
use codex_protocol::permissions::FileSystemPath;
use codex_protocol::permissions::FileSystemSpecialPath;
let temp = tempdir().unwrap();
// Use a system executable as a discovery-only candidate. Never execute it.
let system_binary = std::fs::canonicalize("/bin/true").unwrap();
let system_program = system_binary.file_name().unwrap().to_str().unwrap();
let system_search_path = system_binary.parent().unwrap().to_path_buf();
let protected_root = system_binary
.ancestors()
.find(|path| path.parent() == Some(Path::new("/")))
.unwrap();
std::os::unix::fs::symlink(&system_binary, temp.path().join("bwrap")).unwrap();
let policy = FileSystemSandboxPolicy::restricted(vec![
FileSystemSandboxEntry::new(
FileSystemPath::Special {
@@ -333,8 +384,8 @@ fn root_write_preserves_a_read_only_system_installation() {
policy.has_full_disk_write_access() && unsafe { libc::geteuid() } == 0;
assert_eq!(
find_executable_in_search_paths(
"bwrap",
[temp.path().to_path_buf()],
system_program,
[system_search_path.clone()],
Path::new("/"),
&policy,
Path::new("/"),