From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Derrick Stolee <derrickstolee@github.com>
Cc: Taylor Blau <me@ttaylorr.com>,
git@vger.kernel.org, git-security@googlegroups.com,
Johannes Schindelin <Johannes.Schindelin@gmx.de>,
Ronan Pigott <ronan@rjp.ie>
Subject: Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()
Date: Tue, 15 Nov 2022 20:51:26 +0100 [thread overview]
Message-ID: <221115.86mt8sj8fw.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <d8b732c9-ae41-bf1b-3bb1-6682493bee70@github.com>
On Tue, Nov 15 2022, Derrick Stolee wrote:
> On 11/15/2022 1:53 PM, Taylor Blau wrote:
>> While trying to fix a move based on an uninitialized value (along with a
>> declaration after the first statement), be0fd57228
>> (maintenance --unregister: fix uninit'd data use &
>> -Wdeclaration-after-statement, 2022-11-15) unintentionally introduced a
>> use-after-free.
>>
>> The problem arises when `maintenance_unregister()` sees a non-NULL
>> `config_file` string and thus tries to call
>> git_configset_get_value_multi() to lookup the corresponding values.
>>
>> We store the result off, and then call git_configset_clear(), which
>> frees the pointer that we just stored. We then try to read that
>> now-freed pointer a few lines below, and there we have our
>> use-after-free:
>
> Makes sense why this needs to be pulled out to a larger scope, but
> also why it's so easy to make this mistake.
Yeah, the config API's full of foot-guns, although here we return a
"const struct string_list *", not a "struct string_list *", so in
retrospect this should be rather obvious...
But still, we should probably as #leftoverbits make it behave
consistently wrt naming. I.e. in this case it's
git_configset_get_value_multi() really behaves like a
git_configset_get_string_tmp(), and there's no equivalent of a
git_configset_get_string() (i.e. xstrdup()'d) for *_multi().
>> + struct config_set cs = { { 0 } };
>>
>> argc = parse_options(argc, argv, prefix, options,
>> builtin_maintenance_unregister_usage, 0);
>> @@ -1551,12 +1552,9 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>> options);
>>
>> if (config_file) {
>> - struct config_set cs;
>> -
>> git_configset_init(&cs);
>> git_configset_add_file(&cs, config_file);
>> list = git_configset_get_value_multi(&cs, key);
>> - git_configset_clear(&cs);
>
> That the list depends on the configset and not exist as an
> independent entity is non-obvious, but I'm sure is rooted
> in some kind of memory-saving optimization.
Yes, and it's probably worth keeping that, but I haven't benchmarked
etc. This is only a problem in practice if you're constructing your own
configset, e.g. here because we have a custom config file. So for most
users this API is safe in general, i.e. we free() it, but it's the
config that's in "the_repository" normally, so it outlives any "normal"
code.
>> } else {
>> list = git_config_get_value_multi(key);
>> }
>> @@ -1592,6 +1590,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>> die(_("repository '%s' is not registered"), maintpath);
>> }
>>
>> + git_configset_clear(&cs);
>> free(maintpath);
>> return 0;
>> }
>
> Thanks for drilling down on this. LGTM.
On the related subject of config API foot-guns, it would be great if you
could look over the in-flight series I have to make related parts of the
config API safe by default [1].
8/9 there fixes 6 segfaults, 3 of which are git blame'd to you :), and
9/9 a foot-gun-y interaction with the strvec API, which you'll also
probably find interesting...
1. https://lore.kernel.org/git/cover-v2-0.9-00000000000-20221101T225822Z-avarab@gmail.com/
next prev parent reply other threads:[~2022-11-15 19:59 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-15 18:53 [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister() Taylor Blau
2022-11-15 19:00 ` Derrick Stolee
2022-11-15 19:51 ` Ævar Arnfjörð Bjarmason [this message]
2022-11-15 19:41 ` Ævar Arnfjörð Bjarmason
2022-11-15 19:54 ` Taylor Blau
2022-11-16 13:44 ` Derrick Stolee
2022-11-16 15:14 ` Ævar Arnfjörð Bjarmason
2022-11-16 20:14 ` Taylor Blau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=221115.86mt8sj8fw.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=derrickstolee@github.com \
--cc=git-security@googlegroups.com \
--cc=git@vger.kernel.org \
--cc=me@ttaylorr.com \
--cc=ronan@rjp.ie \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).