mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
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`
This commit is contained in:
committed by
GitHub
Unverified
parent
715fafa23c
commit
b885c3f8b1
Generated
+1
@@ -3280,6 +3280,7 @@ dependencies = [
|
||||
"codex-utils-string",
|
||||
"dirs-next",
|
||||
"dunce",
|
||||
"glob",
|
||||
"pretty_assertions",
|
||||
"rand 0.8.5",
|
||||
"serde",
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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;)+
|
||||
|
||||
@@ -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<PathBuf> = 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<PathBuf>) -> Vec<PathBuf> {
|
||||
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<String> {
|
||||
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<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
|
||||
@@ -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");
|
||||
|
||||
@@ -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<PathBuf> {
|
||||
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<PathBuf>,
|
||||
paths: &mut Vec<PathBuf>,
|
||||
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<PathBuf> {
|
||||
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<String>)> {
|
||||
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<String> {
|
||||
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<PathBuf> {
|
||||
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<PathBuf>) -> Vec<PathBuf> {
|
||||
paths
|
||||
.into_iter()
|
||||
.map(|path| PathBuf::from(path.to_string_lossy().replace('\\', "/")))
|
||||
.collect()
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user