From b885c3f8b1dc66cf1f89b55f06f8f55c293a846a Mon Sep 17 00:00:00 2001 From: efrazer-oai Date: Sun, 19 Apr 2026 14:58:33 -0700 Subject: [PATCH] Filter Windows sandbox roots from SSH config dependencies (#18493) ## Stack 1. Base PR: #18443 stops granting ACLs on `USERPROFILE`. 2. This PR: filters additional SSH-owned profile roots discovered from SSH config. ## Bug The base PR removes the broadest bad grant: `USERPROFILE` itself. That still leaves one important case. A user profile child can be SSH-owned even when its name is not one of our fixed exclusions. For example: ```sshconfig Host devbox IdentityFile ~/.keys/devbox CertificateFile ~/.certs/devbox-cert.pub UserKnownHostsFile ~/.known_hosts_custom Include ~/.ssh/conf.d/*.conf ``` After profile expansion, the sandbox might see these as normal profile children: ```text C:\Users\me\.keys C:\Users\me\.certs C:\Users\me\.known_hosts_custom C:\Users\me\.ssh ``` Those paths have another owner: OpenSSH and the tools that manage SSH identity and host-key state. Codex should not add sandbox ACLs to them. OpenSSH describes this dependency tree in [`ssh_config(5)`](https://man.openbsd.org/ssh_config.5), and the client parser follows the same shape in `readconf.c`: - `Include` recursively reads more config files and expands globs - `IdentityFile` and `CertificateFile` name authentication files - `UserKnownHostsFile`, `GlobalKnownHostsFile`, and `RevokedHostKeys` name host-key files - `ControlPath` and `IdentityAgent` can name profile-owned sockets or control files - these path directives can use forms such as `~`, `%d`, and `${HOME}` ## Change This PR adds a small SSH config dependency scanner. It starts at: ```text ~/.ssh/config ``` Then it returns concrete paths named by `Include` and by path-valued SSH config directives: ```text IdentityFile CertificateFile UserKnownHostsFile GlobalKnownHostsFile RevokedHostKeys ControlPath IdentityAgent ``` For example: ```sshconfig IdentityFile ~/.keys/devbox CertificateFile ~/.certs/devbox-cert.pub Include ~/.ssh/conf.d/*.conf ``` returns paths like: ```text C:\Users\me\.keys\devbox C:\Users\me\.certs\devbox-cert.pub C:\Users\me\.ssh\conf.d\devbox.conf ``` The setup code then maps those paths back to their top-level `USERPROFILE` child and filters matching sandbox roots out of both the writable and readable root lists. ## Why this shape The parser reports what SSH config references. The sandbox setup code decides which `USERPROFILE` roots are unsafe to grant. That keeps the policy simple: 1. expand broad profile grants 2. remove the profile root 3. remove fixed sensitive profile folders 4. remove profile folders referenced by SSH config dependencies If a path has two possible owners, the sandbox steps back. SSH keeps control of SSH config, keys, certificates, known-hosts files, sockets, and included config files. ## Tests - `cargo test -p codex-windows-sandbox --lib` - `just bazel-lock-check` - `just fix -p codex-windows-sandbox` - `git diff --check` --- codex-rs/Cargo.lock | 1 + codex-rs/Cargo.toml | 1 + codex-rs/windows-sandbox-rs/Cargo.toml | 1 + codex-rs/windows-sandbox-rs/src/lib.rs | 3 + .../src/setup_orchestrator.rs | 86 ++++++ .../src/ssh_config_dependencies.rs | 249 ++++++++++++++++++ 6 files changed, 341 insertions(+) create mode 100644 codex-rs/windows-sandbox-rs/src/ssh_config_dependencies.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index e0c399a3b..b492090b9 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3280,6 +3280,7 @@ dependencies = [ "codex-utils-string", "dirs-next", "dunce", + "glob", "pretty_assertions", "rand 0.8.5", "serde", diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index ed6bdc0fd..d10716ce4 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -242,6 +242,7 @@ env_logger = "0.11.9" eventsource-stream = "0.2.3" futures = { version = "0.3", default-features = false } gethostname = "1.1.0" +glob = "0.3" globset = "0.4" hmac = "0.12.1" http = "1.3.1" diff --git a/codex-rs/windows-sandbox-rs/Cargo.toml b/codex-rs/windows-sandbox-rs/Cargo.toml index 6d49f81a3..e7ca02fed 100644 --- a/codex-rs/windows-sandbox-rs/Cargo.toml +++ b/codex-rs/windows-sandbox-rs/Cargo.toml @@ -31,6 +31,7 @@ codex-utils-pty = { workspace = true } codex-utils-absolute-path = { workspace = true } codex-utils-string = { workspace = true } dunce = "1.0" +glob = { workspace = true } serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" tempfile = "3" diff --git a/codex-rs/windows-sandbox-rs/src/lib.rs b/codex-rs/windows-sandbox-rs/src/lib.rs index 90176b08e..3b9601b16 100644 --- a/codex-rs/windows-sandbox-rs/src/lib.rs +++ b/codex-rs/windows-sandbox-rs/src/lib.rs @@ -2,6 +2,9 @@ // from the eventual unsafe cleanup. #![allow(unsafe_op_in_unsafe_fn)] +#[cfg(any(target_os = "windows", test))] +mod ssh_config_dependencies; + macro_rules! windows_modules { ($($name:ident),+ $(,)?) => { $(#[cfg(target_os = "windows")] mod $name;)+ diff --git a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs index 4d0ea4b57..3774ab729 100644 --- a/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs +++ b/codex-rs/windows-sandbox-rs/src/setup_orchestrator.rs @@ -21,6 +21,7 @@ use crate::setup_error::SetupFailure; use crate::setup_error::clear_setup_error_report; use crate::setup_error::failure; use crate::setup_error::read_setup_error_report; +use crate::ssh_config_dependencies::ssh_config_dependency_paths; use anyhow::Context; use anyhow::Result; use anyhow::anyhow; @@ -792,6 +793,7 @@ fn build_payload_roots( 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_ssh_config_dependency_roots(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 @@ -812,6 +814,7 @@ fn build_payload_roots( 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); + read_roots = filter_ssh_config_dependency_roots(read_roots); let write_root_set: HashSet = write_roots.iter().cloned().collect(); read_roots.retain(|root| !write_root_set.contains(root)); (read_roots, write_roots) @@ -903,6 +906,43 @@ fn is_user_profile_root_exclusion(root: &Path, user_profile: &Path) -> bool { .any(|excluded| child_name.eq_ignore_ascii_case(excluded)) } +fn filter_ssh_config_dependency_roots(mut roots: Vec) -> Vec { + let Ok(user_profile) = std::env::var("USERPROFILE") else { + return roots; + }; + let user_profile = Path::new(&user_profile); + let dependency_paths = ssh_config_dependency_paths(user_profile); + roots.retain(|root| !is_ssh_config_dependency_root(root, user_profile, &dependency_paths)); + roots +} + +fn is_ssh_config_dependency_root( + root: &Path, + user_profile: &Path, + dependency_paths: &[PathBuf], +) -> bool { + let Some(child_name) = user_profile_child_name(root, user_profile) else { + return false; + }; + + dependency_paths.iter().any(|path| { + user_profile_child_name(path, user_profile) + .is_some_and(|dependency_child| child_name.eq_ignore_ascii_case(&dependency_child)) + }) +} + +fn user_profile_child_name(path: &Path, user_profile: &Path) -> Option { + let root_key = canonical_path_key(path); + let profile_key = canonical_path_key(user_profile); + let profile_prefix = format!("{}/", profile_key.trim_end_matches('/')); + let relative_key = root_key.strip_prefix(&profile_prefix)?; + relative_key + .split('/') + .next() + .filter(|name| !name.is_empty()) + .map(str::to_string) +} + fn filter_sensitive_write_roots(mut roots: Vec, codex_home: &Path) -> Vec { // 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 @@ -1163,6 +1203,52 @@ mod tests { )); } + #[test] + fn is_ssh_config_dependency_root_blocks_config_dependencies() { + let tmp = TempDir::new().expect("tempdir"); + let user_profile = tmp.path().join("user-profile"); + let documents = user_profile.join("Documents"); + let ssh_dir = user_profile.join(".ssh"); + let key_dir = user_profile.join(".keys"); + let include_dir = user_profile.join(".included"); + let other_root = tmp.path().join("other-root"); + fs::create_dir_all(&documents).expect("create documents"); + fs::create_dir_all(&ssh_dir).expect("create .ssh"); + fs::create_dir_all(&key_dir).expect("create key dir"); + fs::create_dir_all(&include_dir).expect("create include dir"); + fs::create_dir_all(&other_root).expect("create other root"); + fs::write( + ssh_dir.join("config"), + "IdentityFile ~/.keys/id_ed25519\nInclude ~/.included/config\n", + ) + .expect("write ssh config"); + fs::write(key_dir.join("id_ed25519"), "").expect("write key"); + fs::write(include_dir.join("config"), "User git\n").expect("write included config"); + + let dependency_paths = super::ssh_config_dependency_paths(&user_profile); + + assert!(!super::is_ssh_config_dependency_root( + &documents, + &user_profile, + &dependency_paths + )); + assert!(super::is_ssh_config_dependency_root( + &key_dir, + &user_profile, + &dependency_paths + )); + assert!(super::is_ssh_config_dependency_root( + &include_dir.join("config"), + &user_profile, + &dependency_paths + )); + assert!(!super::is_ssh_config_dependency_root( + &other_root, + &user_profile, + &dependency_paths + )); + } + #[test] fn expand_user_profile_root_for_replaces_profile_root_with_children() { let tmp = TempDir::new().expect("tempdir"); diff --git a/codex-rs/windows-sandbox-rs/src/ssh_config_dependencies.rs b/codex-rs/windows-sandbox-rs/src/ssh_config_dependencies.rs new file mode 100644 index 000000000..ae6d82d56 --- /dev/null +++ b/codex-rs/windows-sandbox-rs/src/ssh_config_dependencies.rs @@ -0,0 +1,249 @@ +use std::collections::HashSet; +use std::path::Path; +use std::path::PathBuf; + +const SSH_PROFILE_PATH_DIRECTIVES: &[&str] = &[ + "certificatefile", + "controlpath", + "globalknownhostsfile", + "identityagent", + "identityfile", + "revokedhostkeys", + "userknownhostsfile", +]; + +pub(crate) fn ssh_config_dependency_paths(user_profile: &Path) -> Vec { + let ssh_dir = user_profile.join(".ssh"); + let mut paths = vec![ssh_dir.join("config")]; + visit_config( + &ssh_dir.join("config"), + user_profile, + &ssh_dir, + &mut HashSet::new(), + &mut paths, + /*depth*/ 0, + ); + paths +} + +fn visit_config( + path: &Path, + user_profile: &Path, + ssh_dir: &Path, + visited: &mut HashSet, + paths: &mut Vec, + depth: usize, +) { + if depth == 32 { + return; + } + let key = std::fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()); + if !visited.insert(key) { + return; + } + + let Ok(contents) = std::fs::read_to_string(path) else { + return; + }; + for (key, args) in contents.lines().filter_map(directive) { + match key.to_ascii_lowercase().as_str() { + "include" => { + for arg in args { + for include in include_paths(&arg, user_profile, ssh_dir) { + paths.push(include.clone()); + visit_config(&include, user_profile, ssh_dir, visited, paths, depth + 1); + } + } + } + key if SSH_PROFILE_PATH_DIRECTIVES.contains(&key) => { + for arg in args { + if let Some(path) = + profile_path_arg(&arg, user_profile, /*relative_base*/ None) + { + paths.push(path); + } + } + } + _ => {} + } + } +} + +fn include_paths(arg: &str, user_profile: &Path, ssh_dir: &Path) -> Vec { + let Some(pattern_path) = profile_path_arg(arg, user_profile, Some(ssh_dir)) else { + return Vec::new(); + }; + let pattern = pattern_path.to_string_lossy().replace('\\', "/"); + let Ok(paths) = glob::glob(&pattern) else { + return Vec::new(); + }; + paths.filter_map(Result::ok).collect() +} + +fn directive(line: &str) -> Option<(String, Vec)> { + let mut words = words(line); + let first = words.first()?.to_string(); + if let Some((key, value)) = first.split_once('=') + && !key.is_empty() + { + let mut args = Vec::new(); + if !value.is_empty() { + args.push(value.to_string()); + } + args.extend(words.drain(1..)); + Some((key.to_string(), args)) + } else { + let key = words.remove(0); + if let Some(arg) = words.first_mut() + && let Some(value) = arg.strip_prefix('=') + { + *arg = value.to_string(); + } + words.retain(|arg| !arg.is_empty()); + Some((key, words)) + } +} + +fn words(line: &str) -> Vec { + let mut out = Vec::new(); + let mut word = String::new(); + let mut quote = None; + let mut chars = line.chars().peekable(); + + while let Some(ch) = chars.next() { + match ch { + '#' if quote.is_none() => break, + '\'' | '"' if quote == Some(ch) => quote = None, + '\'' | '"' if quote.is_none() => quote = Some(ch), + '\\' => { + if let Some(&next) = chars.peek() { + if matches!(next, '\'' | '"' | '\\') || (quote.is_none() && next == ' ') { + if let Some(escaped) = chars.next() { + word.push(escaped); + } + } else { + word.push(ch); + } + } else { + word.push(ch); + } + } + ch if ch.is_whitespace() && quote.is_none() => { + if !word.is_empty() { + out.push(std::mem::take(&mut word)); + } + } + ch => word.push(ch), + } + } + if !word.is_empty() { + out.push(word); + } + out +} + +fn profile_path_arg( + arg: &str, + user_profile: &Path, + relative_base: Option<&Path>, +) -> Option { + if arg.eq_ignore_ascii_case("none") { + return None; + } + if arg == "~" || arg == "%d" || arg == "${HOME}" { + return Some(user_profile.to_path_buf()); + } + if let Some(rest) = arg + .strip_prefix("~/") + .or_else(|| arg.strip_prefix(r"~\")) + .or_else(|| arg.strip_prefix("%d/")) + .or_else(|| arg.strip_prefix(r"%d\")) + .or_else(|| arg.strip_prefix("${HOME}/")) + .or_else(|| arg.strip_prefix(r"${HOME}\")) + { + return Some(user_profile.join(rest)); + } + + let path = PathBuf::from(arg); + if path.is_absolute() { + Some(path) + } else { + relative_base.map(|base| base.join(path)) + } +} + +#[cfg(test)] +mod tests { + use super::ssh_config_dependency_paths; + use pretty_assertions::assert_eq; + use std::fs; + use std::path::PathBuf; + use tempfile::TempDir; + + #[test] + fn collects_path_directive_profile_entries() { + let tmp = TempDir::new().expect("tempdir"); + let home = tmp.path(); + fs::create_dir_all(home.join(".ssh")).expect("create .ssh"); + fs::write( + home.join(".ssh/config"), + r#" +Host devbox + IdentityFile ~/.keys/id_ed25519 + IdentityFile '~/.keys/quoted key' + CertificateFile = %d/.certs/devbox-cert.pub + UserKnownHostsFile ${HOME}/.known_hosts_custom + GlobalKnownHostsFile ~/.global_known_hosts + ControlPath ~/.control/%h-%p-%r + IdentityAgent=%d/.agent/socket + RevokedHostKeys ~/.revoked/keys +"#, + ) + .expect("write config"); + + assert_eq!( + vec![ + home.join(".ssh/config"), + home.join(".keys/id_ed25519"), + home.join(".keys/quoted key"), + home.join(".certs/devbox-cert.pub"), + home.join(".known_hosts_custom"), + home.join(".global_known_hosts"), + home.join(".control/%h-%p-%r"), + home.join(".agent/socket"), + home.join(".revoked/keys"), + ], + slash_paths(ssh_config_dependency_paths(home)) + ); + } + + #[test] + fn recursively_collects_include_dependencies() { + let tmp = TempDir::new().expect("tempdir"); + let home = tmp.path(); + let ssh_dir = home.join(".ssh"); + fs::create_dir_all(ssh_dir.join("conf.d")).expect("create conf.d"); + fs::write(ssh_dir.join("config"), "Include conf.d/*.conf\n").expect("write config"); + fs::write( + ssh_dir.join("conf.d/devbox.conf"), + "CertificateFile ~/.included/devbox-cert.pub\n", + ) + .expect("write include"); + + assert_eq!( + vec![ + home.join(".ssh/config"), + ssh_dir.join("conf.d/devbox.conf"), + home.join(".included/devbox-cert.pub"), + ], + slash_paths(ssh_config_dependency_paths(home)) + ); + } + + fn slash_paths(paths: Vec) -> Vec { + paths + .into_iter() + .map(|path| PathBuf::from(path.to_string_lossy().replace('\\', "/"))) + .collect() + } +}