Disallow dubiously-nested submodule git directories

Currently it is technically possible to let a submodule's git
directory point right into the git dir of a sibling submodule.

Example: the git directories of two submodules with the names `hippo`
and `hippo/hooks` would be `.git/modules/hippo/` and
`.git/modules/hippo/hooks/`, respectively, but the latter is already
intended to house the former's hooks.

In most cases, this is just confusing, but there is also a (quite
contrived) attack vector where Git can be fooled into mistaking remote
content for file contents it wrote itself during a recursive clone.

Let's plug this bug.

To do so, we introduce the new function `validate_submodule_git_dir()`
which simply verifies that no git dir exists for any leading directories
of the submodule name (if there are any).

Note: this patch specifically continues to allow sibling modules names
of the form `core/lib`, `core/doc`, etc, as long as `core` is not a
submodule name.

This fixes CVE-2019-1387.

Reported-by: Nicolas Joly <Nicolas.Joly@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 79156fac45dd2050e36376d90ae85c0a1ccbb898..3376b1bb293a414c795c70e1de5b8fa81d4f55d3 100644 (file)
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -678,6 +678,10 @@ static int module_clone(int argc, const char **argv, const char *prefix)
        } else
                path = xstrdup(path);
 
+       if (validate_submodule_git_dir(sm_gitdir, name) < 0)
+               die(_("refusing to create/use '%s' in another submodule's "
+                       "git dir"), sm_gitdir);
+
        if (!file_exists(sm_gitdir)) {
                if (safe_create_leading_directories_const(sm_gitdir) < 0)
                        die(_("could not create directory '%s'"), sm_gitdir);

diff --git a/submodule.c b/submodule.c
index 36f45f5a5a2adedd7e598371f98f6a54880668df..9abc90d9cdfd1663f42bcaaffe526361ba7e40fe 100644 (file)
--- a/submodule.c
+++ b/submodule.c
@@ -1842,6 +1842,47 @@ int parallel_submodules(void)
        return parallel_jobs;
 }
 
+int validate_submodule_git_dir(char *git_dir, const char *submodule_name)
+{
+       size_t len = strlen(git_dir), suffix_len = strlen(submodule_name);
+       char *p;
+       int ret = 0;
+
+       if (len <= suffix_len || (p = git_dir + len - suffix_len)[-1] != '/' ||
+           strcmp(p, submodule_name))
+               BUG("submodule name '%s' not a suffix of git dir '%s'",
+                   submodule_name, git_dir);
+
+       /*
+        * We prevent the contents of sibling submodules' git directories to
+        * clash.
+        *
+        * Example: having a submodule named `hippo` and another one named
+        * `hippo/hooks` would result in the git directories
+        * `.git/modules/hippo/` and `.git/modules/hippo/hooks/`, respectively,
+        * but the latter directory is already designated to contain the hooks
+        * of the former.
+        */
+       for (; *p; p++) {
+               if (is_dir_sep(*p)) {
+                       char c = *p;
+
+                       *p = '\0';
+                       if (is_git_directory(git_dir))
+                               ret = -1;
+                       *p = c;
+
+                       if (ret < 0)
+                               return error(_("submodule git dir '%s' is "
+                                              "inside git dir '%.*s'"),
+                                            git_dir,
+                                            (int)(p - git_dir), git_dir);
+               }
+       }
+
+       return 0;
+}
+
 /*
  * Embeds a single submodules git directory into the superprojects git dir,
  * non recursively.
@@ -1850,7 +1891,7 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,
                                                      const char *path)
 {
        char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
-       const char *new_git_dir;
+       char *new_git_dir;
        const struct submodule *sub;
 
        if (submodule_uses_worktrees(path))
@@ -1868,10 +1909,14 @@ static void relocate_single_git_dir_into_superproject(const char *prefix,
        if (!sub)
                die(_("could not lookup name for submodule '%s'"), path);
 
-       new_git_dir = git_path("modules/%s", sub->name);
+       new_git_dir = git_pathdup("modules/%s", sub->name);
+       if (validate_submodule_git_dir(new_git_dir, sub->name) < 0)
+               die(_("refusing to move '%s' into an existing git dir"),
+                   real_old_git_dir);
        if (safe_create_leading_directories_const(new_git_dir) < 0)
                die(_("could not create directory '%s'"), new_git_dir);
        real_new_git_dir = real_pathdup(new_git_dir, 1);
+       free(new_git_dir);
 
        fprintf(stderr, _("Migrating git directory of '%s%s' from\n'%s' to\n'%s'\n"),
                get_super_prefix_or_empty(), path,

diff --git a/submodule.h b/submodule.h
index 3c239d1ecf445abbec9528bdf27b92aaaa5ec52e..cb1ab07b9a6bde09a9bce31320f726d6b652062c 100644 (file)
--- a/submodule.h
+++ b/submodule.h
@@ -120,6 +120,11 @@ extern int parallel_submodules(void);
  */
 int submodule_to_gitdir(struct strbuf *buf, const char *submodule);
 
+/*
+ * Make sure that no submodule's git dir is nested in a sibling submodule's.
+ */
+int validate_submodule_git_dir(char *git_dir, const char *submodule_name);
+
 #define SUBMODULE_MOVE_HEAD_DRY_RUN (1<<0)
 #define SUBMODULE_MOVE_HEAD_FORCE   (1<<1)
 extern int submodule_move_head(const char *path,

diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index 7c65e7a35c9819411eb1eeb0d8a0e157fae58015..8bd3d0937d9e52f1b8ee48edbb6132d00535c3c5 100755 (executable)
--- a/t/t7415-submodule-names.sh
+++ b/t/t7415-submodule-names.sh
@@ -106,4 +106,27 @@ test_expect_success MINGW 'prevent git~1 squatting on Windows' '
        ! grep gitdir squatting-clone/d/a/git~2
 '
 
+test_expect_success 'git dirs of sibling submodules must not be nested' '
+       git init nested &&
+       test_commit -C nested nested &&
+       (
+               cd nested &&
+               cat >.gitmodules <<-EOF &&
+               [submodule "hippo"]
+                       url = .
+                       path = thing1
+               [submodule "hippo/hooks"]
+                       url = .
+                       path = thing2
+               EOF
+               git clone . thing1 &&
+               git clone . thing2 &&
+               git add .gitmodules thing1 thing2 &&
+               test_tick &&
+               git commit -m nested
+       ) &&
+       test_must_fail git clone --recurse-submodules nested clone 2>err &&
+       test_i18ngrep "is inside git dir" err
+'
+
 test_done