From 392f99a5d2174e6124f829d034bac6755c33119d Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Tue, 1 Oct 2019 23:27:18 +0200
Subject: Disallow dubiously-nested submodule git directories

commit a8dee3ca610f5a1d403634492136c887f83b59d2 upstream.

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.

[jn: backported to 2.11.y:
 - port to git-submodule.sh
 - use explicit chdir to emulate test_commit -C in test]

Reported-by: Nicolas Joly <Nicolas.Joly@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/submodule--helper.c | 24 ++++++++++++++++++++++
 git-submodule.sh            |  5 +++++
 submodule.c                 | 41 +++++++++++++++++++++++++++++++++++++
 submodule.h                 |  5 +++++
 t/t7415-submodule-names.sh  | 23 +++++++++++++++++++++
 5 files changed, 98 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 9e0f985501..6f33a665df 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -638,6 +638,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);
@@ -1111,6 +1115,25 @@ static int check_name(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+/*
+ * Exit non-zero if the proposed submodule repository path is inside
+ * another submodules' git dir.
+ */
+static int validate_git_dir(int argc, const char **argv, const char *prefix)
+{
+	char *sm_gitdir;
+
+	if (argc != 3)
+		usage("git submodule--helper validate-git-dir <path> <name>");
+	sm_gitdir = xstrdup(argv[1]);
+	if (validate_submodule_git_dir(sm_gitdir, argv[2]) < 0) {
+		free(sm_gitdir);
+		return 1;
+	}
+	free(sm_gitdir);
+	return 0;
+}
+
 struct cmd_struct {
 	const char *cmd;
 	int (*fn)(int, const char **, const char *);
@@ -1127,6 +1150,7 @@ static struct cmd_struct commands[] = {
 	{"init", module_init},
 	{"remote-branch", resolve_remote_submodule_branch},
 	{"check-name", check_name},
+	{"validate-git-dir", validate_git_dir},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index 81495c018f..f4e6b0204d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -242,6 +242,11 @@ Use -f if you really want to add it." >&2
 		fi
 
 	else
+		sm_gitdir=".git/modules/$sm_name"
+		if ! git submodule--helper validate-git-dir "$sm_gitdir" "$sm_name"
+		then
+			die "$(eval_gettextln "refusing to create/use '\$sm_gitdir' in another submodule's git dir")"
+		fi
 		if test -d ".git/modules/$sm_name"
 		then
 			if test -z "$force"
diff --git a/submodule.c b/submodule.c
index 6f7d883de9..060e67f7e2 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1251,6 +1251,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))
+		die("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;
+}
+
 void prepare_submodule_repo_env(struct argv_array *out)
 {
 	const char * const *var;
diff --git a/submodule.h b/submodule.h
index d9e197a948..4e7d6f12f1 100644
--- a/submodule.h
+++ b/submodule.h
@@ -68,6 +68,11 @@ int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_nam
 void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
 int parallel_submodules(void);
 
+/*
+ * 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);
+
 /*
  * Prepare the "env_array" parameter of a "struct child_process" for executing
  * a submodule by clearing any repo-specific envirionment variables, but
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
index b3a421d4fd..fd50ae1fb3 100755
--- a/t/t7415-submodule-names.sh
+++ b/t/t7415-submodule-names.sh
@@ -183,4 +183,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 &&
+	(
+		cd nested &&
+		test_commit 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.24.0.393.g34dc348eaf