Keep a namespace box's environment off every command line

crates/tinybox-linux/src/sandbox/args.rs

@@ -775,20 +775,50 @@ pub(super) fn bwrap(spec: &BoxSpec, request: &ExecRequest) -> Result<Vec<String>
         |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/<pid>/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());
     argv.extend(request.argv.iter().cloned());
     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<Option<Vec<u8>>> {
+    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<String> {
     let mut argv = vec![

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<SnapshotId> {

crates/tinybox-linux/src/sandbox/mod_tests.rs

@@ -25,6 +25,7 @@ use super::{NAME, NamespaceSandbox, args};
 #[derive(Debug, Default)]
 struct RecordingHost {
     seen: Mutex<Vec<Vec<String>>>,
+    stdin: Mutex<Vec<Option<Vec<u8>>>>,
 }
 
 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<Vec<u8>> {
+        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<ExecOutput> {
         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::<Vec<_>>();
-    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(())
 }
 

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<Vec<String>> {
+    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() {