[Patch] fix use-after-free in dcigettext.c

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[Patch] fix use-after-free in dcigettext.c

Jeff Law
http://sourceware.org/bugzilla/show_bug.cgi?id=14277

dcigettext.c has a very clear use-after-free problem which can result in
a user process getting a segfault or other error.

                       newmem = (transmem_block_t *) realloc (transmem_list,
                                                              freemem_size);
# ifdef _LIBC
                       if (newmem != NULL)
                         transmem_list = transmem_list->next;
                       else
                         {
                           struct transmem_list *old = transmem_list;

                           transmem_list = transmem_list->next;
                           free (old);
                         }
# endif

If the call to realloc requires transmem_list's memory to be moved, then
the prior pointer is free'd.  Most of the time this doesn't cause a
noticeable problem because the memory is still mapped and the
dereference in transmem_list->next shortly after the realloc call works
fine (this is still wrong obviously).

However, if transmem_list was allocated by mmap (because it was large),
when the original pointer is free'd as a result of the realloc call, the
old memory gets ummapped.  With the memory now unmapped, the
transmem_list->next dereference of the old pointer fails with a segfault.

I don't have a good testcase -- the one provided to me hasn't tripped in
about 12 hours of running or in shorter runs with differing values of
M_MMAP_THRESHOLD and may contain confidential information.   Hopefully
the analysis above is clear enough to show this code is clearly broken.

Attached is a possible fix.


P (897 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] fix use-after-free in dcigettext.c

Roland McGrath-4
While that fix looks like it can't be wrong, looking at the surrounding
code it looks like there's a better fix.  The old value is always available
in the new copy, i.e. NEWMEM->next.  But about 30 lines below, we have:

                  newmem->next = transmem_list;
                  transmem_list = newmem;

It popping the element off and then putting it back on is entirely redundant.
The addition is necessary in the malloc (not realloc) case.  But it could
just be done inside that case.


Thanks,
Roland
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] fix use-after-free in dcigettext.c

Jeff Law
On 06/21/2012 02:47 PM, Roland McGrath wrote:
> While that fix looks like it can't be wrong, looking at the surrounding
> code it looks like there's a better fix.  The old value is always available
> in the new copy, i.e. NEWMEM->next.
Yes, agreed.


>  But about 30 lines below, we have:
>
>  newmem->next = transmem_list;
>  transmem_list = newmem;
>
> It popping the element off and then putting it back on is entirely redundant.
> The addition is necessary in the malloc (not realloc) case.  But it could
> just be done inside that case.
Something like this perhaps?



Reply | Threaded
Open this post in threaded view
|

Re: [Patch] fix use-after-free in dcigettext.c

Roland McGrath-4
That looks right to me.

Your first message mentioned a BZ#, but it's not in your log entry.


Thanks,
Roland
Reply | Threaded
Open this post in threaded view
|

Re: [Patch] fix use-after-free in dcigettext.c

Jeff Law
On 06/21/2012 04:42 PM, Roland McGrath wrote:
> That looks right to me.
>
> Your first message mentioned a BZ#, but it's not in your log entry.
Fixed in the final commit.  NEWS file updated and copyright year updated
as well.
jeff