CVE-2021-36376: Uncontrolled Search Path Element (CWE-427)
value: dandavison delta before 0.8.3 on Windows resolves an executable's pathname as a relative path from the current directory.
CWE description: The product uses a fixed or controlled search path to find resources, but one or more locations in that path can be under the control of unintended actors.
CVSS v3 base score: 7.8
Published: 2021-07-13T13:15Z
Repository: https://github.com/dandavison/delta
Language: Rust
Files changed: Cargo.lock, Cargo.toml, src/bat_utils/less.rs, src/bat_utils/output.rs, src/main.rs
Commit date: 2021-07-10 15:52:06 +0100
Commit message: Do not resolve executables as relative path from current directory (#658)

* Refactor: child pager process creation

* Protect calls to Command::new behind grep_cli::resolve_binary

* Move less env-var setting into less-specific branch
Diff: diff --git a/Cargo.lock b/Cargo.lock
index d29765c..001e858 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -93,10 +93,21 @@ checksum = "cf1de2fe8c75bc145a2f577add951f8134889b4795d47466a54a5c846d691693"
 name = "box_drawing"
 version = "0.1.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "ea27d8d5fd867b17523bf6788b1175fa9867f34669d057e9adaf76e27bcea44b"
 
+[[package]]
+name = "bstr"
+version = "0.2.15"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "a40b47ad93e1a5404e6c18dec46b628214fee441c70f4ab5d6942142cc268a3d"
+dependencies = [
+ "lazy_static",
+ "memchr",
+ "regex-automata",
+]
+
 [[package]]
 name = "bytelines"
 version = "2.2.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "791e4e40d13e1463dee537b254225c12c46ec7328f1817c6264873bc166f615f"
@@ -270,10 +281,11 @@ dependencies = [
  "bytelines",
  "console",
  "dirs-next",
  "error-chain",
  "git2",
+ "grep-cli",
  "itertools",
  "lazy_static",
  "pathdiff",
  "regex",
  "shell-words",
@@ -296,10 +308,40 @@ dependencies = [
  "libgit2-sys",
  "log",
  "url",
 ]
 
+[[package]]
+name = "globset"
+version = "0.4.8"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "10463d9ff00a2a068db14231982f5132edebad0d7660cd956a1c30292dbcbfbd"
+dependencies = [
+ "aho-corasick",
+ "bstr",
+ "fnv",
+ "log",
+ "regex",
+]
+
+[[package]]
+name = "grep-cli"
+version = "0.1.6"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "2dd110c34bb4460d0de5062413b773e385cbf8a85a63fc535590110a09e79e8a"
+dependencies = [
+ "atty",
+ "bstr",
+ "globset",
+ "lazy_static",
+ "log",
+ "regex",
+ "same-file",
+ "termcolor",
+ "winapi-util",
+]
+
 [[package]]
 name = "hashbrown"
 version = "0.8.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "e91b62f79061a0bc2e046024cb7ba44b08419ed238ecbd9adbd787434b9e8c25"
@@ -598,10 +640,16 @@ dependencies = [
  "aho-corasick",
  "memchr",
  "regex-syntax",
 ]
 
+[[package]]
+name = "regex-automata"
+version = "0.1.10"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "6c230d73fb8d8c1b9c0b3135c5142a8acee3a0558fb8db5cf1cb65f8d7862132"
+
 [[package]]
 name = "regex-syntax"
 version = "0.6.22"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "b5eb417147ba9860a96cfe72a0b93bf88fee1744b5636ec99ab20c1aa9376581"
@@ -722,10 +770,19 @@ dependencies = [
  "serde_json",
  "walkdir",
  "yaml-rust",
 ]
 
+[[package]]
+name = "termcolor"
+version = "1.1.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "2dfed899f0eb03f32ee8c6a0aabdb8a7949659e3466561fc0adf54e26d88c5f4"
+dependencies = [
+ "winapi-util",
+]
+
 [[package]]
 name = "terminal_size"
 version = "0.1.15"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "4bd2d183bd3fac5f5fe38ddbeb4dc9aec4a39a9d7d59e7491d900302da01cbe1"
diff --git a/Cargo.toml b/Cargo.toml
index 99a5d6e..e6fd0c9 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -21,10 +21,11 @@ atty = "0.2.14"
 bitflags = "1.2.1"
 box_drawing = "0.1.2"
 bytelines = "2.2.2"
 console = "0.14.1"
 dirs-next = "2.0.0"
+grep-cli = "0.1.6"
 itertools = "0.10.1"
 lazy_static = "1.4"
 pathdiff = "0.2.0"
 regex = "1.4.6"
 shell-words = "1.0.0"
diff --git a/src/bat_utils/less.rs b/src/bat_utils/less.rs
index 79ecd43..1ca9f76 100644
--- a/src/bat_utils/less.rs
+++ b/src/bat_utils/less.rs
@@ -1,10 +1,14 @@
 use std::process::Command;
 
 pub fn retrieve_less_version() -> Option<usize> {
-    let cmd = Command::new("less").arg("--version").output().ok()?;
-    parse_less_version(&cmd.stdout)
+    if let Ok(less_path) = grep_cli::resolve_binary("less") {
+        let cmd = Command::new(less_path).arg("--version").output().ok()?;
+        parse_less_version(&cmd.stdout)
+    } else {
+        None
+    }
 }
 
 fn parse_less_version(output: &[u8]) -> Option<usize> {
     if output.starts_with(b"less ") {
         let version = std::str::from_utf8(&output[5..]).ok()?;
diff --git a/src/bat_utils/output.rs b/src/bat_utils/output.rs
index 1166b4e..5181978 100644
--- a/src/bat_utils/output.rs
+++ b/src/bat_utils/output.rs
@@ -75,82 +75,39 @@ impl OutputType {
             .unwrap_or_else(|| String::from("less"));
 
         let pagerflags =
             shell_words::split(&pager).chain_err(|| "Could not parse pager command.")?;
 
-        match pagerflags.split_first() {
+        Ok(match pagerflags.split_first() {
             Some((pager_name, args)) => {
                 let pager_path = PathBuf::from(pager_name);
 
                 let is_less = pager_path.file_stem() == Some(&OsString::from("less"));
 
-                let mut process = if is_less {
-                    let mut p = Command::new(&pager_path);
-                    if args.is_empty() || replace_arguments_to_less {
-                        p.args(vec!["--RAW-CONTROL-CHARS"]);
-
-                        // Passing '--no-init' fixes a bug with '--quit-if-one-screen' in older
-                        // versions of 'less'. Unfortunately, it also breaks mouse-wheel support.
-                        //
-                        // See: http://www.greenwoodsoftware.com/less/news.530.html
-                        //
-                        // For newer versions (530 or 558 on Windows), we omit '--no-init' as it
-                        // is not needed anymore.
-                        match retrieve_less_version() {
-                            None => {
-                                p.arg("--no-init");
-                            }
-                            Some(version)
-                                if (version < 530 || (cfg!(windows) && version < 558)) =>
-                            {
-                                p.arg("--no-init");
-                            }
-                            _ => {}
-                        }
-
-                        if quit_if_one_screen {
-                            p.arg("--quit-if-one-screen");
-                        }
-                    } else {
-                        p.args(args);
-                    }
-                    p.env("LESSCHARSET", "UTF-8");
-                    p
+                let process = if is_less {
+                    _make_process_from_less_path(
+                        pager_path,
+                        args,
+                        replace_arguments_to_less,
+                        quit_if_one_screen,
+                        config,
+                    )
                 } else {
-                    if pager_path.file_stem() == Some(&OsString::from("delta")) {
-                        eprintln!(
-                            "\
-It looks like you have set delta as the value of $PAGER. \
-This would result in a non-terminating recursion. \
-delta is not an appropriate value for $PAGER \
-(but it is an appropriate value for $GIT_PAGER)."
-                        );
-                        std::process::exit(1);
-                    }
-                    let mut p = Command::new(&pager_path);
-                    p.args(args);
-                    p
+                    _make_process_from_pager_path(pager_path, args)
                 };
-                if is_less && config.navigate {
-                    if let Ok(hist_file) =
-                        navigate::copy_less_hist_file_and_append_navigate_regexp(config)
-                    {
-                        process.env("LESSHISTFILE", hist_file);
-                        if config.show_themes {
-                            process.arg("+n");
-                        }
-                    }
+                if let Some(mut process) = process {
+                    process
+                        .stdin(Stdio::piped())
+                        .spawn()
+                        .map(OutputType::Pager)
+                        .unwrap_or_else(|_| OutputType::stdout())
+                } else {
+                    OutputType::stdout()
                 }
-                Ok(process
-                    .env("LESSANSIENDCHARS", "mK")
-                    .stdin(Stdio::piped())
-                    .spawn()
-                    .map(OutputType::Pager)
-                    .unwrap_or_else(|_| OutputType::stdout()))
             }
-            None => Ok(OutputType::stdout()),
-        }
+            None => OutputType::stdout(),
+        })
     }
 
     fn stdout() -> Self {
         OutputType::Stdout(io::stdout())
     }
@@ -164,10 +121,82 @@ delta is not an appropriate value for $PAGER \
             OutputType::Stdout(ref mut handle) => handle,
         })
     }
 }
 
+fn _make_process_from_less_path(
+    less_path: PathBuf,
+    args: &[String],
+    replace_arguments_to_less: bool,
+    quit_if_one_screen: bool,
+    config: &config::Config,
+) -> Option<Command> {
+    if let Ok(less_path) = grep_cli::resolve_binary(less_path) {
+        let mut p = Command::new(&less_path);
+        if args.is_empty() || replace_arguments_to_less {
+            p.args(vec!["--RAW-CONTROL-CHARS"]);
+
+            // Passing '--no-init' fixes a bug with '--quit-if-one-screen' in older
+            // versions of 'less'. Unfortunately, it also breaks mouse-wheel support.
+            //
+            // See: http://www.greenwoodsoftware.com/less/news.530.html
+            //
+            // For newer versions (530 or 558 on Windows), we omit '--no-init' as it
+            // is not needed anymore.
+            match retrieve_less_version() {
+                None => {
+                    p.arg("--no-init");
+                }
+                Some(version) if (version < 530 || (cfg!(windows) && version < 558)) => {
+                    p.arg("--no-init");
+                }
+                _ => {}
+            }
+
+            if quit_if_one_screen {
+                p.arg("--quit-if-one-screen");
+            }
+        } else {
+            p.args(args);
+        }
+        p.env("LESSCHARSET", "UTF-8");
+        p.env("LESSANSIENDCHARS", "mK");
+        if config.navigate {
+            if let Ok(hist_file) = navigate::copy_less_hist_file_and_append_navigate_regexp(config)
+            {
+                p.env("LESSHISTFILE", hist_file);
+                if config.show_themes {
+                    p.arg("+n");
+                }
+            }
+        }
+        Some(p)
+    } else {
+        None
+    }
+}
+
+fn _make_process_from_pager_path(pager_path: PathBuf, args: &[String]) -> Option<Command> {
+    if pager_path.file_stem() == Some(&OsString::from("delta")) {
+        eprintln!(
+            "\
+It looks like you have set delta as the value of $PAGER. \
+This would result in a non-terminating recursion. \
+delta is not an appropriate value for $PAGER \
+(but it is an appropriate value for $GIT_PAGER)."
+        );
+        std::process::exit(1);
+    }
+    if let Ok(pager_path) = grep_cli::resolve_binary(pager_path) {
+        let mut p = Command::new(&pager_path);
+        p.args(args);
+        Some(p)
+    } else {
+        None
+    }
+}
+
 impl Drop for OutputType {
     fn drop(&mut self) {
         if let OutputType::Pager(ref mut command) = *self {
             let _ = command.wait();
         }
diff --git a/src/main.rs b/src/main.rs
index 94b4c74..c388f49 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -130,11 +130,15 @@ You can also use delta to diff two files: `delta file_A file_B`."
         process::exit(config.error_exit_code);
     };
     let diff_command = "git";
     let minus_file = minus_file.unwrap_or_else(die);
     let plus_file = plus_file.unwrap_or_else(die);
-    let mut diff_process = process::Command::new(PathBuf::from(diff_command))
+    let diff_command_path = match grep_cli::resolve_binary(PathBuf::from(diff_command)) {
+        Ok(path) => path,
+        Err(_) => return config.error_exit_code,
+    };
+    let mut diff_process = process::Command::new(diff_command_path)
         .args(&["diff", "--no-index"])
         .args(&[minus_file, plus_file])
         .stdout(process::Stdio::piped())
         .spawn()
         .unwrap_or_else(|err| {

Vulnerable code:     let cmd = Command::new("less").arg("--version").output().ok()?;
    parse_less_version(&cmd.stdout)
        match pagerflags.split_first() {
                let mut process = if is_less {
                    let mut p = Command::new(&pager_path);
                    if args.is_empty() || replace_arguments_to_less {
                        p.args(vec!["--RAW-CONTROL-CHARS"]);

                        // Passing '--no-init' fixes a bug with '--quit-if-one-screen' in older
                        // versions of 'less'. Unfortunately, it also breaks mouse-wheel support.
                        //
                        // See: http://www.greenwoodsoftware.com/less/news.530.html
                        //
                        // For newer versions (530 or 558 on Windows), we omit '--no-init' as it
                        // is not needed anymore.
                        match retrieve_less_version() {
                            None => {
                                p.arg("--no-init");
                            }
                            Some(version)
                                if (version < 530 || (cfg!(windows) && version < 558)) =>
                            {
                                p.arg("--no-init");
                            }
                            _ => {}
                        }

                        if quit_if_one_screen {
                            p.arg("--quit-if-one-screen");
                        }
                    } else {
                        p.args(args);
                    }
                    p.env("LESSCHARSET", "UTF-8");
                    p
                    if pager_path.file_stem() == Some(&OsString::from("delta")) {
                        eprintln!(
                            "\
It looks like you have set delta as the value of $PAGER. \
This would result in a non-terminating recursion. \
delta is not an appropriate value for $PAGER \
(but it is an appropriate value for $GIT_PAGER)."
                        );
                        std::process::exit(1);
                    }
                    let mut p = Command::new(&pager_path);
                    p.args(args);
                    p
                if is_less && config.navigate {
                    if let Ok(hist_file) =
                        navigate::copy_less_hist_file_and_append_navigate_regexp(config)
                    {
                        process.env("LESSHISTFILE", hist_file);
                        if config.show_themes {
                            process.arg("+n");
                        }
                    }
                Ok(process
                    .env("LESSANSIENDCHARS", "mK")
                    .stdin(Stdio::piped())
                    .spawn()
                    .map(OutputType::Pager)
                    .unwrap_or_else(|_| OutputType::stdout()))
            None => Ok(OutputType::stdout()),
        }
    let mut diff_process = process::Command::new(PathBuf::from(diff_command))
Fixed code: [[package]]
name = "bstr"
version = "0.2.15"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "a40b47ad93e1a5404e6c18dec46b628214fee441c70f4ab5d6942142cc268a3d"
dependencies = [
 "lazy_static",
 "memchr",
 "regex-automata",
]

 "grep-cli",
[[package]]
name = "globset"
version = "0.4.8"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "10463d9ff00a2a068db14231982f5132edebad0d7660cd956a1c30292dbcbfbd"
dependencies = [
 "aho-corasick",
 "bstr",
 "fnv",
 "log",
 "regex",
]

[[package]]
name = "grep-cli"
version = "0.1.6"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "2dd110c34bb4460d0de5062413b773e385cbf8a85a63fc535590110a09e79e8a"
dependencies = [
 "atty",
 "bstr",
 "globset",
 "lazy_static",
 "log",
 "regex",
 "same-file",
 "termcolor",
 "winapi-util",
]

[[package]]
name = "regex-automata"
version = "0.1.10"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "6c230d73fb8d8c1b9c0b3135c5142a8acee3a0558fb8db5cf1cb65f8d7862132"

[[package]]
name = "termcolor"
version = "1.1.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "2dfed899f0eb03f32ee8c6a0aabdb8a7949659e3466561fc0adf54e26d88c5f4"
dependencies = [
 "winapi-util",
]

grep-cli = "0.1.6"
    if let Ok(less_path) = grep_cli::resolve_binary("less") {
        let cmd = Command::new(less_path).arg("--version").output().ok()?;
        parse_less_version(&cmd.stdout)
    } else {
        None
    }
        Ok(match pagerflags.split_first() {
                let process = if is_less {
                    _make_process_from_less_path(
                        pager_path,
                        args,
                        replace_arguments_to_less,
                        quit_if_one_screen,
                        config,
                    )
                    _make_process_from_pager_path(pager_path, args)
                if let Some(mut process) = process {
                    process
                        .stdin(Stdio::piped())
                        .spawn()
                        .map(OutputType::Pager)
                        .unwrap_or_else(|_| OutputType::stdout())
                } else {
                    OutputType::stdout()
            None => OutputType::stdout(),
        })
fn _make_process_from_less_path(
    less_path: PathBuf,
    args: &[String],
    replace_arguments_to_less: bool,
    quit_if_one_screen: bool,
    config: &config::Config,
) -> Option<Command> {
    if let Ok(less_path) = grep_cli::resolve_binary(less_path) {
        let mut p = Command::new(&less_path);
        if args.is_empty() || replace_arguments_to_less {
            p.args(vec!["--RAW-CONTROL-CHARS"]);

            // Passing '--no-init' fixes a bug with '--quit-if-one-screen' in older
            // versions of 'less'. Unfortunately, it also breaks mouse-wheel support.
            //
            // See: http://www.greenwoodsoftware.com/less/news.530.html
            //
            // For newer versions (530 or 558 on Windows), we omit '--no-init' as it
            // is not needed anymore.
            match retrieve_less_version() {
                None => {
                    p.arg("--no-init");
                }
                Some(version) if (version < 530 || (cfg!(windows) && version < 558)) => {
                    p.arg("--no-init");
                }
                _ => {}
            }

            if quit_if_one_screen {
                p.arg("--quit-if-one-screen");
            }
        } else {
            p.args(args);
        }
        p.env("LESSCHARSET", "UTF-8");
        p.env("LESSANSIENDCHARS", "mK");
        if config.navigate {
            if let Ok(hist_file) = navigate::copy_less_hist_file_and_append_navigate_regexp(config)
            {
                p.env("LESSHISTFILE", hist_file);
                if config.show_themes {
                    p.arg("+n");
                }
            }
        }
        Some(p)
    } else {
        None
    }
}

fn _make_process_from_pager_path(pager_path: PathBuf, args: &[String]) -> Option<Command> {
    if pager_path.file_stem() == Some(&OsString::from("delta")) {
        eprintln!(
            "\
It looks like you have set delta as the value of $PAGER. \
This would result in a non-terminating recursion. \
delta is not an appropriate value for $PAGER \
(but it is an appropriate value for $GIT_PAGER)."
        );
        std::process::exit(1);
    }
    if let Ok(pager_path) = grep_cli::resolve_binary(pager_path) {
        let mut p = Command::new(&pager_path);
        p.args(args);
        Some(p)
    } else {
        None
    }
}

    let diff_command_path = match grep_cli::resolve_binary(PathBuf::from(diff_command)) {
        Ok(path) => path,
        Err(_) => return config.error_exit_code,
    };
    let mut diff_process = process::Command::new(diff_command_path)