sequencer.c: fix overflow & segfault in parse_strategy_opts()

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\"" 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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Mar 7, 2023 at 19:21 UTC 15a4cc912e601c1641e549861e284f63e30f562c
2 files changed +25 -2
sequencer.c
+7 -2
@@ -2876,13 +2876,18 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
2876 void parse_strategy_opts(struct replay_opts *opts, char *raw_opts)
2877 {
2878 int i;
2879 + int count;
2880 char *strategy_opts_string = raw_opts;
2881
2882 if (*strategy_opts_string == ' ')
2883 strategy_opts_string++;
2884
2884 - opts->xopts_nr = split_cmdline(strategy_opts_string,
2885 - (const char ***)&opts->xopts);
2885 + count = split_cmdline(strategy_opts_string,
2886 + (const char ***)&opts->xopts);
2887 + if (count < 0)
2888 + die(_("could not split '%s': %s"), strategy_opts_string,
2889 + split_cmdline_strerror(count));
2890 + opts->xopts_nr = count;
2891 for (i = 0; i < opts->xopts_nr; i++) {
2892 const char *arg = opts->xopts[i];
2893
t/t3436-rebase-more-options.sh
+18
@@ -40,6 +40,24 @@ test_expect_success 'setup' '
40 EOF
41 '
42
43 +test_expect_success 'bad -X <strategy-option> arguments: unclosed quote' '
44 + cat >expect <<-\EOF &&
45 + fatal: could not split '\''--bad'\'': unclosed quote
46 + EOF
47 + test_expect_code 128 git rebase -X"bad argument\"" side main >out 2>actual &&
48 + test_must_be_empty out &&
49 + test_cmp expect actual
50 +'
51 +
52 +test_expect_success 'bad -X <strategy-option> arguments: bad escape' '
53 + cat >expect <<-\EOF &&
54 + fatal: could not split '\''--bad'\'': cmdline ends with \
55 + EOF
56 + test_expect_code 128 git rebase -X"bad escape \\" side main >out 2>actual &&
57 + test_must_be_empty out &&
58 + test_cmp expect actual
59 +'
60 +
61 test_expect_success '--ignore-whitespace works with apply backend' '
62 test_must_fail git rebase --apply main side &&
63 git rebase --abort &&