From 45e2d1c252cfb17f13e0451867f010a045cd6931 Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Thu, 2 Jul 2026 19:04:42 -0700 Subject: [PATCH] Harden namespace-aware executable policy matching --- codex-rs/execpolicy/README.md | 2 + codex-rs/execpolicy/src/executable_name.rs | 66 +++- .../execpolicy/src/executable_name_tests.rs | 41 +++ codex-rs/execpolicy/src/parser.rs | 13 +- codex-rs/execpolicy/src/policy.rs | 116 +++++- codex-rs/execpolicy/tests/basic.rs | 340 ++++++++++++++++++ 6 files changed, 562 insertions(+), 16 deletions(-) create mode 100644 codex-rs/execpolicy/src/executable_name_tests.rs diff --git a/codex-rs/execpolicy/README.md b/codex-rs/execpolicy/README.md index 5d1179362b..12319c2b06 100644 --- a/codex-rs/execpolicy/README.md +++ b/codex-rs/execpolicy/README.md @@ -43,6 +43,8 @@ host_executable( - With host-executable resolution enabled, if no exact rule matches, execpolicy may fall back from `/usr/bin/git` to basename rules for `git`. - If `host_executable(name="git", ...)` exists, basename fallback is only allowed for listed absolute paths. - If no `host_executable()` entry exists for a basename, basename fallback is allowed. + - Windows verbatim/device namespace paths cannot be listed by `host_executable`; use an ordinary absolute spelling or an exact prefix rule. Namespace command spellings do not inherit configured host-executable path allowlists. + - The library's restrictive basename overlay is separate from ordinary fallback: it ignores host-executable path lists for `prompt` and `forbidden`, but always discards basename `allow`. ## CLI diff --git a/codex-rs/execpolicy/src/executable_name.rs b/codex-rs/execpolicy/src/executable_name.rs index bb3e8a536c..ce18723a1b 100644 --- a/codex-rs/execpolicy/src/executable_name.rs +++ b/codex-rs/execpolicy/src/executable_name.rs @@ -1,4 +1,8 @@ +#[cfg(windows)] +use std::path::Component; use std::path::Path; +#[cfg(windows)] +use std::path::Prefix; #[cfg(windows)] const WINDOWS_EXECUTABLE_SUFFIXES: [&str; 4] = [".exe", ".cmd", ".bat", ".com"]; @@ -6,14 +10,7 @@ const WINDOWS_EXECUTABLE_SUFFIXES: [&str; 4] = [".exe", ".cmd", ".bat", ".com"]; pub(crate) fn executable_lookup_key(raw: &str) -> String { #[cfg(windows)] { - let raw = raw.to_ascii_lowercase(); - for suffix in WINDOWS_EXECUTABLE_SUFFIXES { - if raw.ends_with(suffix) { - let stripped_len = raw.len() - suffix.len(); - return raw[..stripped_len].to_string(); - } - } - raw + executable_lookup_key_windows(raw) } #[cfg(not(windows))] @@ -23,7 +20,54 @@ pub(crate) fn executable_lookup_key(raw: &str) -> String { } pub(crate) fn executable_path_lookup_key(path: &Path) -> Option { - path.file_name() - .and_then(|name| name.to_str()) - .map(executable_lookup_key) + let raw = path.file_name()?.to_str()?; + + #[cfg(windows)] + { + if has_windows_verbatim_or_device_prefix(path) { + Some(executable_literal_lookup_key_windows(raw)) + } else { + Some(executable_lookup_key_windows(raw)) + } + } + + #[cfg(not(windows))] + { + Some(executable_lookup_key(raw)) + } } + +#[cfg(windows)] +fn executable_lookup_key_windows(raw: &str) -> String { + executable_literal_lookup_key_windows(raw.trim_end_matches([' ', '.'])) +} + +#[cfg(windows)] +fn executable_literal_lookup_key_windows(raw: &str) -> String { + let raw = raw.to_ascii_lowercase(); + for suffix in WINDOWS_EXECUTABLE_SUFFIXES { + if let Some(raw) = raw.strip_suffix(suffix) { + return raw.to_string(); + } + } + raw +} + +#[cfg(windows)] +pub(crate) fn has_windows_verbatim_or_device_prefix(path: &Path) -> bool { + matches!( + path.components().next(), + Some(Component::Prefix(prefix)) + if matches!( + prefix.kind(), + Prefix::Verbatim(_) + | Prefix::VerbatimUNC(_, _) + | Prefix::VerbatimDisk(_) + | Prefix::DeviceNS(_) + ) + ) +} + +#[cfg(all(test, windows))] +#[path = "executable_name_tests.rs"] +mod tests; diff --git a/codex-rs/execpolicy/src/executable_name_tests.rs b/codex-rs/execpolicy/src/executable_name_tests.rs new file mode 100644 index 0000000000..5e24036044 --- /dev/null +++ b/codex-rs/execpolicy/src/executable_name_tests.rs @@ -0,0 +1,41 @@ +use super::*; +use pretty_assertions::assert_eq; + +#[test] +fn windows_executable_lookup_key_matrix() { + let cases = [ + (r"C:\workspace\Git.ExE.", "git", false), + (r"C:\workspace\git.exe ", "git", false), + (r"\\server\share\git.exe.", "git", false), + (r"\\server\share\git.exe ", "git", false), + (r"\\?\C:\workspace\git.exe", "git", true), + (r"\\?\C:\workspace\git.exe.", "git.exe.", true), + (r"\\?\C:\workspace\git.exe ", "git.exe ", true), + (r"\\?\UNC\server\share\git.exe", "git", true), + (r"\\?\UNC\server\share\git.exe.", "git.exe.", true), + (r"\\?\UNC\server\share\git.exe ", "git.exe ", true), + (r"\\.\C:\workspace\git.exe", "git", true), + (r"\\.\C:\workspace\git.exe.", "git.exe.", true), + (r"\\.\C:\workspace\git.exe ", "git.exe ", true), + (r"\\.\UNC\server\share\git.exe", "git", true), + (r"\\.\UNC\server\share\git.exe.", "git.exe.", true), + (r"\\.\UNC\server\share\git.exe ", "git.exe ", true), + ]; + + for (raw_path, expected_key, namespace) in cases { + let path = Path::new(raw_path); + assert_eq!( + ( + executable_path_lookup_key(path).as_deref(), + has_windows_verbatim_or_device_prefix(path), + ), + (Some(expected_key), namespace), + "{}", + raw_path + ); + } + + for alias in ["powershell.exe.", "powershell.exe ", "PowerShell.ExE. . "] { + assert_eq!(executable_lookup_key(alias), "powershell", "{alias}"); + } +} diff --git a/codex-rs/execpolicy/src/parser.rs b/codex-rs/execpolicy/src/parser.rs index 33262db1b3..930cbca168 100644 --- a/codex-rs/execpolicy/src/parser.rs +++ b/codex-rs/execpolicy/src/parser.rs @@ -26,6 +26,8 @@ use crate::error::TextPosition; use crate::error::TextRange; use crate::executable_name::executable_lookup_key; use crate::executable_name::executable_path_lookup_key; +#[cfg(windows)] +use crate::executable_name::has_windows_verbatim_or_device_prefix; use crate::rule::NetworkRule; use crate::rule::NetworkRuleProtocol; use crate::rule::PatternToken; @@ -449,8 +451,14 @@ fn policy_builtins(builder: &mut GlobalsBuilder) { value.get_type() )) })?; - let path = parse_literal_absolute_path(raw)?; - let Some(path_name) = executable_path_lookup_key(path.as_path()) else { + #[cfg(windows)] + if has_windows_verbatim_or_device_prefix(Path::new(raw)) { + return Err(Error::InvalidRule(format!( + "host_executable path `{raw}` must use an ordinary Win32 path spelling; use an exact prefix_rule for namespace paths" + )) + .into()); + } + let Some(path_name) = executable_path_lookup_key(Path::new(raw)) else { return Err(Error::InvalidRule(format!( "host_executable path `{raw}` must have basename `{name}`" )) @@ -462,6 +470,7 @@ fn policy_builtins(builder: &mut GlobalsBuilder) { )) .into()); } + let path = parse_literal_absolute_path(raw)?; if !parsed_paths.iter().any(|existing| existing == &path) { parsed_paths.push(path); } diff --git a/codex-rs/execpolicy/src/policy.rs b/codex-rs/execpolicy/src/policy.rs index 3102264e39..c05a1d06cb 100644 --- a/codex-rs/execpolicy/src/policy.rs +++ b/codex-rs/execpolicy/src/policy.rs @@ -1,7 +1,10 @@ use crate::decision::Decision; use crate::error::Error; use crate::error::Result; +use crate::executable_name::executable_lookup_key; use crate::executable_name::executable_path_lookup_key; +#[cfg(windows)] +use crate::executable_name::has_windows_verbatim_or_device_prefix; use crate::rule::NetworkRule; use crate::rule::NetworkRuleProtocol; use crate::rule::PatternToken; @@ -15,6 +18,7 @@ use multimap::MultiMap; use serde::Deserialize; use serde::Serialize; use std::collections::HashMap; +use std::path::Path; use std::sync::Arc; type HeuristicsFallback<'a> = Option<&'a dyn Fn(&[String]) -> Decision>; @@ -294,6 +298,37 @@ impl Policy { } } + /// Returns exact matches plus lexical basename `Prompt` and `Forbidden` + /// matches. Basename `Allow` matches are always discarded. + /// + /// Unlike ordinary host-executable resolution, restrictive basename + /// matching intentionally ignores `host_executable` path allowlists so an + /// unlisted or relative executable cannot escape an authored restriction. + /// Exact matches, including exact `Allow` rules, retain their meaning. + pub fn matches_for_command_with_restrictive_host_rules( + &self, + cmd: &[String], + heuristics_fallback: HeuristicsFallback<'_>, + ) -> Vec { + let mut matched_rules = self.match_exact_rules(cmd).unwrap_or_default(); + for rule_match in self.match_restrictive_basename_rules(cmd) { + if !matched_rules.contains(&rule_match) { + matched_rules.push(rule_match); + } + } + + if matched_rules.is_empty() + && let Some(heuristics_fallback) = heuristics_fallback + { + vec![RuleMatch::HeuristicsRuleMatch { + command: cmd.to_vec(), + decision: heuristics_fallback(cmd), + }] + } else { + matched_rules + } + } + fn match_exact_rules(&self, cmd: &[String]) -> Option> { let first = cmd.first()?; Some( @@ -308,10 +343,18 @@ impl Policy { let Some(first) = cmd.first() else { return Vec::new(); }; - let Ok(program) = AbsolutePathBuf::try_from(first.clone()) else { + let raw_path = Path::new(first); + let Some(basename) = executable_path_lookup_key(raw_path) else { return Vec::new(); }; - let Some(basename) = executable_path_lookup_key(program.as_path()) else { + #[cfg(windows)] + let namespace_path = has_windows_verbatim_or_device_prefix(raw_path); + #[cfg(not(windows))] + let namespace_path = false; + if namespace_path && self.host_executables_by_name.contains_key(&basename) { + return Vec::new(); + } + let Ok(program) = AbsolutePathBuf::try_from(first.clone()) else { return Vec::new(); }; let Some(rules) = self.rules_by_program.get_vec(&basename) else { @@ -329,7 +372,74 @@ impl Policy { rules .iter() .filter_map(|rule| rule.matches(&basename_command)) - .map(|rule_match| rule_match.with_resolved_program(&program)) + .map(|rule_match| { + if namespace_path { + rule_match + } else { + rule_match.with_resolved_program(&program) + } + }) + .collect() + } + + fn match_restrictive_basename_rules(&self, cmd: &[String]) -> Vec { + let Some(first) = cmd.first() else { + return Vec::new(); + }; + let path = Path::new(first); + let Some(path_key) = executable_path_lookup_key(path) else { + return Vec::new(); + }; + let Some(raw_basename) = path.file_name().and_then(|name| name.to_str()) else { + return Vec::new(); + }; + + #[cfg(windows)] + let normalized_raw_basename = raw_basename + .trim_end_matches([' ', '.']) + .to_ascii_lowercase(); + #[cfg(not(windows))] + let normalized_raw_basename = raw_basename.to_string(); + #[cfg(windows)] + let raw_basename = raw_basename.to_ascii_lowercase(); + #[cfg(not(windows))] + let raw_basename = raw_basename.to_string(); + + let mut basenames = vec![path_key, executable_lookup_key(&raw_basename)]; + basenames.dedup(); + for basename in [normalized_raw_basename, raw_basename] { + if !basenames.contains(&basename) { + basenames.push(basename); + } + } + + #[cfg(windows)] + let namespace_path = has_windows_verbatim_or_device_prefix(path); + #[cfg(not(windows))] + let namespace_path = false; + let resolved_program = (!namespace_path && path.is_absolute()) + .then(|| AbsolutePathBuf::try_from(first.clone()).ok()) + .flatten(); + + basenames + .into_iter() + .flat_map(|basename| { + let Some(rules) = self.rules_by_program.get_vec(&basename) else { + return Vec::new(); + }; + let basename_command = std::iter::once(basename) + .chain(cmd.iter().skip(1).cloned()) + .collect::>(); + rules + .iter() + .filter_map(|rule| rule.matches(&basename_command)) + .filter(|rule_match| rule_match.decision() != Decision::Allow) + .map(|rule_match| match resolved_program.as_ref() { + Some(program) => rule_match.with_resolved_program(program), + None => rule_match, + }) + .collect::>() + }) .collect() } } diff --git a/codex-rs/execpolicy/tests/basic.rs b/codex-rs/execpolicy/tests/basic.rs index f6a86adb27..998d2858a5 100644 --- a/codex-rs/execpolicy/tests/basic.rs +++ b/codex-rs/execpolicy/tests/basic.rs @@ -63,6 +63,12 @@ fn starlark_string(value: &str) -> String { value.replace('\\', "\\\\").replace('"', "\\\"") } +fn parse_policy(policy_src: &str) -> Result { + let mut parser = PolicyParser::new(); + parser.parse("test.rules", policy_src)?; + Ok(parser.build()) +} + #[derive(Clone, Debug, Eq, PartialEq)] enum RuleSnapshot { Prefix(PrefixRule), @@ -961,3 +967,337 @@ host_executable(name = "git", paths = ["{git_path_literal}"]) ); Ok(()) } + +#[test] +fn restrictive_host_rules_overlay_exact_matches_and_filter_basename_allow() -> Result<()> { + let git_name = host_executable_name("git"); + let git_path = host_absolute_path(&["usr", "bin", &git_name]); + let git_literal = starlark_string(&git_path); + let policy = parse_policy(&format!( + r#" +prefix_rule(pattern = ["{git_literal}"], decision = "allow") +prefix_rule(pattern = ["git"], decision = "forbidden") +host_executable(name = "git", paths = ["{git_literal}"]) +"#, + ))?; + + assert_eq!( + policy.matches_for_command_with_restrictive_host_rules( + &[git_path.clone(), "status".to_string()], + Some(&prompt_all), + ), + vec![ + RuleMatch::PrefixRuleMatch { + matched_prefix: vec![git_path.clone()], + decision: Decision::Allow, + resolved_program: None, + justification: None, + }, + RuleMatch::PrefixRuleMatch { + matched_prefix: tokens(&["git"]), + decision: Decision::Forbidden, + resolved_program: Some(absolute_path(&git_path)), + justification: None, + }, + ] + ); + + let policy = parse_policy(r#"prefix_rule(pattern = ["git"], decision = "allow")"#)?; + assert_eq!( + policy.matches_for_command_with_restrictive_host_rules( + &[git_path.clone(), "status".to_string()], + Some(&prompt_all), + ), + vec![RuleMatch::HeuristicsRuleMatch { + command: vec![git_path, "status".to_string()], + decision: Decision::Prompt, + }] + ); + Ok(()) +} + +#[test] +fn restrictive_host_rules_apply_to_relative_and_unlisted_paths() -> Result<()> { + let git_name = host_executable_name("git"); + let listed_git = host_absolute_path(&["usr", "bin", &git_name]); + let unlisted_git = host_absolute_path(&["opt", "bin", &git_name]); + let relative_git = PathBuf::from("tools") + .join(&git_name) + .to_string_lossy() + .into_owned(); + let listed_literal = starlark_string(&listed_git); + + for (wire_decision, decision) in [ + ("prompt", Decision::Prompt), + ("forbidden", Decision::Forbidden), + ] { + let policy = parse_policy(&format!( + r#" +prefix_rule(pattern = ["git"], decision = "{wire_decision}", justification = "review git") +host_executable(name = "git", paths = ["{listed_literal}"]) +"#, + ))?; + for (executable, resolved_program) in [ + (unlisted_git.clone(), Some(absolute_path(&unlisted_git))), + (relative_git.clone(), None), + ] { + assert_eq!( + policy.matches_for_command_with_restrictive_host_rules( + &[executable, "status".to_string()], + Some(&allow_all), + ), + vec![RuleMatch::PrefixRuleMatch { + matched_prefix: tokens(&["git"]), + decision, + resolved_program, + justification: Some("review git".to_string()), + }], + "{wire_decision}" + ); + } + } + Ok(()) +} + +#[cfg(windows)] +#[test] +fn host_executable_rejects_namespace_paths() { + for raw_path in [ + r"\\?\C:\workspace\git.exe", + r"\\?\C:\workspace\git.exe.", + r"\\?\C:\workspace\git.exe ", + r"\\?\UNC\server\share\git.exe", + r"\\?\UNC\server\share\git.exe.", + r"\\?\UNC\server\share\git.exe ", + r"\\.\C:\workspace\git.exe", + r"\\.\C:\workspace\git.exe.", + r"\\.\C:\workspace\git.exe ", + r"\\.\UNC\server\share\git.exe", + r"\\.\UNC\server\share\git.exe.", + r"\\.\UNC\server\share\git.exe ", + ] { + let policy_src = format!( + r#"host_executable(name = "git", paths = ["{}"] )"#, + starlark_string(raw_path) + ); + let error = parse_policy(&policy_src) + .expect_err("namespace host-executable path should be rejected"); + assert!( + error + .to_string() + .contains("must use an ordinary Win32 path spelling"), + "{raw_path}: {error}" + ); + } +} + +#[cfg(windows)] +#[test] +fn namespace_argv_does_not_inherit_host_executable_allowlist() -> Result<()> { + let ordinary_git = r"C:\workspace\git.exe"; + let namespace_git = r"\\?\C:\workspace\git.exe"; + let policy = parse_policy(&format!( + r#" +prefix_rule(pattern = ["git"], decision = "allow") +host_executable(name = "git", paths = ["{}"]) +"#, + starlark_string(ordinary_git) + ))?; + + let command = vec![namespace_git.to_string(), "status".to_string()]; + assert_eq!( + policy.check_with_options( + &command, + &prompt_all, + &MatchOptions { + resolve_host_executables: true, + }, + ), + Evaluation { + decision: Decision::Prompt, + matched_rules: vec![RuleMatch::HeuristicsRuleMatch { + command, + decision: Decision::Prompt, + }], + } + ); + Ok(()) +} + +#[cfg(windows)] +#[test] +fn windows_namespace_host_resolution_matrix() -> Result<()> { + let ordinary_paths = [ + r"C:\workspace\git.exe.", + r"C:\workspace\git.exe ", + r"\\server\share\git.exe.", + r"\\server\share\git.exe ", + ]; + let namespace_paths = [ + r"\\?\C:\workspace\git.exe.", + r"\\?\C:\workspace\git.exe ", + r"\\?\UNC\server\share\git.exe.", + r"\\?\UNC\server\share\git.exe ", + r"\\.\C:\workspace\git.exe.", + r"\\.\C:\workspace\git.exe ", + r"\\.\UNC\server\share\git.exe.", + r"\\.\UNC\server\share\git.exe ", + ]; + let namespace_controls = [ + r"\\?\C:\workspace\git.exe", + r"\\?\UNC\server\share\git.exe", + r"\\.\C:\workspace\git.exe", + r"\\.\UNC\server\share\git.exe", + ]; + let options = MatchOptions { + resolve_host_executables: true, + }; + + for (wire_decision, decision) in [ + ("allow", Decision::Allow), + ("prompt", Decision::Prompt), + ("forbidden", Decision::Forbidden), + ] { + let policy = parse_policy(&format!( + r#"prefix_rule(pattern = ["git"], decision = "{wire_decision}")"# + ))?; + for executable in ordinary_paths { + assert_eq!( + policy.check_with_options( + &[executable.to_string(), "status".to_string()], + &prompt_all, + &options, + ), + Evaluation { + decision, + matched_rules: vec![RuleMatch::PrefixRuleMatch { + matched_prefix: tokens(&["git"]), + decision, + resolved_program: Some(absolute_path(executable)), + justification: None, + }], + }, + "{wire_decision}: {executable}" + ); + } + for executable in namespace_controls { + assert_eq!( + policy.check_with_options( + &[executable.to_string(), "status".to_string()], + &prompt_all, + &options, + ), + Evaluation { + decision, + matched_rules: vec![RuleMatch::PrefixRuleMatch { + matched_prefix: tokens(&["git"]), + decision, + resolved_program: None, + justification: None, + }], + }, + "{wire_decision}: {executable}" + ); + } + for executable in namespace_paths { + let command = vec![executable.to_string(), "status".to_string()]; + assert_eq!( + policy.check_with_options(&command, &prompt_all, &options), + Evaluation { + decision: Decision::Prompt, + matched_rules: vec![RuleMatch::HeuristicsRuleMatch { + command, + decision: Decision::Prompt, + }], + }, + "{wire_decision}: {executable}" + ); + } + } + Ok(()) +} + +#[cfg(windows)] +#[test] +fn restrictive_namespace_rules_filter_allow_and_preserve_restrictions() -> Result<()> { + let namespace_paths = [ + r"\\?\C:\workspace\git.exe.", + r"\\?\C:\workspace\git.exe ", + r"\\?\UNC\server\share\git.exe.", + r"\\?\UNC\server\share\git.exe ", + r"\\.\C:\workspace\git.exe.", + r"\\.\C:\workspace\git.exe ", + r"\\.\UNC\server\share\git.exe.", + r"\\.\UNC\server\share\git.exe ", + ]; + + for rule_head in ["git", "git.exe"] { + for (wire_decision, decision) in [ + ("allow", Decision::Allow), + ("prompt", Decision::Prompt), + ("forbidden", Decision::Forbidden), + ] { + let policy = parse_policy(&format!( + r#"prefix_rule(pattern = ["{rule_head}"], decision = "{wire_decision}")"# + ))?; + for executable in namespace_paths { + let command = vec![executable.to_string(), "status".to_string()]; + let expected = if decision == Decision::Allow { + RuleMatch::HeuristicsRuleMatch { + command, + decision: Decision::Prompt, + } + } else { + RuleMatch::PrefixRuleMatch { + matched_prefix: vec![rule_head.to_string()], + decision, + resolved_program: None, + justification: None, + } + }; + assert_eq!( + policy.matches_for_command_with_restrictive_host_rules( + &[executable.to_string(), "status".to_string()], + Some(&prompt_all), + ), + vec![expected], + "{rule_head}/{wire_decision}: {executable}" + ); + } + } + } + Ok(()) +} + +#[cfg(windows)] +#[test] +fn native_windows_verbatim_trailing_literal_is_distinct_from_ordinary_alias() -> Result<()> { + let temp_dir = tempdir().context("create Windows namespace test directory")?; + let ordinary = temp_dir.path().join("git.exe"); + fs::write(&ordinary, b"ordinary")?; + let current_test_exe = std::env::current_exe().context("locate current test executable")?; + let current_test_exe_len = fs::metadata(¤t_test_exe)?.len(); + let temp_dir_text = temp_dir.path().to_string_lossy(); + let verbatim_root = temp_dir_text.strip_prefix(r"\\?\").map_or_else( + || format!(r"\\?\{temp_dir_text}"), + |path| format!(r"\\?\{path}"), + ); + + for literal_name in ["git.exe.", "git.exe "] { + let verbatim_literal = PathBuf::from(&verbatim_root).join(literal_name); + fs::copy(¤t_test_exe, &verbatim_literal)?; + assert_eq!(fs::read(temp_dir.path().join(literal_name))?, b"ordinary"); + assert_eq!(fs::metadata(&verbatim_literal)?.len(), current_test_exe_len); + let output = std::process::Command::new(&verbatim_literal) + .arg("--list") + .output() + .with_context(|| format!("launch literal namespace executable {literal_name:?}"))?; + fs::remove_file(&verbatim_literal)?; + assert!( + output.status.success(), + "literal namespace executable {literal_name:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + } + Ok(()) +}