From a8dee3ca610f5a1d403634492136c887f83b59d2 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 1 Oct 2019 23:27:18 +0200 Subject: [PATCH] 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 Signed-off-by: Johannes Schindelin --- builtin/submodule--helper.c | 4 ++++ submodule.c | 49 +++++++++++++++++++++++++++++++++++++++++++-- submodule.h | 5 +++++ t/t7415-submodule-names.sh | 23 +++++++++++++++++++++ 4 files changed, 79 insertions(+), 2 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 79156fac45..3376b1bb29 100644 --- 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 36f45f5a5a..9abc90d9cd 100644 --- 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 3c239d1ecf..cb1ab07b9a 100644 --- 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 7c65e7a35c..8bd3d0937d 100755 --- 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 -- 2.11.4.GIT