diff --git a/src/context/mod.rs b/src/context/mod.rs index e0bd280..c7b0c6d 100644 --- a/src/context/mod.rs +++ b/src/context/mod.rs @@ -3,6 +3,7 @@ pub(crate) mod capture; mod local; mod manager; mod schroot; +pub(crate) mod shell; mod ssh; mod unshare; diff --git a/src/context/schroot.rs b/src/context/schroot.rs index 88d5a26..b7316bd 100644 --- a/src/context/schroot.rs +++ b/src/context/schroot.rs @@ -1,6 +1,7 @@ /// Schroot context: execute commands in a schroot session /// Not tested, will need more work! -use super::api::{ContextDriver, LineSink}; +use super::api::{Context, ContextConfig, ContextDriver, LineSink}; +use super::shell::shell_quote; use std::io; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -11,8 +12,6 @@ pub struct SchrootDriver { pub parent: Option>, } -use super::api::{Context, ContextConfig}; - impl SchrootDriver { fn parent(&self) -> Arc { self.parent @@ -106,6 +105,11 @@ impl SchrootDriver { /// Wrap `(program, args)` in `sh -c` when a working directory or /// environment variables are needed. + /// + /// Everything interpolated into the resulting shell string — the `cd` + /// target, env keys and values, the program and each argument — is + /// POSIX-shell-quoted (see [`shell_quote`]), so metacharacters (spaces, + /// quotes, `$`, ...) can neither split words nor trigger expansion. fn wrap_command( program: &str, args: &[String], @@ -119,17 +123,21 @@ impl SchrootDriver { let mut shell_cmd = String::new(); if let Some(dir) = cwd { - shell_cmd.push_str(&format!("cd {} && ", dir)); + shell_cmd.push_str(&format!("cd {} && ", shell_quote(dir))); } if !env.is_empty() { shell_cmd.push_str("env "); for (k, v) in env { - shell_cmd.push_str(&format!("{}={} ", k, v)); + shell_cmd.push_str(&format!("{}={} ", shell_quote(k), shell_quote(v))); } } - shell_cmd.push_str(&format!("{} {}", program, args.join(" "))); + shell_cmd.push_str(&shell_quote(program)); + for arg in args { + shell_cmd.push(' '); + shell_cmd.push_str(&shell_quote(arg)); + } actual_program = "sh".to_string(); actual_args = vec!["-c".to_string(), shell_cmd]; @@ -260,9 +268,13 @@ impl ContextDriver for SchrootDriver { &[ "-c".to_string(), format!( - "echo -ne '{}' > '{}'", - content.replace("'", "'\\''"), - path.to_string_lossy() + // `printf '%s'` writes the content verbatim (the previous + // `echo -ne` mangled backslashes, and dash's echo prints + // "-ne" literally). Content and path are shell-quoted so + // metacharacters in either cannot break out. + "printf '%s' {} > {}", + shell_quote(content), + shell_quote(&path.to_string_lossy()) ), ], &[], @@ -284,3 +296,74 @@ impl ContextDriver for SchrootDriver { Ok(status.success()) } } + +#[cfg(test)] +mod tests { + use super::SchrootDriver; + + /// Without cwd/env the program and args go to schroot as direct argv + /// (no shell involved), so they must pass through untouched. + #[test] + fn wrap_command_passthrough_without_env_or_cwd() { + let (prog, args) = SchrootDriver::wrap_command( + "make", + &["install".to_string(), "DEST=x y".to_string()], + &[], + None, + ); + assert_eq!(prog, "make"); + assert_eq!(args, vec!["install".to_string(), "DEST=x y".to_string()]); + } + + /// A value with a space must stay a single env assignment: previously + /// DEB_BUILD_OPTIONS="parallel=4 nocheck" made sh treat `nocheck` as + /// the command to run. + #[test] + fn wrap_command_quotes_env_values_cwd_and_args() { + let (prog, args) = SchrootDriver::wrap_command( + "dpkg-buildpackage", + &["-us".to_string(), "-uc".to_string()], + &[( + "DEB_BUILD_OPTIONS".to_string(), + "parallel=4 nocheck".to_string(), + )], + Some("/build/pkg 1.0"), + ); + assert_eq!(prog, "sh"); + assert_eq!(args[0], "-c"); + assert_eq!( + args[1], + "cd '/build/pkg 1.0' && env 'DEB_BUILD_OPTIONS'='parallel=4 nocheck' \ + 'dpkg-buildpackage' '-us' '-uc'" + ); + } + + /// The definitive check: a real shell must execute the wrapped command + /// exactly as intended — cwd applied, env set verbatim, the inner + /// program invoked with its argument — despite quotes in the values. + #[cfg(unix)] + #[test] + fn wrapped_command_survives_shell_parsing() { + let dir = tempfile::tempdir().unwrap(); + let (prog, args) = SchrootDriver::wrap_command( + "printenv", + &["SOME_OPT".to_string()], + &[( + "SOME_OPT".to_string(), + "parallel=4 noch'eck \"x\"".to_string(), + )], + Some(dir.path().to_str().unwrap()), + ); + let output = std::process::Command::new(&prog) + .arg(&args[0]) + .arg(&args[1]) + .output() + .unwrap(); + assert!(output.status.success()); + // printenv's output ends with a newline. + assert_eq!( + String::from_utf8_lossy(&output.stdout), + "parallel=4 noch'eck \"x\"\n" + ); + } +} diff --git a/src/context/shell.rs b/src/context/shell.rs new file mode 100644 index 0000000..59be34a --- /dev/null +++ b/src/context/shell.rs @@ -0,0 +1,109 @@ +//! POSIX-shell quoting for command strings assembled by the remote/chroot +//! execution contexts. +//! +//! Unlike [`super::local`] — which spawns programs directly through +//! `std::process::Command`, with no shell in between — the SSH, schroot and +//! unshare drivers ultimately hand a *string* to a shell (`ssh +//! channel.exec`, `sh -c`, `bash -c`). Every program name, argument, +//! path or environment value interpolated into such a string must be +//! quoted, or shell metacharacters (`;`, `|`, `&`, quotes, `$`, backticks, +//! globs, whitespace, ...) are reinterpreted by the shell: at best the +//! command breaks, at worst it executes injected input. + +/// Quote `s` for safe interpolation into a POSIX shell command line. +/// +/// The result is `s` wrapped in single quotes, with every embedded single +/// quote replaced by the standard `'\''` sequence (close the quoting, an +/// escaped literal quote, reopen). Whatever the input contains — spaces, +/// newlines, `"`, `'`, `$`, backticks, globs, `;` — the shell parses the +/// result back into exactly `s` as a single word. The empty string becomes +/// `''` (one empty argument, not zero arguments). +/// +/// Use this for *every* value interpolated into a shell command string: +/// programs, arguments, `cd` targets, `env` assignments (both key and +/// value) and paths. It is safe (though redundant) to quote values that are +/// known to need no quoting. +pub(crate) fn shell_quote(s: &str) -> String { + let mut quoted = String::with_capacity(s.len() + 2); + quoted.push('\''); + for c in s.chars() { + if c == '\'' { + quoted.push_str("'\\''"); + } else { + quoted.push(c); + } + } + quoted.push('\''); + quoted +} + +#[cfg(test)] +mod tests { + use super::shell_quote; + + #[test] + fn plain_word() { + assert_eq!(shell_quote("plain"), "'plain'"); + } + + #[test] + fn spaces_stay_one_word() { + assert_eq!(shell_quote("parallel=4 nocheck"), "'parallel=4 nocheck'"); + assert_eq!( + shell_quote(" leading and trailing "), + "' leading and trailing '" + ); + } + + #[test] + fn embedded_single_quotes() { + assert_eq!(shell_quote("it's"), "'it'\\''s'"); + assert_eq!(shell_quote("''"), r"''\'''\'''"); + } + + #[test] + fn double_quotes_and_metacharacters() { + assert_eq!( + shell_quote("say \"hi\" $HOME `id` ; | & * ?"), + "'say \"hi\" $HOME `id` ; | & * ?'" + ); + } + + #[test] + fn dollar_and_backtick_do_not_expand() { + assert_eq!(shell_quote("$HOME"), "'$HOME'"); + assert_eq!(shell_quote("$(rm -rf /)"), "'$(rm -rf /)'"); + assert_eq!(shell_quote("`touch /tmp/pwned`"), "'`touch /tmp/pwned`'"); + } + + #[test] + fn empty_string() { + assert_eq!(shell_quote(""), "''"); + } + + #[test] + fn unicode_preserved() { + assert_eq!(shell_quote("héllo→wörld ✓"), "'héllo→wörld ✓'"); + } + + #[test] + fn newlines_preserved() { + assert_eq!(shell_quote("a\nb"), "'a\nb'"); + } + + /// The definitive check: a real shell must parse the quoted string back + /// into the original value as a single argument, without expanding or + /// executing anything inside it. + #[cfg(unix)] + #[test] + fn round_trips_through_sh() { + let tricky = "a'b\"c $HOME `echo pwned` ; | & \n x*y"; + let output = std::process::Command::new("sh") + .arg("-c") + .arg(format!("printf '%s' {}", shell_quote(tricky))) + .output() + .unwrap(); + assert!(output.status.success()); + assert_eq!(String::from_utf8_lossy(&output.stdout), tricky); + } +} diff --git a/src/context/ssh.rs b/src/context/ssh.rs index d5d5b35..8c3f6c6 100644 --- a/src/context/ssh.rs +++ b/src/context/ssh.rs @@ -2,6 +2,7 @@ /// Context driver: Copies over SFTP with ssh2, executes commands over ssh2 channels use super::api::{ContextDriver, LineSink, Stream}; use super::capture::pump; +use super::shell::shell_quote; use log::debug; use ssh2; use std::fs; @@ -53,6 +54,41 @@ pub struct SshDriver { pub port: Option, } +impl SshDriver { + /// Build the remote shell command line: `export` assignments for `env`, + /// an optional `cd` to `cwd`, then `program` with its `args`. + /// + /// The line is executed verbatim by the remote login shell through + /// `channel.exec`, so every component is POSIX-shell-quoted (see + /// [`shell_quote`]): metacharacters in arguments, paths or environment + /// values can neither break out of their word nor be expanded by the + /// remote shell. + fn build_command_line( + env: &[(String, String)], + cwd: Option<&str>, + program: &str, + args: &[String], + ) -> String { + let mut cmd_line = String::new(); + for (key, value) in env { + cmd_line.push_str(&format!( + "export {}={}; ", + shell_quote(key), + shell_quote(value) + )); + } + if let Some(dir) = cwd { + cmd_line.push_str(&format!("cd {} && ", shell_quote(dir))); + } + cmd_line.push_str(&shell_quote(program)); + for arg in args { + cmd_line.push(' '); + cmd_line.push_str(&shell_quote(arg)); + } + cmd_line + } +} + impl ContextDriver for SshDriver { fn ensure_available(&self, src: &Path, dest_root: &str) -> io::Result { let sess = connect_ssh(&self.host, self.user.as_deref(), self.port)?; @@ -106,22 +142,7 @@ impl ContextDriver for SshDriver { // Construct command line with env vars // TODO: No, use ssh2 channel.set_env - let mut cmd_line = String::new(); - for (key, value) in env { - cmd_line.push_str(&format!( - "export {}='{}'; ", - key, - value.replace("'", "'\\''") - )); - } - if let Some(dir) = cwd { - cmd_line.push_str(&format!("cd {} && ", dir)); - } - cmd_line.push_str(program); - for arg in args { - cmd_line.push(' '); - cmd_line.push_str(arg); // TODO: escape - } + let cmd_line = Self::build_command_line(env, cwd, program, args); debug!("Executing SSH command: {}", cmd_line); @@ -152,23 +173,8 @@ impl ContextDriver for SshDriver { let sess = connect_ssh(&self.host, self.user.as_deref(), self.port)?; let mut channel = sess.channel_session().map_err(io::Error::other)?; - // Construct command line with env vars (same escaping as `run`) - let mut cmd_line = String::new(); - for (key, value) in env { - cmd_line.push_str(&format!( - "export {}='{}'; ", - key, - value.replace("'", "'\\''") - )); - } - if let Some(dir) = cwd { - cmd_line.push_str(&format!("cd {} && ", dir)); - } - cmd_line.push_str(program); - for arg in args { - cmd_line.push(' '); - cmd_line.push_str(arg); // TODO: escape - } + // Construct command line with env vars (same quoting as `run`) + let cmd_line = Self::build_command_line(env, cwd, program, args); debug!("Executing SSH command (captured): {}", cmd_line); @@ -200,23 +206,8 @@ impl ContextDriver for SshDriver { let sess = connect_ssh(&self.host, self.user.as_deref(), self.port)?; let mut channel = sess.channel_session().map_err(io::Error::other)?; - // Construct command line with env vars - let mut cmd_line = String::new(); - for (key, value) in env { - cmd_line.push_str(&format!( - "export {}='{}'; ", - key, - value.replace("'", "'\\''") - )); - } - if let Some(dir) = cwd { - cmd_line.push_str(&format!("cd {} && ", dir)); - } - cmd_line.push_str(program); - for arg in args { - cmd_line.push(' '); - cmd_line.push_str(arg); // TODO: escape - } + // Construct command line with env vars (same quoting as `run`) + let cmd_line = Self::build_command_line(env, cwd, program, args); channel.exec(&cmd_line).map_err(io::Error::other)?; @@ -266,7 +257,11 @@ impl ContextDriver for SshDriver { let sess = connect_ssh(&self.host, self.user.as_deref(), self.port)?; let mut channel = sess.channel_session().map_err(io::Error::other)?; // TODO: use sftp - let cmd = format!("cp -a {:?} {:?}", src, dest); + let cmd = format!( + "cp -a {} {}", + shell_quote(&src.to_string_lossy()), + shell_quote(&dest.to_string_lossy()) + ); debug!("Executing remote copy: {}", cmd); channel.exec(&cmd).map_err(io::Error::other)?; channel.wait_close().map_err(io::Error::other)?; @@ -360,3 +355,65 @@ impl SshDriver { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::SshDriver; + + /// Program, arguments and the `cd` target must each be a single, + /// quoted word; `$` in the cwd must not be expanded. + #[test] + fn command_line_quotes_program_args_and_cwd() { + let line = SshDriver::build_command_line( + &[], + Some("/tmp/some dir/$HOST"), + "make", + &["install".to_string(), "PREFIX=/opt/my app".to_string()], + ); + assert_eq!( + line, + "cd '/tmp/some dir/$HOST' && 'make' 'install' 'PREFIX=/opt/my app'" + ); + } + + /// Env keys and values are quoted too (values used to be escaped by + /// hand, keys and everything else not at all). + #[test] + fn command_line_quotes_env_keys_and_values() { + let line = SshDriver::build_command_line( + &[( + "DEB_BUILD_OPTIONS".to_string(), + "parallel=4 nocheck".to_string(), + )], + None, + "dpkg-buildpackage", + &["-us".to_string(), "-uc".to_string()], + ); + assert_eq!( + line, + "export 'DEB_BUILD_OPTIONS'='parallel=4 nocheck'; 'dpkg-buildpackage' '-us' '-uc'" + ); + } + + /// The definitive check: a real shell must execute the assembled line + /// exactly as intended — one argument through, one env value verbatim — + /// even when both contain quotes, spaces and `$`. + #[cfg(unix)] + #[test] + fn command_line_survives_shell_parsing() { + let dir = tempfile::tempdir().unwrap(); + let line = SshDriver::build_command_line( + &[("OPT".to_string(), "a b'c \"$d\"".to_string())], + Some(dir.path().to_str().unwrap()), + "printenv", + &["OPT".to_string()], + ); + let output = std::process::Command::new("sh") + .arg("-c") + .arg(&line) + .output() + .unwrap(); + assert!(output.status.success()); + assert_eq!(String::from_utf8_lossy(&output.stdout), "a b'c \"$d\"\n"); + } +} diff --git a/src/context/unshare.rs b/src/context/unshare.rs index 6847dae..a7be3a1 100644 --- a/src/context/unshare.rs +++ b/src/context/unshare.rs @@ -1,4 +1,5 @@ use super::api::{Context, ContextCommand, ContextDriver, LineSink}; +use super::shell::shell_quote; use log::debug; use std::fs; use std::io; @@ -484,21 +485,85 @@ impl UnshareDriver { // Build the bash command: set up /dev/pts and run the program // /proc should already be bind-mounted from the host before entering the namespace - let program_args = args - .iter() - .map(|a| format!("\"{a}\"")) - .collect::>() - .join(" "); - cmd.arg("--") .arg("bash") .arg("-c") - .arg(format!( - "mkdir -p /dev/pts; mount -t devpts devpts /dev/pts 2>/dev/null || true; touch /dev/ptmx; mount --bind /dev/pts/ptmx /dev/ptmx 2>/dev/null || true; {} {}", - program, - program_args - )); + .arg(build_namespace_script(program, args)); cmd } } + +/// Build the shell script executed by `bash -c` inside the user namespace: +/// bring up `/dev/pts`, then run `program` with `args`. +/// +/// The script is parsed by bash, so the program and every argument are +/// POSIX-shell-quoted (see [`shell_quote`]): quotes, `$`, backticks or +/// whitespace inside them can neither split the command into different +/// words nor trigger expansion. (Previously arguments were wrapped in +/// unescaped double quotes, so a `"` in an argument broke out and +/// `$`/backticks still expanded.) +fn build_namespace_script(program: &str, args: &[String]) -> String { + let mut script = String::from( + "mkdir -p /dev/pts; mount -t devpts devpts /dev/pts 2>/dev/null || true; touch /dev/ptmx; mount --bind /dev/pts/ptmx /dev/ptmx 2>/dev/null || true; ", + ); + script.push_str(&shell_quote(program)); + for arg in args { + script.push(' '); + script.push_str(&shell_quote(arg)); + } + script +} + +#[cfg(test)] +mod tests { + use super::build_namespace_script; + + fn tail_after_devpts_setup(script: &str) -> &str { + script + .rsplit_once("|| true; ") + .map(|(_, rest)| rest) + .unwrap() + .trim_end() + } + + /// Program and arguments must each be a single, quoted word at the end + /// of the `/dev/pts` setup script. + #[test] + fn script_quotes_program_and_args() { + let script = build_namespace_script( + "make", + &[ + "install".to_string(), + "a b".to_string(), + "PREFIX=/opt/my app".to_string(), + ], + ); + assert_eq!( + tail_after_devpts_setup(&script), + "'make' 'install' 'a b' 'PREFIX=/opt/my app'" + ); + } + + /// An argument containing a double quote must not break out of the + /// script (arguments used to be wrapped in unescaped `"`), and `$`/ + /// backticks must stay literal for bash. + #[test] + fn script_neutralizes_quotes_and_expansions() { + let script = build_namespace_script( + "echo", + &["$(touch /tmp/pwned) `id` \"; rm -rf /\"".to_string()], + ); + assert_eq!( + tail_after_devpts_setup(&script), + "'echo' '$(touch /tmp/pwned) `id` \"; rm -rf /\"'" + ); + } + + /// Empty arguments must survive as one empty word (`''`), not vanish. + #[test] + fn script_preserves_empty_args() { + let script = build_namespace_script("prog", &[String::new(), "x".to_string()]); + assert_eq!(tail_after_devpts_setup(&script), "'prog' '' 'x'"); + } +}