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 <johannes.schindelin@gmx.de>

Johannes Schindelin committed Sep 13, 2019 at 16:32 UTC 6d8684161ee9c03bed5cb69ae76dfdddb85a0003

2 files changed +20 -3

compat/mingw.c

+6 -3

@@ -872,7 +872,7 @@ static const char *quote_arg(const char *arg)

872 p++;

873 len++;

874 }

875 - if (*p == '"')

875 + if (*p == '"' || !*p)

876 n += count*2 + 1;

877 continue;

878 }

@@ -894,16 +894,19 @@ static const char *quote_arg(const char *arg)

894 count++;

895 *d++ = *arg++;

896 }

897 - if (*arg == '"') {

897 + if (*arg == '"' || !*arg) {

898 while (count-- > 0)

899 *d++ = '\\';

900 + /* don't escape the surrounding end quote */

901 + if (!*arg)

902 + break;

903 *d++ = '\\';

904 }

905 }

906 *d++ = *arg++;

907 }

908 *d++ = '"';

906 - *d++ = 0;

909 + *d++ = '\0';

910 return q;

911 }

912

t/t7416-submodule-dash-url.sh

+14

@@ -31,4 +31,18 @@ test_expect_success 'clone rejects unprotected dash' '

31 test_i18ngrep ignoring err

32 '

34 +test_expect_success 'trailing backslash is handled correctly' '

35 + git init testmodule &&

36 + test_commit -C testmodule c &&

37 + git submodule add ./testmodule &&

38 + : ensure that the name ends in a double backslash &&

39 + sed -e "s|\\(submodule \"testmodule\\)\"|\\1\\\\\\\\\"|" \

40 + -e "s|url = .*|url = \" --should-not-be-an-option\"|" \

41 + <.gitmodules >.new &&

42 + mv .new .gitmodules &&

43 + git commit -am "Add testmodule" &&

44 + test_must_fail git clone --verbose --recurse-submodules . dolly 2>err &&

45 + test_i18ngrep ! "unknown option" err

46 +'

47 +

48 test_done