fix!: State::from_tree() now performs name validation.

Previously, malicious trees could be used to create a index with
invalid names, which is one step closer to actually abusing it.

gix-index/Cargo.toml

@@ -27,6 +27,7 @@ gix-features = { version = "^0.38.1", path = "../gix-features", features = [
 gix-hash = { version = "^0.14.2", path = "../gix-hash" }
 gix-bitmap = { version = "^0.2.11", path = "../gix-bitmap" }
 gix-object = { version = "^0.42.1", path = "../gix-object" }
+gix-validate = { version = "^0.8.4", path = "../gix-validate" }
 gix-traverse = { version = "^0.39.0", path = "../gix-traverse" }
 gix-lock = { version = "^13.0.0", path = "../gix-lock" }
 gix-fs = { version = "^0.10.2", path = "../gix-fs" }

gix-index/src/init.rs

@@ -1,4 +1,6 @@
-mod from_tree {
+#[allow(clippy::empty_docs)]
+///
+pub mod from_tree {
     use std::collections::VecDeque;
 
     use bstr::{BStr, BString, ByteSlice, ByteVec};
@@ -10,6 +12,19 @@ mod from_tree {
         Entry, PathStorage, State, Version,
     };
 
+    /// The error returned by [State::from_tree()].
+    #[derive(Debug, thiserror::Error)]
+    #[allow(missing_docs)]
+    pub enum Error {
+        #[error("The path \"{path}\" is invalid")]
+        InvalidComponent {
+            path: BString,
+            source: gix_validate::path::component::Error,
+        },
+        #[error(transparent)]
+        Traversal(#[from] gix_traverse::tree::breadthfirst::Error),
+    }
+
     /// Initialization
     impl State {
         /// Return a new and empty in-memory index assuming the given `object_hash`.
@@ -32,23 +47,42 @@ mod from_tree {
         }
         /// Create an index [`State`] by traversing `tree` recursively, accessing sub-trees
         /// with `objects`.
+        /// `validate` is used to determine which validations to perform on every path component we see.
         ///
         /// **No extension data is currently produced**.
-        pub fn from_tree<Find>(tree: &gix_hash::oid, objects: Find) -> Result<Self, breadthfirst::Error>
+        pub fn from_tree<Find>(
+            tree: &gix_hash::oid,
+            objects: Find,
+            validate: gix_validate::path::component::Options,
+        ) -> Result<Self, Error>
         where
             Find: gix_object::Find,
         {
             let _span = gix_features::trace::coarse!("gix_index::State::from_tree()");
             let mut buf = Vec::new();
-            let root = objects.find_tree_iter(tree, &mut buf)?;
-            let mut delegate = CollectEntries::new();
-            breadthfirst(root, breadthfirst::State::default(), &objects, &mut delegate)?;
+            let root = objects
+                .find_tree_iter(tree, &mut buf)
+                .map_err(breadthfirst::Error::from)?;
+            let mut delegate = CollectEntries::new(validate);
+            match breadthfirst(root, breadthfirst::State::default(), &objects, &mut delegate) {
+                Ok(()) => {}
+                Err(gix_traverse::tree::breadthfirst::Error::Cancelled) => {
+                    let (path, err) = delegate
+                        .invalid_path
+                        .take()
+                        .expect("cancellation only happens on validation error");
+                    return Err(Error::InvalidComponent { path, source: err });
+                }
+                Err(err) => return Err(err.into()),
+            }
 
             let CollectEntries {
                 mut entries,
                 path_backing,
                 path: _,
                 path_deque: _,
+                validate: _,
+                invalid_path: _,
             } = delegate;
 
             entries.sort_by(|a, b| Entry::cmp_filepaths(a.path_in(&path_backing), b.path_in(&path_backing)));
@@ -76,15 +110,19 @@ mod from_tree {
         path_backing: PathStorage,
         path: BString,
         path_deque: VecDeque<BString>,
+        validate: gix_validate::path::component::Options,
+        invalid_path: Option<(BString, gix_validate::path::component::Error)>,
     }
 
     impl CollectEntries {
-        pub fn new() -> CollectEntries {
+        pub fn new(validate: gix_validate::path::component::Options) -> CollectEntries {
             CollectEntries {
                 entries: Vec::new(),
                 path_backing: Vec::new(),
                 path: BString::default(),
                 path_deque: VecDeque::new(),
+                validate,
+                invalid_path: None,
             }
         }
 
@@ -93,6 +131,11 @@ mod from_tree {
                 self.path.push(b'/');
             }
             self.path.push_str(name);
+            if self.invalid_path.is_none() {
+                if let Err(err) = gix_validate::path::component(name, None, self.validate) {
+                    self.invalid_path = Some((self.path.clone(), err))
+                }
+            }
         }
 
         pub fn add_entry(&mut self, entry: &tree::EntryRef<'_>) {
@@ -103,6 +146,18 @@ mod from_tree {
                 EntryKind::Link => Mode::SYMLINK,
                 EntryKind::Commit => Mode::COMMIT,
             };
+            // There are leaf-names that require special validation, specific to their mode.
+            // Double-validate just for this case, as the previous validation didn't know the mode yet.
+            if self.invalid_path.is_none() {
+                let start = self.path.rfind_byte(b'/').map(|pos| pos + 1).unwrap_or_default();
+                if let Err(err) = gix_validate::path::component(
+                    self.path[start..].as_ref(),
+                    (entry.mode.kind() == EntryKind::Link).then_some(gix_validate::path::component::Mode::Symlink),
+                    self.validate,
+                ) {
+                    self.invalid_path = Some((self.path.clone(), err));
+                }
+            }
 
             let path_start = self.path_backing.len();
             self.path_backing.extend_from_slice(&self.path);
@@ -117,6 +172,14 @@ mod from_tree {
 
             self.entries.push(new_entry);
         }
+
+        fn determine_action(&self) -> Action {
+            if self.invalid_path.is_none() {
+                Action::Continue
+            } else {
+                Action::Cancel
+            }
+        }
     }
 
     impl Visit for CollectEntries {
@@ -127,12 +190,12 @@ mod from_tree {
                 .expect("every call is matched with push_tracked_path_component");
         }
 
-        fn push_back_tracked_path_component(&mut self, component: &bstr::BStr) {
+        fn push_back_tracked_path_component(&mut self, component: &BStr) {
             self.push_element(component);
             self.path_deque.push_back(self.path.clone());
         }
 
-        fn push_path_component(&mut self, component: &bstr::BStr) {
+        fn push_path_component(&mut self, component: &BStr) {
             self.push_element(component);
         }
 
@@ -144,13 +207,13 @@ mod from_tree {
             }
         }
 
-        fn visit_tree(&mut self, _entry: &gix_object::tree::EntryRef<'_>) -> gix_traverse::tree::visit::Action {
-            Action::Continue
+        fn visit_tree(&mut self, _entry: &gix_object::tree::EntryRef<'_>) -> Action {
+            self.determine_action()
         }
 
-        fn visit_nontree(&mut self, entry: &gix_object::tree::EntryRef<'_>) -> gix_traverse::tree::visit::Action {
+        fn visit_nontree(&mut self, entry: &gix_object::tree::EntryRef<'_>) -> Action {
             self.add_entry(entry);
-            Action::Continue
+            self.determine_action()
         }
     }
 }

gix-index/src/lib.rs

@@ -26,7 +26,9 @@ pub mod entry;
 
 mod access;
 
-mod init;
+///
+#[allow(clippy::empty_docs)]
+pub mod init;
 
 ///
 #[allow(clippy::empty_docs)]
@@ -116,8 +118,8 @@ pub struct AccelerateLookup<'a> {
 ///
 /// # A note on safety
 ///
-/// An index (i.e. [`State`]) created [from a tree](State::from_tree()) is not guaranteed to have valid entry paths as those
-/// depend on the names contained in trees entirely, without applying any level of validation.
+/// An index (i.e. [`State`]) created by hand is not guaranteed to have valid entry paths as they are entirely controlled
+/// by the caller, without applying any level of validation.
 ///
 /// This means that before using these paths to recreate files on disk, *they must be validated*.
 ///

gix-index/tests/Cargo.toml

@@ -19,8 +19,9 @@ gix-features-parallel = ["gix-features/parallel"]
 [dev-dependencies]
 gix-index = { path = ".." }
 gix-features = { path = "../../gix-features", features = ["rustsha1", "progress"] }
-gix-testtools = { path = "../../tests/tools"}
-gix = { path = "../../gix", default-features = false, features = ["index"] }
-gix-hash = { path = "../../gix-hash"}
+gix-testtools = { path = "../../tests/tools" }
+gix-odb = { path = "../../gix-odb" }
+gix-object = { path = "../../gix-object" }
+gix-hash = { path = "../../gix-hash" }
 filetime = "0.2.15"
 bstr = { version = "1.3.0", default-features = false }

gix-index/tests/fixtures/make_index/v2.sh

@@ -8,3 +8,5 @@ git config index.threads 2
 touch a
 git add a
 git commit -m "empty"
+
+git rev-parse @^{tree} > head.tree

gix-index/tests/fixtures/make_index/v2_all_file_kinds.sh

@@ -22,3 +22,5 @@ mkdir d
 
 git add .
 git commit -m "init"
+
+git rev-parse @^{tree} > head.tree

gix-index/tests/fixtures/make_index/v2_more_files.sh

@@ -11,3 +11,5 @@ mkdir d
 
 git add .
 git commit -m "empty"
+
+git rev-parse @^{tree} > head.tree

gix-index/tests/fixtures/make_index/v2_sparse_index_no_dirs.sh

@@ -17,4 +17,4 @@ git config --worktree index.sparse true
 echo "/*" > .git/info/sparse-checkout &&
 echo "!/*/" >> .git/info/sparse-checkout
 
-git checkout main
+git checkout main

gix-index/tests/fixtures/make_index/v3_sparse_index_non_cone.sh

@@ -13,4 +13,4 @@ mkdir d
 git add .
 git commit -m "init"
 
-git sparse-checkout set c1/c2 --no-cone
+git sparse-checkout set c1/c2 --no-cone

gix-index/tests/fixtures/make_index/v4_more_files_IEOT.sh

@@ -12,3 +12,5 @@ touch x
 
 git add .
 git commit -m "empty"
+
+git rev-parse @^{tree} > head.tree

gix-index/tests/fixtures/make_traverse_literal_separators.sh

@@ -0,0 +1,44 @@
+#!/bin/bash
+set -eu -o pipefail
+
+# Makes a repo carrying a literally named file, which may even contain "/".
+# File content is from stdin. Arguments are repo name, file name, and file mode.
+function make_repo() (
+  local repo="$1" file="$2" mode="$3"
+  local blob_hash_escaped tree_hash commit_hash branch
+
+  git init -- "$repo"
+  cd -- "$repo" # Temporary, as the function body is a ( ) subshell.
+
+  blob_hash_escaped="$(git hash-object -w --stdin | sed 's/../\\x&/g')"
+
+  tree_hash="$(
+    printf "%s %s\\0$blob_hash_escaped" "$mode" "$file" |
+    git hash-object -t tree -w --stdin --literally
+  )"
+
+  commit_hash="$(git commit-tree -m 'Initial commit' "$tree_hash")"
+
+  branch="$(git symbolic-ref --short HEAD)"
+  git branch -f -- "$branch" "$commit_hash"
+  test -z "${DEBUG_FIXTURE-}" || git show # TODO: How should verbosity be controlled?
+  git rev-parse @^{tree} > head.tree
+)
+
+make_repo traverse_dotdot_slashes ../outside 100644 \
+  <<<'A file outside the working tree, somehow.'
+
+make_repo traverse_dotgit_slashes .git/hooks/pre-commit 100755 <<'EOF'
+#!/bin/sh
+printf 'Vulnerable!\n'
+date >vulnerable
+EOF
+
+make_repo traverse_dotdot_backslashes '..\outside' 100644 \
+  <<<'A file outside the working tree, somehow.'
+
+make_repo traverse_dotgit_backslashes '.git\hooks\pre-commit' 100755 <<'EOF'
+#!/bin/sh
+printf 'Vulnerable!\n'
+date >vulnerable
+EOF

gix-index/tests/index/file/read.rs

@@ -11,7 +11,7 @@ use crate::{hex_to_id, index::Fixture, loose_file_path};
 fn verify(index: gix_index::File) -> gix_index::File {
     index.verify_integrity().unwrap();
     index.verify_entries().unwrap();
-    index.verify_extensions(false, gix::objs::find::Never).unwrap();
+    index.verify_extensions(false, gix_object::find::Never).unwrap();
     index
 }
 

gix-index/tests/index/file/write.rs

@@ -228,7 +228,7 @@ fn compare_states_against_baseline(
 
 fn compare_states(actual: &State, actual_version: Version, expected: &State, options: Options, fixture: &str) {
     actual.verify_entries().expect("valid");
-    actual.verify_extensions(false, gix::objs::find::Never).expect("valid");
+    actual.verify_extensions(false, gix_object::find::Never).expect("valid");
 
     assert_eq!(
         actual.version(),