* [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()
@ 2022-11-15 18:53 Taylor Blau
2022-11-15 19:00 ` Derrick Stolee
2022-11-15 19:41 ` Ævar Arnfjörð Bjarmason
0 siblings, 2 replies; 8+ messages in thread
From: Taylor Blau @ 2022-11-15 18:53 UTC (permalink / raw)
To: git, git-security
Cc: Johannes Schindelin, Ronan Pigott,
Ævar Arnfjörð Bjarmason, Derrick Stolee
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(-)
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()
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
2022-11-15 19:41 ` Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 8+ messages in thread
From: Derrick Stolee @ 2022-11-15 19:00 UTC (permalink / raw)
To: Taylor Blau, git, git-security
Cc: Johannes Schindelin, Ronan Pigott,
Ævar Arnfjörð Bjarmason
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.
> + 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.
> } 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()
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:41 ` Ævar Arnfjörð Bjarmason
2022-11-15 19:54 ` Taylor Blau
1 sibling, 1 reply; 8+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-15 19:41 UTC (permalink / raw)
To: Taylor Blau
Cc: git, git-security, Johannes Schindelin, Ronan Pigott,
Derrick Stolee
On Tue, Nov 15 2022, 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:
>
> $ ./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!
> 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...
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()
2022-11-15 19:00 ` Derrick Stolee
@ 2022-11-15 19:51 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 8+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-15 19:51 UTC (permalink / raw)
To: Derrick Stolee
Cc: Taylor Blau, git, git-security, Johannes Schindelin, Ronan Pigott
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/
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()
2022-11-15 19:41 ` Ævar Arnfjörð Bjarmason
@ 2022-11-15 19:54 ` Taylor Blau
2022-11-16 13:44 ` Derrick Stolee
0 siblings, 1 reply; 8+ messages in thread
From: Taylor Blau @ 2022-11-15 19:54 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Taylor Blau, git, Johannes Schindelin, Ronan Pigott,
Derrick Stolee
On Tue, Nov 15, 2022 at 08:41:44PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > 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).
> > 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()
2022-11-15 19:54 ` Taylor Blau
@ 2022-11-16 13:44 ` Derrick Stolee
2022-11-16 15:14 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 8+ messages in thread
From: Derrick Stolee @ 2022-11-16 13:44 UTC (permalink / raw)
To: Taylor Blau, Ævar Arnfjörð Bjarmason
Cc: git, Johannes Schindelin, Ronan Pigott
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.
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()
2022-11-16 13:44 ` Derrick Stolee
@ 2022-11-16 15:14 ` Ævar Arnfjörð Bjarmason
2022-11-16 20:14 ` Taylor Blau
0 siblings, 1 reply; 8+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-16 15:14 UTC (permalink / raw)
To: Derrick Stolee; +Cc: Taylor Blau, git, Johannes Schindelin, Ronan Pigott
On Wed, Nov 16 2022, Derrick Stolee wrote:
> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()
2022-11-16 15:14 ` Ævar Arnfjörð Bjarmason
@ 2022-11-16 20:14 ` Taylor Blau
0 siblings, 0 replies; 8+ messages in thread
From: Taylor Blau @ 2022-11-16 20:14 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Derrick Stolee, Taylor Blau, git, Johannes Schindelin,
Ronan Pigott, Jeff Hostetler
On Wed, Nov 16, 2022 at 04:14:30PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-11-16 20:14 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).