CVE-2018-17456: Improper Neutralization of Argument Delimiters in a Command ('Argument Injection') (CWE-88)
value: Git before 2.14.5, 2.15.x before 2.15.3, 2.16.x before 2.16.5, 2.17.x before 2.17.2, 2.18.x before 2.18.1, and 2.19.x before 2.19.1 allows remote code execution during processing of a recursive "git clone" of a superproject if a .gitmodules file has a URL field beginning with a '-' character.
CWE description: The product constructs a string for a command to be executed by a separate component
in another control sphere, but it does not properly delimit the
intended arguments, options, or switches within that command string.
CVSS v3 base score: 9.8
Published: 2018-10-06T14:29Z
Repository: https://github.com/git/git
Language: C
Files changed: fsck.c, t/t7417-submodule-path-url.sh
Commit date: 2018-09-27 11:41:31 -0700
Commit message: fsck: detect submodule paths starting with dash

As with urls, submodule paths with dashes are ignored by
git, but may end up confusing older versions. Detecting them
via fsck lets us prevent modern versions of git from being a
vector to spread broken .gitmodules to older versions.

Compared to blocking leading-dash urls, though, this
detection may be less of a good idea:

  1. While such paths provide confusing and broken results,
     they don't seem to actually work as option injections
     against anything except "cd". In particular, the
     submodule code seems to canonicalize to an absolute
     path before running "git clone" (so it passes
     /your/clone/-sub).

  2. It's more likely that we may one day make such names
     actually work correctly. Even after we revert this fsck
     check, it will continue to be a hassle until hosting
     servers are all updated.

On the other hand, it's not entirely clear that the behavior
in older versions is safe. And if we do want to eventually
allow this, we may end up doing so with a special syntax
anyway (e.g., writing "./-sub" in the .gitmodules file, and
teaching the submodule code to canonicalize it when
comparing).

So on balance, this is probably a good protection.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

Diff: diff --git a/fsck.c b/fsck.c
index c472856527..5d9b25fac0 100644
--- a/fsck.c
+++ b/fsck.c
@@ -63,10 +63,11 @@ static struct oidset gitmodules_done = OIDSET_INIT;
 	FUNC(GITMODULES_BLOB, ERROR) \
 	FUNC(GITMODULES_PARSE, ERROR) \
 	FUNC(GITMODULES_NAME, ERROR) \
 	FUNC(GITMODULES_SYMLINK, ERROR) \
 	FUNC(GITMODULES_URL, ERROR) \
+	FUNC(GITMODULES_PATH, ERROR) \
 	/* warnings */ \
 	FUNC(BAD_FILEMODE, WARN) \
 	FUNC(EMPTY_NAME, WARN) \
 	FUNC(FULL_PATHNAME, WARN) \
 	FUNC(HAS_DOT, WARN) \
@@ -950,10 +951,16 @@ static int fsck_gitmodules_fn(const char *var, const char *value, void *vdata)
 	    looks_like_command_line_option(value))
 		data->ret |= report(data->options, data->obj,
 				    FSCK_MSG_GITMODULES_URL,
 				    "disallowed submodule url: %s",
 				    value);
+	if (!strcmp(key, "path") && value &&
+	    looks_like_command_line_option(value))
+		data->ret |= report(data->options, data->obj,
+				    FSCK_MSG_GITMODULES_PATH,
+				    "disallowed submodule path: %s",
+				    value);
 	free(name);
 
 	return 0;
 }
 
diff --git a/t/t7417-submodule-path-url.sh b/t/t7417-submodule-path-url.sh
index 638293f0da..756af8c4d6 100755
--- a/t/t7417-submodule-path-url.sh
+++ b/t/t7417-submodule-path-url.sh
@@ -15,6 +15,14 @@ test_expect_success 'clone rejects unprotected dash' '
 	test_when_finished "rm -rf dst" &&
 	git clone --recurse-submodules . dst 2>err &&
 	test_i18ngrep ignoring err
 '
 
+test_expect_success 'fsck rejects unprotected dash' '
+	test_when_finished "rm -rf dst" &&
+	git init --bare dst &&
+	git -C dst config transfer.fsckObjects true &&
+	test_must_fail git push dst HEAD 2>err &&
+	grep gitmodulesPath err
+'
+
 test_done

Fixed code: 	FUNC(GITMODULES_PATH, ERROR) \
	if (!strcmp(key, "path") && value &&
	    looks_like_command_line_option(value))
		data->ret |= report(data->options, data->obj,
				    FSCK_MSG_GITMODULES_PATH,
				    "disallowed submodule path: %s",
				    value);
test_expect_success 'fsck rejects unprotected dash' '
	test_when_finished "rm -rf dst" &&
	git init --bare dst &&
	git -C dst config transfer.fsckObjects true &&
	test_must_fail git push dst HEAD 2>err &&
	grep gitmodulesPath err
'