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>

diff --git a/compat/mingw.c b/compat/mingw.c
index 8b6fa0db446aee9888ff6484560131143c7e22e5..459ee20df66ea844c60ba68c9a3d0e3b439e1290 100644 (file)
--- 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 459193c9765063f341c0aa17ee567073cd1b59ff..2966e9307199beb04c3a61933ad88a1d3e6bcb5b 100755 (executable)
--- 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