From 0cb7d689451350e033addaf928c27a1fbbfa6e56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krist=C3=B3f=20T=C3=B3th?= Date: Thu, 17 Sep 2026 12:26:01 +0200 Subject: [PATCH] Keep env values off the bwrap command line --- AGENTS.md | 4 +++ README.md | 2 ++ src/env.rs | 53 ++++++++++++++++++--------------------- src/sandbox.rs | 53 +++++++++++++++++++++++---------------- tests/e2e/env.rs | 43 +++++++++++++++++++++++++++++++ tests/unit/env.rs | 64 ++++++++++++++++++++++++++++++----------------- 6 files changed, 146 insertions(+), 73 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 96d1f44..f340c1f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -23,3 +23,7 @@ Later bwrap arguments override earlier ones for the same path. This has caused m - User `--rw`/`--ro` escape hatches must come **after** mode setup so they can override sandbox restrictions. Take extreme care when reordering any arguments in `sandbox.rs` or refactor things and test thoroughly. + +### Env values must never become bwrap arguments + +`/proc//cmdline` is world-readable, and the same string is exposed inside every sandbox at `/run/agent-sandbox/bwrap-args`. `--setenv KEY VALUE` therefore publishes every secret the user puts in `env = [...]`. `set_sandbox_env` in `sandbox.rs` sets the whole child environment on the bwrap process instead, and bwrap passes its own environment on. Do not "simplify" it back to `--setenv`; `tests/e2e/env.rs` guards this. diff --git a/README.md b/README.md index fdaa41d..938e36f 100644 --- a/README.md +++ b/README.md @@ -33,6 +33,8 @@ Both modes clamp the environment the child sees so prompt-injected agents can't Disable the built-in policy entirely with `--no-env-filter` (or `env-filter = false` in the config file) to pass the parent env through unchanged. User `--setenv`/`--unsetenv` escape hatches still apply. +Env values never become `bwrap` arguments. The final environment is set on the `bwrap` process itself, so a secret passed via `env = ["KEY=VALUE"]` stays out of the world-readable `/proc//cmdline`, out of `ps` output, and out of the exposed command line below. + ## Seccomp Both modes apply a seccomp-BPF syscall allowlist derived from Podman's default profile. Dangerous syscalls (`mount`, `unshare`, `ptrace`, `bpf`, `perf_event_open`, `io_uring_*`, `keyctl`, `kexec_*`, …) return `ENOSYS`. Disable with `--no-seccomp` or `seccomp = false` in the config file. diff --git a/src/env.rs b/src/env.rs index b09af0a..ca99ad3 100644 --- a/src/env.rs +++ b/src/env.rs @@ -1,25 +1,27 @@ -pub fn keepenv_args(keys: &[String], parent_env: &[(String, String)]) -> Vec { - let mut args = Vec::new(); - for key in keys { - if let Some(value) = parent_env.iter().find_map(|(k, v)| (k == key).then_some(v)) { - args.push("--setenv".to_string()); - args.push(key.clone()); - args.push(value.clone()); - } - } - args +use std::collections::BTreeMap; + +pub type SandboxEnv = BTreeMap; + +pub fn find_parent_value(parent_env: &[(String, String)], key: &str) -> Option { + parent_env + .iter() + .find_map(|(k, v)| (k == key).then(|| v.clone())) } -pub fn whitelist_env_args(parent_env: &[(String, String)]) -> Vec { - let mut args = vec!["--clearenv".to_string()]; - for (key, value) in parent_env { - if whitelist_keeps(key) { - args.push("--setenv".to_string()); - args.push(key.clone()); - args.push(value.clone()); - } - } - args +pub fn copy_parent_env(parent_env: &[(String, String)]) -> SandboxEnv { + select_vars(parent_env, |_| true) +} + +fn select_vars(parent_env: &[(String, String)], permits: impl Fn(&str) -> bool) -> SandboxEnv { + parent_env + .iter() + .filter(|(key, _)| permits(key)) + .cloned() + .collect() +} + +pub fn apply_whitelist(parent_env: &[(String, String)]) -> SandboxEnv { + select_vars(parent_env, whitelist_keeps) } fn whitelist_keeps(key: &str) -> bool { @@ -96,15 +98,8 @@ const WHITELIST_KEEP_PREFIXES: &[&str] = &[ "OTEL_", ]; -pub fn blacklist_env_args(parent_env: &[(String, String)]) -> Vec { - let mut args = Vec::new(); - for (key, _) in parent_env { - if blacklist_drops(key) { - args.push("--unsetenv".to_string()); - args.push(key.clone()); - } - } - args +pub fn apply_blacklist(parent_env: &[(String, String)]) -> SandboxEnv { + select_vars(parent_env, |key| !blacklist_drops(key)) } fn blacklist_drops(key: &str) -> bool { diff --git a/src/sandbox.rs b/src/sandbox.rs index aa8a3b3..b728e0a 100644 --- a/src/sandbox.rs +++ b/src/sandbox.rs @@ -71,8 +71,7 @@ fn build_command(config: &SandboxConfig) -> Result<(Bwrap, BwrapCommandLine), Sa add_rw_bind(&mut cmd, spec)?; } - add_env_policy(&mut cmd, config); - add_user_env_overrides(&mut cmd, config); + set_sandbox_env(&mut cmd, config); cmd.args(["--remount-ro", "/"]); cmd.arg("--die-with-parent"); @@ -97,36 +96,48 @@ fn build_command(config: &SandboxConfig) -> Result<(Bwrap, BwrapCommandLine), Sa Ok((bwrap, command_line)) } -fn add_env_policy(cmd: &mut Command, config: &SandboxConfig) { - if !config.env_filter { - return; - } +/// bwrap passes its own environment on to the child. `--setenv` would instead +/// publish every value in the world-readable `/proc//cmdline`. +fn set_sandbox_env(cmd: &mut Command, config: &SandboxConfig) { let parent_env: Vec<(String, String)> = std::env::vars().collect(); - let args = match config.mode { - SandboxMode::Blacklist => env::blacklist_env_args(&parent_env), - SandboxMode::Whitelist => env::whitelist_env_args(&parent_env), - }; - cmd.args(args); + + let mut sandbox_env = apply_env_filter(&parent_env, config); + apply_user_env_overrides(&mut sandbox_env, &parent_env, config); + + cmd.env_clear(); + cmd.envs(sandbox_env); } -fn add_user_env_overrides(cmd: &mut Command, config: &SandboxConfig) { - let mut keep_keys: Vec = Vec::new(); +fn apply_env_filter(parent_env: &[(String, String)], config: &SandboxConfig) -> env::SandboxEnv { + if !config.env_filter { + return env::copy_parent_env(parent_env); + } + match config.mode { + SandboxMode::Blacklist => env::apply_blacklist(parent_env), + SandboxMode::Whitelist => env::apply_whitelist(parent_env), + } +} + +fn apply_user_env_overrides( + sandbox_env: &mut env::SandboxEnv, + parent_env: &[(String, String)], + config: &SandboxConfig, +) { for entry in &config.env { match entry { EnvEntry::Set(key, value) => { - cmd.arg("--setenv").arg(key).arg(value); + sandbox_env.insert(key.clone(), value.clone()); + } + EnvEntry::Keep(key) => { + if let Some(value) = env::find_parent_value(parent_env, key) { + sandbox_env.insert(key.clone(), value); + } } - EnvEntry::Keep(key) => keep_keys.push(key.clone()), } } - if !keep_keys.is_empty() { - let parent_env: Vec<(String, String)> = std::env::vars().collect(); - cmd.args(env::keepenv_args(&keep_keys, &parent_env)); - } - for key in &config.unsetenv { - cmd.arg("--unsetenv").arg(key); + sandbox_env.remove(key); } } diff --git a/tests/e2e/env.rs b/tests/e2e/env.rs index 73e3701..1689679 100644 --- a/tests/e2e/env.rs +++ b/tests/e2e/env.rs @@ -16,6 +16,49 @@ fn printenv_inside(args: &[&str], vars: &[(&str, &str)], query: &[&str]) -> Stri .expect("agent-sandbox binary failed to execute"); String::from_utf8_lossy(&output.stdout).into_owned() } +#[test] +fn user_env_value_never_reaches_the_command_line() { + let stdout = dry_run_command_line(&["--env", "MY_SECRET=hunter2"], &[]); + assert!( + !stdout.contains("hunter2"), + "user env value leaked into the bwrap command line: {stdout}" + ); +} + +fn dry_run_command_line(args: &[&str], vars: &[(&str, &str)]) -> String { + let mut cmd = Sandbox::new(args); + cmd.arg("--dry-run"); + for (k, v) in vars { + cmd.env(k, v); + } + let output = cmd + .args(["--", "true"]) + .output() + .expect("agent-sandbox binary failed to execute"); + String::from_utf8_lossy(&output.stdout).into_owned() +} + +#[test] +fn kept_host_env_value_never_reaches_the_command_line() { + let stdout = dry_run_command_line(&[], &[("TERM", "xterm-canary")]); + assert!( + !stdout.contains("xterm-canary"), + "kept host env value leaked into the bwrap command line: {stdout}" + ); +} + +#[test] +fn passed_through_env_value_never_reaches_the_command_line() { + let stdout = dry_run_command_line( + &["--env", "PASSED_THROUGH"], + &[("PASSED_THROUGH", "from-host-canary")], + ); + assert!( + !stdout.contains("from-host-canary"), + "passed-through env value leaked into the bwrap command line: {stdout}" + ); +} + #[test] fn whitelist_keeps_identity_and_terminal_vars() { let stdout = printenv_inside( diff --git a/tests/unit/env.rs b/tests/unit/env.rs index e13bfb1..1ddf3ca 100644 --- a/tests/unit/env.rs +++ b/tests/unit/env.rs @@ -1,37 +1,55 @@ use super::*; -#[test] -fn keepenv_emits_setenv_for_present_key() { - let parent = vec![("XDG_RUNTIME_DIR".into(), "/run/user/1000".into())]; - let args = keepenv_args(&["XDG_RUNTIME_DIR".into()], &parent); - assert_eq!(args, vec!["--setenv", "XDG_RUNTIME_DIR", "/run/user/1000"]); +fn parent(pairs: &[(&str, &str)]) -> Vec<(String, String)> { + pairs + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect() } #[test] -fn keepenv_skips_absent_keys() { - let parent = vec![("HOME".into(), "/home/me".into())]; - let args = keepenv_args(&["XDG_RUNTIME_DIR".into()], &parent); - assert!(args.is_empty()); +fn copy_parent_env_retains_every_var() { + let sandbox_env = copy_parent_env(&parent(&[("GH_TOKEN", "secret"), ("XDG_SEAT", "seat0")])); + assert_eq!(sandbox_env.get("GH_TOKEN").unwrap(), "secret"); + assert_eq!(sandbox_env.get("XDG_SEAT").unwrap(), "seat0"); } #[test] -fn keepenv_preserves_caller_key_order() { - let parent = vec![ - ("B".into(), "2".into()), - ("A".into(), "1".into()), - ("C".into(), "3".into()), - ]; - let args = keepenv_args(&["A".into(), "B".into(), "C".into()], &parent); +fn whitelist_retains_allowed_var() { + let sandbox_env = apply_whitelist(&parent(&[("TERM", "xterm")])); + assert_eq!(sandbox_env.get("TERM").unwrap(), "xterm"); +} + +#[test] +fn whitelist_removes_unlisted_var() { + let sandbox_env = apply_whitelist(&parent(&[("SOME_RANDOM_NOISE_VAR", "leak")])); + assert!(sandbox_env.is_empty()); +} + +#[test] +fn blacklist_removes_secret_var() { + let sandbox_env = apply_blacklist(&parent(&[("GH_TOKEN", "secret"), ("MY_NICE_VAR", "hello")])); + assert!(!sandbox_env.contains_key("GH_TOKEN")); + assert_eq!(sandbox_env.get("MY_NICE_VAR").unwrap(), "hello"); +} + +#[test] +fn blacklist_carves_out_vendor_prefix() { + let sandbox_env = apply_blacklist(&parent(&[("ANTHROPIC_API_KEY", "key")])); + assert_eq!(sandbox_env.get("ANTHROPIC_API_KEY").unwrap(), "key"); +} + +#[test] +fn find_parent_value_returns_present_value() { + let parent_env = parent(&[("XDG_RUNTIME_DIR", "/run/user/1000")]); assert_eq!( - args, - vec![ - "--setenv", "A", "1", "--setenv", "B", "2", "--setenv", "C", "3" - ] + find_parent_value(&parent_env, "XDG_RUNTIME_DIR").unwrap(), + "/run/user/1000" ); } #[test] -fn keepenv_empty_keys_yields_nothing() { - let parent = vec![("A".into(), "1".into())]; - assert!(keepenv_args(&[], &parent).is_empty()); +fn find_parent_value_returns_none_for_absent_key() { + let parent_env = parent(&[("HOME", "/home/me")]); + assert!(find_parent_value(&parent_env, "XDG_RUNTIME_DIR").is_none()); }