{"thread":{"id":"59351","subject":"[PATCH] sequencer.c: fix overflow & segfault in parse_strategy_opts()","startedAt":"2023-03-07T18:29:33Z","lastAt":"2023-03-08T16:20:28Z","messageCount":4,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"473144","messageId":"patch-1.1-f6a06e25cf3-20230307T182039Z-avarab@gmail.com","threadId":"59351","inReplyTo":null,"subject":"[PATCH] sequencer.c: fix overflow & segfault in parse_strategy_opts()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-03-07T18:21:59Z","receivedAt":"2023-03-07T18:29:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The split_cmdline() function introduced in [1] returns an \"int\". If\nit's negative it signifies an error. The option parsing in [2] didn't\naccount for this, and assigned the value directly to the \"size_t\nxopts_nr\". We'd then attempt to loop over all of these elements, and\naccess uninitialized memory.\n\nThere's a few things that use this for option parsing, but one way to\ntrigger it is with a bad value to \"-X <strategy-option>\", e.g:\n\n\tgit rebase -X\"bad argument\\\"\"\n\nIn another context this might be a security issue, but in this case\nsomeone who's already able to inject arguments directly to our\ncommands would be past other defenses, making this potential\nescalation a moot point.\n\nAs the example above & test case shows the error reporting leaves\nsomething to be desired. The function will loop over the\nwhitespace-split values, but when it encounters an error we'll only\nreport the first element, which is OK, not the second \"argument\\\"\"\nwhose quote is unbalanced.\n\nThis is an inherent limitation of the current API, and the issue\naffects other API users. Let's not attempt to fix that now. If and\nwhen that happens these tests will need to be adjusted to assert the\nnew output.\n\n1. 2b11e3170e9 (If you have a config containing something like this:,\n   2006-06-05)\n2. ca6c6b45dd9 (sequencer (rebase -i): respect strategy/strategy_opts\n   settings, 2017-01-02)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nCI & branch for this at\nhttps://github.com/avar/git/tree/avar/sequencer-xopts-nr-overflow\n\nNot a new issue, but I figured with other discussions in this area\nkicking this out the door sooner than later was better.\n\n sequencer.c                    |  9 +++++++--\n t/t3436-rebase-more-options.sh | 18 ++++++++++++++++++\n 2 files changed, 25 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3e4a1972897..79c615193b6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2876,13 +2876,18 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n {\n \tint i;\n+\tint count;\n \tchar *strategy_opts_string = raw_opts;\n \n \tif (*strategy_opts_string == ' ')\n \t\tstrategy_opts_string++;\n \n-\topts->xopts_nr = split_cmdline(strategy_opts_string,\n-\t\t\t\t       (const char ***)&opts->xopts);\n+\tcount = split_cmdline(strategy_opts_string,\n+\t\t\t      (const char ***)&opts->xopts);\n+\tif (count < 0)\n+\t\tdie(_(\"could not split '%s': '%s'\"), strategy_opts_string,\n+\t\t\t    split_cmdline_strerror(count));\n+\topts->xopts_nr = count;\n \tfor (i = 0; i < opts->xopts_nr; i++) {\n \t\tconst char *arg = opts->xopts[i];\n \ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex 94671d3c465..195ace34559 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -40,6 +40,24 @@ test_expect_success 'setup' '\n \tEOF\n '\n \n+test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' '\n+\tcat >expect <<-\\EOF &&\n+\tfatal: could not split '\\''--bad'\\'': '\\''unclosed quote'\\''\n+\tEOF\n+\ttest_expect_code 128 git rebase -X\"bad argument\\\"\" side main >out 2>actual &&\n+\ttest_must_be_empty out &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'bad -X <strategy-option> arguments: bad escape' '\n+\tcat >expect <<-\\EOF &&\n+\tfatal: could not split '\\''--bad'\\'': '\\''cmdline ends with \\'\\''\n+\tEOF\n+\ttest_expect_code 128 git rebase -X\"bad escape \\\\\" side main >out 2>actual &&\n+\ttest_must_be_empty out &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--ignore-whitespace works with apply backend' '\n \ttest_must_fail git rebase --apply main side &&\n \tgit rebase --abort &&\n-- \n2.40.0.rc1.1034.g5867a1b10c5\n\n"},{"id":"473150","messageId":"xmqq5ybcxs1r.fsf@gitster.g","threadId":"59351","inReplyTo":"patch-1.1-f6a06e25cf3-20230307T182039Z-avarab@gmail.com","subject":"Re: [PATCH] sequencer.c: fix overflow & segfault in parse_strategy_opts()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-07T19:47:12Z","receivedAt":"2023-03-07T19:55:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> There's a few things that use this for option parsing, but one way to\n> trigger it is with a bad value to \"-X <strategy-option>\", e.g:\n>\n> \tgit rebase -X\"bad argument\\\"\"\n\nWow, that is nasty ;-).\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 3e4a1972897..79c615193b6 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2876,13 +2876,18 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n>  void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n>  {\n>  \tint i;\n> +\tint count;\n>  \tchar *strategy_opts_string = raw_opts;\n>  \n>  \tif (*strategy_opts_string == ' ')\n>  \t\tstrategy_opts_string++;\n>  \n> -\topts->xopts_nr = split_cmdline(strategy_opts_string,\n> -\t\t\t\t       (const char ***)&opts->xopts);\n> +\tcount = split_cmdline(strategy_opts_string,\n> +\t\t\t      (const char ***)&opts->xopts);\n> +\tif (count < 0)\n> +\t\tdie(_(\"could not split '%s': '%s'\"), strategy_opts_string,\n> +\t\t\t    split_cmdline_strerror(count));\n\nThis made me look at split_cmdline_strerror().  It is a table lookup\ninto split_cmdline_errors[] in alias.c which looks like this:\n\n    static const char *split_cmdline_errors[] = {\n            N_(\"cmdline ends with \\\\\"),\n            N_(\"unclosed quote\"),\n            N_(\"too many arguments\"),\n    };\n\nSo the result is properly localized, but I suspect that the string\nafter : should not be enclosed within a pair of single quotes.\n\n\tdie(_(\"could not split '%s': %s\", strategy_opts_string,\n\t\tsplit_cmdline_strerror(count)));\n\nOther than that, nice find.\n\nThanks.\n"},{"id":"473159","messageId":"xmqqttywuowi.fsf@gitster.g","threadId":"59351","inReplyTo":"xmqq5ybcxs1r.fsf@gitster.g","subject":"Re: [PATCH] sequencer.c: fix overflow & segfault in parse_strategy_opts()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-07T23:23:25Z","receivedAt":"2023-03-07T23:23:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> This made me look at split_cmdline_strerror().  It is a table lookup\n> into split_cmdline_errors[] in alias.c which looks like this:\n>\n>     static const char *split_cmdline_errors[] = {\n>             N_(\"cmdline ends with \\\\\"),\n>             N_(\"unclosed quote\"),\n>             N_(\"too many arguments\"),\n>     };\n>\n> So the result is properly localized, but I suspect that the string\n> after : should not be enclosed within a pair of single quotes.\n>\n> \tdie(_(\"could not split '%s': %s\", strategy_opts_string,\n> \t\tsplit_cmdline_strerror(count)));\n>\n> Other than that, nice find.\n\nI'll queue this on top.\n\n----- >8 ---------- >8 ---------- >8 ---------- >8 -----\nSubject: [PATCH] SQUASH: no point in quoting strerror like messages\n\nThe error message is taken from a limited and fixed set of strings\nand we do not usually enclose strerror(errno) inside a pair of\nsingle quotes.\n---\n sequencer.c                    | 2 +-\n t/t3436-rebase-more-options.sh | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex cc59a1c491..e4a3f0081f 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2885,7 +2885,7 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n \tcount = split_cmdline(strategy_opts_string,\n \t\t\t      (const char ***)&opts->xopts);\n \tif (count < 0)\n-\t\tdie(_(\"could not split '%s': '%s'\"), strategy_opts_string,\n+\t\tdie(_(\"could not split '%s': %s\"), strategy_opts_string,\n \t\t\t    split_cmdline_strerror(count));\n \topts->xopts_nr = count;\n \tfor (i = 0; i < opts->xopts_nr; i++) {\ndiff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\nindex 195ace3455..c3184c9ade 100755\n--- a/t/t3436-rebase-more-options.sh\n+++ b/t/t3436-rebase-more-options.sh\n@@ -42,7 +42,7 @@ test_expect_success 'setup' '\n \n test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' '\n \tcat >expect <<-\\EOF &&\n-\tfatal: could not split '\\''--bad'\\'': '\\''unclosed quote'\\''\n+\tfatal: could not split '\\''--bad'\\'': unclosed quote\n \tEOF\n \ttest_expect_code 128 git rebase -X\"bad argument\\\"\" side main >out 2>actual &&\n \ttest_must_be_empty out &&\n@@ -51,7 +51,7 @@ test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' '\n \n test_expect_success 'bad -X <strategy-option> arguments: bad escape' '\n \tcat >expect <<-\\EOF &&\n-\tfatal: could not split '\\''--bad'\\'': '\\''cmdline ends with \\'\\''\n+\tfatal: could not split '\\''--bad'\\'': cmdline ends with \\\n \tEOF\n \ttest_expect_code 128 git rebase -X\"bad escape \\\\\" side main >out 2>actual &&\n \ttest_must_be_empty out &&\n-- \n2.40.0-rc2\n\n"},{"id":"473200","messageId":"d1fcbbe5-52ce-54d0-bad6-97997a2a72c3@dunelm.org.uk","threadId":"59351","inReplyTo":"patch-1.1-f6a06e25cf3-20230307T182039Z-avarab@gmail.com","subject":"Re: [PATCH] sequencer.c: fix overflow & segfault in parse_strategy_opts()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-03-08T16:20:18Z","receivedAt":"2023-03-08T16:20:28Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Ævar\n\nOn 07/03/2023 18:21, Ævar Arnfjörð Bjarmason wrote:\n> The split_cmdline() function introduced in [1] returns an \"int\". If\n> it's negative it signifies an error. The option parsing in [2] didn't\n> account for this, and assigned the value directly to the \"size_t\n> xopts_nr\". We'd then attempt to loop over all of these elements, and\n> access uninitialized memory.\n> \n> There's a few things that use this for option parsing, but one way to\n> trigger it is with a bad value to \"-X <strategy-option>\", e.g:\n> \n> \tgit rebase -X\"bad argument\\\"\"\n\nAs Junio said that's nasty, thanks for catching it. I think what Junio \nhas queued in seen is fine. The root cause of the issue is that we don't \nquote the --strategy-option arguments properly. I've got some cleanups \nbased on top of this at \nhttps://github.com/phillipwood/git/commits/sequencer-merge-strategy-options \nto address that. I'll submit them once I've cleaned them up.\n\nBest Wishes\n\nPhillip\n\n> In another context this might be a security issue, but in this case\n> someone who's already able to inject arguments directly to our\n> commands would be past other defenses, making this potential\n> escalation a moot point.\n> \n> As the example above & test case shows the error reporting leaves\n> something to be desired. The function will loop over the\n> whitespace-split values, but when it encounters an error we'll only\n> report the first element, which is OK, not the second \"argument\\\"\"\n> whose quote is unbalanced.\n> \n> This is an inherent limitation of the current API, and the issue\n> affects other API users. Let's not attempt to fix that now. If and\n> when that happens these tests will need to be adjusted to assert the\n> new output.\n> \n> 1. 2b11e3170e9 (If you have a config containing something like this:,\n>     2006-06-05)\n> 2. ca6c6b45dd9 (sequencer (rebase -i): respect strategy/strategy_opts\n>     settings, 2017-01-02)\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n> \n> CI & branch for this at\n> https://github.com/avar/git/tree/avar/sequencer-xopts-nr-overflow\n> \n> Not a new issue, but I figured with other discussions in this area\n> kicking this out the door sooner than later was better.\n> \n>   sequencer.c                    |  9 +++++++--\n>   t/t3436-rebase-more-options.sh | 18 ++++++++++++++++++\n>   2 files changed, 25 insertions(+), 2 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 3e4a1972897..79c615193b6 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2876,13 +2876,18 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n>   void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)\n>   {\n>   \tint i;\n> +\tint count;\n>   \tchar *strategy_opts_string = raw_opts;\n>   \n>   \tif (*strategy_opts_string == ' ')\n>   \t\tstrategy_opts_string++;\n>   \n> -\topts->xopts_nr = split_cmdline(strategy_opts_string,\n> -\t\t\t\t       (const char ***)&opts->xopts);\n> +\tcount = split_cmdline(strategy_opts_string,\n> +\t\t\t      (const char ***)&opts->xopts);\n> +\tif (count < 0)\n> +\t\tdie(_(\"could not split '%s': '%s'\"), strategy_opts_string,\n> +\t\t\t    split_cmdline_strerror(count));\n> +\topts->xopts_nr = count;\n>   \tfor (i = 0; i < opts->xopts_nr; i++) {\n>   \t\tconst char *arg = opts->xopts[i];\n>   \n> diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh\n> index 94671d3c465..195ace34559 100755\n> --- a/t/t3436-rebase-more-options.sh\n> +++ b/t/t3436-rebase-more-options.sh\n> @@ -40,6 +40,24 @@ test_expect_success 'setup' '\n>   \tEOF\n>   '\n>   \n> +test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' '\n> +\tcat >expect <<-\\EOF &&\n> +\tfatal: could not split '\\''--bad'\\'': '\\''unclosed quote'\\''\n> +\tEOF\n> +\ttest_expect_code 128 git rebase -X\"bad argument\\\"\" side main >out 2>actual &&\n> +\ttest_must_be_empty out &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'bad -X <strategy-option> arguments: bad escape' '\n> +\tcat >expect <<-\\EOF &&\n> +\tfatal: could not split '\\''--bad'\\'': '\\''cmdline ends with \\'\\''\n> +\tEOF\n> +\ttest_expect_code 128 git rebase -X\"bad escape \\\\\" side main >out 2>actual &&\n> +\ttest_must_be_empty out &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>   test_expect_success '--ignore-whitespace works with apply backend' '\n>   \ttest_must_fail git rebase --apply main side &&\n>   \tgit rebase --abort &&\n"}]}