Auto merge of #87704 - ChrisDenton:win-resolve-exe, r=yaahc

Windows: Resolve `process::Command` program without using the current directory

Currently `std::process::Command` searches many directories for the executable to run, including the current directory. This has lead to a [CVE for `ripgrep`](https://cve.circl.lu/cve/CVE-2021-3013) but presumably other command line utilities could be similarly vulnerable if they run commands. This was [discussed on the internals forum](https://internals.rust-lang.org/t/std-command-resolve-to-avoid-security-issues-on-windows/14800). Also discussed was [which directories should be searched](https://internals.rust-lang.org/t/windows-where-should-command-new-look-for-executables/15015).

EDIT: This PR originally removed all implicit paths. They've now been added back as laid out in the rest of this comment.

## Old Search Strategy

The old search strategy is [documented here][1]. Additionally Rust adds searching the child's paths (see also #37519). So the full list of paths that were searched was:

1. The directories that are listed in the child's `PATH` environment variable.
2. The directory from which the application loaded.
3. The current directory for the parent process.
4. The 32-bit Windows system directory.
5. The 16-bit Windows system directory.
6. The Windows directory.
7. The directories that are listed in the PATH environment variable.

## New Search Strategy

The new strategy removes the current directory from the searched paths.

1. The directories that are listed in the child's PATH environment variable.
2. The directory from which the application loaded.
3. The 32-bit Windows system directory.
4. The Windows directory.
5. The directories that are listed in the parent's PATH environment variable.

Note that it also removes the 16-bit system directory, mostly because there isn't a function to get it. I do not anticipate this being an issue in modern Windows.

## Impact

Removing the current directory should fix CVE's like the one linked above. However, it's possible some Windows users of affected Rust CLI applications have come to expect the old behaviour.

This change could also affect small Windows-only script-like programs that assumed the current directory would be used. The user would need to use `.\file.exe` instead of the bare application name.

This PR could break tests, especially those that test the exact output of error messages (e.g. Cargo) as this does change the error messages is some cases.

[1]: https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa#parameters

library/std/src/process/tests.rs

@@ -297,23 +297,23 @@ fn test_interior_nul_in_progname_is_error() {
 
 #[test]
 fn test_interior_nul_in_arg_is_error() {
-    match Command::new("echo").arg("has-some-\0\0s-inside").spawn() {
+    match Command::new("rustc").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() {
+    match Command::new("rustc").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() {
+    match Command::new("rustc").current_dir("has-some-\0\0s-inside").spawn() {
         Err(e) => assert_eq!(e.kind(), ErrorKind::InvalidInput),
         Ok(_) => panic!(),
     }

library/std/src/sys/windows/c.rs

@@ -734,6 +734,7 @@ if #[cfg(not(target_vendor = "uwp"))] {
             lpSecurityAttributes: LPSECURITY_ATTRIBUTES,
         ) -> BOOL;
         pub fn SetThreadStackGuarantee(_size: *mut c_ulong) -> BOOL;
+        pub fn GetWindowsDirectoryW(lpBuffer: LPWSTR, uSize: UINT) -> UINT;
     }
 }
 }
@@ -773,6 +774,7 @@ extern "system" {
     pub fn LeaveCriticalSection(CriticalSection: *mut CRITICAL_SECTION);
     pub fn DeleteCriticalSection(CriticalSection: *mut CRITICAL_SECTION);
 
+    pub fn GetSystemDirectoryW(lpBuffer: LPWSTR, uSize: UINT) -> UINT;
     pub fn RemoveDirectoryW(lpPathName: LPCWSTR) -> BOOL;
     pub fn SetFileAttributesW(lpFileName: LPCWSTR, dwFileAttributes: DWORD) -> BOOL;
     pub fn SetLastError(dwErrCode: DWORD);

library/std/src/sys/windows/path.rs

@@ -1,9 +1,8 @@
 use super::{c, fill_utf16_buf, to_u16s};
-use crate::ffi::OsStr;
+use crate::ffi::{OsStr, OsString};
 use crate::io;
 use crate::mem;
-use crate::path::Path;
-use crate::path::Prefix;
+use crate::path::{Path, PathBuf, Prefix};
 use crate::ptr;
 
 #[cfg(test)]
@@ -32,6 +31,25 @@ pub fn is_verbatim_sep(b: u8) -> bool {
     b == b'\\'
 }
 
+/// Returns true if `path` looks like a lone filename.
+pub(crate) fn is_file_name(path: &OsStr) -> bool {
+    !path.bytes().iter().copied().any(is_sep_byte)
+}
+pub(crate) fn has_trailing_slash(path: &OsStr) -> bool {
+    let is_verbatim = path.bytes().starts_with(br"\\?\");
+    let is_separator = if is_verbatim { is_verbatim_sep } else { is_sep_byte };
+    if let Some(&c) = path.bytes().last() { is_separator(c) } else { false }
+}
+
+/// Appends a suffix to a path.
+///
+/// Can be used to append an extension without removing an existing extension.
+pub(crate) fn append_suffix(path: PathBuf, suffix: &OsStr) -> PathBuf {
+    let mut path = OsString::from(path);
+    path.push(suffix);
+    path.into()
+}
+
 pub fn parse_prefix(path: &OsStr) -> Option<Prefix<'_>> {
     use Prefix::{DeviceNS, Disk, Verbatim, VerbatimDisk, VerbatimUNC, UNC};
 

library/std/src/sys/windows/process.rs

@@ -7,24 +7,24 @@ use crate::cmp;
 use crate::collections::BTreeMap;
 use crate::convert::{TryFrom, TryInto};
 use crate::env;
-use crate::env::split_paths;
+use crate::env::consts::{EXE_EXTENSION, EXE_SUFFIX};
 use crate::ffi::{OsStr, OsString};
 use crate::fmt;
-use crate::fs;
 use crate::io::{self, Error, ErrorKind};
 use crate::mem;
 use crate::num::NonZeroI32;
-use crate::os::windows::ffi::OsStrExt;
+use crate::os::windows::ffi::{OsStrExt, OsStringExt};
 use crate::os::windows::io::{AsRawHandle, FromRawHandle, IntoRawHandle};
-use crate::path::Path;
+use crate::path::{Path, PathBuf};
 use crate::ptr;
 use crate::sys::c;
 use crate::sys::c::NonZeroDWORD;
-use crate::sys::cvt;
 use crate::sys::fs::{File, OpenOptions};
 use crate::sys::handle::Handle;
+use crate::sys::path;
 use crate::sys::pipe::{self, AnonPipe};
 use crate::sys::stdio;
+use crate::sys::{cvt, to_u16s};
 use crate::sys_common::mutex::StaticMutex;
 use crate::sys_common::process::{CommandEnv, CommandEnvs};
 use crate::sys_common::{AsInner, IntoInner};
@@ -258,31 +258,19 @@ impl Command {
         needs_stdin: bool,
     ) -> io::Result<(Process, StdioPipes)> {
         let maybe_env = self.env.capture_if_changed();
-        // To have the spawning semantics of unix/windows stay the same, we need
-        // to read the *child's* PATH if one is provided. See #15149 for more
-        // details.
-        let program = maybe_env.as_ref().and_then(|env| {
-            if let Some(v) = env.get(&EnvKey::new("PATH")) {
-                // Split the value and test each path to see if the
-                // program exists.
-                for path in split_paths(&v) {
-                    let path = path
-                        .join(self.program.to_str().unwrap())
-                        .with_extension(env::consts::EXE_EXTENSION);
-                    if fs::metadata(&path).is_ok() {
-                        return Some(path.into_os_string());
-                    }
-                }
-            }
-            None
-        });
 
         let mut si = zeroed_startupinfo();
         si.cb = mem::size_of::<c::STARTUPINFO>() as c::DWORD;
         si.dwFlags = c::STARTF_USESTDHANDLES;
 
-        let program = program.as_ref().unwrap_or(&self.program);
-        let mut cmd_str = make_command_line(program, &self.args, self.force_quotes_enabled)?;
+        let child_paths = if let Some(env) = maybe_env.as_ref() {
+            env.get(&EnvKey::new("PATH")).map(|s| s.as_os_str())
+        } else {
+            None
+        };
+        let program = resolve_exe(&self.program, child_paths)?;
+        let mut cmd_str =
+            make_command_line(program.as_os_str(), &self.args, self.force_quotes_enabled)?;
         cmd_str.push(0); // add null terminator
 
         // stolen from the libuv code.
@@ -321,9 +309,10 @@ impl Command {
         si.hStdOutput = stdout.as_raw_handle();
         si.hStdError = stderr.as_raw_handle();
 
+        let program = to_u16s(&program)?;
         unsafe {
             cvt(c::CreateProcessW(
-                ptr::null(),
+                program.as_ptr(),
                 cmd_str.as_mut_ptr(),
                 ptr::null_mut(),
                 ptr::null_mut(),
@@ -361,6 +350,132 @@ impl fmt::Debug for Command {
     }
 }
 
+// Resolve `exe_path` to the executable name.
+//
+// * If the path is simply a file name then use the paths given by `search_paths` to find the executable.
+// * Otherwise use the `exe_path` as given.
+//
+// This function may also append `.exe` to the name. The rationale for doing so is as follows:
+//
+// It is a very strong convention that Windows executables have the `exe` extension.
+// In Rust, it is common to omit this extension.
+// Therefore this functions first assumes `.exe` was intended.
+// It falls back to the plain file name if a full path is given and the extension is omitted
+// or if only a file name is given and it already contains an extension.
+fn resolve_exe<'a>(exe_path: &'a OsStr, child_paths: Option<&OsStr>) -> io::Result<PathBuf> {
+    // Early return if there is no filename.
+    if exe_path.is_empty() || path::has_trailing_slash(exe_path) {
+        return Err(io::Error::new_const(
+            io::ErrorKind::InvalidInput,
+            &"program path has no file name",
+        ));
+    }
+    // Test if the file name has the `exe` extension.
+    // This does a case-insensitive `ends_with`.
+    let has_exe_suffix = if exe_path.len() >= EXE_SUFFIX.len() {
+        exe_path.bytes()[exe_path.len() - EXE_SUFFIX.len()..]
+            .eq_ignore_ascii_case(EXE_SUFFIX.as_bytes())
+    } else {
+        false
+    };
+
+    // If `exe_path` is an absolute path or a sub-path then don't search `PATH` for it.
+    if !path::is_file_name(exe_path) {
+        if has_exe_suffix {
+            // The application name is a path to a `.exe` file.
+            // Let `CreateProcessW` figure out if it exists or not.
+            return Ok(exe_path.into());
+        }
+        let mut path = PathBuf::from(exe_path);
+
+        // Append `.exe` if not already there.
+        path = path::append_suffix(path, EXE_SUFFIX.as_ref());
+        if path.try_exists().unwrap_or(false) {
+            return Ok(path);
+        } else {
+            // It's ok to use `set_extension` here because the intent is to
+            // remove the extension that was just added.
+            path.set_extension("");
+            return Ok(path);
+        }
+    } else {
+        ensure_no_nuls(exe_path)?;
+        // From the `CreateProcessW` docs:
+        // > If the file name does not contain an extension, .exe is appended.
+        // Note that this rule only applies when searching paths.
+        let has_extension = exe_path.bytes().contains(&b'.');
+
+        // Search the directories given by `search_paths`.
+        let result = search_paths(child_paths, |mut path| {
+            path.push(&exe_path);
+            if !has_extension {
+                path.set_extension(EXE_EXTENSION);
+            }
+            if let Ok(true) = path.try_exists() { Some(path) } else { None }
+        });
+        if let Some(path) = result {
+            return Ok(path);
+        }
+    }
+    // If we get here then the executable cannot be found.
+    Err(io::Error::new_const(io::ErrorKind::NotFound, &"program not found"))
+}
+
+// Calls `f` for every path that should be used to find an executable.
+// Returns once `f` returns the path to an executable or all paths have been searched.
+fn search_paths<F>(child_paths: Option<&OsStr>, mut f: F) -> Option<PathBuf>
+where
+    F: FnMut(PathBuf) -> Option<PathBuf>,
+{
+    // 1. Child paths
+    // This is for consistency with Rust's historic behaviour.
+    if let Some(paths) = child_paths {
+        for path in env::split_paths(paths).filter(|p| !p.as_os_str().is_empty()) {
+            if let Some(path) = f(path) {
+                return Some(path);
+            }
+        }
+    }
+
+    // 2. Application path
+    if let Ok(mut app_path) = env::current_exe() {
+        app_path.pop();
+        if let Some(path) = f(app_path) {
+            return Some(path);
+        }
+    }
+
+    // 3 & 4. System paths
+    // SAFETY: This uses `fill_utf16_buf` to safely call the OS functions.
+    unsafe {
+        if let Ok(Some(path)) = super::fill_utf16_buf(
+            |buf, size| c::GetSystemDirectoryW(buf, size),
+            |buf| f(PathBuf::from(OsString::from_wide(buf))),
+        ) {
+            return Some(path);
+        }
+        #[cfg(not(target_vendor = "uwp"))]
+        {
+            if let Ok(Some(path)) = super::fill_utf16_buf(
+                |buf, size| c::GetWindowsDirectoryW(buf, size),
+                |buf| f(PathBuf::from(OsString::from_wide(buf))),
+            ) {
+                return Some(path);
+            }
+        }
+    }
+
+    // 5. Parent paths
+    if let Some(parent_paths) = env::var_os("PATH") {
+        for path in env::split_paths(&parent_paths).filter(|p| !p.as_os_str().is_empty()) {
+            if let Some(path) = f(path) {
+                return Some(path);
+            }
+        }
+    }
+    None
+}
+
 impl Stdio {
     fn to_handle(&self, stdio_id: c::DWORD, pipe: &mut Option<AnonPipe>) -> io::Result<Handle> {
         match *self {

library/std/src/sys/windows/process/tests.rs

@@ -128,3 +128,55 @@ fn windows_env_unicode_case() {
         }
     }
 }
+
+// UWP applications run in a restricted environment which means this test may not work.
+#[cfg(not(target_vendor = "uwp"))]
+#[test]
+fn windows_exe_resolver() {
+    use super::resolve_exe;
+    use crate::io;
+
+    // Test a full path, with and without the `exe` extension.
+    let mut current_exe = env::current_exe().unwrap();
+    assert!(resolve_exe(current_exe.as_ref(), None).is_ok());
+    current_exe.set_extension("");
+    assert!(resolve_exe(current_exe.as_ref(), None).is_ok());
+
+    // Test lone file names.
+    assert!(resolve_exe(OsStr::new("cmd"), None).is_ok());
+    assert!(resolve_exe(OsStr::new("cmd.exe"), None).is_ok());
+    assert!(resolve_exe(OsStr::new("cmd.EXE"), None).is_ok());
+    assert!(resolve_exe(OsStr::new("fc"), None).is_ok());
+
+    // Invalid file names should return InvalidInput.
+    assert_eq!(resolve_exe(OsStr::new(""), None).unwrap_err().kind(), io::ErrorKind::InvalidInput);
+    assert_eq!(
+        resolve_exe(OsStr::new("\0"), None).unwrap_err().kind(),
+        io::ErrorKind::InvalidInput
+    );
+    // Trailing slash, therefore there's no file name component.
+    assert_eq!(
+        resolve_exe(OsStr::new(r"C:\Path\to\"), None).unwrap_err().kind(),
+        io::ErrorKind::InvalidInput
+    );
+
+    /*
+    Some of the following tests may need to be changed if you are deliberately
+    changing the behaviour of `resolve_exe`.
+    */
+
+    let paths = env::var_os("PATH").unwrap();
+    env::set_var("PATH", "");
+
+    assert_eq!(resolve_exe(OsStr::new("rustc"), None).unwrap_err().kind(), io::ErrorKind::NotFound);
+
+    let child_paths = Some(paths.as_os_str());
+    assert!(resolve_exe(OsStr::new("rustc"), child_paths).is_ok());
+
+    // The resolver looks in system directories even when `PATH` is empty.
+    assert!(resolve_exe(OsStr::new("cmd.exe"), None).is_ok());
+
+    // The application's directory is also searched.
+    let current_exe = env::current_exe().unwrap();
+    assert!(resolve_exe(current_exe.file_name().unwrap().as_ref(), None).is_ok());
+}