Skip to content

Commit d73e276

Browse files
yuzhu-oaicopyberry
yuzhu-oai
authored and
copyberry
committed
Fix Linux sandbox startup with multiple denied files
## Why Bubblewrap consumes and closes the file descriptor for each `--ro-bind-data` mount. Reusing one descriptor across multiple file masks prevents the sandbox from starting. ## What changed Open and preserve a separate `/dev/null` descriptor for each empty-file mask. ## Testing Extend unit tests to verify distinct descriptors for multiple denied files and missing-path masks. Add integration coverage for exact-path and glob deny rules, checking that the sandbox starts, denied files remain unreadable and unwritable, and allowed files remain accessible. GitOrigin-RevId: 85158c83699afd36bb7aca453b2652d777cf65e4
1 parent b06b7d2 commit d73e276

3 files changed

Lines changed: 184 additions & 15 deletions

File tree

‎codex-rs/linux-sandbox/src/bwrap.rs‎

Lines changed: 54 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1253,10 +1253,10 @@ fn append_read_only_subpath_args(
12531253
}
12541254

12551255
fn append_empty_file_bind_data_args(bwrap_args: &mut BwrapArgs, path: &Path) -> Result<()> {
1256-
if bwrap_args.preserved_files.is_empty() {
1257-
bwrap_args.preserved_files.push(File::open("/dev/null")?);
1258-
}
1259-
let null_fd = bwrap_args.preserved_files[0].as_raw_fd().to_string();
1256+
// Bubblewrap consumes and closes the descriptor for each bind-data mount.
1257+
let null_file = File::open("/dev/null")?;
1258+
let null_fd = null_file.as_raw_fd().to_string();
1259+
bwrap_args.preserved_files.push(null_file);
12601260
bwrap_args.args.push("--ro-bind-data".to_string());
12611261
bwrap_args.args.push(null_fd);
12621262
bwrap_args.args.push(path_to_string(path));
@@ -2005,6 +2005,7 @@ mod tests {
20052005
let temp_dir = TempDir::new().expect("temp dir");
20062006
let workspace = temp_dir.path().join("workspace");
20072007
let blocked = workspace.join("blocked");
2008+
let second_blocked = workspace.join("second-blocked");
20082009
std::fs::create_dir_all(&workspace).expect("create workspace");
20092010

20102011
let workspace_root =
@@ -2021,28 +2022,38 @@ mod tests {
20212022
access: FileSystemAccessMode::Read,
20222023
missing_path_behavior: None,
20232024
},
2025+
FileSystemSandboxEntry {
2026+
path: AbsolutePathBuf::from_absolute_path(&second_blocked)
2027+
.expect("absolute second blocked")
2028+
.into(),
2029+
access: FileSystemAccessMode::Read,
2030+
missing_path_behavior: None,
2031+
},
20242032
]);
20252033

20262034
let args = create_filesystem_args(&policy, temp_dir.path(), BwrapOptions::default())
20272035
.expect("filesystem args");
20282036

20292037
assert_empty_file_bound_without_perms(&args.args, &blocked);
2038+
assert_empty_file_bound_without_perms(&args.args, &second_blocked);
20302039
assert_empty_directory_mounted_read_only(&args.args, &workspace.join(".git"));
20312040
assert_empty_directory_mounted_read_only(&args.args, &workspace.join(".agents"));
20322041
assert_empty_directory_mounted_read_only(&args.args, &workspace.join(".codex"));
2033-
assert_eq!(args.preserved_files.len(), 1);
2042+
assert_eq!(args.preserved_files.len(), 2);
2043+
assert_bind_data_uses_distinct_preserved_fds(&args);
20342044
assert_eq!(
20352045
synthetic_mount_target_paths(&args),
20362046
vec![
20372047
blocked.clone(),
2048+
second_blocked.clone(),
20382049
workspace.join(".git"),
20392050
workspace.join(".agents"),
20402051
workspace.join(".codex"),
20412052
workspace.join(".aws"),
20422053
]
20432054
);
20442055
assert!(
2045-
!blocked.exists(),
2056+
!blocked.exists() && !second_blocked.exists(),
20462057
"missing path mask should not materialize host-side metadata paths at arg construction time",
20472058
);
20482059
}
@@ -2848,7 +2859,9 @@ mod tests {
28482859
fn split_policy_masks_root_read_file_carveouts() {
28492860
let temp_dir = TempDir::new().expect("temp dir");
28502861
let blocked_file = temp_dir.path().join("blocked.txt");
2862+
let second_blocked_file = temp_dir.path().join("second-blocked.txt");
28512863
std::fs::write(&blocked_file, "secret").expect("create blocked file");
2864+
std::fs::write(&second_blocked_file, "dummy secret").expect("create second blocked file");
28522865
let blocked_file =
28532866
AbsolutePathBuf::from_absolute_path(&blocked_file).expect("absolute blocked file");
28542867
let policy = FileSystemSandboxPolicy::restricted(vec![
@@ -2864,20 +2877,46 @@ mod tests {
28642877
access: FileSystemAccessMode::Deny,
28652878
missing_path_behavior: None,
28662879
},
2880+
FileSystemSandboxEntry {
2881+
path: AbsolutePathBuf::from_absolute_path(&second_blocked_file)
2882+
.expect("absolute second blocked file")
2883+
.into(),
2884+
access: FileSystemAccessMode::Deny,
2885+
missing_path_behavior: None,
2886+
},
28672887
]);
28682888

28692889
let args = create_filesystem_args(&policy, temp_dir.path(), BwrapOptions::default())
28702890
.expect("filesystem args");
2871-
let blocked_file_str = path_to_string(blocked_file.as_path());
2872-
2873-
assert_eq!(args.preserved_files.len(), 1);
2891+
assert_eq!(args.preserved_files.len(), 2);
2892+
assert_bind_data_uses_distinct_preserved_fds(&args);
28742893
assert!(args.synthetic_mount_targets.is_empty());
2875-
assert!(args.args.windows(5).any(|window| {
2876-
window[0] == "--perms"
2877-
&& window[1] == "000"
2878-
&& window[2] == "--ro-bind-data"
2879-
&& window[4] == blocked_file_str
2880-
}));
2894+
for blocked_file in [blocked_file.as_path(), second_blocked_file.as_path()] {
2895+
assert!(args.args.windows(5).any(|window| {
2896+
window[0] == "--perms"
2897+
&& window[1] == "000"
2898+
&& window[2] == "--ro-bind-data"
2899+
&& window[4] == path_to_string(blocked_file)
2900+
}));
2901+
}
2902+
}
2903+
2904+
fn assert_bind_data_uses_distinct_preserved_fds(args: &BwrapArgs) {
2905+
let mount_fds = args
2906+
.args
2907+
.windows(3)
2908+
.filter(|window| window[0] == "--ro-bind-data")
2909+
.map(|window| window[1].parse::<i32>().expect("bind-data fd"))
2910+
.collect::<Vec<_>>();
2911+
let distinct_fds = mount_fds.iter().copied().collect::<HashSet<_>>();
2912+
assert_eq!(mount_fds.len(), distinct_fds.len());
2913+
assert_eq!(
2914+
distinct_fds,
2915+
args.preserved_files
2916+
.iter()
2917+
.map(AsRawFd::as_raw_fd)
2918+
.collect()
2919+
);
28812920
}
28822921

28832922
#[test]

‎codex-rs/linux-sandbox/tests/suite/denied_files_tests.rs‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
//! Multiple denied files must not prevent sandbox startup or expose protected contents.
2+
3+
use super::LONG_TIMEOUT_MS;
4+
use super::codex_linux_sandbox_exe;
5+
use super::create_env_from_core_vars;
6+
use super::run_cmd_result_with_permission_profile_for_cwd;
7+
use super::should_skip_bwrap_tests;
8+
use codex_protocol::models::PermissionProfile;
9+
use codex_protocol::permissions::FileSystemAccessMode;
10+
use codex_protocol::permissions::FileSystemPath;
11+
use codex_protocol::permissions::FileSystemSandboxEntry;
12+
use codex_protocol::permissions::FileSystemSandboxPolicy;
13+
use codex_protocol::permissions::FileSystemSpecialPath;
14+
use codex_protocol::permissions::NetworkSandboxPolicy;
15+
use codex_utils_absolute_path::AbsolutePathBuf;
16+
use pretty_assertions::assert_eq;
17+
18+
enum DeniedFileRules {
19+
ExactPaths,
20+
Globs,
21+
}
22+
23+
#[test_case::test_case(DeniedFileRules::ExactPaths; "exact_paths")]
24+
#[test_case::test_case(DeniedFileRules::Globs; "globs")]
25+
#[tokio::test]
26+
async fn sandbox_starts_with_multiple_denied_files(rules: DeniedFileRules) {
27+
if should_skip_bwrap_tests().await {
28+
eprintln!("skipping bwrap test: bwrap sandbox prerequisites are unavailable");
29+
return;
30+
}
31+
32+
let temp = tempfile::tempdir().expect("tempdir");
33+
let workspace = AbsolutePathBuf::try_from(temp.path()).expect("absolute workspace");
34+
std::fs::create_dir(workspace.join("nested")).expect("create nested directory");
35+
std::fs::write(workspace.join("AGENTS.md"), "project instructions\n")
36+
.expect("write allowed instructions");
37+
let denied_files = [
38+
(workspace.join("one.key"), "dummy first key"),
39+
(workspace.join("nested/two.key"), "dummy second key"),
40+
(workspace.join(".env.local"), "dummy environment"),
41+
];
42+
for (path, contents) in &denied_files {
43+
std::fs::write(path, contents).expect("write denied dummy file");
44+
}
45+
46+
let sandbox_helper = codex_linux_sandbox_exe();
47+
let helper_dir = AbsolutePathBuf::try_from(sandbox_helper.parent().expect("helper parent"))
48+
.expect("absolute helper directory");
49+
let mut entries = vec![
50+
FileSystemSandboxEntry::new(
51+
FileSystemPath::Special {
52+
value: FileSystemSpecialPath::Minimal,
53+
},
54+
FileSystemAccessMode::Read,
55+
),
56+
FileSystemSandboxEntry::new(helper_dir.into(), FileSystemAccessMode::Read),
57+
FileSystemSandboxEntry::new(workspace.clone().into(), FileSystemAccessMode::Write),
58+
];
59+
match rules {
60+
DeniedFileRules::ExactPaths => {
61+
entries.extend(denied_files.iter().map(|(path, _)| {
62+
FileSystemSandboxEntry::new(path.clone().into(), FileSystemAccessMode::Deny)
63+
}));
64+
}
65+
DeniedFileRules::Globs => {
66+
for suffix in ["**/*.key", "**/.env.local"] {
67+
entries.push(FileSystemSandboxEntry::new(
68+
FileSystemPath::GlobPattern {
69+
pattern: format!("{}/{suffix}", workspace.display()),
70+
},
71+
FileSystemAccessMode::Deny,
72+
));
73+
}
74+
}
75+
}
76+
let permission_profile = PermissionProfile::from_runtime_permissions(
77+
&FileSystemSandboxPolicy::restricted(entries),
78+
NetworkSandboxPolicy::Enabled,
79+
);
80+
let output = run_cmd_result_with_permission_profile_for_cwd(
81+
&[
82+
"/bin/sh",
83+
"-c",
84+
r#"set -eu
85+
cat AGENTS.md
86+
for blocked in one.key nested/two.key .env.local; do
87+
if (: < "$blocked") 2>/dev/null; then
88+
printf 'read unexpectedly allowed: %s\n' "$blocked" >&2
89+
exit 10
90+
fi
91+
if (printf changed > "$blocked") 2>/dev/null; then
92+
printf 'write unexpectedly allowed: %s\n' "$blocked" >&2
93+
exit 11
94+
fi
95+
done
96+
printf 'allowed\n' > allowed.txt
97+
cat allowed.txt
98+
"#,
99+
],
100+
workspace.clone(),
101+
permission_profile,
102+
create_env_from_core_vars(),
103+
LONG_TIMEOUT_MS,
104+
/*use_legacy_landlock*/ false,
105+
)
106+
.await
107+
.expect("sandbox should start with multiple denied files");
108+
109+
assert_eq!(
110+
(output.exit_code, output.stdout.text, output.stderr.text),
111+
(
112+
0,
113+
"project instructions\nallowed\n".to_string(),
114+
String::new()
115+
)
116+
);
117+
assert_eq!(
118+
std::fs::read_to_string(workspace.join("allowed.txt")).expect("read allowed file"),
119+
"allowed\n"
120+
);
121+
for (path, contents) in &denied_files {
122+
assert_eq!(
123+
std::fs::read_to_string(path).expect("read host dummy file"),
124+
*contents
125+
);
126+
}
127+
}

‎codex-rs/linux-sandbox/tests/suite/landlock.rs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@ mod nested_metadata_tests;
3434
#[path = "root_metadata_tests.rs"]
3535
mod root_metadata_tests;
3636

37+
#[path = "denied_files_tests.rs"]
38+
mod denied_files_tests;
39+
3740
// At least on GitHub CI, the arm64 tests appear to need longer timeouts.
3841

3942
#[cfg(not(target_arch = "aarch64"))]

0 commit comments

Comments
 (0)