cli: fix arbitrary execution of program bug

This fixes a bug only present on Windows that would permit someone to
execute an arbitrary program if they crafted an appropriate directory
tree. Namely, if someone put an executable named 'xz.exe' in the root of
a directory tree and one ran 'rg -z foo' from the root of that tree,
then the 'xz.exe' executable in that tree would execute if there are any
'xz' files anywhere in the tree.

The root cause of this problem is that 'CreateProcess' on Windows will
implicitly look in the current working directory for an executable when
it is given a relative path to a program. Rust's standard library allows
this behavior to occur, so we work around it here. We work around it by
explicitly resolving programs like 'xz' via 'PATH'. That way, we only
ever pass an absolute path to 'CreateProcess', which avoids the implicit
behavior of checking the current working directory.

This fix doesn't apply to non-Windows systems as it is believed to only
impact Windows. In theory, the bug could apply on Unix if '.' is in
one's PATH, but at that point, you reap what you sow.

While the extent to which this is a security problem isn't clear, I
think users generally expect to be able to download or clone
repositories from the Internet and run ripgrep on them without fear of
anything too awful happening. Being able to execute an arbitrary program
probably violates that expectation. Therefore, CVE-2021-3013[1] was
created for this issue.

We apply the same logic to the --pre command, since the --pre command is
likely in a user's config file and it would be surprising for something
that the user is searching to modify which preprocessor command is used.

The --pre and -z/--search-zip flags are the only two ways that ripgrep
will invoke external programs, so this should cover any possible
exploitable cases of this bug.

[1] - https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-3013

CHANGELOG.md

@@ -15,6 +15,13 @@ Now it looks like this:
 FOO: binary file matches (found "\0" byte around offset XXX)
 ```
 
+Security fixes:
+
+* [CVE-2021-3013](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-3013):
+  Fixes a security hole on Windows where running ripgrep with either the
+  `-z/--search-zip` or `--pre` flags can result in running arbitrary
+  executables from the current directory.
+
 Feature enhancements:
 
 * Added or improved file type filtering for Bazel, dvc, FlatBuffers, Futhark,

crates/cli/src/decompress.rs

@@ -1,7 +1,7 @@
 use std::ffi::{OsStr, OsString};
 use std::fs::File;
 use std::io;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::process::Command;
 
 use globset::{Glob, GlobSet, GlobSetBuilder};
@@ -24,7 +24,7 @@ struct DecompressionCommand {
     /// The glob that matches this command.
     glob: String,
     /// The command or binary name.
-    bin: OsString,
+    bin: PathBuf,
     /// The arguments to invoke with the command.
     args: Vec<OsString>,
 }
@@ -83,23 +83,60 @@ impl DecompressionMatcherBuilder {
     ///
     /// The syntax for the glob is documented in the
     /// [`globset` crate](https://docs.rs/globset/#syntax).
+    ///
+    /// The `program` given is resolved with respect to `PATH` and turned
+    /// into an absolute path internally before being executed by the current
+    /// platform. Notably, on Windows, this avoids a security problem where
+    /// passing a relative path to `CreateProcess` will automatically search
+    /// the current directory for a matching program. If the program could
+    /// not be resolved, then it is silently ignored and the association is
+    /// dropped. For this reason, callers should prefer `try_associate`.
     pub fn associate<P, I, A>(
         &mut self,
         glob: &str,
         program: P,
         args: I,
     ) -> &mut DecompressionMatcherBuilder
+    where
+        P: AsRef<OsStr>,
+        I: IntoIterator<Item = A>,
+        A: AsRef<OsStr>,
+    {
+        let _ = self.try_associate(glob, program, args);
+        self
+    }
+
+    /// Associates a glob with a command to decompress files matching the glob.
+    ///
+    /// If multiple globs match the same file, then the most recently added
+    /// glob takes precedence.
+    ///
+    /// The syntax for the glob is documented in the
+    /// [`globset` crate](https://docs.rs/globset/#syntax).
+    ///
+    /// The `program` given is resolved with respect to `PATH` and turned
+    /// into an absolute path internally before being executed by the current
+    /// platform. Notably, on Windows, this avoids a security problem where
+    /// passing a relative path to `CreateProcess` will automatically search
+    /// the current directory for a matching program. If the program could not
+    /// be resolved, then an error is returned.
+    pub fn try_associate<P, I, A>(
+        &mut self,
+        glob: &str,
+        program: P,
+        args: I,
+    ) -> Result<&mut DecompressionMatcherBuilder, CommandError>
     where
         P: AsRef<OsStr>,
         I: IntoIterator<Item = A>,
         A: AsRef<OsStr>,
     {
         let glob = glob.to_string();
-        let bin = program.as_ref().to_os_string();
+        let bin = resolve_binary(Path::new(program.as_ref()))?;
         let args =
             args.into_iter().map(|a| a.as_ref().to_os_string()).collect();
         self.commands.push(DecompressionCommand { glob, bin, args });
-        self
+        Ok(self)
     }
 }
 
@@ -340,6 +377,70 @@ impl io::Read for DecompressionReader {
     }
 }
 
+/// Resolves a path to a program to a path by searching for the program in
+/// `PATH`.
+///
+/// If the program could not be resolved, then an error is returned.
+///
+/// The purpose of doing this instead of passing the path to the program
+/// directly to Command::new is that Command::new will hand relative paths
+/// to CreateProcess on Windows, which will implicitly search the current
+/// working directory for the executable. This could be undesirable for
+/// security reasons. e.g., running ripgrep with the -z/--search-zip flag on an
+/// untrusted directory tree could result in arbitrary programs executing on
+/// Windows.
+///
+/// Note that this could still return a relative path if PATH contains a
+/// relative path. We permit this since it is assumed that the user has set
+/// this explicitly, and thus, desires this behavior.
+///
+/// On non-Windows, this is a no-op.
+pub fn resolve_binary<P: AsRef<Path>>(
+    prog: P,
+) -> Result<PathBuf, CommandError> {
+    use std::env;
+
+    fn is_exe(path: &Path) -> bool {
+        let md = match path.metadata() {
+            Err(_) => return false,
+            Ok(md) => md,
+        };
+        !md.is_dir()
+    }
+
+    let prog = prog.as_ref();
+    if !cfg!(windows) || prog.is_absolute() {
+        return Ok(prog.to_path_buf());
+    }
+    let syspaths = match env::var_os("PATH") {
+        Some(syspaths) => syspaths,
+        None => {
+            let msg = "system PATH environment variable not found";
+            return Err(CommandError::io(io::Error::new(
+                io::ErrorKind::Other,
+                msg,
+            )));
+        }
+    };
+    for syspath in env::split_paths(&syspaths) {
+        if syspath.as_os_str().is_empty() {
+            continue;
+        }
+        let abs_prog = syspath.join(prog);
+        if is_exe(&abs_prog) {
+            return Ok(abs_prog.to_path_buf());
+        }
+        if abs_prog.extension().is_none() {
+            let abs_prog = abs_prog.with_extension("exe");
+            if is_exe(&abs_prog) {
+                return Ok(abs_prog.to_path_buf());
+            }
+        }
+    }
+    let msg = format!("{}: could not find executable in PATH", prog.display());
+    return Err(CommandError::io(io::Error::new(io::ErrorKind::Other, msg)));
+}
+
 fn default_decompression_commands() -> Vec<DecompressionCommand> {
     const ARGS_GZIP: &[&str] = &["gzip", "-d", "-c"];
     const ARGS_BZIP: &[&str] = &["bzip2", "-d", "-c"];
@@ -350,29 +451,36 @@ fn default_decompression_commands() -> Vec<DecompressionCommand> {
     const ARGS_ZSTD: &[&str] = &["zstd", "-q", "-d", "-c"];
     const ARGS_UNCOMPRESS: &[&str] = &["uncompress", "-c"];
 
-    fn cmd(glob: &str, args: &[&str]) -> DecompressionCommand {
-        DecompressionCommand {
+    fn add(glob: &str, args: &[&str], cmds: &mut Vec<DecompressionCommand>) {
+        let bin = match resolve_binary(Path::new(args[0])) {
+            Ok(bin) => bin,
+            Err(err) => {
+                debug!("{}", err);
+                return;
+            }
+        };
+        cmds.push(DecompressionCommand {
             glob: glob.to_string(),
-            bin: OsStr::new(&args[0]).to_os_string(),
+            bin,
             args: args
                 .iter()
                 .skip(1)
                 .map(|s| OsStr::new(s).to_os_string())
                 .collect(),
-        }
+        });
     }
-    vec![
-        cmd("*.gz", ARGS_GZIP),
-        cmd("*.tgz", ARGS_GZIP),
-        cmd("*.bz2", ARGS_BZIP),
-        cmd("*.tbz2", ARGS_BZIP),
-        cmd("*.xz", ARGS_XZ),
-        cmd("*.txz", ARGS_XZ),
-        cmd("*.lz4", ARGS_LZ4),
-        cmd("*.lzma", ARGS_LZMA),
-        cmd("*.br", ARGS_BROTLI),
-        cmd("*.zst", ARGS_ZSTD),
-        cmd("*.zstd", ARGS_ZSTD),
-        cmd("*.Z", ARGS_UNCOMPRESS),
-    ]
+    let mut cmds = vec![];
+    add("*.gz", ARGS_GZIP, &mut cmds);
+    add("*.tgz", ARGS_GZIP, &mut cmds);
+    add("*.bz2", ARGS_BZIP, &mut cmds);
+    add("*.tbz2", ARGS_BZIP, &mut cmds);
+    add("*.xz", ARGS_XZ, &mut cmds);
+    add("*.txz", ARGS_XZ, &mut cmds);
+    add("*.lz4", ARGS_LZ4, &mut cmds);
+    add("*.lzma", ARGS_LZMA, &mut cmds);
+    add("*.br", ARGS_BROTLI, &mut cmds);
+    add("*.zst", ARGS_ZSTD, &mut cmds);
+    add("*.zstd", ARGS_ZSTD, &mut cmds);
+    add("*.Z", ARGS_UNCOMPRESS, &mut cmds);
+    cmds
 }

crates/cli/src/lib.rs

@@ -179,8 +179,8 @@ mod process;
 mod wtr;
 
 pub use decompress::{
-    DecompressionMatcher, DecompressionMatcherBuilder, DecompressionReader,
-    DecompressionReaderBuilder,
+    resolve_binary, DecompressionMatcher, DecompressionMatcherBuilder,
+    DecompressionReader, DecompressionReaderBuilder,
 };
 pub use escape::{escape, escape_os, unescape, unescape_os};
 pub use human::{parse_human_readable_size, ParseSizeError};

crates/core/args.rs

@@ -290,7 +290,7 @@ impl Args {
         let mut builder = SearchWorkerBuilder::new();
         builder
             .json_stats(matches.is_present("json"))
-            .preprocessor(matches.preprocessor())
+            .preprocessor(matches.preprocessor())?
             .preprocessor_globs(matches.preprocessor_globs()?)
             .search_zip(matches.is_present("search-zip"))
             .binary_detection_implicit(matches.binary_detection_implicit())

crates/core/search.rs

@@ -115,9 +115,14 @@ impl SearchWorkerBuilder {
     pub fn preprocessor(
         &mut self,
         cmd: Option<PathBuf>,
-    ) -> &mut SearchWorkerBuilder {
-        self.config.preprocessor = cmd;
-        self
+    ) -> crate::Result<&mut SearchWorkerBuilder> {
+        if let Some(ref prog) = cmd {
+            let bin = cli::resolve_binary(prog)?;
+            self.config.preprocessor = Some(bin);
+        } else {
+            self.config.preprocessor = None;
+        }
+        Ok(self)
     }
 
     /// Set the globs for determining which files should be run through the