mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Do not grant Windows sandbox ACLs on USERPROFILE (#18443)
## Stack 1. This PR: expand and filter `USERPROFILE` roots. 2. Follow-up: #18493 filters SSH config dependency roots on top of this base. ## Bug On Windows, Codex can grant the sandbox ACL access to the whole user profile directory. That means the sandbox ACL can be applied under paths like: ```text C:\Users\me\.ssh C:\Users\me\.tsh ``` This breaks SSH. Windows OpenSSH checks permissions on SSH config and key material. If Codex adds a sandbox group ACL to those files, OpenSSH can reject the config or keys. The bad interaction is: 1. Codex asks the Windows sandbox to grant access to `USERPROFILE`. 2. The sandbox applies ACLs under that root. 3. SSH-owned files get an extra ACL entry. 4. OpenSSH rejects those files because their permissions are no longer strict enough. ## Why this happens more now Codex now has more flows that naturally start in the user profile: - a new chat can start in the user directory - a project can be rooted in the user directory - a user can start the Codex CLI from the user directory Those are valid user actions. The bug is that `USERPROFILE` is too broad a sandbox root. ## Change This PR keeps the useful behavior of starting from the user profile without granting the profile root itself. The new flow is: 1. collect the normal read and write roots 2. if a root is exactly `USERPROFILE`, replace it with the direct children of `USERPROFILE` 3. remove `USERPROFILE` itself from the final root list 4. apply the existing user-profile read exclusions to both read and write roots 5. add `.tsh` and `.brev` to that exclusion list So this input: ```text C:\Users\me ``` becomes roots like: ```text C:\Users\me\Desktop C:\Users\me\Documents C:\Users\me\Downloads ``` and does not include: ```text C:\Users\me C:\Users\me\.ssh C:\Users\me\.tsh C:\Users\me\.brev ``` If `USERPROFILE` cannot be listed, expansion falls back to the profile root and the later filter removes it. That keeps the failure mode closed for this bug. ## Why this shape The sandbox still gets access to ordinary profile folders when the user starts from home. The sandbox no longer grants access to the profile root itself. All filtering happens after expansion, for both read and write roots. That gives us one simple rule: expand broad profile grants first, then remove roots the sandbox must not own. ## Tests - `just fmt` - `cargo test -p codex-windows-sandbox` - `just fix -p codex-windows-sandbox` - `git diff --check`
This commit is contained in:
committed by
GitHub
Unverified
parent
ce0e28ea6f
commit
715fafa23c
@@ -40,8 +40,10 @@ pub const ONLINE_USERNAME: &str = "CodexSandboxOnline";
|
||||
const ERROR_CANCELLED: u32 = 1223;
|
||||
const SECURITY_BUILTIN_DOMAIN_RID: u32 = 0x0000_0020;
|
||||
const DOMAIN_ALIAS_RID_ADMINS: u32 = 0x0000_0220;
|
||||
const USERPROFILE_READ_ROOT_EXCLUSIONS: &[&str] = &[
|
||||
const USERPROFILE_ROOT_EXCLUSIONS: &[&str] = &[
|
||||
".ssh",
|
||||
".tsh",
|
||||
".brev",
|
||||
".gnupg",
|
||||
".aws",
|
||||
".azure",
|
||||
@@ -333,7 +335,7 @@ fn profile_read_roots(user_profile: &Path) -> Vec<PathBuf> {
|
||||
.map(|entry| (entry.file_name(), entry.path()))
|
||||
.filter(|(name, _)| {
|
||||
let name = name.to_string_lossy();
|
||||
!USERPROFILE_READ_ROOT_EXCLUSIONS
|
||||
!USERPROFILE_ROOT_EXCLUSIONS
|
||||
.iter()
|
||||
.any(|excluded| name.eq_ignore_ascii_case(excluded))
|
||||
})
|
||||
@@ -787,6 +789,9 @@ fn build_payload_roots(
|
||||
request.env_map,
|
||||
)
|
||||
};
|
||||
let write_roots = expand_user_profile_root(write_roots);
|
||||
let write_roots = filter_user_profile_root(write_roots);
|
||||
let write_roots = filter_user_profile_root_exclusions(write_roots);
|
||||
let write_roots = filter_sensitive_write_roots(write_roots, request.codex_home);
|
||||
let mut read_roots = if let Some(roots) = overrides.read_roots.as_deref() {
|
||||
// An explicit override is the split policy's complete readable set. Keep only the
|
||||
@@ -804,6 +809,9 @@ fn build_payload_roots(
|
||||
} else {
|
||||
gather_read_roots(request.command_cwd, request.policy, request.codex_home)
|
||||
};
|
||||
read_roots = expand_user_profile_root(read_roots);
|
||||
read_roots = filter_user_profile_root(read_roots);
|
||||
read_roots = filter_user_profile_root_exclusions(read_roots);
|
||||
let write_root_set: HashSet<PathBuf> = write_roots.iter().cloned().collect();
|
||||
read_roots.retain(|root| !write_root_set.contains(root));
|
||||
(read_roots, write_roots)
|
||||
@@ -834,6 +842,67 @@ fn build_payload_deny_write_paths(
|
||||
deny_write_paths
|
||||
}
|
||||
|
||||
fn expand_user_profile_root(roots: Vec<PathBuf>) -> Vec<PathBuf> {
|
||||
let Ok(user_profile) = std::env::var("USERPROFILE") else {
|
||||
return roots;
|
||||
};
|
||||
expand_user_profile_root_for(roots, Path::new(&user_profile))
|
||||
}
|
||||
|
||||
fn expand_user_profile_root_for(roots: Vec<PathBuf>, user_profile: &Path) -> Vec<PathBuf> {
|
||||
let user_profile_key = canonical_path_key(user_profile);
|
||||
let mut expanded = Vec::new();
|
||||
for root in roots {
|
||||
if canonical_path_key(&root) == user_profile_key {
|
||||
expanded.extend(profile_read_roots(user_profile));
|
||||
} else {
|
||||
expanded.push(root);
|
||||
}
|
||||
}
|
||||
|
||||
expanded.sort_by_key(|root| canonical_path_key(root));
|
||||
expanded.dedup_by(|a, b| canonical_path_key(a.as_path()) == canonical_path_key(b.as_path()));
|
||||
expanded
|
||||
}
|
||||
|
||||
fn filter_user_profile_root(mut roots: Vec<PathBuf>) -> Vec<PathBuf> {
|
||||
let Ok(user_profile) = std::env::var("USERPROFILE") else {
|
||||
return roots;
|
||||
};
|
||||
let user_profile_key = canonical_path_key(Path::new(&user_profile));
|
||||
roots.retain(|root| canonical_path_key(root) != user_profile_key);
|
||||
roots
|
||||
}
|
||||
|
||||
fn filter_user_profile_root_exclusions(mut roots: Vec<PathBuf>) -> Vec<PathBuf> {
|
||||
let Ok(user_profile) = std::env::var("USERPROFILE") else {
|
||||
return roots;
|
||||
};
|
||||
let user_profile = Path::new(&user_profile);
|
||||
roots.retain(|root| !is_user_profile_root_exclusion(root, user_profile));
|
||||
roots
|
||||
}
|
||||
|
||||
fn is_user_profile_root_exclusion(root: &Path, user_profile: &Path) -> bool {
|
||||
let root_key = canonical_path_key(root);
|
||||
let profile_key = canonical_path_key(user_profile);
|
||||
let profile_prefix = format!("{}/", profile_key.trim_end_matches('/'));
|
||||
let Some(relative_key) = root_key.strip_prefix(&profile_prefix) else {
|
||||
return false;
|
||||
};
|
||||
let Some(child_name) = relative_key
|
||||
.split('/')
|
||||
.next()
|
||||
.filter(|name| !name.is_empty())
|
||||
else {
|
||||
return false;
|
||||
};
|
||||
|
||||
USERPROFILE_ROOT_EXCLUSIONS
|
||||
.iter()
|
||||
.any(|excluded| child_name.eq_ignore_ascii_case(excluded))
|
||||
}
|
||||
|
||||
fn filter_sensitive_write_roots(mut roots: Vec<PathBuf>, codex_home: &Path) -> Vec<PathBuf> {
|
||||
// Never grant capability write access to CODEX_HOME or anything under CODEX_HOME/.sandbox,
|
||||
// CODEX_HOME/.sandbox-bin, or CODEX_HOME/.sandbox-secrets. These locations contain sandbox
|
||||
@@ -1029,13 +1098,15 @@ mod tests {
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
let user_profile = tmp.path();
|
||||
let allowed_dir = user_profile.join("Documents");
|
||||
let allowed_file = user_profile.join(".gitconfig");
|
||||
let allowed_file = user_profile.join("settings.json");
|
||||
let excluded_dir = user_profile.join(".ssh");
|
||||
let excluded_tsh = user_profile.join(".tsh");
|
||||
let excluded_case_variant = user_profile.join(".AWS");
|
||||
|
||||
fs::create_dir_all(&allowed_dir).expect("create allowed dir");
|
||||
fs::write(&allowed_file, "safe").expect("create allowed file");
|
||||
fs::create_dir_all(&excluded_dir).expect("create excluded dir");
|
||||
fs::create_dir_all(&excluded_tsh).expect("create excluded tsh dir");
|
||||
fs::create_dir_all(&excluded_case_variant).expect("create excluded case variant");
|
||||
|
||||
let roots = profile_read_roots(user_profile);
|
||||
@@ -1055,6 +1126,83 @@ mod tests {
|
||||
assert_eq!(vec![missing_profile], roots);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn is_user_profile_root_exclusion_blocks_configured_children() {
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
let user_profile = tmp.path().join("user-profile");
|
||||
let documents = user_profile.join("Documents");
|
||||
let app_data = user_profile.join("AppData");
|
||||
let ssh_child = user_profile.join(".ssh").join("config");
|
||||
let tsh_child = user_profile.join(".tsh").join("keys");
|
||||
let other_root = tmp.path().join("other-root");
|
||||
fs::create_dir_all(&documents).expect("create documents");
|
||||
fs::create_dir_all(&app_data).expect("create app data");
|
||||
fs::create_dir_all(&ssh_child).expect("create ssh child");
|
||||
fs::create_dir_all(&tsh_child).expect("create tsh child");
|
||||
fs::create_dir_all(&other_root).expect("create other root");
|
||||
|
||||
assert!(!super::is_user_profile_root_exclusion(
|
||||
&documents,
|
||||
&user_profile
|
||||
));
|
||||
assert!(!super::is_user_profile_root_exclusion(
|
||||
&app_data,
|
||||
&user_profile
|
||||
));
|
||||
assert!(super::is_user_profile_root_exclusion(
|
||||
&ssh_child,
|
||||
&user_profile
|
||||
));
|
||||
assert!(super::is_user_profile_root_exclusion(
|
||||
&tsh_child,
|
||||
&user_profile
|
||||
));
|
||||
assert!(!super::is_user_profile_root_exclusion(
|
||||
&other_root,
|
||||
&user_profile
|
||||
));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn expand_user_profile_root_for_replaces_profile_root_with_children() {
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
let user_profile = tmp.path().join("user-profile");
|
||||
let documents = user_profile.join("Documents");
|
||||
let excluded = user_profile.join(".local");
|
||||
let other_root = tmp.path().join("other-root");
|
||||
fs::create_dir_all(&documents).expect("create documents");
|
||||
fs::create_dir_all(&excluded).expect("create excluded dir");
|
||||
fs::create_dir_all(&other_root).expect("create other root");
|
||||
|
||||
let roots = super::expand_user_profile_root_for(
|
||||
vec![user_profile.clone(), other_root.clone()],
|
||||
&user_profile,
|
||||
);
|
||||
let actual: HashSet<PathBuf> = roots.into_iter().collect();
|
||||
let expected: HashSet<PathBuf> = [documents, excluded, other_root].into_iter().collect();
|
||||
|
||||
assert_eq!(expected, actual);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn expanded_write_roots_still_drop_protected_codex_home() {
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
let user_profile = tmp.path().join("user-profile");
|
||||
let codex_home = user_profile.join("CodexHome");
|
||||
let documents = user_profile.join("Documents");
|
||||
fs::create_dir_all(&codex_home).expect("create codex home");
|
||||
fs::create_dir_all(&documents).expect("create documents");
|
||||
|
||||
let mut roots =
|
||||
super::expand_user_profile_root_for(vec![user_profile.clone()], &user_profile);
|
||||
let user_profile_key = super::canonical_path_key(&user_profile);
|
||||
roots.retain(|root| super::canonical_path_key(root) != user_profile_key);
|
||||
roots.retain(|root| !super::is_user_profile_root_exclusion(root, &user_profile));
|
||||
let roots = super::filter_sensitive_write_roots(roots, &codex_home);
|
||||
|
||||
assert_eq!(vec![documents], roots);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn gather_read_roots_includes_helper_bin_dir() {
|
||||
let tmp = TempDir::new().expect("tempdir");
|
||||
|
||||
Reference in New Issue
Block a user