fix(gix-submodule): don't follow submodule names with relative paths in them

This made it possible to trick submodule repos to be opened outside of the
actual repository.

Co-authored-by: Sebastian Thiel <sebastian.thiel@icloud.com>

Author: codex

gix-validate/src/submodule.rs

@@ -28,15 +28,10 @@ pub fn name(name: &BStr) -> Result<&BStr, name::Error> {
     if name.is_empty() {
         return Err(name::Error::Empty);
     }
-    match name.find(b"..") {
-        Some(pos) => {
-            let &b = name.get(pos + 2).ok_or(name::Error::ParentComponent)?;
-            if b == b'/' || b == b'\\' {
-                Err(name::Error::ParentComponent)
-            } else {
-                Ok(name)
-            }
+    for component in name.as_bytes().split(|b| *b == b'/' || *b == b'\\') {
+        if component == b".." {
+            return Err(name::Error::ParentComponent);
         }
-        None => Ok(name),
     }
+    Ok(name)
 }

gix/src/submodule/errors.rs

@@ -75,12 +75,42 @@ pub mod open {
         #[error(transparent)]
         OpenRepository(#[from] crate::open::Error),
         #[error(transparent)]
+        GitDir(#[from] crate::submodule::git_dir_try_old_form::Error),
+        #[error(transparent)]
         PathConfiguration(#[from] gix_submodule::config::path::Error),
         #[error(transparent)]
         WorktreeDirInaccessible(#[from] std::io::Error),
     }
 }
 
+///
+pub mod git_dir_try_old_form {
+    /// The error returned by [Submodule::git_dir_try_old_form()](crate::Submodule::git_dir_try_old_form()).
+    #[derive(Debug, thiserror::Error)]
+    #[allow(missing_docs)]
+    pub enum Error {
+        #[error(transparent)]
+        PathConfiguration(#[from] gix_submodule::config::path::Error),
+        #[error(transparent)]
+        GitDir(#[from] gix_validate::submodule::name::Error),
+    }
+}
+
+///
+pub mod state {
+    /// The error returned by [Submodule::state()](crate::Submodule::state()).
+    #[derive(Debug, thiserror::Error)]
+    #[allow(missing_docs)]
+    pub enum Error {
+        #[error(transparent)]
+        GitDirTryOldForm(#[from] crate::submodule::git_dir_try_old_form::Error),
+        #[error(transparent)]
+        GitDir(#[from] gix_validate::submodule::name::Error),
+        #[error(transparent)]
+        PathConfiguration(#[from] gix_submodule::config::path::Error),
+    }
+}
+
 ///
 pub mod index_id {
     /// The error returned by [Submodule::index_id()](crate::Submodule::index_id()).

gix/src/submodule/mod.rs

@@ -84,10 +84,20 @@ struct IsActiveState {
 
 ///Access
 impl Submodule<'_> {
-    /// Return the submodule's name.
+    /// Return the submodule's configured name as it appears in `submodule.<name>.*`.
+    ///
+    /// Note that this name is not guaranteed to be valid and may contain traversal components if
+    /// the configuration was crafted manually.
+    ///
+    /// Use [`validated_name()`](Self::validated_name()) to obtain a validated submodule name.
     pub fn name(&self) -> &BStr {
         self.name.as_ref()
     }
+
+    /// Return the submodule's name after validating it for safe use in paths like `.git/modules/<name>`.
+    pub fn validated_name(&self) -> Result<&BStr, gix_validate::submodule::name::Error> {
+        gix_validate::submodule::name(self.name())
+    }
     /// Return the path at which the submodule can be found, relative to the repository.
     ///
     /// For details, see [gix_submodule::File::path()].
@@ -194,13 +204,14 @@ impl Submodule<'_> {
 
     /// Return the path at which the repository of the submodule should be located.
     ///
-    /// The directory might not exist yet.
-    pub fn git_dir(&self) -> PathBuf {
-        self.state
+    /// The retunred directory might not exist yet.
+    pub fn git_dir(&self) -> Result<PathBuf, gix_validate::submodule::name::Error> {
+        Ok(self
+            .state
             .repo
             .common_dir()
             .join("modules")
-            .join(gix_path::from_bstr(self.name()))
+            .join(gix_path::from_bstr(self.validated_name()?)))
     }
 
     /// Return the path to the location at which the workdir would be checked out.
@@ -223,20 +234,21 @@ impl Submodule<'_> {
     /// invalid - it's left to the caller to try to open it.
     ///
     /// Also note that the returned path may not actually exist.
-    pub fn git_dir_try_old_form(&self) -> Result<PathBuf, config::path::Error> {
-        let worktree_gitdir_or_modules_gitdir = if self.worktree_gitdir()?.is_dir() {
-            self.worktree_gitdir()?
+    pub fn git_dir_try_old_form(&self) -> Result<PathBuf, git_dir_try_old_form::Error> {
+        let worktree_gitdir = self.worktree_gitdir()?;
+        let worktree_gitdir_or_modules_gitdir = if worktree_gitdir.is_dir() {
+            worktree_gitdir
         } else {
-            self.git_dir()
+            self.git_dir()?
         };
         Ok(worktree_gitdir_or_modules_gitdir)
     }
 
     /// Query various parts of the submodule and assemble it into state information.
     #[doc(alias = "status", alias = "git2")]
-    pub fn state(&self) -> Result<State, config::path::Error> {
+    pub fn state(&self) -> Result<State, state::Error> {
         let maybe_old_path = self.git_dir_try_old_form()?;
-        let git_dir = self.git_dir();
+        let git_dir = self.git_dir()?;
         let worktree_git = self.worktree_gitdir()?;
         let superproject_configuration = self
             .state
@@ -298,15 +310,15 @@ impl Submodule<'_> {
 pub mod status {
     use gix_submodule::config;
 
-    use super::{head_id, index_id, open, Status};
+    use super::{head_id, index_id, open, state, Status};
     use crate::Submodule;
 
     /// The error returned by [Submodule::status()].
     #[derive(Debug, thiserror::Error)]
     #[allow(missing_docs)]
     pub enum Error {
         #[error(transparent)]
-        State(#[from] config::path::Error),
+        State(#[from] state::Error),
         #[error(transparent)]
         HeadId(#[from] head_id::Error),
         #[error(transparent)]

gix/tests/gix/submodule.rs

@@ -462,7 +462,7 @@ mod open {
 
             assert_ne!(
                 sm.git_dir_try_old_form()?,
-                sm.git_dir(),
+                sm.git_dir()?,
                 "compat git dir should be the worktree location"
             );
             let sm_repo = sm.open()?.expect("initialized");
@@ -497,7 +497,6 @@ mod advisory {
     fn traversal_names_do_not_escape_the_modules_directory() -> crate::Result {
         let fixture = gix_testtools::scripted_fixture_writable("make_submodule_traversal_advisory.sh")?;
         let repo_dir = fixture.path().join("victim-repo");
-        let redirected_repo = fixture.path().join("escaped-target.git");
 
         let repo = gix::open_opts(&repo_dir, crate::restricted())?;
         let sm = repo
@@ -506,15 +505,40 @@ mod advisory {
             .next()
             .expect("one malicious submodule");
 
+        assert!(matches!(
+            sm.git_dir(),
+            Err(gix_validate::submodule::name::Error::ParentComponent)
+        ));
+        assert!(matches!(
+            sm.git_dir_try_old_form(),
+            Err(gix::submodule::git_dir_try_old_form::Error::GitDir(
+                gix_validate::submodule::name::Error::ParentComponent
+            ))
+        ));
+
+        assert!(matches!(
+            sm.open(),
+            Err(gix::submodule::open::Error::GitDir(
+                gix::submodule::git_dir_try_old_form::Error::GitDir(
+                    gix_validate::submodule::name::Error::ParentComponent
+                )
+            ))
+        ));
+        assert!(matches!(
+            sm.state(),
+            Err(gix::submodule::state::Error::GitDirTryOldForm(
+                gix::submodule::git_dir_try_old_form::Error::GitDir(
+                    gix_validate::submodule::name::Error::ParentComponent
+                )
+            ))
+        ));
+
+        let redirected_repo = fixture.path().join("escaped-target.git");
         assert!(
-            sm.open()?.is_none(),
-            "opening a submodule must not reach the external repository at {}",
+            redirected_repo.is_dir(),
+            "the attacker-controlled repository does indeex exist at {}",
             redirected_repo.display()
         );
-        assert!(
-            !sm.state()?.repository_exists,
-            "submodule names with traversal components must not resolve to repositories outside `.git/modules`"
-        );
         Ok(())
     }