debian/patches/submodule-config-verify-submodule-names-as-paths.diff

From 34bfead2ba4d31eab35b551023fe9fa0b8a80459 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Mon, 30 Apr 2018 03:25:25 -0400
Subject: submodule-config: verify submodule names as paths

commit 0383bbb9015898cbc79abd7b64316484d7713b44 upstream.

Submodule "names" come from the untrusted .gitmodules file,
but we blindly append them to $GIT_DIR/modules to create our
on-disk repo paths. This means you can do bad things by
putting "../" into the name (among other things).

Let's sanity-check these names to avoid building a path that
can be exploited. There are two main decisions:

  1. What should the allowed syntax be?

     It's tempting to reuse verify_path(), since submodule
     names typically come from in-repo paths. But there are
     two reasons not to:

       a. It's technically more strict than what we need, as
          we really care only about breaking out of the
          $GIT_DIR/modules/ hierarchy.  E.g., having a
          submodule named "foo/.git" isn't actually
          dangerous, and it's possible that somebody has
          manually given such a funny name.

       b. Since we'll eventually use this checking logic in
          fsck to prevent downstream repositories, it should
          be consistent across platforms. Because
          verify_path() relies on is_dir_sep(), it wouldn't
          block "foo\..\bar" on a non-Windows machine.

  2. Where should we enforce it? These days most of the
     .gitmodules reads go through submodule-config.c, so
     I've put it there in the reading step. That should
     cover all of the C code.

     We also construct the name for "git submodule add"
     inside the git-submodule.sh script. This is probably
     not a big deal for security since the name is coming
     from the user anyway, but it would be polite to remind
     them if the name they pick is invalid (and we need to
     expose the name-checker to the shell anyway for our
     test scripts).

     This patch issues a warning when reading .gitmodules
     and just ignores the related config entry completely.
     This will generally end up producing a sensible error,
     as it works the same as a .gitmodules file which is
     missing a submodule entry (so "submodule update" will
     barf, but "git clone --recurse-submodules" will print
     an error but not abort the clone.

     There is one minor oddity, which is that we print the
     warning once per malformed config key (since that's how
     the config subsystem gives us the entries). So in the
     new test, for example, the user would see three
     warnings. That's OK, since the intent is that this case
     should never come up outside of malicious repositories
     (and then it might even benefit the user to see the
     message multiple times).

Credit for finding this vulnerability and the proof of
concept from which the test script was adapted goes to
Etienne Stalmans.

[jn: the original patch expects 'git clone' to succeed in
 the test because v2.13.0-rc0~10^2~3 (clone: teach
 --recurse-submodules to optionally take a pathspec,
 2017-03-17) makes 'git clone' skip invalid submodules.
 Updated the test to pass in older Git versions where the
 submodule name check makes 'git clone' fail.]

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/submodule--helper.c | 26 ++++++++++++-
 git-submodule.sh            |  5 +++
 submodule-config.c          | 31 +++++++++++++++
 submodule-config.h          |  7 ++++
 t/t7415-submodule-names.sh  | 77 +++++++++++++++++++++++++++++++++++++
 5 files changed, 145 insertions(+), 1 deletion(-)
 create mode 100755 t/t7415-submodule-names.sh

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5f9f..b4278e3a71 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,6 +1076,29 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,
 	return 0;
 }
 
+/*
+ * Exit non-zero if any of the submodule names given on the command line is
+ * invalid. If no names are given, filter stdin to print only valid names
+ * (which is primarily intended for testing).
+ */
+static int check_name(int argc, const char **argv, const char *prefix)
+{
+	if (argc > 1) {
+		while (*++argv) {
+			if (check_submodule_name(*argv) < 0)
+				return 1;
+		}
+	} else {
+		struct strbuf buf = STRBUF_INIT;
+		while (strbuf_getline(&buf, stdin) != EOF) {
+			if (!check_submodule_name(buf.buf))
+				printf("%s\n", buf.buf);
+		}
+		strbuf_release(&buf);
+	}
+	return 0;
+}
+
 struct cmd_struct {
 	const char *cmd;
 	int (*fn)(int, const char **, const char *);
@@ -1090,7 +1113,8 @@ static struct cmd_struct commands[] = {
 	{"resolve-relative-url", resolve_relative_url},
 	{"resolve-relative-url-test", resolve_relative_url_test},
 	{"init", module_init},
-	{"remote-branch", resolve_remote_submodule_branch}
+	{"remote-branch", resolve_remote_submodule_branch},
+	{"check-name", check_name},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a135d6..cee4ddc04b 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -225,6 +225,11 @@ Use -f if you really want to add it." >&2
 		sm_name="$sm_path"
 	fi
 
+	if ! git submodule--helper check-name "$sm_name"
+	then
+		die "$(eval_gettext "'$sm_name' is not a valid submodule name")"
+	fi
+
 	# perhaps the path exists and is already a git repo, else clone it
 	if test -e "$sm_path"
 	then
diff --git a/submodule-config.c b/submodule-config.c
index 098085be69..2697f7a145 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -163,6 +163,31 @@ static struct submodule *cache_lookup_name(struct submodule_cache *cache,
 	return NULL;
 }
 
+int check_submodule_name(const char *name)
+{
+	/* Disallow empty names */
+	if (!*name)
+		return -1;
+
+	/*
+	 * Look for '..' as a path component. Check both '/' and '\\' as
+	 * separators rather than is_dir_sep(), because we want the name rules
+	 * to be consistent across platforms.
+	 */
+	goto in_component; /* always start inside component */
+	while (*name) {
+		char c = *name++;
+		if (c == '/' || c == '\\') {
+in_component:
+			if (name[0] == '.' && name[1] == '.' &&
+			    (!name[2] || name[2] == '/' || name[2] == '\\'))
+				return -1;
+		}
+	}
+
+	return 0;
+}
+
 static int name_and_item_from_var(const char *var, struct strbuf *name,
 				  struct strbuf *item)
 {
@@ -174,6 +199,12 @@ static int name_and_item_from_var(const char *var, struct strbuf *name,
 		return 0;
 
 	strbuf_add(name, subsection, subsection_len);
+	if (check_submodule_name(name->buf) < 0) {
+		warning(_("ignoring suspicious submodule name: %s"), name->buf);
+		strbuf_release(name);
+		return 0;
+	}
+
 	strbuf_addstr(item, key);
 
 	return 1;
diff --git a/submodule-config.h b/submodule-config.h
index d05c542d2c..01b8e1c10a 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -31,4 +31,11 @@ const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
 		const char *path);
 void submodule_free(void);
 
+/*
+ * Returns 0 if the name is syntactically acceptable as a submodule "name"
+ * (e.g., that may be found in the subsection of a .gitmodules file) and -1
+ * otherwise.
+ */
+int check_submodule_name(const char *name);
+
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/t/t7415-submodule-names.sh b/t/t7415-submodule-names.sh
new file mode 100755
index 0000000000..de95ba8034
--- /dev/null
+++ b/t/t7415-submodule-names.sh
@@ -0,0 +1,77 @@
+#!/bin/sh
+
+test_description='check handling of .. in submodule names
+
+Exercise the name-checking function on a variety of names, and then give a
+real-world setup that confirms we catch this in practice.
+'
+. ./test-lib.sh
+
+test_expect_success 'check names' '
+	cat >expect <<-\EOF &&
+	valid
+	valid/with/paths
+	EOF
+
+	git submodule--helper check-name >actual <<-\EOF &&
+	valid
+	valid/with/paths
+
+	../foo
+	/../foo
+	..\foo
+	\..\foo
+	foo/..
+	foo/../
+	foo\..
+	foo\..\
+	foo/../bar
+	EOF
+
+	test_cmp expect actual
+'
+
+test_expect_success 'create innocent subrepo' '
+	git init innocent &&
+	git -C innocent commit --allow-empty -m foo
+'
+
+test_expect_success 'submodule add refuses invalid names' '
+	test_must_fail \
+		git submodule add --name ../../modules/evil "$PWD/innocent" evil
+'
+
+test_expect_success 'add evil submodule' '
+	git submodule add "$PWD/innocent" evil &&
+
+	mkdir modules &&
+	cp -r .git/modules/evil modules &&
+	write_script modules/evil/hooks/post-checkout <<-\EOF &&
+	echo >&2 "RUNNING POST CHECKOUT"
+	EOF
+
+	git config -f .gitmodules submodule.evil.update checkout &&
+	git config -f .gitmodules --rename-section \
+		submodule.evil submodule.../../modules/evil &&
+	git add modules &&
+	git commit -am evil
+'
+
+# This step seems like it shouldn't be necessary, since the payload is
+# contained entirely in the evil submodule. But due to the vagaries of the
+# submodule code, checking out the evil module will fail unless ".git/modules"
+# exists. Adding another submodule (with a name that sorts before "evil") is an
+# easy way to make sure this is the case in the victim clone.
+test_expect_success 'add other submodule' '
+	git submodule add "$PWD/innocent" another-module &&
+	git add another-module &&
+	git commit -am another
+'
+
+test_expect_success 'clone evil superproject' '
+	test_might_fail git clone --recurse-submodules . victim >output 2>&1 &&
+	cat output &&
+	! grep "RUNNING POST CHECKOUT" output
+'
+
+test_done
-- 
2.17.0.921.gf22659ad46