{"thread":{"id":"47831","subject":"[PATCH] sq_dequote: fix extra consumption of source string","startedAt":"2018-02-13T23:41:56Z","lastAt":"2018-02-19T03:45:48Z","messageCount":2,"participants":["Jeff King","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"339241","messageId":"20180213234149.GA21964@sigill.intra.peff.net","threadId":"47831","inReplyTo":null,"subject":"[PATCH] sq_dequote: fix extra consumption of source string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-13T23:41:49Z","receivedAt":"2018-02-13T23:41:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This fixes a (probably harmless) parsing problem in\nsq_dequote_step(), in which we parse some bogus input\nincorrectly rather than complaining that it's bogus.\n\nOur shell-dequoting function is very strict: it can unquote\neverything generated by sq_quote(), but not arbitrary\nstrings. In particular, it only allows characters outside of\nthe single-quoted string if they are immediately backslashed\nand then the single-quoted string is resumed. So:\n\n  'foo'\\''bar'\n\nis OK. But these are not:\n\n  'foo'\\'bar\n  'foo'\\'\n  'foo'\\'\\''bar'\n\neven though they are all valid shell. The parser has a funny\ncorner case here. When we see a backslashed character, we\nkeep incrementing the \"src\" pointer as we parse it. For a\nsingle sq_dequote() call, that's OK; our next step is to\nbail with an error, and we don't care where \"src\" points.\n\nBut if we're parsing multiple strings with sq_dequote_to_argv(),\nthen our next step is to see if the string is followed by\nwhitespace. Because we erroneously incremented the \"src\"\npointer, we don't barf on the bogus backslash that we\nskipped. Instead, we may find whitespace that immediately\nfollows it, and continue as if all is well (skipping the\nbackslashed character completely!).\n\nIn practice, this shouldn't be a big deal. The input is\nbogus, and our sq_quote() would never generate this bogus\ninput. In all but one callers, we are parsing input created\nby an earlier call to sq_quote(). That final case is \"git\nshell\", which parses shell-quoting generated by the client.\nAnd in that case we use the singular sq_quote(), which has\nalways behaved correctly.\n\nOne might also wonder if you could provoke a read past the\nend of the string. But the answer is no; we still parse\ncharacter by character, and would never advance past a NUL.\n\nThis patch implements the minimal fix, along with\ndocumenting the restriction (which confused at least me\nwhile reading the code). We should possibly consider\nbeing more liberal in accepting valid shell-quoted words. I\nsuspect the code may actually be simpler, and it would be\nmore friendly to anybody generating or editing input by\nhand. But I wanted to fix just the immediate bug in this\npatch.\n\nWe don't have a direct way to unit-test the sq_dequote()\nfunctions, but we can do this by feeding input to\nGIT_CONFIG_PARAMETERS (which is not normally a user-facing\ninterface, but serves here as it expects to see sq_quote()\ninput from \"git -c\"). I've included both a bogus example,\nand a related \"good\" one to confirm that we still parse it\ncorrectly.\n\nNoticed-by: Michael Haggerty <mhagger@alum.mit.edu>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nPhew. That was a lot of explanation for a tiny bug. But it\nreally took me a while to convince myself that there were no\nother lurking problems.\n\n quote.c                | 12 +++++++++---\n t/t1300-repo-config.sh | 23 +++++++++++++++++++++++\n 2 files changed, 32 insertions(+), 3 deletions(-)\n\ndiff --git a/quote.c b/quote.c\nindex de2922ddd6..44f47bd3dc 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -94,9 +94,15 @@ static char *sq_dequote_step(char *arg, char **next)\n \t\t\t\t*next = NULL;\n \t\t\treturn arg;\n \t\tcase '\\\\':\n-\t\t\tc = *++src;\n-\t\t\tif (need_bs_quote(c) && *++src == '\\'') {\n-\t\t\t\t*dst++ = c;\n+\t\t\t/*\n+\t\t\t * Allow backslashed characters outside of\n+\t\t\t * single-quotes only if they need escaping,\n+\t\t\t * and only if we resume the single-quoted part\n+\t\t\t * afterward.\n+\t\t\t */\n+\t\t\tif (need_bs_quote(src[1]) && src[2] == '\\'') {\n+\t\t\t\t*dst++ = src[1];\n+\t\t\t\tsrc += 2;\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t/* Fallthrough */\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex cbeb9bebee..4f8e6f5fde 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1206,6 +1206,29 @@ test_expect_success 'git -c is not confused by empty environment' '\n \tGIT_CONFIG_PARAMETERS=\"\" git -c x.one=1 config --list\n '\n \n+sq=\"'\"\n+test_expect_success 'detect bogus GIT_CONFIG_PARAMETERS' '\n+\tcat >expect <<-\\EOF &&\n+\tenv.one one\n+\tenv.two two\n+\tEOF\n+\tGIT_CONFIG_PARAMETERS=\"${sq}env.one=one${sq} ${sq}env.two=two${sq}\" \\\n+\t\tgit config --get-regexp \"env.*\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-EOF &&\n+\tenv.one one${sq}\n+\tenv.two two\n+\tEOF\n+\tGIT_CONFIG_PARAMETERS=\"${sq}env.one=one${sq}\\\\$sq$sq$sq ${sq}env.two=two${sq}\" \\\n+\t\tgit config --get-regexp \"env.*\" >actual &&\n+\ttest_cmp expect actual &&\n+\n+\ttest_must_fail env \\\n+\t\tGIT_CONFIG_PARAMETERS=\"${sq}env.one=one${sq}\\\\$sq ${sq}env.two=two${sq}\" \\\n+\t\tgit config --get-regexp \"env.*\"\n+'\n+\n test_expect_success 'git config --edit works' '\n \tgit config -f tmp test.value no &&\n \techo test.value=yes >expect &&\n-- \n2.16.1.464.gc4bae515b7\n"},{"id":"339597","messageId":"CAMy9T_Ho6cypqgtp9gnZtbgnO-F778pe2B3qAbu1XoTBQw9xLQ@mail.gmail.com","threadId":"47831","inReplyTo":"20180213234149.GA21964@sigill.intra.peff.net","subject":"Re: [PATCH] sq_dequote: fix extra consumption of source string","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2018-02-19T03:45:38Z","receivedAt":"2018-02-19T03:45:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On Wed, Feb 14, 2018 at 12:41 AM, Jeff King <peff@peff.net> wrote:\n> This fixes a (probably harmless) parsing problem in\n> sq_dequote_step(), in which we parse some bogus input\n> incorrectly rather than complaining that it's bogus.\n> [...]\n\nLGTM. Thanks for taking care of this.\n\nMichael\n"}]}