diff --git a/crates/agent-tui-daemon/src/governance.rs b/crates/agent-tui-daemon/src/governance.rs index 68e5336..cd43533 100644 --- a/crates/agent-tui-daemon/src/governance.rs +++ b/crates/agent-tui-daemon/src/governance.rs @@ -10,9 +10,10 @@ //! Two evaluators ship in v1: //! - [`AllowAllEvaluator`] — always Allows. Useful in unsafe-by-default test //! setups and as the trivial baseline. -//! - [`AllowlistEvaluator`] — checks `Spawn` actions against a binary -//! allowlist (CSV via `--allowed-binaries` or env). `Input`/`Eval` pass -//! through. Wildcards (`*`) are honored but audit-logged. +//! - [`AllowlistEvaluator`] — checks `Spawn` actions against a canonical +//! absolute-path binary allowlist (CSV via `--allowed-binaries` or env). +//! `Input`/`Eval` pass through. Wildcards (`*`) are honored but +//! audit-logged. //! //! OPA-WASM (`agent-tui --policy `) lands in a follow-on cycle. @@ -54,16 +55,14 @@ impl Evaluator for AllowAllEvaluator { } /// Binary allowlist enforcement on `Spawn`; pass-through for other action -/// kinds. Entries without path separators are command names and only match -/// bare `argv[0]` values such as `git`; entries with path separators are -/// canonical executable paths and only match path-style invocations such as -/// `/usr/bin/git` or `./tool`. Empty allowlist means "everything allowed" -/// (effective baseline for development); the explicit `*` wildcard means -/// "anything but I want the audit log to know about it". +/// kinds. Entries must be absolute paths and are canonicalized at daemon +/// startup; spawn requests must also invoke an absolute path. Empty allowlist +/// means "everything allowed" (effective baseline for development); the +/// explicit `*` wildcard means "anything but I want the audit log to know +/// about it". pub struct AllowlistEvaluator { - binaries: HashSet, paths: HashSet, - unresolved_paths: HashSet, + invalid_entries: HashSet, wildcard: bool, } @@ -73,27 +72,24 @@ impl AllowlistEvaluator { #[must_use] pub fn new(binaries: Vec) -> Self { let wildcard = binaries.iter().any(|s| s == "*"); - let mut command_names = HashSet::new(); let mut paths = HashSet::new(); - let mut unresolved_paths = HashSet::new(); + let mut invalid_entries = HashSet::new(); for binary in binaries.into_iter().filter(|s| s != "*") { - if contains_path_separator(&binary) { - match canonicalize_allowlist_path(&binary) { - Ok(path) => { - paths.insert(path); - } - Err(_) => { - unresolved_paths.insert(binary); - } + match canonicalize_allowlist_path(&binary) { + Ok(path) => { + paths.insert(path); + } + Err(AllowlistPathError::NotAbsolute) => { + invalid_entries.insert(format!("{binary} (not absolute)")); + } + Err(AllowlistPathError::Io) => { + invalid_entries.insert(format!("{binary} (unresolved)")); } - } else { - command_names.insert(binary); } } Self { - binaries: command_names, paths, - unresolved_paths, + invalid_entries, wildcard, } } @@ -115,7 +111,7 @@ impl AllowlistEvaluator { impl Evaluator for AllowlistEvaluator { async fn evaluate(&self, action: &Action) -> Decision { let audit_id = Uuid::new_v4(); - if let ActionDetail::Spawn { argv, cwd } = &action.detail { + if let ActionDetail::Spawn { argv, .. } = &action.detail { let requested = argv.first().map(String::as_str).unwrap_or_default(); let comm = spawn_display_name(requested); if self.wildcard { @@ -125,9 +121,7 @@ impl Evaluator for AllowlistEvaluator { reason: format!("wildcard allowlist; spawn={comm}"), }; } - let empty_allowlist = self.binaries.is_empty() - && self.paths.is_empty() - && self.unresolved_paths.is_empty(); + let empty_allowlist = self.paths.is_empty() && self.invalid_entries.is_empty(); if empty_allowlist { return Decision { audit_id, @@ -135,43 +129,36 @@ impl Evaluator for AllowlistEvaluator { reason: "empty allowlist (development mode)".into(), }; } - if contains_path_separator(requested) { - return match canonicalize_spawn_path(requested, cwd) { - Ok(path) if self.paths.contains(&path) => Decision { - audit_id, - verdict: Verdict::Allow, - reason: format!("allowlisted path: {}", path.display()), - }, - Ok(path) => Decision { - audit_id, - verdict: Verdict::Deny, - reason: format!( - "binary path {} not in allowlist; permitted: {}", - path.display(), - self.allowed_summary() - ), - }, - Err(e) => Decision { - audit_id, - verdict: Verdict::Deny, - reason: format!("binary path {requested} could not be resolved: {e}"), - }, - }; - } - if self.binaries.contains(requested) { + if !Path::new(requested).is_absolute() { return Decision { audit_id, - verdict: Verdict::Allow, - reason: format!("allowlisted command: {requested}"), + verdict: Verdict::Deny, + reason: format!( + "binary {comm} must be invoked by absolute path when allowlist is configured; permitted: {}", + self.allowed_summary() + ), }; } - return Decision { - audit_id, - verdict: Verdict::Deny, - reason: format!( - "binary {comm} not in allowlist; permitted: {}", - self.allowed_summary() - ), + return match canonicalize_spawn_path(requested) { + Ok(path) if self.paths.contains(&path) => Decision { + audit_id, + verdict: Verdict::Allow, + reason: format!("allowlisted path: {}", path.display()), + }, + Ok(path) => Decision { + audit_id, + verdict: Verdict::Deny, + reason: format!( + "binary path {} not in allowlist; permitted: {}", + path.display(), + self.allowed_summary() + ), + }, + Err(e) => Decision { + audit_id, + verdict: Verdict::Deny, + reason: format!("binary path {requested} could not be resolved: {e}"), + }, }; } Decision { @@ -184,13 +171,12 @@ impl Evaluator for AllowlistEvaluator { impl AllowlistEvaluator { fn allowed_summary(&self) -> String { - let mut entries = self.binaries.iter().cloned().collect::>(); - entries.extend(self.paths.iter().map(|p| p.display().to_string())); - entries.extend( - self.unresolved_paths - .iter() - .map(|p| format!("{p} (unresolved)")), - ); + let mut entries = self + .paths + .iter() + .map(|p| p.display().to_string()) + .collect::>(); + entries.extend(self.invalid_entries.iter().cloned()); entries.sort(); if entries.is_empty() { "(none)".to_string() @@ -204,30 +190,21 @@ fn spawn_display_name(path: &str) -> String { path.rsplit(['/', '\\']).next().unwrap_or(path).to_string() } -fn contains_path_separator(path: &str) -> bool { - path.contains('/') || path.contains('\\') +enum AllowlistPathError { + NotAbsolute, + Io, } -fn canonicalize_allowlist_path(path: &str) -> std::io::Result { +fn canonicalize_allowlist_path(path: &str) -> Result { let path = Path::new(path); - if path.is_absolute() { - path.canonicalize() - } else { - std::env::current_dir()?.join(path).canonicalize() + if !path.is_absolute() { + return Err(AllowlistPathError::NotAbsolute); } + path.canonicalize().map_err(|_| AllowlistPathError::Io) } -fn canonicalize_spawn_path(path: &str, cwd: &str) -> std::io::Result { - let path = Path::new(path); - if path.is_absolute() { - return path.canonicalize(); - } - let base = if cwd.trim().is_empty() { - std::env::current_dir()? - } else { - PathBuf::from(cwd) - }; - base.join(path).canonicalize() +fn canonicalize_spawn_path(path: &str) -> std::io::Result { + Path::new(path).canonicalize() } /// Per-daemon governance state. @@ -329,8 +306,10 @@ mod tests { #[tokio::test] async fn allowlist_denies_unknown_binary() { let g = Governance::new(Arc::new(AllowlistEvaluator::new(vec![ - "bash".into(), - "zsh".into(), + std::env::current_exe() + .expect("current test binary") + .to_string_lossy() + .into_owned(), ]))); let d = g .check(build::spawn(vec!["/usr/bin/nethack".into()], "/".into())) @@ -339,17 +318,17 @@ mod tests { } #[tokio::test] - async fn allowlist_allows_known_binary_by_basename() { + async fn allowlist_denies_basename_invocation() { let g = Governance::new(Arc::new(AllowlistEvaluator::new(vec!["bash".into()]))); let d = g .check(build::spawn(vec!["bash".into(), "-i".into()], "/".into())) .await; - assert_eq!(d.verdict, Verdict::Allow); + assert_eq!(d.verdict, Verdict::Deny); } #[tokio::test] - async fn basename_allowlist_does_not_allow_path_invocation() { - let g = Governance::new(Arc::new(AllowlistEvaluator::new(vec!["git".into()]))); + async fn relative_allowlist_entry_does_not_allow_path_invocation() { + let g = Governance::new(Arc::new(AllowlistEvaluator::new(vec!["./git".into()]))); let d = g .check(build::spawn(vec!["./git".into()], "/".into())) .await; @@ -394,9 +373,11 @@ mod tests { #[tokio::test] async fn audit_event_emitted_on_check() { - let g = Governance::new(Arc::new(AllowlistEvaluator::new(vec!["bash".into()]))); + let exe = std::env::current_exe().expect("current test binary"); + let exe_str = exe.to_string_lossy().into_owned(); + let g = Governance::new(Arc::new(AllowlistEvaluator::new(vec![exe_str.clone()]))); let mut sub = g.subscribe(); - let _ = g.check(build::spawn(vec!["bash".into()], "/".into())).await; + let _ = g.check(build::spawn(vec![exe_str], "/".into())).await; let evt = sub.recv().await.expect("event"); assert_eq!(evt.action_kind, ActionKind::Spawn); assert_eq!(evt.verdict, Verdict::Allow); diff --git a/crates/agent-tui-daemon/src/handlers/spawn.rs b/crates/agent-tui-daemon/src/handlers/spawn.rs index a256ecc..42d822a 100644 --- a/crates/agent-tui-daemon/src/handlers/spawn.rs +++ b/crates/agent-tui-daemon/src/handlers/spawn.rs @@ -4,7 +4,7 @@ //! and registers the pane. use std::collections::BTreeMap; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::Arc; use agent_tui_adapter::PaneInfo; @@ -50,6 +50,7 @@ pub async fn run( if let Some(resp) = policy_response(&decision) { return resp; } + let argv = canonicalize_absolute_argv0(argv); let (cols, rows) = size.unwrap_or((DEFAULT_COLS, DEFAULT_ROWS)); let engine: Arc = Arc::new(AlacrittyEngine::new(cols, rows)); @@ -131,6 +132,18 @@ fn basename(path: &str) -> String { stem.to_string() } +fn canonicalize_absolute_argv0(mut argv: Vec) -> Vec { + if let Some(first) = argv.first_mut() { + let path = Path::new(first); + if path.is_absolute() { + if let Ok(canonical) = path.canonicalize() { + *first = canonical.to_string_lossy().into_owned(); + } + } + } + argv +} + /// Translate a non-Allow `Decision` into a `POLICY_*` Response. Allow returns /// `None`, signalling the handler should proceed. fn policy_response(decision: &agent_tui_protocol::Decision) -> Option { diff --git a/crates/agent-tui-daemon/tests/round_trip.rs b/crates/agent-tui-daemon/tests/round_trip.rs index 4e8a1b8..069ae5c 100644 --- a/crates/agent-tui-daemon/tests/round_trip.rs +++ b/crates/agent-tui-daemon/tests/round_trip.rs @@ -23,12 +23,19 @@ use agent_tui_protocol::{ use base64::Engine as _; use interprocess::local_socket::tokio::Stream; use interprocess::local_socket::traits::tokio::Stream as _; +use std::os::unix::fs::PermissionsExt; use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; use tokio::time::timeout; use uuid::Uuid; /// Spin up an isolated daemon on a temp socket dir and return its connect URL. async fn boot_daemon() -> (DaemonConfig, agent_tui_daemon::DaemonHandle) { + boot_daemon_with_allowed(None).await +} + +async fn boot_daemon_with_allowed( + allowed_binaries: Option, +) -> (DaemonConfig, agent_tui_daemon::DaemonHandle) { // macOS sockaddr_un.sun_path is 104 bytes; the full layout path is // `/.sock`. Use a short session id (8 hex chars) and // anchor the root under /tmp directly so the result fits. @@ -41,7 +48,7 @@ async fn boot_daemon() -> (DaemonConfig, agent_tui_daemon::DaemonHandle) { layout: layout.clone(), engine: "alacritty".into(), binary_version: "0.0.0-test".into(), - allowed_binaries: None, + allowed_binaries, monitor_parent: None, idle_timeout_secs: None, adopt_handoff: None, @@ -219,6 +226,70 @@ async fn spawn_list_die_lifecycle() { ); } +#[tokio::test] +async fn allowed_binaries_rejects_basename_with_request_controlled_path() { + let shell = std::fs::canonicalize("/bin/sh").expect("/bin/sh canonicalizes"); + let shell = shell.to_string_lossy().into_owned(); + let (cfg, _h) = boot_daemon_with_allowed(Some(shell.clone())).await; + + let evil_dir = short_temp_root("at-path"); + std::fs::create_dir_all(&evil_dir).expect("mkdir evil dir"); + let fake_shell = evil_dir.join("sh"); + std::fs::write(&fake_shell, "#!/bin/sh\nprintf pwned\n").expect("write fake shell"); + let mut perms = std::fs::metadata(&fake_shell) + .expect("fake shell metadata") + .permissions(); + perms.set_mode(0o755); + std::fs::set_permissions(&fake_shell, perms).expect("chmod fake shell"); + + let denied = round_trip( + &cfg, + Command::Spawn { + argv: vec!["sh".into(), "-c".into(), "printf allowed".into()], + cwd: Some(evil_dir.to_string_lossy().into_owned()), + size: None, + stdin: agent_tui_protocol::request::StdinMode::default(), + env: vec![("PATH".into(), ".".into())], + }, + ) + .await; + assert!( + !denied.response.success, + "basename spawn should be denied: {denied:?}" + ); + let error = denied.response.error.expect("policy error"); + assert_eq!(error.code, ErrorCode::PolicyDenied); + assert!( + error.message.contains("absolute path"), + "error should explain absolute-path requirement: {error:?}" + ); + + let allowed = round_trip( + &cfg, + Command::Spawn { + argv: vec![shell, "-c".into(), "printf allowed; sleep 0.1".into()], + cwd: Some(evil_dir.to_string_lossy().into_owned()), + size: Some((40, 4)), + stdin: agent_tui_protocol::request::StdinMode::default(), + env: vec![("PATH".into(), ".".into())], + }, + ) + .await; + assert!( + allowed.response.success, + "canonical absolute shell should be allowed: {allowed:?}" + ); + + let _ = round_trip( + &cfg, + Command::Die { + pane: None, + grace: None, + }, + ) + .await; +} + #[tokio::test] async fn daemon_wire_smoke() { let (cfg, _h) = boot_daemon().await; diff --git a/crates/agent-tui/skill-data/core/references/commands.md b/crates/agent-tui/skill-data/core/references/commands.md index 52e6053..477ae81 100644 --- a/crates/agent-tui/skill-data/core/references/commands.md +++ b/crates/agent-tui/skill-data/core/references/commands.md @@ -19,7 +19,7 @@ silently return. --timeout Per-command timeout (ms) --content-boundaries Wrap snapshot payloads in nonced boundary markers --max-output Truncate snapshot payloads at N chars ---allowed-binaries Allowlist of binary basenames `spawn` accepts (`*` = any) +--allowed-binaries Allowlist of absolute executable paths `spawn` accepts (`*` = any) -h, --help Print help -V, --version Print version ``` diff --git a/crates/agent-tui/src/cli.rs b/crates/agent-tui/src/cli.rs index aedffdf..3c103a5 100644 --- a/crates/agent-tui/src/cli.rs +++ b/crates/agent-tui/src/cli.rs @@ -50,10 +50,9 @@ pub struct GlobalArgs { /// Truncate snapshot payloads at N characters. #[arg(long, value_name = "N", global = true)] pub max_output: Option, - /// Comma-separated allowlist of binary basenames or executable paths - /// `spawn` will accept. Bare names only match bare argv[0] values; path - /// invocations require an exact canonical path entry. `*` allows - /// everything (audit-only). Empty / unset = no restriction. + /// Comma-separated allowlist of absolute executable paths `spawn` will + /// accept. Entries and spawn argv[0] are canonicalized before comparison. + /// `*` allows everything (audit-only). Empty / unset = no restriction. /// Env: `AGENT_TUI_ALLOWED_BINARIES`. #[arg( long, diff --git a/docs/design/RFC.md b/docs/design/RFC.md index 30f165f..f29ea12 100644 --- a/docs/design/RFC.md +++ b/docs/design/RFC.md @@ -621,7 +621,7 @@ Default evaluator: in-process Rust predicate chain. v1 also ships a Rego adapter ### 11.2 Binary allowlist -`--allowed-binaries ` (or env `AGENT_TUI_ALLOWED_BINARIES`) gates `spawn`. Wildcard `*` allowed but audit-logged. Agent-mode default whitelist: `bash, zsh, fish, vim, nvim, nano, less, more, cat, grep, git, make, npm, pnpm, go, cargo, python, node, kubectl, k9s, lazygit, tmux, htop, btop, claude, codex, aider`. +`--allowed-binaries ` (or env `AGENT_TUI_ALLOWED_BINARIES`) gates `spawn`. Entries must be absolute executable paths; entries and spawn `argv[0]` are canonicalized before comparison. Wildcard `*` is allowed but audit-logged. ### 11.3 Content-boundary markers with per-snapshot nonces