From 6d8684161ee9c03bed5cb69ae76dfdddb85a0003 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 13 Sep 2019 16:32:43 +0200 Subject: mingw: fix quoting of arguments We need to be careful to follow proper quoting rules. For example, if an argument contains spaces, we have to quote them. Double-quotes need to be escaped. Backslashes need to be escaped, but only if they are followed by a double-quote character. We need to be _extra_ careful to consider the case where an argument ends in a backslash _and_ needs to be quoted: in this case, we append a double-quote character, i.e. the backslash now has to be escaped! The current code, however, fails to recognize that, and therefore can turn an argument that ends in a single backslash into a quoted argument that now ends in an escaped double-quote character. This allows subsequent command-line parameters to be split and part of them being mistaken for command-line options, e.g. through a maliciously-crafted submodule URL during a recursive clone. Technically, we would not need to quote _all_ arguments which end in a backslash _unless_ the argument needs to be quoted anyway. For example, `test\` would not need to be quoted, while `test \` would need to be. To keep the code simple, however, and therefore easier to reason about and ensure its correctness, we now _always_ quote an argument that ends in a backslash. This addresses CVE-2019-1350. Signed-off-by: Johannes Schindelin --- compat/mingw.c | 9 ++++++--- t/t7416-submodule-dash-url.sh | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 3 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/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 -- cgit v1.2.3 From ad1559252945179e28fba7d693494051352810c5 Mon Sep 17 00:00:00 2001 From: Garima Singh Date: Wed, 18 Sep 2019 16:03:59 -0400 Subject: tests: add a helper to stress test argument quoting On Windows, we have to do all the command-line argument quoting ourselves. Worse: we have to have two versions of said quoting, one for MSYS2 programs (which have their own dequoting rules) and the rest. We care mostly about the rest, and to make sure that that works, let's have a stress test that comes up with all kinds of awkward arguments, verifying that a spawned sub-process receives those unharmed. Signed-off-by: Garima Singh Signed-off-by: Johannes Schindelin --- t/helper/test-run-command.c | 118 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 116 insertions(+), 2 deletions(-) diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index d24d157379..6c801cb529 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 -#include +#include "gettext.h" +#include "parse-options.h" static int number_callbacks; static int parallel_next(struct child_process *cp, @@ -49,11 +49,125 @@ 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; + struct strbuf out = STRBUF_INIT; + struct argv_array args = ARGV_ARRAY_INIT; + struct option options[] = { + OPT_INTEGER('n', "trials", &trials, "Number of trials"), + OPT_END() + }; + const char * const usage[] = { + "test-run-command quote-stress-test ", + NULL + }; + + argc = parse_options(argc, argv, NULL, options, usage, 0); + + for (i = 0; i < trials; i++) { + struct child_process cp = CHILD_PROCESS_INIT; + size_t arg_count = 1 + (my_random() % 5), arg_offset; + int ret = 0; + + argv_array_clear(&args); + argv_array_pushl(&args, "test-run-command", + "quote-echo", NULL); + arg_offset = args.argc; + 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); + } + + 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; -- cgit v1.2.3 From 55953c77c0bfcb727ffd7e293e4661b7a24b791b Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 20 Sep 2019 19:09:39 +0200 Subject: quote-stress-test: accept arguments to test via the command-line When the stress test reported a problem with quoting certain arguments, it is helpful to have a facility to play with those arguments in order to find out whether variations of those arguments are affected, too. Let's allow `test-run-command quote-stress-test -- ` to be used for that purpose. Signed-off-by: Johannes Schindelin --- t/helper/test-run-command.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index 6c801cb529..bdbc5ec56a 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -83,25 +83,34 @@ static int quote_stress_test(int argc, const char **argv) for (i = 0; i < trials; i++) { struct child_process cp = CHILD_PROCESS_INIT; - size_t arg_count = 1 + (my_random() % 5), arg_offset; + size_t arg_count, arg_offset; int ret = 0; argv_array_clear(&args); argv_array_pushl(&args, "test-run-command", "quote-echo", NULL); arg_offset = args.argc; - 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 (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); + } } cp.argv = args.argv; -- cgit v1.2.3 From 7530a6287e20a74b9fe6d4ca3a66df0f0f5cc52c Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Thu, 19 Sep 2019 23:46:31 +0200 Subject: quote-stress-test: allow skipping some trials When the, say, 93rd trial run fails, it is a good idea to have a way to skip the first 92 trials and dig directly into the 93rd in a debugger. Signed-off-by: Johannes Schindelin --- t/helper/test-run-command.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index bdbc5ec56a..07989f78ec 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -67,11 +67,12 @@ static int quote_stress_test(int argc, const char **argv) * were passed in. */ char special[] = ".?*\\^_\"'`{}()[]<>@~&+:;$%"; // \t\r\n\a"; - int i, j, k, trials = 100; + int i, j, k, trials = 100, skip = 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 trials"), OPT_END() }; const char * const usage[] = { @@ -113,6 +114,9 @@ static int quote_stress_test(int argc, const char **argv) } } + if (i < skip) + continue; + cp.argv = args.argv; strbuf_reset(&out); if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0) < 0) -- cgit v1.2.3 From 379e51d1ae668a1f26d50eb59b3f8befc1eb8883 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 20 Sep 2019 00:12:37 +0200 Subject: quote-stress-test: offer to test quoting arguments for MSYS2 sh It is unfortunate that we need to quote arguments differently on Windows, depending whether we build a command-line for MSYS2's `sh` or for other Windows executables. We already have a test helper to verify the latter, with this patch we can also verify the former. Signed-off-by: Johannes Schindelin --- t/helper/test-run-command.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index 07989f78ec..b622334407 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -67,12 +67,13 @@ static int quote_stress_test(int argc, const char **argv) * were passed in. */ char special[] = ".?*\\^_\"'`{}()[]<>@~&+:;$%"; // \t\r\n\a"; - int i, j, k, trials = 100, skip = 0; + 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 trials"), + OPT_BOOL('m', "msys2", &msys2, "Test quoting for MSYS2's sh"), OPT_END() }; const char * const usage[] = { @@ -82,14 +83,20 @@ static int quote_stress_test(int argc, const char **argv) 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); - argv_array_pushl(&args, "test-run-command", - "quote-echo", NULL); + 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) { -- cgit v1.2.3