From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 08 Mar 2023 16:20:18 GMT
Subject: Re: [PATCH] sequencer.c: fix overflow & segfault in parse_strategy_opts()
Message-ID: <d1fcbbe5-52ce-54d0-bad6-97997a2a72c3@dunelm.org.uk>
In-Reply-To: <patch-1.1-f6a06e25cf3-20230307T182039Z-avarab@gmail.com>

Hi Ævar

On 07/03/2023 18:21, Ævar Arnfjörð Bjarmason wrote:
> The split_cmdline() function introduced in [1] returns an "int". If
> it's negative it signifies an error. The option parsing in [2] didn't
> account for this, and assigned the value directly to the "size_t
> xopts_nr". We'd then attempt to loop over all of these elements, and
> access uninitialized memory.
> 
> There's a few things that use this for option parsing, but one way to
> trigger it is with a bad value to "-X <strategy-option>", e.g:
> 
> 	git rebase -X"bad argument\""

As Junio said that's nasty, thanks for catching it. I think what Junio 
has queued in seen is fine. The root cause of the issue is that we don't 
quote the --strategy-option arguments properly. I've got some cleanups 
based on top of this at 
https://github.com/phillipwood/git/commits/sequencer-merge-strategy-options 
to address that. I'll submit them once I've cleaned them up.

Best Wishes

Phillip

> In another context this might be a security issue, but in this case
> someone who's already able to inject arguments directly to our
> commands would be past other defenses, making this potential
> escalation a moot point.
> 
> As the example above & test case shows the error reporting leaves
> something to be desired. The function will loop over the
> whitespace-split values, but when it encounters an error we'll only
> report the first element, which is OK, not the second "argument\""
> whose quote is unbalanced.
> 
> This is an inherent limitation of the current API, and the issue
> affects other API users. Let's not attempt to fix that now. If and
> when that happens these tests will need to be adjusted to assert the
> new output.
> 
> 1. 2b11e3170e9 (If you have a config containing something like this:,
>     2006-06-05)
> 2. ca6c6b45dd9 (sequencer (rebase -i): respect strategy/strategy_opts
>     settings, 2017-01-02)
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> CI & branch for this at
> https://github.com/avar/git/tree/avar/sequencer-xopts-nr-overflow
> 
> Not a new issue, but I figured with other discussions in this area
> kicking this out the door sooner than later was better.
> 
>   sequencer.c                    |  9 +++++++--
>   t/t3436-rebase-more-options.sh | 18 ++++++++++++++++++
>   2 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 3e4a1972897..79c615193b6 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2876,13 +2876,18 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
>   void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
>   {
>   	int i;
> +	int count;
>   	char *strategy_opts_string = raw_opts;
>   
>   	if (*strategy_opts_string == ' ')
>   		strategy_opts_string++;
>   
> -	opts->xopts_nr = split_cmdline(strategy_opts_string,
> -				       (const char ***)&opts->xopts);
> +	count = split_cmdline(strategy_opts_string,
> +			      (const char ***)&opts->xopts);
> +	if (count < 0)
> +		die(_("could not split '%s': '%s'"), strategy_opts_string,
> +			    split_cmdline_strerror(count));
> +	opts->xopts_nr = count;
>   	for (i = 0; i < opts->xopts_nr; i++) {
>   		const char *arg = opts->xopts[i];
>   
> diff --git a/t/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
> index 94671d3c465..195ace34559 100755
> --- a/t/t3436-rebase-more-options.sh
> +++ b/t/t3436-rebase-more-options.sh
> @@ -40,6 +40,24 @@ test_expect_success 'setup' '
>   	EOF
>   '
>   
> +test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' '
> +	cat >expect <<-\EOF &&
> +	fatal: could not split '\''--bad'\'': '\''unclosed quote'\''
> +	EOF
> +	test_expect_code 128 git rebase -X"bad argument\"" side main >out 2>actual &&
> +	test_must_be_empty out &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'bad -X <strategy-option> arguments: bad escape' '
> +	cat >expect <<-\EOF &&
> +	fatal: could not split '\''--bad'\'': '\''cmdline ends with \'\''
> +	EOF
> +	test_expect_code 128 git rebase -X"bad escape \\" side main >out 2>actual &&
> +	test_must_be_empty out &&
> +	test_cmp expect actual
> +'
> +
>   test_expect_success '--ignore-whitespace works with apply backend' '
>   	test_must_fail git rebase --apply main side &&
>   	git rebase --abort &&