fix(security): scrub child process environments
This commit is contained in:
@@ -0,0 +1,181 @@
|
||||
//! Sanitized environment handling for child processes.
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::ffi::{OsStr, OsString};
|
||||
|
||||
/// Convert a string env map into owned OS strings for child env helpers.
|
||||
pub fn string_map_env(
|
||||
env: &HashMap<String, String>,
|
||||
) -> impl Iterator<Item = (OsString, OsString)> + '_ {
|
||||
env.iter()
|
||||
.map(|(key, value)| (OsString::from(key), OsString::from(value)))
|
||||
}
|
||||
|
||||
/// Return the environment for a child process after dropping parent secrets.
|
||||
///
|
||||
/// `overrides` are trusted call-site values, such as sandbox markers, hook
|
||||
/// variables, MCP server config, or RLM context path. They are applied after the
|
||||
/// parent allowlist so explicit values win.
|
||||
pub fn sanitized_child_env<I, K, V>(overrides: I) -> Vec<(OsString, OsString)>
|
||||
where
|
||||
I: IntoIterator<Item = (K, V)>,
|
||||
K: AsRef<OsStr>,
|
||||
V: AsRef<OsStr>,
|
||||
{
|
||||
let mut env = Vec::new();
|
||||
for (key, value) in std::env::vars_os() {
|
||||
if is_allowed_parent_env_key(&key) {
|
||||
upsert_env(&mut env, key, value);
|
||||
}
|
||||
}
|
||||
for (key, value) in overrides {
|
||||
upsert_env(
|
||||
&mut env,
|
||||
key.as_ref().to_os_string(),
|
||||
value.as_ref().to_os_string(),
|
||||
);
|
||||
}
|
||||
env
|
||||
}
|
||||
|
||||
pub fn apply_to_command<I, K, V>(cmd: &mut std::process::Command, overrides: I)
|
||||
where
|
||||
I: IntoIterator<Item = (K, V)>,
|
||||
K: AsRef<OsStr>,
|
||||
V: AsRef<OsStr>,
|
||||
{
|
||||
cmd.env_clear();
|
||||
for (key, value) in sanitized_child_env(overrides) {
|
||||
cmd.env(key, value);
|
||||
}
|
||||
}
|
||||
|
||||
pub fn apply_to_tokio_command<I, K, V>(cmd: &mut tokio::process::Command, overrides: I)
|
||||
where
|
||||
I: IntoIterator<Item = (K, V)>,
|
||||
K: AsRef<OsStr>,
|
||||
V: AsRef<OsStr>,
|
||||
{
|
||||
cmd.env_clear();
|
||||
for (key, value) in sanitized_child_env(overrides) {
|
||||
cmd.env(key, value);
|
||||
}
|
||||
}
|
||||
|
||||
pub fn apply_to_pty_command<I, K, V>(cmd: &mut portable_pty::CommandBuilder, overrides: I)
|
||||
where
|
||||
I: IntoIterator<Item = (K, V)>,
|
||||
K: AsRef<OsStr>,
|
||||
V: AsRef<OsStr>,
|
||||
{
|
||||
cmd.env_clear();
|
||||
for (key, value) in sanitized_child_env(overrides) {
|
||||
cmd.env(key, value);
|
||||
}
|
||||
}
|
||||
|
||||
fn is_allowed_parent_env_key(key: &OsStr) -> bool {
|
||||
let key = key.to_string_lossy();
|
||||
let normalized = key.to_ascii_uppercase();
|
||||
matches!(
|
||||
normalized.as_str(),
|
||||
"PATH"
|
||||
| "HOME"
|
||||
| "USER"
|
||||
| "USERNAME"
|
||||
| "LOGNAME"
|
||||
| "LANG"
|
||||
| "LANGUAGE"
|
||||
| "LC_ALL"
|
||||
| "LC_CTYPE"
|
||||
| "LC_MESSAGES"
|
||||
| "TERM"
|
||||
| "COLORTERM"
|
||||
| "NO_COLOR"
|
||||
| "FORCE_COLOR"
|
||||
| "SHELL"
|
||||
| "TMPDIR"
|
||||
| "TMP"
|
||||
| "TEMP"
|
||||
| "__CF_USER_TEXT_ENCODING"
|
||||
| "SYSTEMROOT"
|
||||
| "WINDIR"
|
||||
| "COMSPEC"
|
||||
| "PATHEXT"
|
||||
| "USERPROFILE"
|
||||
| "HOMEDRIVE"
|
||||
| "HOMEPATH"
|
||||
) || normalized.starts_with("LC_")
|
||||
}
|
||||
|
||||
fn upsert_env(env: &mut Vec<(OsString, OsString)>, key: OsString, value: OsString) {
|
||||
let normalized = normalize_key(&key);
|
||||
env.retain(|(existing, _)| normalize_key(existing) != normalized);
|
||||
env.push((key, value));
|
||||
}
|
||||
|
||||
fn normalize_key(key: &OsStr) -> String {
|
||||
key.to_string_lossy().to_ascii_uppercase()
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use std::sync::{Mutex, OnceLock};
|
||||
|
||||
fn env_lock() -> &'static Mutex<()> {
|
||||
static LOCK: OnceLock<Mutex<()>> = OnceLock::new();
|
||||
LOCK.get_or_init(|| Mutex::new(()))
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sanitized_child_env_drops_parent_secret_like_values() {
|
||||
let _guard = env_lock().lock().expect("env lock");
|
||||
let previous = std::env::var_os("DEEPSEEK_CHILD_ENV_TEST_SECRET");
|
||||
unsafe {
|
||||
std::env::set_var("DEEPSEEK_CHILD_ENV_TEST_SECRET", "parent-secret");
|
||||
}
|
||||
|
||||
let env = sanitized_child_env(std::iter::empty::<(OsString, OsString)>());
|
||||
|
||||
match previous {
|
||||
Some(value) => unsafe {
|
||||
std::env::set_var("DEEPSEEK_CHILD_ENV_TEST_SECRET", value);
|
||||
},
|
||||
None => unsafe {
|
||||
std::env::remove_var("DEEPSEEK_CHILD_ENV_TEST_SECRET");
|
||||
},
|
||||
}
|
||||
|
||||
assert!(
|
||||
env.iter()
|
||||
.all(|(key, _)| key != "DEEPSEEK_CHILD_ENV_TEST_SECRET")
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn explicit_child_env_values_win_over_parent_allowlist() {
|
||||
let _guard = env_lock().lock().expect("env lock");
|
||||
let previous = std::env::var_os("PATH");
|
||||
unsafe {
|
||||
std::env::set_var("PATH", "/parent/bin");
|
||||
}
|
||||
|
||||
let env = sanitized_child_env([(OsString::from("PATH"), OsString::from("/explicit/bin"))]);
|
||||
|
||||
match previous {
|
||||
Some(value) => unsafe {
|
||||
std::env::set_var("PATH", value);
|
||||
},
|
||||
None => unsafe {
|
||||
std::env::remove_var("PATH");
|
||||
},
|
||||
}
|
||||
|
||||
let path = env
|
||||
.iter()
|
||||
.find(|(key, _)| normalize_key(key) == "PATH")
|
||||
.map(|(_, value)| value);
|
||||
assert_eq!(path, Some(&OsString::from("/explicit/bin")));
|
||||
}
|
||||
}
|
||||
@@ -16,6 +16,7 @@ mod acp_server;
|
||||
mod audit;
|
||||
mod auto_reasoning;
|
||||
mod automation_manager;
|
||||
mod child_env;
|
||||
mod client;
|
||||
mod command_safety;
|
||||
mod commands;
|
||||
@@ -3548,9 +3549,7 @@ fn run_sandbox_command(args: SandboxArgs) -> Result<()> {
|
||||
.current_dir(&exec_env.cwd)
|
||||
.stdout(Stdio::piped())
|
||||
.stderr(Stdio::piped());
|
||||
for (key, value) in &exec_env.env {
|
||||
cmd.env(key, value);
|
||||
}
|
||||
child_env::apply_to_command(&mut cmd, child_env::string_map_env(&exec_env.env));
|
||||
|
||||
let mut child = cmd
|
||||
.spawn()
|
||||
|
||||
@@ -16,6 +16,7 @@ use serde::{Deserialize, Serialize};
|
||||
use tokio::io::{AsyncBufReadExt, AsyncWriteExt};
|
||||
use tokio::process::{Child, ChildStdin, ChildStdout};
|
||||
|
||||
use crate::child_env;
|
||||
use crate::network_policy::{Decision, NetworkPolicyDecider, host_from_url};
|
||||
use crate::utils::write_atomic;
|
||||
|
||||
@@ -666,9 +667,7 @@ impl McpConnection {
|
||||
.stderr(std::process::Stdio::null())
|
||||
.kill_on_drop(true);
|
||||
|
||||
for (key, value) in &config.env {
|
||||
cmd.env(key, value);
|
||||
}
|
||||
child_env::apply_to_tokio_command(&mut cmd, child_env::string_map_env(&config.env));
|
||||
|
||||
let mut child = cmd.spawn().with_context(|| {
|
||||
let env_keys: Vec<&str> = config.env.keys().map(String::as_str).collect();
|
||||
|
||||
@@ -16,6 +16,7 @@
|
||||
//! that happens to contain "REQ" or "FINAL" can't be confused with control
|
||||
//! messages.
|
||||
|
||||
use std::ffi::OsString;
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::process::Stdio;
|
||||
use std::time::{Duration, Instant};
|
||||
@@ -25,6 +26,8 @@ use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader};
|
||||
use tokio::process::{Child, ChildStdin, ChildStdout, Command};
|
||||
use uuid::Uuid;
|
||||
|
||||
use crate::child_env;
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Public types
|
||||
// ---------------------------------------------------------------------------
|
||||
@@ -192,9 +195,15 @@ impl PythonRuntime {
|
||||
.stderr(Stdio::piped())
|
||||
.kill_on_drop(true);
|
||||
|
||||
if let Some(path) = context_path {
|
||||
cmd.env("RLM_CONTEXT_FILE", path);
|
||||
}
|
||||
let context_env = context_path
|
||||
.map(|path| {
|
||||
vec![(
|
||||
OsString::from("RLM_CONTEXT_FILE"),
|
||||
path.as_os_str().to_os_string(),
|
||||
)]
|
||||
})
|
||||
.unwrap_or_default();
|
||||
child_env::apply_to_tokio_command(&mut cmd, context_env);
|
||||
|
||||
let mut child = cmd
|
||||
.spawn()
|
||||
|
||||
@@ -25,6 +25,7 @@ use std::os::unix::process::CommandExt;
|
||||
use portable_pty::{CommandBuilder, PtySize, native_pty_system};
|
||||
|
||||
use super::shell_output::{summarize_output, truncate_with_meta};
|
||||
use crate::child_env;
|
||||
use crate::sandbox::{
|
||||
CommandSpec,
|
||||
ExecEnv,
|
||||
@@ -765,10 +766,7 @@ impl ShellManager {
|
||||
cmd.stdin(Stdio::piped());
|
||||
}
|
||||
|
||||
// Set environment variables from exec_env
|
||||
for (key, value) in &exec_env.env {
|
||||
cmd.env(key, value);
|
||||
}
|
||||
child_env::apply_to_command(&mut cmd, child_env::string_map_env(&exec_env.env));
|
||||
|
||||
let mut child = cmd
|
||||
.spawn()
|
||||
@@ -904,9 +902,7 @@ impl ShellManager {
|
||||
}
|
||||
install_parent_death_signal(&mut cmd);
|
||||
|
||||
for (key, value) in &exec_env.env {
|
||||
cmd.env(key, value);
|
||||
}
|
||||
child_env::apply_to_command(&mut cmd, child_env::string_map_env(&exec_env.env));
|
||||
|
||||
let mut child = cmd
|
||||
.spawn()
|
||||
@@ -1010,9 +1006,7 @@ impl ShellManager {
|
||||
cmd.arg(arg);
|
||||
}
|
||||
cmd.cwd(working_dir);
|
||||
for (key, value) in &exec_env.env {
|
||||
cmd.env(key, value);
|
||||
}
|
||||
child_env::apply_to_pty_command(&mut cmd, child_env::string_map_env(&exec_env.env));
|
||||
|
||||
let child = pair
|
||||
.slave
|
||||
@@ -1048,9 +1042,7 @@ impl ShellManager {
|
||||
cmd.process_group(0);
|
||||
}
|
||||
|
||||
for (key, value) in &exec_env.env {
|
||||
cmd.env(key, value);
|
||||
}
|
||||
child_env::apply_to_command(&mut cmd, child_env::string_map_env(&exec_env.env));
|
||||
|
||||
let mut child = cmd
|
||||
.spawn()
|
||||
|
||||
@@ -2,8 +2,14 @@ use super::*;
|
||||
|
||||
use crate::tools::spec::ToolContext;
|
||||
use serde_json::{Value, json};
|
||||
use std::sync::{Mutex, OnceLock};
|
||||
use tempfile::tempdir;
|
||||
|
||||
fn env_lock() -> &'static Mutex<()> {
|
||||
static LOCK: OnceLock<Mutex<()>> = OnceLock::new();
|
||||
LOCK.get_or_init(|| Mutex::new(()))
|
||||
}
|
||||
|
||||
fn echo_command(message: &str) -> String {
|
||||
format!("echo {message}")
|
||||
}
|
||||
@@ -82,6 +88,49 @@ fn failed_network_shell_result(stdout: &str, stderr: &str) -> ShellResult {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
#[cfg(unix)]
|
||||
fn shell_execution_scrubs_parent_env_and_keeps_explicit_env() {
|
||||
let _guard = env_lock().lock().expect("env lock");
|
||||
let previous = std::env::var_os("DEEPSEEK_CHILD_ENV_SHELL_SECRET");
|
||||
unsafe {
|
||||
std::env::set_var("DEEPSEEK_CHILD_ENV_SHELL_SECRET", "parent-secret");
|
||||
}
|
||||
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
let mut manager = ShellManager::new(tmp.path().to_path_buf());
|
||||
let mut extra = std::collections::HashMap::new();
|
||||
extra.insert(
|
||||
"DEEPSEEK_CHILD_ENV_EXPLICIT".to_string(),
|
||||
"explicit-value".to_string(),
|
||||
);
|
||||
|
||||
let result = manager
|
||||
.execute_with_options_env(
|
||||
"printf '%s\\n%s\\n' \"${DEEPSEEK_CHILD_ENV_SHELL_SECRET-unset}\" \"${DEEPSEEK_CHILD_ENV_EXPLICIT-unset}\"",
|
||||
None,
|
||||
5000,
|
||||
false,
|
||||
None,
|
||||
false,
|
||||
None,
|
||||
extra,
|
||||
)
|
||||
.expect("execute");
|
||||
|
||||
match previous {
|
||||
Some(value) => unsafe {
|
||||
std::env::set_var("DEEPSEEK_CHILD_ENV_SHELL_SECRET", value);
|
||||
},
|
||||
None => unsafe {
|
||||
std::env::remove_var("DEEPSEEK_CHILD_ENV_SHELL_SECRET");
|
||||
},
|
||||
}
|
||||
|
||||
assert_eq!(result.status, ShellStatus::Completed);
|
||||
assert_eq!(result.stdout, "unset\nexplicit-value\n");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_sync_execution() {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
|
||||
Reference in New Issue
Block a user