threads / patch / 58803

patchbuiltin/gc.c: fix use-after-free in maintenance_unregister()

Subject: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()

## tl;dr

8 messages between Nov 15, 2022 and Nov 16, 2022. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Taylor Blau· Nov 15, 2022, 18:53 UTC · lore

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:

    $ ./t7900-maintenance.sh -vxi --run=23 --valgrind
    [...]
    + git maintenance unregister --config-file ./other
    ==3048727== Invalid read of size 8
    ==3048727==    at 0x1869CA: maintenance_unregister (gc.c:1590)
    ==3048727==    by 0x188F42: cmd_maintenance (gc.c:2651)
    ==3048727==    by 0x128C62: run_builtin (git.c:466)
    ==3048727==    by 0x12907E: handle_builtin (git.c:721)
    ==3048727==    by 0x1292EC: run_argv (git.c:788)
    ==3048727==    by 0x12988E: cmd_main (git.c:926)
    ==3048727==    by 0x21ED39: main (common-main.c:57)
    ==3048727==  Address 0x4b38bc8 is 24 bytes inside a block of size 64 free'd
    ==3048727==    at 0x484617B: free (vg_replace_malloc.c:872)
    ==3048727==    by 0x2D207E: free_individual_entries (hashmap.c:188)
    ==3048727==    by 0x2D2153: hashmap_clear_ (hashmap.c:207)
    ==3048727==    by 0x270B5C: git_configset_clear (config.c:2375)
    ==3048727==    by 0x1869AC: maintenance_unregister (gc.c:1585)
    ==3048727==    by 0x188F42: cmd_maintenance (gc.c:2651)
    ==3048727==    by 0x128C62: run_builtin (git.c:466)
    ==3048727==    by 0x12907E: handle_builtin (git.c:721)
    ==3048727==    by 0x1292EC: run_argv (git.c:788)
    ==3048727==    by 0x12988E: cmd_main (git.c:926)
    ==3048727==    by 0x21ED39: main (common-main.c:57)
    [...]

Resolve this via a partial-revert of be0fd57228. The config_set struct now gets a zero initialization, which makes free()-ing it a noop even without calling git_configset_init(). When we do initialize it to a non-zero value, it is only free()'d after our last read of `list`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
I am disappointed in myself for finding this only after I pushed out a
hotfix to 'next' and rebuild the downstream branches.

This should be a minimal fix on top of Ævar's patch to get 'next' building again.

 builtin/gc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
Show changes to builtin/gc.c +2 −4
diff --git a/builtin/gc.c b/builtin/gc.c
index d87cf84041..38882a1e35 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1543,6 +1543,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 	int found = 0;
 	struct string_list_item *item;
 	const struct string_list *list;
+	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);
 	} 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;
 }
--
2.38.0.16.g393fd4c6db
Derrick Stolee· Nov 15, 2022, 19:00 UTC · re: Taylor Blau · lore

Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()

On 11/15/2022 1:53 PM, Taylor Blau wrote:
Show 14 quoted lines
> 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.

Show 14 quoted lines
> +	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.

Show 11 quoted lines
>  	} 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.
-Stolee
Ævar Arnfjörð Bjarmason· Nov 15, 2022, 19:51 UTC · re: Derrick Stolee · lore

Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()

On Tue, Nov 15 2022, Derrick Stolee wrote:
Show 18 quoted lines
> 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().

Show 18 quoted lines
>> +	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.

Show 13 quoted lines
>>  	} 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/
Ævar Arnfjörð Bjarmason· Nov 15, 2022, 19:41 UTC · re: Taylor Blau · lore

Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()

On Tue, Nov 15 2022, Taylor Blau wrote:
Show 52 quoted lines
> 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:
>
>     $ ./t7900-maintenance.sh -vxi --run=23 --valgrind
>     [...]
>     + git maintenance unregister --config-file ./other
>     ==3048727== Invalid read of size 8
>     ==3048727==    at 0x1869CA: maintenance_unregister (gc.c:1590)
>     ==3048727==    by 0x188F42: cmd_maintenance (gc.c:2651)
>     ==3048727==    by 0x128C62: run_builtin (git.c:466)
>     ==3048727==    by 0x12907E: handle_builtin (git.c:721)
>     ==3048727==    by 0x1292EC: run_argv (git.c:788)
>     ==3048727==    by 0x12988E: cmd_main (git.c:926)
>     ==3048727==    by 0x21ED39: main (common-main.c:57)
>     ==3048727==  Address 0x4b38bc8 is 24 bytes inside a block of size 64 free'd
>     ==3048727==    at 0x484617B: free (vg_replace_malloc.c:872)
>     ==3048727==    by 0x2D207E: free_individual_entries (hashmap.c:188)
>     ==3048727==    by 0x2D2153: hashmap_clear_ (hashmap.c:207)
>     ==3048727==    by 0x270B5C: git_configset_clear (config.c:2375)
>     ==3048727==    by 0x1869AC: maintenance_unregister (gc.c:1585)
>     ==3048727==    by 0x188F42: cmd_maintenance (gc.c:2651)
>     ==3048727==    by 0x128C62: run_builtin (git.c:466)
>     ==3048727==    by 0x12907E: handle_builtin (git.c:721)
>     ==3048727==    by 0x1292EC: run_argv (git.c:788)
>     ==3048727==    by 0x12988E: cmd_main (git.c:926)
>     ==3048727==    by 0x21ED39: main (common-main.c:57)
>     [...]
>
> Resolve this via a partial-revert of be0fd57228. The config_set struct
> now gets a zero initialization, which makes free()-ing it a noop even
> without calling git_configset_init(). When we do initialize it to a
> non-zero value, it is only free()'d after our last read of `list`.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> I am disappointed in myself for finding this only after I pushed out a
> hotfix to 'next' and rebuild the downstream branches.
>
> This should be a minimal fix on top of Ævar's patch to get 'next'
> building again.

I'm also disappointed in myself, sorry. I *did* test it locally with valgrind, but obviously fat-fingered it somehow and tested the wrong version. Sorry!

Show 12 quoted lines
>  builtin/gc.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index d87cf84041..38882a1e35 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1543,6 +1543,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>  	int found = 0;
>  	struct string_list_item *item;
>  	const struct string_list *list;
> +	struct config_set cs = { { 0 } };

Just "{ 0 }" here instead? I see it may have been copied from some older pre-image though, and they'll do the same in either case, so it's not important...

Taylor Blau· Nov 15, 2022, 19:54 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()

On Tue, Nov 15, 2022 at 08:41:44PM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 9 quoted lines
> > I am disappointed in myself for finding this only after I pushed out a
> > hotfix to 'next' and rebuild the downstream branches.
> >
> > This should be a minimal fix on top of Ævar's patch to get 'next'
> > building again.
>
> I'm also disappointed in myself, sorry. I *did* test it locally with
> valgrind, but obviously fat-fingered it somehow and tested the wrong
> version. Sorry!

It's OK. Let's not beat ourselves up too much, and instead focusing on making sure the quality for the next pushout is higher (which is more on me than it is on you).

Show 16 quoted lines
> >  builtin/gc.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index d87cf84041..38882a1e35 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -1543,6 +1543,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
> >  	int found = 0;
> >  	struct string_list_item *item;
> >  	const struct string_list *list;
> > +	struct config_set cs = { { 0 } };
>
> Just "{ 0 }" here instead? I see it may have been copied from some older
> pre-image though, and they'll do the same in either case, so it's not
> important...
Copying from other zero-initializations of `struct config_set`:
    $ git grep -oh 'struct config_set.*= {.*' | sort | uniq -c
          3 struct config_set cs = { { 0 } };

Thanks, Taylor

Derrick Stolee· Nov 16, 2022, 13:44 UTC · re: Taylor Blau · lore

Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()

On 11/15/22 2:54 PM, Taylor Blau wrote:
Show 15 quoted lines
> On Tue, Nov 15, 2022 at 08:41:44PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>> @@ -1543,6 +1543,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>>>  	int found = 0;
>>>  	struct string_list_item *item;
>>>  	const struct string_list *list;
>>> +	struct config_set cs = { { 0 } };
>>
>> Just "{ 0 }" here instead? I see it may have been copied from some older
>> pre-image though, and they'll do the same in either case, so it's not
>> important...
> 
> Copying from other zero-initializations of `struct config_set`:
> 
>     $ git grep -oh 'struct config_set.*= {.*' | sort | uniq -c
>           3 struct config_set cs = { { 0 } };

Yes, without the double braces the compiler will complain on macOS, I believe.

Thanks, -Stolee

Ævar Arnfjörð Bjarmason· Nov 16, 2022, 15:14 UTC · re: Derrick Stolee · lore

Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()

On Wed, Nov 16 2022, Derrick Stolee wrote:
Show 19 quoted lines
> On 11/15/22 2:54 PM, Taylor Blau wrote:
>> On Tue, Nov 15, 2022 at 08:41:44PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>>> @@ -1543,6 +1543,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>>>>  	int found = 0;
>>>>  	struct string_list_item *item;
>>>>  	const struct string_list *list;
>>>> +	struct config_set cs = { { 0 } };
>>>
>>> Just "{ 0 }" here instead? I see it may have been copied from some older
>>> pre-image though, and they'll do the same in either case, so it's not
>>> important...
>> 
>> Copying from other zero-initializations of `struct config_set`:
>> 
>>     $ git grep -oh 'struct config_set.*= {.*' | sort | uniq -c
>>           3 struct config_set cs = { { 0 } };
>
> Yes, without the double braces the compiler will complain on
> macOS, I believe.

Ah, that was sorted in 54795d37d9e (config.mak.dev: disable suggest braces error on old clang versions, 2022-10-10).

It's fine here, we can follow-up for the #leftoverbits of changing those some other time.

Taylor Blau· Nov 16, 2022, 20:14 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()

On Wed, Nov 16, 2022 at 04:14:30PM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 13 quoted lines
> >> Copying from other zero-initializations of `struct config_set`:
> >>
> >>     $ git grep -oh 'struct config_set.*= {.*' | sort | uniq -c
> >>           3 struct config_set cs = { { 0 } };
> >
> > Yes, without the double braces the compiler will complain on
> > macOS, I believe.
>
> Ah, that was sorted in 54795d37d9e (config.mak.dev: disable suggest
> braces error on old clang versions, 2022-10-10).
>
> It's fine here, we can follow-up for the #leftoverbits of changing those
> some other time.
Thanks for the reference to 54795d37d9e, Ævar.
There are a small handful of these sitting around in our codebase:
    $ git grep '{ { '
    builtin/gc.c:   struct config_set cs = { { 0 } };
    builtin/worktree.c:             struct config_set cs = { { 0 } };
    oidmap.h:#define OIDMAP_INIT { { NULL } }
    oidset.h:#define OIDSET_INIT { { 0 } }
    reftable/stack_test.c:  struct reftable_ref_record refs[2] = { { NULL } };
    reftable/stack_test.c:  struct reftable_log_record logs[2] = { { NULL } };
    reftable/stack_test.c:  struct reftable_ref_record refs[2] = { { NULL } };
    reftable/stack_test.c:  struct reftable_log_record logs[2] = { { NULL } };
    reftable/stack_test.c:  struct reftable_log_record logs[20] = { { NULL } };
    reset.c:        struct tree_desc desc[2] = { { NULL }, { NULL } };
    t/helper/test-ref-store.c:static struct flag_definition empty_flags[] = { { NULL, 0 } };
    worktree.c:     struct config_set cs = { { 0 } };

But I tend to agree that I'm not all that eager to go and change these for the sake of changing them. I think we can slowly let them age out of the codebase when we're touching nearby areas in the future.

Thanks, Taylor

← back to recent threads