main/git: security fixes (CVE-2018-11233, CVE-2018-11235)

Fixes #8949

3 files changed, 360 insertions, 8 deletions

diff --git a/main/git/APKBUILD b/main/git/APKBUILD
index c454d452bf..cdafd68ccc 100644
--- a/main/git/APKBUILD
+++ b/main/git/APKBUILD
@@ -2,7 +2,7 @@
 # Maintainer: Natanael Copa <ncopa@alpinelinux.org>
 pkgname=git
 pkgver=2.11.3
-pkgrel=0
+pkgrel=1
 pkgdesc="A distributed version control system"
 url="https://www.git-scm.com/"
 arch="all"
@@ -10,10 +10,6 @@ license="GPL2+"
 depends=
 replaces="git-perl"
 
-# secfixes:
-#   2.11.3:
-#   - CVE-2017-1000117
-
 # note that order matters
 subpackages="$pkgname-doc
 	$pkgname-bash-completion:completion:noarch
@@ -38,6 +34,8 @@ source="https://www.kernel.org/pub/software/scm/git/git-$pkgver.tar.xz
 	bb-tar.patch
 	git-daemon.initd
 	git-daemon.confd
+	CVE-2018-11233.patch
+	CVE-2018-11235.patch
 	"
 
 _makeopts="
@@ -50,6 +48,13 @@ _makeopts="
 _gitcoredir=/usr/libexec/git-core
 _builddir="$srcdir"/$pkgname-$pkgver
 
+# secfixes:
+#   2.11.3-r1:
+#     - CVE-2018-11233
+#     - CVE-2018-11235
+#   2.11.3-r0:
+#     - CVE-2017-1000117
+
 build() {
 	cd "$_builddir"
 	make -j1 prefix=/usr DESTDIR="$pkgdir" $_makeopts perl/perl.mak || return 1
@@ -222,12 +227,18 @@ _git_perl() {
 md5sums="1e961079c5a38eb6a79f4a479550aade  git-2.11.3.tar.xz
 0549894076c2081d886fa5ee44c20e23  bb-tar.patch
 8bb51e7742a171891a51967b84c8088e  git-daemon.initd
-33427921f86acc26d1578d8b308e5baa  git-daemon.confd"
+33427921f86acc26d1578d8b308e5baa  git-daemon.confd
+c46121013e7329a583c1d0d885b4a5bd  CVE-2018-11233.patch
+af7b89141594387b0d05c13cfbb43e5b  CVE-2018-11235.patch"
 sha256sums="7343bbd489f59531d66bc086393f0d5f530b5175927c29fb97b07f9d2cbc31ac  git-2.11.3.tar.xz
 968e996a306dab643970c5ce1ac40926146b01b9c38a8fe81c74340a0302dbc7  bb-tar.patch
 ff9b5beefbe55ba6340f1c4acced195515002d0ccb431a8fcd5b1078eacd2e59  git-daemon.initd
-4703ba2372c661fb674a29fea7f64983f8b1b3136d971663509249655bca6e21  git-daemon.confd"
+4703ba2372c661fb674a29fea7f64983f8b1b3136d971663509249655bca6e21  git-daemon.confd
+93d9e1f4df65da8237fa1c93e544ad3a807a9dcabaf167644893b02465ff0080  CVE-2018-11233.patch
+7c0171e4258901bf154be43ba357888a27b1f7e618477704c413e60b1c0e15d5  CVE-2018-11235.patch"
 sha512sums="13f7dc80031c4376839debc0d900d1a6e79c1fe9d7cd4abe045ddac73bab059222abeb4ab6b9ccb06f11d6dc3ea4a219b653b679fb8142b75f4a4f820ecb4df4  git-2.11.3.tar.xz
 85767b5e03137008d6a96199e769e3979f75d83603ac8cb13a3481a915005637409a4fd94e0720da2ec6cd1124f35eba7cf20109a94816c4b4898a81fbc46bd2  bb-tar.patch
 89528cdd14c51fd568aa61cf6c5eae08ea0844e59f9af9292da5fc6c268261f4166017d002d494400945e248df6b844e2f9f9cd2d9345d516983f5a110e4c42a  git-daemon.initd
-fbf1f425206a76e2a8f82342537ed939ff7e623d644c086ca2ced5f69b36734695f9f80ebda1728f75a94d6cd2fcb71bf845b64239368caab418e4d368c141ec  git-daemon.confd"
+fbf1f425206a76e2a8f82342537ed939ff7e623d644c086ca2ced5f69b36734695f9f80ebda1728f75a94d6cd2fcb71bf845b64239368caab418e4d368c141ec  git-daemon.confd
+5201ff46b774a4f88fe8c4162394311ed85f4f507ebae1bce71e4dc199bb5ffc60f38c718bb6343c660560b93e648778fc6c4ad3917f95f0f44e31aecc6a6e38  CVE-2018-11233.patch
+b09b988ecf6cd2814b44120509cb455f1c2694641d50b62762861708d27e8bf5dc7af5fcb2e5acf589e005b98b9ba04d7bdef5c1e02bd66470eb3faab16cbf3d  CVE-2018-11235.patch"
diff --git a/main/git/CVE-2018-11233.patch b/main/git/CVE-2018-11233.patch
new file mode 100644
index 0000000000..081bc2658d
--- /dev/null
+++ b/main/git/CVE-2018-11233.patch
@@ -0,0 +1,44 @@
+From 4e2897b63b034fbf1ce5e52e3fbf14184f2ef0f0 Mon Sep 17 00:00:00 2001
+From: Jeff King <peff@peff.net>
+Date: Sun, 13 May 2018 12:09:42 -0400
+Subject: is_ntfs_dotgit: use a size_t for traversing string
+
+commit 11a9f4d807a0d71dc6eff51bb87baf4ca2cccf1d upstream.
+
+We walk through the "name" string using an int, which can
+wrap to a negative value and cause us to read random memory
+before our array (e.g., by creating a tree with a name >2GB,
+since "int" is still 32 bits even on most 64-bit platforms).
+Worse, this is easy to trigger during the fsck_tree() check,
+which is supposed to be protecting us from malicious
+garbage.
+
+Note one bit of trickiness in the existing code: we
+sometimes assign -1 to "len" at the end of the loop, and
+then rely on the "len++" in the for-loop's increment to take
+it back to 0. This is still legal with a size_t, since
+assigning -1 will turn into SIZE_MAX, which then wraps
+around to 0 on increment.
+
+Signed-off-by: Jeff King <peff@peff.net>
+Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
+---
+ path.c | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/path.c b/path.c
+index 4c054037cc..2f12d8eac6 100644
+--- a/path.c
++++ b/path.c
+@@ -1235,7 +1235,7 @@ static int only_spaces_and_periods(const char *path, size_t len, size_t skip)
+ 
+ int is_ntfs_dotgit(const char *name)
+ {
+-	int len;
++	size_t len;
+ 
+ 	for (len = 0; ; len++)
+ 		if (!name[len] || name[len] == '\\' || is_dir_sep(name[len])) {
+-- 
+2.17.0.921.gf22659ad46
+
diff --git a/main/git/CVE-2018-11235.patch b/main/git/CVE-2018-11235.patch
new file mode 100644
index 0000000000..18f00b2781
--- /dev/null
+++ b/main/git/CVE-2018-11235.patch
@@ -0,0 +1,297 @@
+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
+