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.

Author: Byron

Cargo.lock

@@ -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/Cargo.toml

@@ -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/init.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/src/lib.rs

@@ -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 }