From 3da5cb9d9cf49e64b8186785eac8a9364f708a37 Mon Sep 17 00:00:00 2001
From: Jiahao XU <Jiahao_XU@outlook.com>
Date: Sun, 25 Sep 2022 23:32:34 +1000
Subject: [PATCH] Check `source_file_path` to ensure it is valid (#421)

* Add new variant `BinstallError::DuplicateSourceFilePath`
* Check `bin.source` in `collect_bin_files`
* Add new variant `BinstallError::InvalidSourceFilePath`
* Make sure generated source_file_path cannot access outside curdir
* Add new variant `BinstallError::EmptySourceFilePath`
* Ensure source_file_path is not empty

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
---
 crates/binstalk/src/bins.rs        | 33 ++++++++++++++++++++++++++++--
 crates/binstalk/src/errors.rs      | 30 +++++++++++++++++++++++++++
 crates/binstalk/src/ops/resolve.rs | 11 ++++++++++
 3 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/crates/binstalk/src/bins.rs b/crates/binstalk/src/bins.rs
index cb66bbf39..4d8971848 100644
--- a/crates/binstalk/src/bins.rs
+++ b/crates/binstalk/src/bins.rs
@@ -1,11 +1,12 @@
 use std::{
     borrow::Cow,
-    path::{Path, PathBuf},
+    path::{Component, Path, PathBuf},
 };
 
 use cargo_toml::Product;
 use compact_str::CompactString;
 use log::debug;
+use normalize_path::NormalizePath;
 use serde::Serialize;
 use tinytemplate::TinyTemplate;
 
@@ -15,6 +16,18 @@ use crate::{
     manifests::cargo_toml_binstall::{PkgFmt, PkgMeta},
 };
 
+/// Return true if the path does not look outside of current dir
+///
+///  * `path` - must be normalized before passing to this function
+fn is_valid_path(path: &Path) -> bool {
+    !matches!(
+        path.components().next(),
+        // normalized path cannot have curdir or parentdir,
+        // so checking prefix/rootdir is enough.
+        Some(Component::Prefix(..) | Component::RootDir)
+    )
+}
+
 pub struct BinFile {
     pub base_name: CompactString,
     pub source: PathBuf,
@@ -47,7 +60,23 @@ impl BinFile {
         // Generate install paths
         // Source path is the download dir + the generated binary path
         let source_file_path = if let Some(bin_dir) = &data.meta.bin_dir {
-            ctx.render(bin_dir)?
+            let path = ctx.render(bin_dir)?;
+            let path_normalized = Path::new(&path).normalize();
+
+            if path_normalized.components().next().is_none() {
+                return Err(BinstallError::EmptySourceFilePath);
+            }
+
+            if !is_valid_path(&path_normalized) {
+                return Err(BinstallError::InvalidSourceFilePath {
+                    path: path_normalized.into_owned(),
+                });
+            }
+
+            match path_normalized {
+                Cow::Borrowed(..) => path,
+                Cow::Owned(path) => path.to_string_lossy().into_owned(),
+            }
         } else {
             let name = ctx.name;
             let target = ctx.target;
diff --git a/crates/binstalk/src/errors.rs b/crates/binstalk/src/errors.rs
index 0ebe41f87..a1abe80d7 100644
--- a/crates/binstalk/src/errors.rs
+++ b/crates/binstalk/src/errors.rs
@@ -284,6 +284,33 @@ pub enum BinstallError {
     #[diagnostic(severity(error), code(binstall::cargo_manifest))]
     CargoTomlMissingPackage(CompactString),
 
+    /// bin-dir configuration provided generates duplicate source path.
+    ///
+    /// - Code: `binstall::cargo_manifest`
+    /// - Exit: 90
+    #[error("bin-dir configuration provided generates duplicate source path: {path}")]
+    #[diagnostic(severity(error), code(binstall::SourceFilePath))]
+    DuplicateSourceFilePath { path: PathBuf },
+
+    /// bin-dir configuration provided generates source path outside
+    /// of the temporary dir.
+    ///
+    /// - Code: `binstall::cargo_manifest`
+    /// - Exit: 91
+    #[error(
+        "bin-dir configuration provided generates source path outside of the temporary dir: {path}"
+    )]
+    #[diagnostic(severity(error), code(binstall::SourceFilePath))]
+    InvalidSourceFilePath { path: PathBuf },
+
+    /// bin-dir configuration provided generates empty source path.
+    ///
+    /// - Code: `binstall::cargo_manifest`
+    /// - Exit: 92
+    #[error("bin-dir configuration provided generates empty source path")]
+    #[diagnostic(severity(error), code(binstall::SourceFilePath))]
+    EmptySourceFilePath,
+
     /// A wrapped error providing the context of which crate the error is about.
     #[error("for crate {crate_name}")]
     CrateContext {
@@ -319,6 +346,9 @@ impl BinstallError {
             NoViableTargets => 87,
             BinFileNotFound(_) => 88,
             CargoTomlMissingPackage(_) => 89,
+            DuplicateSourceFilePath { .. } => 90,
+            InvalidSourceFilePath { .. } => 91,
+            EmptySourceFilePath => 92,
             CrateContext { error, .. } => error.exit_number(),
         };
 
diff --git a/crates/binstalk/src/ops/resolve.rs b/crates/binstalk/src/ops/resolve.rs
index b59f848eb..4addb38cb 100644
--- a/crates/binstalk/src/ops/resolve.rs
+++ b/crates/binstalk/src/ops/resolve.rs
@@ -1,4 +1,5 @@
 use std::{
+    collections::BTreeSet,
     path::{Path, PathBuf},
     sync::Arc,
 };
@@ -356,6 +357,16 @@ fn collect_bin_files(
         .map(|p| bins::BinFile::from_product(&bin_data, p))
         .collect::<Result<Vec<_>, BinstallError>>()?;
 
+    let mut source_set = BTreeSet::new();
+
+    for bin in &bin_files {
+        if !source_set.insert(&bin.source) {
+            return Err(BinstallError::DuplicateSourceFilePath {
+                path: bin.source.clone(),
+            });
+        }
+    }
+
     Ok(bin_files)
 }