| From e7552c07af3edb76594f4ad9ec45559d6cf47612 Mon Sep 17 00:00:00 2001 |
| From: Johannes Schindelin <johannes.schindelin@gmx.de> |
| 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 <johannes.schindelin@gmx.de> |
| (cherry picked from commit 6d8684161ee9c03bed5cb69ae76dfdddb85a0003) |
| Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> |
| --- |
| 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 34b3880b29..94b0746f2f 100644 |
| --- a/compat/mingw.c |
| +++ b/compat/mingw.c |
| @@ -1051,7 +1051,7 @@ static const char *quote_arg(const char *arg) |
| p++; |
| len++; |
| } |
| - if (*p == '"') |
| + if (*p == '"' || !*p) |
| n += count*2 + 1; |
| continue; |
| } |
| @@ -1073,16 +1073,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 1cd2c1c1ea..5ba041f537 100755 |
| --- a/t/t7416-submodule-dash-url.sh |
| +++ b/t/t7416-submodule-dash-url.sh |
| @@ -46,4 +46,18 @@ test_expect_success 'fsck rejects unprotected dash' ' |
| grep gitmodulesUrl 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 |
| -- |
| 2.24.0.393.g34dc348eaf |
| |