fix(linux-sandbox): mount /dev in bwrap sandbox (#12081)
## Summary - Updates the Linux bubblewrap sandbox args to mount a minimal `/dev` using `--dev /dev` instead of only binding `/dev/null`. tools needing entropy (git, crypto libs, etc.) can fail. - Changed mount order so `--dev /dev` is added before writable-root `--bind` mounts, preserving writable `/dev/*` submounts like `/dev/shm` ## Why Fixes sandboxed command failures when reading `/dev/urandom` (and similar standard device-node access). Fixes https://github.com/openai/codex/issues/12056
This commit is contained in:
parent
18eb640a47
commit
4fe99b086f
4 changed files with 145 additions and 28 deletions
11
.github/workflows/rust-ci.yml
vendored
11
.github/workflows/rust-ci.yml
vendored
|
|
@ -581,6 +581,17 @@ jobs:
|
|||
tool: nextest
|
||||
version: 0.9.103
|
||||
|
||||
- name: Enable unprivileged user namespaces (Linux)
|
||||
if: runner.os == 'Linux'
|
||||
run: |
|
||||
# Required for bubblewrap to work on Linux CI runners.
|
||||
sudo sysctl -w kernel.unprivileged_userns_clone=1
|
||||
# Ubuntu 24.04+ can additionally gate unprivileged user namespaces
|
||||
# behind AppArmor.
|
||||
if sudo sysctl -a 2>/dev/null | grep -q '^kernel.apparmor_restrict_unprivileged_userns'; then
|
||||
sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0
|
||||
fi
|
||||
|
||||
- name: tests
|
||||
id: test
|
||||
run: cargo nextest run --all-features --no-fail-fast --target ${{ matrix.target }} --cargo-profile ci-test --timings
|
||||
|
|
|
|||
|
|
@ -135,11 +135,12 @@ fn create_bwrap_flags(
|
|||
///
|
||||
/// The mount order is important:
|
||||
/// 1. `--ro-bind / /` makes the entire filesystem read-only.
|
||||
/// 2. `--bind <root> <root>` re-enables writes for allowed roots.
|
||||
/// 3. `--ro-bind <subpath> <subpath>` re-applies read-only protections under
|
||||
/// 2. `--dev /dev` mounts a minimal writable `/dev` with standard device nodes
|
||||
/// (including `/dev/urandom`) even under a read-only root.
|
||||
/// 3. `--bind <root> <root>` re-enables writes for allowed roots, including
|
||||
/// writable subpaths under `/dev` (for example, `/dev/shm`).
|
||||
/// 4. `--ro-bind <subpath> <subpath>` re-applies read-only protections under
|
||||
/// those writable roots so protected subpaths win.
|
||||
/// 4. `--dev-bind /dev/null /dev/null` preserves the common sink even under a
|
||||
/// read-only root.
|
||||
fn create_filesystem_args(sandbox_policy: &SandboxPolicy, cwd: &Path) -> Result<Vec<String>> {
|
||||
if !sandbox_policy.has_full_disk_read_access() {
|
||||
return Err(CodexErr::UnsupportedOperation(
|
||||
|
|
@ -151,12 +152,18 @@ fn create_filesystem_args(sandbox_policy: &SandboxPolicy, cwd: &Path) -> Result<
|
|||
let writable_roots = sandbox_policy.get_writable_roots_with_cwd(cwd);
|
||||
ensure_mount_targets_exist(&writable_roots)?;
|
||||
|
||||
let mut args = Vec::new();
|
||||
|
||||
// Read-only root, then selectively re-enable writes.
|
||||
args.push("--ro-bind".to_string());
|
||||
args.push("/".to_string());
|
||||
args.push("/".to_string());
|
||||
// Read-only root, then mount a minimal device tree.
|
||||
// In bubblewrap (`bubblewrap.c`, `SETUP_MOUNT_DEV`), `--dev /dev` creates
|
||||
// the standard minimal nodes: null, zero, full, random, urandom, and tty.
|
||||
// `/dev` must be mounted before writable roots so explicit `/dev/*`
|
||||
// writable binds remain visible.
|
||||
let mut args = vec![
|
||||
"--ro-bind".to_string(),
|
||||
"/".to_string(),
|
||||
"/".to_string(),
|
||||
"--dev".to_string(),
|
||||
"/dev".to_string(),
|
||||
];
|
||||
|
||||
for writable_root in &writable_roots {
|
||||
let root = writable_root.root.as_path();
|
||||
|
|
@ -180,12 +187,15 @@ fn create_filesystem_args(sandbox_policy: &SandboxPolicy, cwd: &Path) -> Result<
|
|||
}
|
||||
|
||||
if !subpath.exists() {
|
||||
if let Some(first_missing) = find_first_non_existent_component(&subpath)
|
||||
&& is_within_allowed_write_paths(&first_missing, &allowed_write_paths)
|
||||
// Keep this in the per-subpath loop: each protected subpath can have
|
||||
// a different first missing component that must be blocked
|
||||
// independently (for example, `/repo/.git` vs `/repo/.codex`).
|
||||
if let Some(first_missing_component) = find_first_non_existent_component(&subpath)
|
||||
&& is_within_allowed_write_paths(&first_missing_component, &allowed_write_paths)
|
||||
{
|
||||
args.push("--ro-bind".to_string());
|
||||
args.push("/dev/null".to_string());
|
||||
args.push(path_to_string(&first_missing));
|
||||
args.push(path_to_string(&first_missing_component));
|
||||
}
|
||||
continue;
|
||||
}
|
||||
|
|
@ -197,11 +207,6 @@ fn create_filesystem_args(sandbox_policy: &SandboxPolicy, cwd: &Path) -> Result<
|
|||
}
|
||||
}
|
||||
|
||||
// Ensure `/dev/null` remains usable regardless of the root bind.
|
||||
args.push("--dev-bind".to_string());
|
||||
args.push("/dev/null".to_string());
|
||||
args.push("/dev/null".to_string());
|
||||
|
||||
Ok(args)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -307,7 +307,9 @@ fn close_fd_or_panic(fd: libc::c_int, context: &str) {
|
|||
fn is_proc_mount_failure(stderr: &str) -> bool {
|
||||
stderr.contains("Can't mount proc")
|
||||
&& stderr.contains("/newroot/proc")
|
||||
&& stderr.contains("Invalid argument")
|
||||
&& (stderr.contains("Invalid argument")
|
||||
|| stderr.contains("Operation not permitted")
|
||||
|| stderr.contains("Permission denied"))
|
||||
}
|
||||
|
||||
/// Build the inner command that applies seccomp after bubblewrap.
|
||||
|
|
@ -381,6 +383,18 @@ mod tests {
|
|||
assert_eq!(is_proc_mount_failure(stderr), true);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn detects_proc_mount_operation_not_permitted_failure() {
|
||||
let stderr = "bwrap: Can't mount proc on /newroot/proc: Operation not permitted";
|
||||
assert_eq!(is_proc_mount_failure(stderr), true);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn detects_proc_mount_permission_denied_failure() {
|
||||
let stderr = "bwrap: Can't mount proc on /newroot/proc: Permission denied";
|
||||
assert_eq!(is_proc_mount_failure(stderr), true);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn ignores_non_proc_mount_errors() {
|
||||
let stderr = "bwrap: Can't bind mount /dev/null: Operation not permitted";
|
||||
|
|
@ -407,9 +421,8 @@ mod tests {
|
|||
"--ro-bind".to_string(),
|
||||
"/".to_string(),
|
||||
"/".to_string(),
|
||||
"--dev-bind".to_string(),
|
||||
"/dev/null".to_string(),
|
||||
"/dev/null".to_string(),
|
||||
"--dev".to_string(),
|
||||
"/dev".to_string(),
|
||||
"--unshare-pid".to_string(),
|
||||
"--proc".to_string(),
|
||||
"/proc".to_string(),
|
||||
|
|
|
|||
|
|
@ -56,7 +56,7 @@ async fn run_cmd_output(
|
|||
writable_roots: &[PathBuf],
|
||||
timeout_ms: u64,
|
||||
) -> codex_core::exec::ExecToolCallOutput {
|
||||
run_cmd_result_with_writable_roots(cmd, writable_roots, timeout_ms, false)
|
||||
run_cmd_result_with_writable_roots(cmd, writable_roots, timeout_ms, false, false)
|
||||
.await
|
||||
.expect("sandboxed command should execute")
|
||||
}
|
||||
|
|
@ -67,6 +67,7 @@ async fn run_cmd_result_with_writable_roots(
|
|||
writable_roots: &[PathBuf],
|
||||
timeout_ms: u64,
|
||||
use_bwrap_sandbox: bool,
|
||||
network_access: bool,
|
||||
) -> Result<codex_core::exec::ExecToolCallOutput> {
|
||||
let cwd = std::env::current_dir().expect("cwd should exist");
|
||||
let sandbox_cwd = cwd.clone();
|
||||
|
|
@ -89,7 +90,7 @@ async fn run_cmd_result_with_writable_roots(
|
|||
.map(|p| AbsolutePathBuf::try_from(p.as_path()).unwrap())
|
||||
.collect(),
|
||||
read_only_access: Default::default(),
|
||||
network_access: false,
|
||||
network_access,
|
||||
// Exclude tmp-related folders from writable roots because we need a
|
||||
// folder that is writable by tests but that we intentionally disallow
|
||||
// writing to in the sandbox.
|
||||
|
|
@ -112,6 +113,13 @@ async fn run_cmd_result_with_writable_roots(
|
|||
|
||||
fn is_bwrap_unavailable_output(output: &codex_core::exec::ExecToolCallOutput) -> bool {
|
||||
output.stderr.text.contains(BWRAP_UNAVAILABLE_ERR)
|
||||
|| (output
|
||||
.stderr
|
||||
.text
|
||||
.contains("Can't mount proc on /newroot/proc")
|
||||
&& (output.stderr.text.contains("Operation not permitted")
|
||||
|| output.stderr.text.contains("Permission denied")
|
||||
|| output.stderr.text.contains("Invalid argument")))
|
||||
}
|
||||
|
||||
async fn should_skip_bwrap_tests() -> bool {
|
||||
|
|
@ -120,6 +128,7 @@ async fn should_skip_bwrap_tests() -> bool {
|
|||
&[],
|
||||
NETWORK_TIMEOUT_MS,
|
||||
true,
|
||||
true,
|
||||
)
|
||||
.await
|
||||
{
|
||||
|
|
@ -168,14 +177,90 @@ async fn test_root_write() {
|
|||
|
||||
#[tokio::test]
|
||||
async fn test_dev_null_write() {
|
||||
run_cmd(
|
||||
if should_skip_bwrap_tests().await {
|
||||
eprintln!("skipping bwrap test: bwrap sandbox prerequisites are unavailable");
|
||||
return;
|
||||
}
|
||||
|
||||
let output = run_cmd_result_with_writable_roots(
|
||||
&["bash", "-lc", "echo blah > /dev/null"],
|
||||
&[],
|
||||
// We have seen timeouts when running this test in CI on GitHub,
|
||||
// so we are using a generous timeout until we can diagnose further.
|
||||
LONG_TIMEOUT_MS,
|
||||
true,
|
||||
true,
|
||||
)
|
||||
.await;
|
||||
.await
|
||||
.expect("sandboxed command should execute");
|
||||
|
||||
assert_eq!(output.exit_code, 0);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn bwrap_populates_minimal_dev_nodes() {
|
||||
if should_skip_bwrap_tests().await {
|
||||
eprintln!("skipping bwrap test: bwrap sandbox prerequisites are unavailable");
|
||||
return;
|
||||
}
|
||||
|
||||
let output = run_cmd_result_with_writable_roots(
|
||||
&[
|
||||
"bash",
|
||||
"-lc",
|
||||
"for node in null zero full random urandom tty; do [ -c \"/dev/$node\" ] || { echo \"missing /dev/$node\" >&2; exit 1; }; done",
|
||||
],
|
||||
&[],
|
||||
LONG_TIMEOUT_MS,
|
||||
true,
|
||||
true,
|
||||
)
|
||||
.await
|
||||
.expect("sandboxed command should execute");
|
||||
|
||||
assert_eq!(output.exit_code, 0);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn bwrap_preserves_writable_dev_shm_bind_mount() {
|
||||
if should_skip_bwrap_tests().await {
|
||||
eprintln!("skipping bwrap test: bwrap sandbox prerequisites are unavailable");
|
||||
return;
|
||||
}
|
||||
if !std::path::Path::new("/dev/shm").exists() {
|
||||
eprintln!("skipping bwrap test: /dev/shm is unavailable in this environment");
|
||||
return;
|
||||
}
|
||||
|
||||
let target_file = match NamedTempFile::new_in("/dev/shm") {
|
||||
Ok(file) => file,
|
||||
Err(err) => {
|
||||
eprintln!("skipping bwrap test: failed to create /dev/shm temp file: {err}");
|
||||
return;
|
||||
}
|
||||
};
|
||||
let target_path = target_file.path().to_path_buf();
|
||||
std::fs::write(&target_path, "host-before").expect("seed /dev/shm file");
|
||||
|
||||
let output = run_cmd_result_with_writable_roots(
|
||||
&[
|
||||
"bash",
|
||||
"-lc",
|
||||
&format!("printf sandbox-after > {}", target_path.to_string_lossy()),
|
||||
],
|
||||
&[PathBuf::from("/dev/shm")],
|
||||
LONG_TIMEOUT_MS,
|
||||
true,
|
||||
true,
|
||||
)
|
||||
.await
|
||||
.expect("sandboxed command should execute");
|
||||
|
||||
assert_eq!(output.exit_code, 0);
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(&target_path).expect("read /dev/shm file"),
|
||||
"sandbox-after"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
|
@ -306,7 +391,7 @@ async fn sandbox_blocks_nc() {
|
|||
#[tokio::test]
|
||||
async fn sandbox_blocks_git_and_codex_writes_inside_writable_root() {
|
||||
if should_skip_bwrap_tests().await {
|
||||
eprintln!("skipping bwrap test: vendored bwrap was not built in this environment");
|
||||
eprintln!("skipping bwrap test: bwrap sandbox prerequisites are unavailable");
|
||||
return;
|
||||
}
|
||||
|
||||
|
|
@ -329,6 +414,7 @@ async fn sandbox_blocks_git_and_codex_writes_inside_writable_root() {
|
|||
&[tmpdir.path().to_path_buf()],
|
||||
LONG_TIMEOUT_MS,
|
||||
true,
|
||||
true,
|
||||
)
|
||||
.await,
|
||||
".git write should be denied under bubblewrap",
|
||||
|
|
@ -344,6 +430,7 @@ async fn sandbox_blocks_git_and_codex_writes_inside_writable_root() {
|
|||
&[tmpdir.path().to_path_buf()],
|
||||
LONG_TIMEOUT_MS,
|
||||
true,
|
||||
true,
|
||||
)
|
||||
.await,
|
||||
".codex write should be denied under bubblewrap",
|
||||
|
|
@ -355,7 +442,7 @@ async fn sandbox_blocks_git_and_codex_writes_inside_writable_root() {
|
|||
#[tokio::test]
|
||||
async fn sandbox_blocks_codex_symlink_replacement_attack() {
|
||||
if should_skip_bwrap_tests().await {
|
||||
eprintln!("skipping bwrap test: vendored bwrap was not built in this environment");
|
||||
eprintln!("skipping bwrap test: bwrap sandbox prerequisites are unavailable");
|
||||
return;
|
||||
}
|
||||
|
||||
|
|
@ -380,6 +467,7 @@ async fn sandbox_blocks_codex_symlink_replacement_attack() {
|
|||
&[tmpdir.path().to_path_buf()],
|
||||
LONG_TIMEOUT_MS,
|
||||
true,
|
||||
true,
|
||||
)
|
||||
.await,
|
||||
".codex symlink replacement should be denied",
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue