diff --git a/crates/tinybox-linux/src/sandbox/args.rs b/crates/tinybox-linux/src/sandbox/args.rs index d82bf693..b2dcfdb9 100644 --- a/crates/tinybox-linux/src/sandbox/args.rs +++ b/crates/tinybox-linux/src/sandbox/args.rs @@ -775,13 +775,14 @@ pub(super) fn bwrap(spec: &BoxSpec, request: &ExecRequest) -> Result |cwd| cwd.display().to_string(), )); - // The box's own environment first, then the request's on top, matching - // every other backend. + // The variables reach bwrap on its stdin (`--args 0`, see `environment`), + // never on this command line: `/proc//cmdline` is readable by every + // process of the user, and on an SSH host the line also travels inside + // the remote shell command. argv.push("--clearenv".to_owned()); - for (key, value) in spec.env.iter().chain(&request.env) { - argv.push("--setenv".to_owned()); - argv.push(key.clone()); - argv.push(value.clone()); + if !(spec.env.is_empty() && request.env.is_empty()) { + argv.push("--args".to_owned()); + argv.push("0".to_owned()); } argv.push("--".to_owned()); @@ -789,6 +790,35 @@ pub(super) fn bwrap(spec: &BoxSpec, request: &ExecRequest) -> Result Ok(argv) } +/// What `bwrap --args 0` reads on stdin: `--setenv KEY VALUE` for the box's +/// own environment, then the request's on top (matching every other +/// backend), each part NUL-terminated. `None` when there is nothing to set, +/// in which case [`bwrap`] adds no `--args`. +/// +/// This takes the command's stdin: namespace exec forwards the caller's stdin +/// only to a command with no environment, and refuses a command that has both. +/// +/// # Errors +/// [`Error::Backend`] for a key or value holding a NUL, which would end the +/// entry early and let the rest be read as further `bwrap` options. +pub(super) fn environment(spec: &BoxSpec, request: &ExecRequest) -> Result>> { + let mut bytes = Vec::new(); + for (key, value) in spec.env.iter().chain(&request.env) { + if key.contains('\0') || value.contains('\0') { + return Err(Error::Backend { + sandbox: NAME.to_owned(), + operation: "pass the command environment", + message: format!("the entry for {key:?} holds a NUL byte"), + }); + } + for part in ["--setenv", key.as_str(), value.as_str()] { + bytes.extend_from_slice(part.as_bytes()); + bytes.push(0); + } + } + Ok((!bytes.is_empty()).then_some(bytes)) +} + /// The read-only root the sandbox sees. fn root_filesystem() -> Vec { let mut argv = vec![ diff --git a/crates/tinybox-linux/src/sandbox/mod.rs b/crates/tinybox-linux/src/sandbox/mod.rs index ab951f1a..e19710a4 100644 --- a/crates/tinybox-linux/src/sandbox/mod.rs +++ b/crates/tinybox-linux/src/sandbox/mod.rs @@ -180,9 +180,27 @@ impl Sandbox for NamespaceSandbox { self.setrlimit_fallback, supports_expand_environment, )?; + // bwrap reads the environment on stdin (`--args 0`), so a command + // with an environment cannot also be given stdin of its own; that is + // refused rather than one of them being dropped. + let mut run = ExecRequest::new(argv); + match (args::environment(&info.spec, request)?, &request.stdin) { + (Some(_), Some(_)) => { + return Err(Error::Backend { + sandbox: NAME.to_owned(), + operation: "pass the command's stdin", + message: "a namespace box cannot give a command stdin while it also has an \ + environment: bubblewrap reads the environment on stdin" + .to_owned(), + }); + } + (Some(environment), None) => run = run.with_stdin(environment), + (None, Some(stdin)) => run = run.with_stdin(stdin.clone()), + (None, None) => {} + } // A command that runs and fails is a result, so the raw output is // returned rather than being turned into a backend error. - self.host.run(&ExecRequest::new(argv)).await + self.host.run(&run).await } async fn snapshot(&self, _id: &BoxId) -> Result { diff --git a/crates/tinybox-linux/src/sandbox/mod_tests.rs b/crates/tinybox-linux/src/sandbox/mod_tests.rs index 9d374ec4..f36ad5b1 100644 --- a/crates/tinybox-linux/src/sandbox/mod_tests.rs +++ b/crates/tinybox-linux/src/sandbox/mod_tests.rs @@ -25,6 +25,7 @@ use super::{NAME, NamespaceSandbox, args}; #[derive(Debug, Default)] struct RecordingHost { seen: Mutex>>, + stdin: Mutex>>>, } impl RecordingHost { @@ -36,6 +37,16 @@ impl RecordingHost { self.seen().last().cloned().unwrap_or_default() } + /// The stdin the last command was given. + fn last_stdin(&self) -> Option> { + self.stdin + .lock() + .unwrap_or_else(PoisonError::into_inner) + .last() + .cloned() + .flatten() + } + fn ran_anything(&self) -> bool { !self.seen().is_empty() } @@ -49,6 +60,10 @@ impl Host for RecordingHost { async fn run(&self, request: &ExecRequest) -> Result { self.seen().push(request.argv.clone()); + self.stdin + .lock() + .unwrap_or_else(PoisonError::into_inner) + .push(request.stdin.clone()); if request.argv == ["systemd-run", "--help"] { return Ok(ExecOutput::new( 0, @@ -283,13 +298,84 @@ async fn the_environment_is_cleared_and_then_rebuilt() -> Result<()> { let argv = host.last(); // The caller's own environment must not leak in: it routinely holds tokens. assert!(has(&argv, "--clearenv")); - let set = argv - .iter() - .enumerate() - .filter(|(_, part)| part.as_str() == "--setenv") - .filter_map(|(index, _)| argv.get(index + 1).cloned()) - .collect::>(); - assert_eq!(set, ["FROM_BOX", "FROM_COMMAND"]); + // No variable is on the command line, where every local process (and, + // for an SSH host, the remote shell) can read it; bwrap reads them on + // stdin, the box's first and the command's on top. + assert!(!has(&argv, "--setenv"), "{argv:?}"); + assert!(!argv.iter().any(|part| part.contains("FROM_")), "{argv:?}"); + assert!( + argv.windows(2).any(|w| w[0] == "--args" && w[1] == "0"), + "{argv:?}" + ); + assert_eq!( + host.last_stdin(), + Some(b"--setenv\0FROM_BOX\x001\0--setenv\0FROM_COMMAND\x002\0".to_vec()) + ); + Ok(()) +} + +#[tokio::test] +async fn without_an_environment_bwrap_reads_no_arguments_from_stdin() -> Result<()> { + let (sandbox, host) = sandbox(); + let info = sandbox.create(&spec()?).await?; + + sandbox.exec(&info.id, &ExecRequest::new(["true"])).await?; + + assert!(!has(&host.last(), "--args")); + assert_eq!(host.last_stdin(), None); + Ok(()) +} + +#[tokio::test] +async fn without_an_environment_the_callers_stdin_reaches_the_command() -> Result<()> { + let (sandbox, host) = sandbox(); + let info = sandbox.create(&spec()?).await?; + + sandbox + .exec( + &info.id, + &ExecRequest::new(["cat"]).with_stdin(b"payload".to_vec()), + ) + .await?; + + assert!(!has(&host.last(), "--args")); + assert_eq!(host.last_stdin(), Some(b"payload".to_vec())); + Ok(()) +} + +#[tokio::test] +async fn stdin_and_an_environment_together_are_refused_not_dropped() -> Result<()> { + let (sandbox, host) = sandbox(); + let info = sandbox.create(&spec()?.with_env("TOKEN", "1")).await?; + let before = host.seen().len(); + + let result = sandbox + .exec( + &info.id, + &ExecRequest::new(["cat"]).with_stdin(b"payload".to_vec()), + ) + .await; + + assert!(matches!(result, Err(Error::Backend { .. })), "{result:?}"); + assert_eq!(host.seen().len(), before, "a command ran"); + Ok(()) +} + +#[tokio::test] +async fn a_nul_in_an_environment_value_is_refused_before_anything_runs() -> Result<()> { + let (sandbox, host) = sandbox(); + let info = sandbox.create(&spec()?).await?; + let before = host.seen().len(); + + let result = sandbox + .exec( + &info.id, + &ExecRequest::new(["env"]).with_env("TOKEN", "a\0--bind"), + ) + .await; + + assert!(matches!(result, Err(Error::Backend { .. })), "{result:?}"); + assert_eq!(host.seen().len(), before, "a command ran"); Ok(()) } diff --git a/crates/tinybox-linux/tests/live_namespaces.rs b/crates/tinybox-linux/tests/live_namespaces.rs index f60f3e74..84984098 100644 --- a/crates/tinybox-linux/tests/live_namespaces.rs +++ b/crates/tinybox-linux/tests/live_namespaces.rs @@ -256,6 +256,82 @@ async fn live_the_callers_environment_does_not_leak_in() -> Result<()> { Ok(()) } +#[tokio::test] +async fn live_an_environment_value_is_on_no_command_line_while_the_box_runs() -> Result<()> { + if !enabled() { + return Ok(()); + } + let dir = workspace()?; + let sandbox = sandbox(); + let secret = format!("namespace-secret-{}", std::process::id()); + let spec = spec(dir.path())?.with_env("BOX_SECRET", secret.clone()); + let info = sandbox.create(&spec).await?; + + // The box signals through the workspace once it runs, then waits, so the + // host can look at every process while bwrap and the command are alive. + let running = { + let sandbox = sandbox.clone(); + let id = info.id.clone(); + tokio::spawn(async move { + let script = "printf '%s' \"$BOX_SECRET\" > seen; touch ready; sleep 3"; + sandbox + .exec(&id, &ExecRequest::new(["/bin/sh", "-c", script])) + .await + }) + }; + let ready = dir.path().join("ready"); + for _ in 0..100 { + if ready.exists() { + break; + } + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + } + let leaked = command_lines_containing(&secret)?; + let output = running.await.map_err(|error| Error::Backend { + sandbox: "namespace".to_owned(), + operation: "join the box command", + message: error.to_string(), + })??; + sandbox.destroy(&info.id).await?; + + assert!(ready.exists(), "the box command never started"); + assert!( + leaked.is_empty(), + "environment value on a command line: {leaked:?}" + ); + assert!(output.succeeded(), "{}", output.stderr_lossy()); + // The command still received it, from bwrap's stdin. + let seen = + fs::read_to_string(dir.path().join("seen")).map_err(|error| Error::io("read", &error))?; + assert_eq!(seen, secret); + Ok(()) +} + +/// Every host process command line that holds `needle`, NULs shown as spaces. +fn command_lines_containing(needle: &str) -> Result> { + let mut found = Vec::new(); + for entry in fs::read_dir("/proc").map_err(|error| Error::io("read /proc", &error))? { + let path = entry + .map_err(|error| Error::io("read /proc", &error))? + .path(); + if !path + .file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.bytes().all(|byte| byte.is_ascii_digit())) + { + continue; + } + if let Ok(cmdline) = fs::read(path.join("cmdline")) + && cmdline + .windows(needle.len()) + .any(|window| window == needle.as_bytes()) + { + found.push(String::from_utf8_lossy(&cmdline).replace('\0', " ")); + } + } + Ok(found) +} + #[tokio::test] async fn live_an_argument_is_never_interpreted_as_a_shell_would() -> Result<()> { if !enabled() {