Keep a resource jail's environment off every command line

crates/tinybox-linux/src/lib.rs

@@ -47,6 +47,6 @@ mod sandbox;
 pub use probe::{NamespaceProbe, UsernsStatus};
 
 pub use sandbox::{
-    NAME, NamespaceSandbox, NativeJailCommand, WORKSPACE_MOUNT, native_jail_argv,
+    NAME, NamespaceSandbox, NativeJailCommand, NativeJailLaunch, WORKSPACE_MOUNT, native_jail_argv,
     native_jail_argv_with_programs, trusted_systemd_run_program,
 };

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

@@ -342,7 +342,7 @@ pub struct NativeJailCommand {
 /// # Errors
 /// Returns an error when a request is invalid, unsupported, or systemd's
 /// command-argument expansion behavior cannot be verified.
-pub fn native_jail_argv(request: &NativeJailCommand) -> std::io::Result<Vec<String>> {
+pub fn native_jail_argv(request: &NativeJailCommand) -> std::io::Result<NativeJailLaunch> {
     native_jail_argv_with_programs(
         request,
         &trusted_systemd_run_program()?,
@@ -360,13 +360,50 @@ pub fn native_jail_argv_with_programs(
     request: &NativeJailCommand,
     systemd_run: &Path,
     bwrap: &Path,
-) -> std::io::Result<Vec<String>> {
+) -> std::io::Result<NativeJailLaunch> {
     validate_native_jail_command(request)?;
     let systemd_run = validate_trusted_program(systemd_run)?;
     let bwrap = validate_trusted_program(bwrap)?;
     let supports_expand_environment = systemd_run_supports_safe_arguments(&systemd_run)?;
+    let env_file = if request.env.is_empty() {
+        None
+    } else {
+        let avoid: Vec<&Path> = std::iter::once(request.root.as_path())
+            .chain(request.read_only.iter().map(PathBuf::as_path))
+            .chain(request.read_write.iter().map(PathBuf::as_path))
+            .collect();
+        Some(super::env_file::EnvArgsFile::create(&request.env, &avoid)?)
+    };
     let argv = native_bwrap_argv(request, supports_expand_environment, &systemd_run, &bwrap);
-    Ok(native_launcher_argv(wrap_scope_command(argv)))
+    Ok(NativeJailLaunch {
+        argv: native_launcher_argv(
+            env_file.as_ref().map(super::env_file::EnvArgsFile::path),
+            wrap_scope_command(argv),
+        ),
+        env_file,
+    })
+}
+
+/// A native jail's launcher command, and the private file carrying its
+/// environment (see `env_file`).
+///
+/// Keep it alive until the launcher has started: the launcher deletes the file
+/// once it has it open, and dropping this removes it if the launcher never ran.
+#[derive(Debug)]
+pub struct NativeJailLaunch {
+    /// The launcher's program and arguments. Holds no environment value.
+    pub argv: Vec<String>,
+    env_file: Option<super::env_file::EnvArgsFile>,
+}
+
+impl NativeJailLaunch {
+    /// Where the target's environment waits for the launcher, if it has one.
+    #[must_use]
+    pub fn env_file(&self) -> Option<&Path> {
+        self.env_file
+            .as_ref()
+            .map(super::env_file::EnvArgsFile::path)
+    }
 }
 
 /// Sample cgroup counters from inside the transient scope before it is stopped.
@@ -673,27 +710,33 @@ fn native_bwrap_argv(
         request.cwd.display().to_string(),
         "--clearenv".into(),
     ]);
-    for (key, value) in &request.env {
-        argv.extend(["--setenv".into(), key.clone(), value.clone()]);
+    // The target environment is read from the private file the launcher opened
+    // as descriptor 9, never from this command line (see `env_file`).
+    if !request.env.is_empty() {
+        argv.extend(["--args".into(), super::env_file::ENV_ARGS_FD.into()]);
     }
     argv.push("--".into());
     argv.extend(request.argv.iter().cloned());
     // A scope launcher inherits the host process's descriptors. The trusted
     // shim closes every non-stdio descriptor before opening systemd's bus or
     // starting bubblewrap, preventing a pre-opened manager/cgroup FD from
-    // bypassing the mount and network namespaces. Target values remain argv
-    // data and the shim receives only a fixed, sanitized launcher environment.
+    // bypassing the mount and network namespaces. The target environment is
+    // not argv data: it travels in the private file on descriptor 9, and the
+    // shim receives only a fixed, sanitized launcher environment.
     argv
 }
 
-fn native_launcher_argv(argv: Vec<String>) -> Vec<String> {
+/// The launcher shim, given the private environment file (or `-` when the
+/// target has no environment) and then the scope command.
+fn native_launcher_argv(env_file: Option<&Path>, argv: Vec<String>) -> Vec<String> {
     let mut launcher = vec![
         "/bin/bash".to_owned(),
         "--noprofile".to_owned(),
         "--norc".to_owned(),
         "-c".to_owned(),
-        "[ -d /proc/self/fd ] || exit 125; shopt -s nullglob; fdpaths=(/proc/self/fd/*); ((${#fdpaths[@]} > 0)) || exit 125; for fdpath in \"${fdpaths[@]}\"; do fd=${fdpath##*/}; case $fd in 0|1|2|*[!0-9]*) continue;; esac; eval \"exec ${fd}>&-\" || exit 125; done; unit=; for arg in \"$@\"; do case $arg in --unit=*) unit=${arg#--unit=};; esac; done; [ -n \"$unit\" ] || exit 125; \"$@\"; status=$?; if ! /usr/bin/systemctl --user stop \"$unit\"; then /usr/bin/systemctl --user is-active --quiet \"$unit\" && exit 125; fi; exit \"$status\"".to_owned(),
+        "[ -d /proc/self/fd ] || exit 125; shopt -s nullglob; envfile=$1; shift; keep=; if [ \"$envfile\" != - ]; then exec 9<\"$envfile\" || exit 125; command -p rm -f -- \"$envfile\" || exit 125; command -p rmdir -- \"${envfile%/*}\" 2>/dev/null; keep=9; fi; fdpaths=(/proc/self/fd/*); ((${#fdpaths[@]} > 0)) || exit 125; for fdpath in \"${fdpaths[@]}\"; do fd=${fdpath##*/}; case $fd in 0|1|2|*[!0-9]*) continue;; esac; [ \"$fd\" = \"$keep\" ] && continue; eval \"exec ${fd}>&-\" || exit 125; done; unit=; for arg in \"$@\"; do case $arg in --unit=*) unit=${arg#--unit=};; esac; done; [ -n \"$unit\" ] || exit 125; \"$@\"; status=$?; if ! /usr/bin/systemctl --user stop \"$unit\"; then /usr/bin/systemctl --user is-active --quiet \"$unit\" && exit 125; fi; exit \"$status\"".to_owned(),
         "tinybox-native-launcher".to_owned(),
+        env_file.map_or_else(|| "-".to_owned(), |path| path.display().to_string()),
     ];
     launcher.extend(argv);
     launcher

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

@@ -0,0 +1,133 @@
+//! Keeping a jailed command's environment out of every command line.
+//!
+//! `/proc/<pid>/cmdline` is readable by every process of the same user, so a
+//! `--setenv TOKEN value` on the `bwrap`, `systemd-run` or launcher command
+//! line hands the value to all of them. The `--setenv` pairs are written,
+//! NUL-separated, to a private file (mode 0600 in a fresh 0700 directory) that
+//! the launcher opens as descriptor 9 and deletes before anything else runs;
+//! `bwrap --args 9` reads them from there. Setting the variables on the
+//! launcher's own environment instead would expose them to the host-side
+//! programs on the way (the shell, `systemd-run`), and let a value such as
+//! `LD_PRELOAD` run code there.
+
+use std::collections::BTreeMap;
+use std::fmt::Write as _;
+use std::io::{self, Read as _};
+use std::path::{Path, PathBuf};
+
+/// The descriptor the launcher opens the file on, and `bwrap --args` reads.
+pub(super) const ENV_ARGS_FD: &str = "9";
+
+/// A private file holding a jailed command's `--setenv` arguments.
+///
+/// Removed when dropped, in case the launcher never ran to remove it.
+#[derive(Debug)]
+pub(super) struct EnvArgsFile {
+    path: PathBuf,
+}
+
+impl EnvArgsFile {
+    /// Writes `env` as `--setenv KEY VALUE` triples into a new private file in
+    /// `$XDG_RUNTIME_DIR` (else the temp directory), refusing a directory that
+    /// lies under any of `avoid`, since the jailed program could then read it.
+    ///
+    /// # Errors
+    /// `InvalidInput` when a key or value holds a NUL; `PermissionDenied` when
+    /// every candidate directory is under `avoid`; otherwise the I/O error.
+    pub(super) fn create(env: &BTreeMap<String, String>, avoid: &[&Path]) -> io::Result<Self> {
+        let mut contents = Vec::new();
+        for (key, value) in env {
+            if key.contains('\0') || value.contains('\0') {
+                return Err(io::Error::new(
+                    io::ErrorKind::InvalidInput,
+                    "an environment entry holds a NUL",
+                ));
+            }
+            for part in ["--setenv", key.as_str(), value.as_str()] {
+                contents.extend_from_slice(part.as_bytes());
+                contents.push(0);
+            }
+        }
+        let base = private_base(avoid)?;
+        let dir = base.join(format!("tinybox-env-{}", random_hex()?));
+        let path = dir.join("env");
+        if let Err(error) = write_private(&dir, &path, &contents) {
+            let _ = std::fs::remove_file(&path);
+            let _ = std::fs::remove_dir(&dir);
+            return Err(error);
+        }
+        Ok(Self { path })
+    }
+
+    /// Where the file is; the launcher's first argument.
+    #[must_use]
+    pub(super) fn path(&self) -> &Path {
+        &self.path
+    }
+}
+
+impl Drop for EnvArgsFile {
+    fn drop(&mut self) {
+        let _ = std::fs::remove_file(&self.path);
+        if let Some(dir) = self.path.parent() {
+            let _ = std::fs::remove_dir(dir);
+        }
+    }
+}
+
+/// Creates `dir` (0700) and writes `contents` into a new `path` (0600) in it.
+#[cfg(unix)]
+fn write_private(dir: &Path, path: &Path, contents: &[u8]) -> io::Result<()> {
+    use std::io::Write as _;
+    use std::os::unix::fs::{DirBuilderExt as _, OpenOptionsExt as _};
+
+    std::fs::DirBuilder::new().mode(0o700).create(dir)?;
+    std::fs::OpenOptions::new()
+        .write(true)
+        .create_new(true)
+        .mode(0o600)
+        .open(path)?
+        .write_all(contents)
+}
+
+/// The native jail runs only on Linux; elsewhere there is nothing to launch.
+#[cfg(not(unix))]
+fn write_private(_dir: &Path, _path: &Path, _contents: &[u8]) -> io::Result<()> {
+    Err(io::Error::new(
+        io::ErrorKind::Unsupported,
+        "private environment files need a Unix host",
+    ))
+}
+
+/// `$XDG_RUNTIME_DIR` when set and outside `avoid`, else the temp directory,
+/// canonicalized before the check so a link into a grant is caught.
+fn private_base(avoid: &[&Path]) -> io::Result<PathBuf> {
+    std::env::var_os("XDG_RUNTIME_DIR")
+        .map(PathBuf::from)
+        .filter(|dir| dir.is_absolute())
+        .into_iter()
+        .chain(std::iter::once(std::env::temp_dir()))
+        .filter_map(|dir| dir.canonicalize().ok())
+        .find(|dir| dir.is_dir() && !avoid.iter().any(|grant| dir.starts_with(grant)))
+        .ok_or_else(|| {
+            io::Error::new(
+                io::ErrorKind::PermissionDenied,
+                "no private directory outside the jail's grants for its environment",
+            )
+        })
+}
+
+/// 24 hex characters from the kernel's random source.
+fn random_hex() -> io::Result<String> {
+    let mut random = [0_u8; 12];
+    std::fs::File::open("/dev/urandom")?.read_exact(&mut random)?;
+    let mut out = String::new();
+    for byte in random {
+        let _ = write!(out, "{byte:02x}");
+    }
+    Ok(out)
+}
+
+#[cfg(all(test, unix))]
+#[path = "env_file_tests.rs"]
+mod tests;

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

@@ -0,0 +1,65 @@
+//! The private environment file: contents, permissions and cleanup.
+
+use super::*;
+use std::os::unix::fs::PermissionsExt as _;
+
+fn env(pairs: &[(&str, &str)]) -> BTreeMap<String, String> {
+    pairs
+        .iter()
+        .map(|(key, value)| ((*key).to_owned(), (*value).to_owned()))
+        .collect()
+}
+
+#[test]
+fn the_file_holds_nul_separated_setenv_triples() -> io::Result<()> {
+    let file = EnvArgsFile::create(&env(&[("A", "1"), ("TOKEN", "s3cret value")]), &[])?;
+
+    let contents = std::fs::read(file.path())?;
+
+    assert_eq!(
+        contents,
+        b"--setenv\0A\x001\0--setenv\0TOKEN\0s3cret value\0".to_vec()
+    );
+    Ok(())
+}
+
+#[test]
+fn the_file_and_its_directory_are_private_and_removed_on_drop() -> io::Result<()> {
+    let file = EnvArgsFile::create(&env(&[("K", "v")]), &[])?;
+    let path = file.path().to_path_buf();
+    let dir = path
+        .parent()
+        .map(Path::to_path_buf)
+        .ok_or(io::ErrorKind::NotFound)?;
+
+    assert_eq!(
+        std::fs::metadata(&path)?.permissions().mode() & 0o777,
+        0o600
+    );
+    assert_eq!(std::fs::metadata(&dir)?.permissions().mode() & 0o777, 0o700);
+
+    drop(file);
+    assert!(!path.exists());
+    assert!(!dir.exists());
+    Ok(())
+}
+
+#[test]
+fn a_nul_in_an_entry_is_refused() {
+    for pairs in [[("K\0", "v")], [("K", "v\0")]] {
+        let kind = EnvArgsFile::create(&env(&pairs), &[])
+            .err()
+            .map(|e| e.kind());
+        assert_eq!(kind, Some(io::ErrorKind::InvalidInput));
+    }
+}
+
+#[test]
+fn a_directory_inside_a_grant_is_never_used() {
+    // Every candidate (the runtime dir and the temp dir) lies under `/`.
+    let root = Path::new("/");
+    let kind = EnvArgsFile::create(&env(&[("K", "v")]), &[root])
+        .err()
+        .map(|e| e.kind());
+    assert_eq!(kind, Some(io::ErrorKind::PermissionDenied));
+}

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

@@ -10,10 +10,11 @@ use tinybox_core::{
 };
 
 mod args;
+mod env_file;
 
 pub use args::{
-    NativeJailCommand, WORKSPACE_MOUNT, native_jail_argv, native_jail_argv_with_programs,
-    trusted_systemd_run_program,
+    NativeJailCommand, NativeJailLaunch, WORKSPACE_MOUNT, native_jail_argv,
+    native_jail_argv_with_programs, trusted_systemd_run_program,
 };
 
 /// The name this sandbox registers under.

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

@@ -452,7 +452,7 @@ fn native_jail_command_keeps_limits_and_target_environment_after_namespace_setup
     let grant = root.path().join("read-only");
     std::fs::create_dir(&grant)?;
     let systemd_run = crate::trusted_systemd_run_program()?;
-    let argv = crate::native_jail_argv_with_programs(
+    let launch = crate::native_jail_argv_with_programs(
         &crate::NativeJailCommand {
             root: root.path().to_path_buf(),
             cwd,
@@ -470,6 +470,7 @@ fn native_jail_command_keeps_limits_and_target_environment_after_namespace_setup
         &systemd_run,
         Path::new("/bin/true"),
     )?;
+    let argv = launch.argv.clone();
     assert_eq!(argv.first().map(String::as_str), Some("/bin/bash"));
     assert!(argv.contains(&systemd_run.display().to_string()));
     assert!(systemd_run.is_absolute());
@@ -493,15 +494,21 @@ fn native_jail_command_keeps_limits_and_target_environment_after_namespace_setup
     );
     assert!(has(&argv, "--ro-bind"));
     assert!(has(&argv, grant.to_str().ok_or("grant path is not UTF-8")?));
-    assert!(has(&argv, "BASH_ENV"));
-    let systemd_end = argv
+    // The target environment is on no command line, the launcher's included:
+    // `/proc/<pid>/cmdline` is readable by every process of the user.
+    assert!(!argv.iter().any(|part| part.contains("BASH_ENV")));
+    assert!(!argv.iter().any(|part| part.contains("/tmp/target-init")));
+    assert!(!has(&argv, "--setenv"));
+    let args_at = argv
         .iter()
-        .position(|part| part == "--")
-        .ok_or("missing systemd delimiter")?;
-    assert!(
-        !argv[..systemd_end]
-            .iter()
-            .any(|part| part.contains("BASH_ENV"))
+        .position(|part| part == "--args")
+        .ok_or("bwrap reads the environment from a descriptor")?;
+    assert_eq!(argv.get(args_at + 1).map(String::as_str), Some("9"));
+    let file = launch.env_file().ok_or("an environment file")?;
+    assert!(has(&argv, file.to_str().ok_or("file path is not UTF-8")?));
+    assert_eq!(
+        std::fs::read(file)?,
+        b"--setenv\0BASH_ENV\0/tmp/target-init\0".to_vec()
     );
     Ok(())
 }