From 7c64bf1b9b6e8e97ab652a4922f1c0e68ebc77f0 Mon Sep 17 00:00:00 2001 From: Kamal Marhubi Date: Fri, 15 Jan 2016 15:29:45 -0500 Subject: [PATCH] std: Properly handle interior NULs in std::process This reports an error at the point of calling `Command::spawn()` or one of its equivalents. Fixes https://github.com/rust-lang/rust/issues/30858 Fixes https://github.com/rust-lang/rust/issues/30862 --- src/libstd/process.rs | 56 +++++++++++++++++++-- src/libstd/sys/unix/ext/process.rs | 6 +-- src/libstd/sys/unix/process.rs | 72 ++++++++++++++++++++------ src/libstd/sys/windows/process.rs | 81 +++++++++++++++++++----------- 4 files changed, 163 insertions(+), 52 deletions(-) diff --git a/src/libstd/process.rs b/src/libstd/process.rs index 7197dfa8b2d47..2456226b54cde 100644 --- a/src/libstd/process.rs +++ b/src/libstd/process.rs @@ -343,11 +343,7 @@ impl fmt::Debug for Command { /// non-utf8 data is lossily converted using the utf8 replacement /// character. fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - try!(write!(f, "{:?}", self.inner.program)); - for arg in &self.inner.args { - try!(write!(f, " {:?}", arg)); - } - Ok(()) + self.inner.fmt(f) } } @@ -877,4 +873,54 @@ mod tests { assert!(output.contains("RUN_TEST_NEW_ENV=123"), "didn't find RUN_TEST_NEW_ENV inside of:\n\n{}", output); } + + // Regression tests for #30858. + #[test] + fn test_interior_nul_in_progname_is_error() { + match Command::new("has-some-\0\0s-inside").spawn() { + Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput), + Ok(_) => panic!(), + } + } + + #[test] + fn test_interior_nul_in_arg_is_error() { + match Command::new("echo").arg("has-some-\0\0s-inside").spawn() { + Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput), + Ok(_) => panic!(), + } + } + + #[test] + fn test_interior_nul_in_args_is_error() { + match Command::new("echo").args(&["has-some-\0\0s-inside"]).spawn() { + Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput), + Ok(_) => panic!(), + } + } + + #[test] + fn test_interior_nul_in_current_dir_is_error() { + match Command::new("echo").current_dir("has-some-\0\0s-inside").spawn() { + Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput), + Ok(_) => panic!(), + } + } + + // Regression tests for #30862. + #[test] + fn test_interior_nul_in_env_key_is_error() { + match env_cmd().env("has-some-\0\0s-inside", "value").spawn() { + Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput), + Ok(_) => panic!(), + } + } + + #[test] + fn test_interior_nul_in_env_value_is_error() { + match env_cmd().env("key", "has-some-\0\0s-inside").spawn() { + Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput), + Ok(_) => panic!(), + } + } } diff --git a/src/libstd/sys/unix/ext/process.rs b/src/libstd/sys/unix/ext/process.rs index e1111f25db721..212aeb0406eba 100644 --- a/src/libstd/sys/unix/ext/process.rs +++ b/src/libstd/sys/unix/ext/process.rs @@ -49,17 +49,17 @@ pub trait CommandExt { #[stable(feature = "rust1", since = "1.0.0")] impl CommandExt for process::Command { fn uid(&mut self, id: uid_t) -> &mut process::Command { - self.as_inner_mut().uid = Some(id); + self.as_inner_mut().uid(id); self } fn gid(&mut self, id: gid_t) -> &mut process::Command { - self.as_inner_mut().gid = Some(id); + self.as_inner_mut().gid(id); self } fn session_leader(&mut self, on: bool) -> &mut process::Command { - self.as_inner_mut().session_leader = on; + self.as_inner_mut().session_leader(on); self } } diff --git a/src/libstd/sys/unix/process.rs b/src/libstd/sys/unix/process.rs index 4a91cece143a9..04b39f0851a3f 100644 --- a/src/libstd/sys/unix/process.rs +++ b/src/libstd/sys/unix/process.rs @@ -31,57 +31,95 @@ use sys::{self, cvt, cvt_r}; #[derive(Clone)] pub struct Command { - pub program: CString, - pub args: Vec, - pub env: Option>, - pub cwd: Option, - pub uid: Option, - pub gid: Option, - pub session_leader: bool, + program: CString, + args: Vec, + env: Option>, // Guaranteed to have no NULs. + cwd: Option, + uid: Option, + gid: Option, + session_leader: bool, + saw_nul: bool, } impl Command { pub fn new(program: &OsStr) -> Command { + let mut saw_nul = false; Command { - program: os2c(program), + program: os2c(program, &mut saw_nul), args: Vec::new(), env: None, cwd: None, uid: None, gid: None, session_leader: false, + saw_nul: saw_nul, } } pub fn arg(&mut self, arg: &OsStr) { - self.args.push(os2c(arg)); + self.args.push(os2c(arg, &mut self.saw_nul)); } pub fn args<'a, I: Iterator>(&mut self, args: I) { - self.args.extend(args.map(os2c)); + let mut saw_nul = self.saw_nul; + self.args.extend(args.map(|arg| os2c(arg, &mut saw_nul))); + self.saw_nul = saw_nul; } fn init_env_map(&mut self) { if self.env.is_none() { + // Will not add NULs to env: preexisting environment will not contain any. self.env = Some(env::vars_os().collect()); } } pub fn env(&mut self, key: &OsStr, val: &OsStr) { + let k = OsString::from_vec(os2c(key, &mut self.saw_nul).into_bytes()); + let v = OsString::from_vec(os2c(val, &mut self.saw_nul).into_bytes()); + + // Will not add NULs to env: return without inserting if any were seen. + if self.saw_nul { + return; + } + self.init_env_map(); - self.env.as_mut().unwrap().insert(key.to_os_string(), val.to_os_string()); + self.env.as_mut() + .unwrap() + .insert(k, v); } pub fn env_remove(&mut self, key: &OsStr) { self.init_env_map(); - self.env.as_mut().unwrap().remove(&key.to_os_string()); + self.env.as_mut().unwrap().remove(key); } pub fn env_clear(&mut self) { self.env = Some(HashMap::new()) } pub fn cwd(&mut self, dir: &OsStr) { - self.cwd = Some(os2c(dir)); + self.cwd = Some(os2c(dir, &mut self.saw_nul)); + } + pub fn uid(&mut self, id: uid_t) { + self.uid = Some(id); } + pub fn gid(&mut self, id: gid_t) { + self.gid = Some(id); + } + pub fn session_leader(&mut self, session_leader: bool) { + self.session_leader = session_leader; + } +} + +fn os2c(s: &OsStr, saw_nul: &mut bool) -> CString { + CString::new(s.as_bytes()).unwrap_or_else(|_e| { + *saw_nul = true; + CString::new("").unwrap() + }) } -fn os2c(s: &OsStr) -> CString { - CString::new(s.as_bytes()).unwrap() +impl fmt::Debug for Command { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + try!(write!(f, "{:?}", self.program)); + for arg in &self.args { + try!(write!(f, " {:?}", arg)); + } + Ok(()) + } } //////////////////////////////////////////////////////////////////////////////// @@ -175,6 +213,10 @@ impl Process { in_fd: Stdio, out_fd: Stdio, err_fd: Stdio) -> io::Result { + if cfg.saw_nul { + return Err(io::Error::new(ErrorKind::InvalidInput, "nul byte found in provided data")); + } + let dirp = cfg.cwd.as_ref().map(|c| c.as_ptr()).unwrap_or(ptr::null()); let (envp, _a, _b) = make_envp(cfg.env.as_ref()); diff --git a/src/libstd/sys/windows/process.rs b/src/libstd/sys/windows/process.rs index 8b17a0531621d..61cf28be16f91 100644 --- a/src/libstd/sys/windows/process.rs +++ b/src/libstd/sys/windows/process.rs @@ -18,7 +18,7 @@ use env; use ffi::{OsString, OsStr}; use fmt; use fs; -use io::{self, Error}; +use io::{self, Error, ErrorKind}; use libc::c_void; use mem; use os::windows::ffi::OsStrExt; @@ -43,13 +43,21 @@ fn mk_key(s: &OsStr) -> OsString { }) } +fn ensure_no_nuls>(str: T) -> io::Result { + if str.as_ref().encode_wide().any(|b| b == 0) { + Err(io::Error::new(ErrorKind::InvalidInput, "nul byte found in provided data")) + } else { + Ok(str) + } +} + #[derive(Clone)] pub struct Command { - pub program: OsString, - pub args: Vec, - pub env: Option>, - pub cwd: Option, - pub detach: bool, // not currently exposed in std::process + program: OsString, + args: Vec, + env: Option>, + cwd: Option, + detach: bool, // not currently exposed in std::process } impl Command { @@ -92,6 +100,16 @@ impl Command { } } +impl fmt::Debug for Command { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + try!(write!(f, "{:?}", self.program)); + for arg in &self.args { + try!(write!(f, " {:?}", arg)); + } + Ok(()) + } +} + //////////////////////////////////////////////////////////////////////////////// // Processes //////////////////////////////////////////////////////////////////////////////// @@ -153,7 +171,7 @@ impl Process { si.hStdError = stderr.raw(); let program = program.as_ref().unwrap_or(&cfg.program); - let mut cmd_str = make_command_line(program, &cfg.args); + let mut cmd_str = try!(make_command_line(program, &cfg.args)); cmd_str.push(0); // add null terminator // stolen from the libuv code. @@ -162,8 +180,8 @@ impl Process { flags |= c::DETACHED_PROCESS | c::CREATE_NEW_PROCESS_GROUP; } - let (envp, _data) = make_envp(cfg.env.as_ref()); - let (dirp, _data) = make_dirp(cfg.cwd.as_ref()); + let (envp, _data) = try!(make_envp(cfg.env.as_ref())); + let (dirp, _data) = try!(make_dirp(cfg.cwd.as_ref())); let mut pi = zeroed_process_information(); try!(unsafe { // `CreateProcess` is racy! @@ -265,22 +283,24 @@ fn zeroed_process_information() -> c::PROCESS_INFORMATION { } } -// Produces a wide string *without terminating null* -fn make_command_line(prog: &OsStr, args: &[OsString]) -> Vec { +// Produces a wide string *without terminating null*; returns an error if +// `prog` or any of the `args` contain a nul. +fn make_command_line(prog: &OsStr, args: &[OsString]) -> io::Result> { // Encode the command and arguments in a command line string such // that the spawned process may recover them using CommandLineToArgvW. let mut cmd: Vec = Vec::new(); - append_arg(&mut cmd, prog); + try!(append_arg(&mut cmd, prog)); for arg in args { cmd.push(' ' as u16); - append_arg(&mut cmd, arg); + try!(append_arg(&mut cmd, arg)); } - return cmd; + return Ok(cmd); - fn append_arg(cmd: &mut Vec, arg: &OsStr) { + fn append_arg(cmd: &mut Vec, arg: &OsStr) -> io::Result<()> { // If an argument has 0 characters then we need to quote it to ensure // that it actually gets passed through on the command line or otherwise // it will be dropped entirely when parsed on the other end. + try!(ensure_no_nuls(arg)); let arg_bytes = &arg.as_inner().inner.as_inner(); let quote = arg_bytes.iter().any(|c| *c == b' ' || *c == b'\t') || arg_bytes.is_empty(); @@ -312,11 +332,12 @@ fn make_command_line(prog: &OsStr, args: &[OsString]) -> Vec { } cmd.push('"' as u16); } + Ok(()) } } fn make_envp(env: Option<&collections::HashMap>) - -> (*mut c_void, Vec) { + -> io::Result<(*mut c_void, Vec)> { // On Windows we pass an "environment block" which is not a char**, but // rather a concatenation of null-terminated k=v\0 sequences, with a final // \0 to terminate. @@ -325,26 +346,27 @@ fn make_envp(env: Option<&collections::HashMap>) let mut blk = Vec::new(); for pair in env { - blk.extend(pair.0.encode_wide()); + blk.extend(try!(ensure_no_nuls(pair.0)).encode_wide()); blk.push('=' as u16); - blk.extend(pair.1.encode_wide()); + blk.extend(try!(ensure_no_nuls(pair.1)).encode_wide()); blk.push(0); } blk.push(0); - (blk.as_mut_ptr() as *mut c_void, blk) + Ok((blk.as_mut_ptr() as *mut c_void, blk)) } - _ => (ptr::null_mut(), Vec::new()) + _ => Ok((ptr::null_mut(), Vec::new())) } } -fn make_dirp(d: Option<&OsString>) -> (*const u16, Vec) { +fn make_dirp(d: Option<&OsString>) -> io::Result<(*const u16, Vec)> { + match d { Some(dir) => { - let mut dir_str: Vec = dir.encode_wide().collect(); + let mut dir_str: Vec = try!(ensure_no_nuls(dir)).encode_wide().collect(); dir_str.push(0); - (dir_str.as_ptr(), dir_str) + Ok((dir_str.as_ptr(), dir_str)) }, - None => (ptr::null(), Vec::new()) + None => Ok((ptr::null(), Vec::new())) } } @@ -397,11 +419,12 @@ mod tests { #[test] fn test_make_command_line() { fn test_wrapper(prog: &str, args: &[&str]) -> String { - String::from_utf16( - &make_command_line(OsStr::new(prog), - &args.iter() - .map(|a| OsString::from(a)) - .collect::>())).unwrap() + let command_line = &make_command_line(OsStr::new(prog), + &args.iter() + .map(|a| OsString::from(a)) + .collect::>()) + .unwrap(); + String::from_utf16(command_line).unwrap() } assert_eq!(