From d819a25360ba38dfec31e37413963adf5688db80 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Mon, 24 Sep 2018 04:32:15 -0400
Subject: [PATCH 1/2] submodule--helper: use "--" to signal end of clone
 options

commit 98afac7a7cefdca0d2c4917dd8066a59f7088265 upstream.

When we clone a submodule, we call "git clone $url $path".
But there's nothing to say that those components can't begin
with a dash themselves, confusing git-clone into thinking
they're options. Let's pass "--" to make it clear what we
expect.

There's no test here, because it's actually quite hard to
make these names work, even with "git clone" parsing them
correctly. And we're going to restrict these cases even
further in future commits. So we'll leave off testing until
then; this is just the minimal fix to prevent us from doing
something stupid with a badly formed entry.

[jn: backported to 2.1.y by applying to git-submodule.sh
 instead of submodule--helper]

Reported-by: joernchen <joernchen@phenoelit.de>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

submodule-config: ban submodule urls that start with dash

commit f6adec4e329ef0e25e14c63b735a5956dc67b8bc upstream.

The previous commit taught the submodule code to invoke our
"git clone $url $path" with a "--" separator so that we
aren't confused by urls or paths that start with dashes.

However, that's just one code path. It's not clear if there
are others, and it would be an easy mistake to add one in
the future. Moreover, even with the fix in the previous
commit, it's quite hard to actually do anything useful with
such an entry. Any url starting with a dash must fall into
one of three categories:

 - it's meant as a file url, like "-path". But then any
   clone is not going to have the matching path, since it's
   by definition relative inside the newly created clone. If
   you spell it as "./-path", the submodule code sees the
   "/" and translates this to an absolute path, so it at
   least works (assuming the receiver has the same
   filesystem layout as you). But that trick does not apply
   for a bare "-path".

 - it's meant as an ssh url, like "-host:path". But this
   already doesn't work, as we explicitly disallow ssh
   hostnames that begin with a dash (to avoid option
   injection against ssh).

 - it's a remote-helper scheme, like "-scheme::data". This
   _could_ work if the receiver bends over backwards and
   creates a funny-named helper like "git-remote--scheme".
   But normally there would not be any helper that matches.

Since such a url does not work today and is not likely to do
anything useful in the future, let's simply disallow them
entirely. That protects the existing "git clone" path (in a
belt-and-suspenders way), along with any others that might
exist.

[jn: backported to 2.1.y by porting to shell]
[pc: backported to 1.8.3.1 by using $sm_path instead of $displayname
 and split tests into a separate commit]

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

submodule-config: ban submodule paths that start with a dash

commit 273c61496f88c6495b886acb1041fe57965151da upstream.

We recently banned submodule urls that look like
command-line options. This is the matching change to ban
leading-dash paths.

As with the urls, this should not break any use cases that
currently work. Even with our "--" separator passed to
git-clone, git-submodule.sh gets confused. Without the code
portion of this patch, the clone of "-sub" added in t7417
would yield results like:

    /path/to/git-submodule: 410: cd: Illegal option -s
    /path/to/git-submodule: 417: cd: Illegal option -s
    /path/to/git-submodule: 410: cd: Illegal option -s
    /path/to/git-submodule: 417: cd: Illegal option -s
    Fetched in submodule path '-sub', but it did not contain b56243f8f4eb91b2f1f8109452e659f14dd3fbe4. D
irect fetching of that commit failed.

Moreover, naively adding such a submodule doesn't work:

  $ git submodule add $url -sub
  The following path is ignored by one of your .gitignore files:
  -sub

even though there is no such ignore pattern (the test script
hacks around this with a well-placed "git mv").

Unlike leading-dash urls, though, it's possible that such a
path _could_ be useful if we eventually made it work. So
this commit should be seen not as recommending a particular
policy, but rather temporarily closing off a broken and
possibly dangerous code-path. We may revisit this decision
later.

[jn: ported to git-submodule.sh
 pc: split the test into a separate commit ]

fsck: detect submodule urls starting with dash

commit a124133e1e6ab5c7a9fef6d0e6bcb084e3455b46 upstream.

Urls with leading dashes can cause mischief on older
versions of Git. We should detect them so that they can be
rejected by receive.fsckObjects, preventing modern versions
of git from being a vector by which attacks can spread.

[jn: backported to 2.1.y: using error_func instead of report
 to report fsck errors]

[pc: split tests into a separate commit]

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

fsck: detect submodule paths starting with dash

commit 1a7fd1fb2998002da6e9ff2ee46e1bdd25ee8404 upstream.

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.

[jn: backported to 2.1.y: using error_func instead of report
 to report fsck errors]

[pc: split test to a separate commit]
---
 fsck.c           | 10 ++++++++++
 git-submodule.sh | 20 +++++++++++++++-----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/fsck.c b/fsck.c
index 811724125..90d641066 100644
--- a/fsck.c
+++ b/fsck.c
@@ -442,6 +442,16 @@ static int fsck_gitmodules_fn(const char *var, const char *value, void *vdata)
 		data->ret += data->error_func(data->obj, FSCK_ERROR,
 					      "disallowed submodule name: %s",
 					      name);
+	if (!strcmp(key, "url") && value &&
+	    looks_like_command_line_option(value))
+		data->ret += data->error_func(data->obj, FSCK_ERROR,
+					      "disallowed submodule url: %s",
+					      value);
+	if (!strcmp(key, "path") && value &&
+	    looks_like_command_line_option(value))
+		data->ret += data->error_func(data->obj, FSCK_ERROR,
+					      "disallowed submodule path: %s",
+					      value);
 	free(name);
 
 	return 0;
diff --git a/git-submodule.sh b/git-submodule.sh
index e958ce840..b5176ecc3 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -205,6 +205,11 @@ module_name()
 	re=$(printf '%s\n' "$1" | sed -e 's/[].[^$\\*]/\\&/g')
 	name=$( git config -f .gitmodules --get-regexp '^submodule\..*\.path$' |
 		sed -n -e 's|^submodule\.\(.*\)\.path '"$re"'$|\1|p' )
+	case "$sm_path" in
+	-*)
+		die "$(eval_gettext "Submodule path '\$sm_path' may be interpreted as a command-line option")"
+		;;
+	esac
 	test -z "$name" &&
 	die "$(eval_gettext "No submodule mapping found in .gitmodules for path '\$sm_path'")"
 	check_module_name "$name"
@@ -248,7 +253,7 @@ module_clone()
 		(
 			clear_local_git_env
 			git clone $quiet -n ${reference:+"$reference"} \
-				--separate-git-dir "$gitdir" "$url" "$sm_path"
+				--separate-git-dir "$gitdir" -- "$url" "$sm_path"
 		) ||
 		die "$(eval_gettext "Clone of '\$url' into submodule path '\$sm_path' failed")"
 	fi
@@ -547,11 +552,13 @@ cmd_init()
 		if test -z "$(git config "submodule.$name.url")"
 		then
 			url=$(git config -f .gitmodules submodule."$name".url)
-			test -z "$url" &&
-			die "$(eval_gettext "No url found for submodule path '\$sm_path' in .gitmodules")"
-
-			# Possibly a url relative to parent
 			case "$url" in
+			"")
+				die "$(eval_gettext "No url found for submodule path '\$sm_path' in .gitmodules")"
+				;;
+			-*)
+				die "$(eval_gettext "Submodule at path '\$sm_path' has url '\$url' which may be interpreted as a command-line option")"
+				;;
 			./*|../*)
 				url=$(resolve_relative_url "$url") || exit
 				;;
@@ -1213,6 +1220,9 @@ cmd_sync()
 
 		# Possibly a url relative to parent
 		case "$url" in
+		-*)
+			die "$(eval_gettext "Submodule at path '\$sm_path' has url '\$url' which may be interpreted as a command-line option")"
+			;;
 		./*|../*)
 			# rewrite foo/bar as ../.. to find path from
 			# submodule work tree to superproject work tree
-- 
2.14.4