Merge branch 'fix-mingw-quoting-bug'

This patch fixes a vulnerability in the Windows-specific code where a
submodule names ending in a backslash were quoted incorrectly, and that
bug could be abused to insert command-line parameters e.g. to `ssh` in a
recursive clone.

Note: this bug is Windows-only, as we have to construct a command line
for the process-to-spawn, unlike Linux/macOS, where `execv()` accepts an
already-split command line.

While at it, other quoting issues are fixed as well.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

3 files changed, 156 insertions, 5 deletions

diff --git a/compat/mingw.c b/compat/mingw.c
index 8b6fa0db44..459ee20df6 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -872,7 +872,7 @@ static const char *quote_arg(const char *arg)
 				p++;
 				len++;
 			}
-			if (*p == '"')
+			if (*p == '"' || !*p)
 				n += count*2 + 1;
 			continue;
 		}
@@ -894,16 +894,19 @@ static const char *quote_arg(const char *arg)
 				count++;
 				*d++ = *arg++;
 			}
-			if (*arg == '"') {
+			if (*arg == '"' || !*arg) {
 				while (count-- > 0)
 					*d++ = '\\';
+				/* don't escape the surrounding end quote */
+				if (!*arg)
+					break;
 				*d++ = '\\';
 			}
 		}
 		*d++ = *arg++;
 	}
 	*d++ = '"';
-	*d++ = 0;
+	*d++ = '\0';
 	return q;
 }
 
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index d24d157379..b622334407 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -12,8 +12,8 @@
 #include "run-command.h"
 #include "argv-array.h"
 #include "strbuf.h"
-#include <string.h>
-#include <errno.h>
+#include "gettext.h"
+#include "parse-options.h"
 
 static int number_callbacks;
 static int parallel_next(struct child_process *cp,
@@ -49,11 +49,145 @@ static int task_finished(int result,
 	return 1;
 }
 
+static uint64_t my_random_next = 1234;
+
+static uint64_t my_random(void)
+{
+	uint64_t res = my_random_next;
+	my_random_next = my_random_next * 1103515245 + 12345;
+	return res;
+}
+
+static int quote_stress_test(int argc, const char **argv)
+{
+	/*
+	 * We are running a quote-stress test.
+	 * spawn a subprocess that runs quote-stress with a
+	 * special option that echoes back the arguments that
+	 * were passed in.
+	 */
+	char special[] = ".?*\\^_\"'`{}()[]<>@~&+:;$%"; // \t\r\n\a";
+	int i, j, k, trials = 100, skip = 0, msys2 = 0;
+	struct strbuf out = STRBUF_INIT;
+	struct argv_array args = ARGV_ARRAY_INIT;
+	struct option options[] = {
+		OPT_INTEGER('n', "trials", &trials, "Number of trials"),
+		OPT_INTEGER('s', "skip", &skip, "Skip <n> trials"),
+		OPT_BOOL('m', "msys2", &msys2, "Test quoting for MSYS2's sh"),
+		OPT_END()
+	};
+	const char * const usage[] = {
+		"test-run-command quote-stress-test <options>",
+		NULL
+	};
+
+	argc = parse_options(argc, argv, NULL, options, usage, 0);
+
+	setenv("MSYS_NO_PATHCONV", "1", 0);
+
+	for (i = 0; i < trials; i++) {
+		struct child_process cp = CHILD_PROCESS_INIT;
+		size_t arg_count, arg_offset;
+		int ret = 0;
+
+		argv_array_clear(&args);
+		if (msys2)
+			argv_array_pushl(&args, "sh", "-c",
+					 "printf %s\\\\0 \"$@\"", "skip", NULL);
+		else
+			argv_array_pushl(&args, "test-run-command",
+					 "quote-echo", NULL);
+		arg_offset = args.argc;
+
+		if (argc > 0) {
+			trials = 1;
+			arg_count = argc;
+			for (j = 0; j < arg_count; j++)
+				argv_array_push(&args, argv[j]);
+		} else {
+			arg_count = 1 + (my_random() % 5);
+			for (j = 0; j < arg_count; j++) {
+				char buf[20];
+				size_t min_len = 1;
+				size_t arg_len = min_len +
+					(my_random() % (ARRAY_SIZE(buf) - min_len));
+
+				for (k = 0; k < arg_len; k++)
+					buf[k] = special[my_random() %
+						ARRAY_SIZE(special)];
+				buf[arg_len] = '\0';
+
+				argv_array_push(&args, buf);
+			}
+		}
+
+		if (i < skip)
+			continue;
+
+		cp.argv = args.argv;
+		strbuf_reset(&out);
+		if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0) < 0)
+			return error("Failed to spawn child process");
+
+		for (j = 0, k = 0; j < arg_count; j++) {
+			const char *arg = args.argv[j + arg_offset];
+
+			if (strcmp(arg, out.buf + k))
+				ret = error("incorrectly quoted arg: '%s', "
+					    "echoed back as '%s'",
+					     arg, out.buf + k);
+			k += strlen(out.buf + k) + 1;
+		}
+
+		if (k != out.len)
+			ret = error("got %d bytes, but consumed only %d",
+				     (int)out.len, (int)k);
+
+		if (ret) {
+			fprintf(stderr, "Trial #%d failed. Arguments:\n", i);
+			for (j = 0; j < arg_count; j++)
+				fprintf(stderr, "arg #%d: '%s'\n",
+					(int)j, args.argv[j + arg_offset]);
+
+			strbuf_release(&out);
+			argv_array_clear(&args);
+
+			return ret;
+		}
+
+		if (i && (i % 100) == 0)
+			fprintf(stderr, "Trials completed: %d\n", (int)i);
+	}
+
+	strbuf_release(&out);
+	argv_array_clear(&args);
+
+	return 0;
+}
+
+static int quote_echo(int argc, const char **argv)
+{
+	while (argc > 1) {
+		fwrite(argv[1], strlen(argv[1]), 1, stdout);
+		fputc('\0', stdout);
+		argv++;
+		argc--;
+	}
+
+	return 0;
+}
+
 int cmd_main(int argc, const char **argv)
 {
 	struct child_process proc = CHILD_PROCESS_INIT;
 	int jobs;
 
+	if (argc >= 2 && !strcmp(argv[1], "quote-stress-test"))
+		return !!quote_stress_test(argc - 1, argv + 1);
+
+	if (argc >= 2 && !strcmp(argv[1], "quote-echo"))
+		return !!quote_echo(argc - 1, argv + 1);
+
 	if (argc < 3)
 		return 1;
 	proc.argv = (const char **)argv + 2;
diff --git a/t/t7416-submodule-dash-url.sh b/t/t7416-submodule-dash-url.sh
index 459193c976..2966e93071 100755
--- a/t/t7416-submodule-dash-url.sh
+++ b/t/t7416-submodule-dash-url.sh
@@ -31,4 +31,18 @@ test_expect_success 'clone rejects unprotected dash' '
 	test_i18ngrep ignoring err
 '
 
+test_expect_success 'trailing backslash is handled correctly' '
+	git init testmodule &&
+	test_commit -C testmodule c &&
+	git submodule add ./testmodule &&
+	: ensure that the name ends in a double backslash &&
+	sed -e "s|\\(submodule \"testmodule\\)\"|\\1\\\\\\\\\"|" \
+		-e "s|url = .*|url = \" --should-not-be-an-option\"|" \
+		<.gitmodules >.new &&
+	mv .new .gitmodules &&
+	git commit -am "Add testmodule" &&
+	test_must_fail git clone --verbose --recurse-submodules . dolly 2>err &&
+	test_i18ngrep ! "unknown option" err
+'
+
 test_done