malloc: Add integrity check to largebin nextsizes
Checks
Context |
Check |
Description |
redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Build passed
|
linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Build passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Test passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Test passed
|
redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
Commit Message
If attacker overwrites the bk_nextsize link in the first chunk of a
largebin that later has a smaller chunk inserted into it, malloc will
write a heap pointer into an attacker-controlled address [0].
This patch adds an integrity check to mitigate this attack.
[0]: https://github.com/shellphish/how2heap/blob/master/glibc_2.39/large_bin_attack.c
Signed-off-by: Ben Kallus <benjamin.p.kallus.gr@dartmouth.edu>
---
malloc/malloc.c | 3 +++
1 file changed, 3 insertions(+)
Comments
Ben Kallus <benjamin.p.kallus.gr@dartmouth.edu> writes:
> If attacker overwrites the bk_nextsize link in the first chunk of a
> largebin that later has a smaller chunk inserted into it, malloc will
> write a heap pointer into an attacker-controlled address [0].
LGTM. Do you need someone to commit this on your behalf?
Reviewed-by: DJ Delorie <dj@redhat.com>
> @@ -4244,6 +4244,9 @@ _int_malloc (mstate av, size_t bytes)
At this point bck = bin() and fwd = bck->fd
> fwd = bck;
> bck = bck->bk;
So here, fwd = bin() and bck = bin()->bk (the last chunk in the chain,
which may or may not be part of the nextsize chain)
> + if (__glibc_unlikely (fwd->fd->bk_nextsize->fd_nextsize != fwd->fd))
> + malloc_printerr ("malloc(): largebin double linked list corrupted (nextsize)");
fwd->fd is thus the first chunk in the chain, which is the first of its
size (by definition).
fwd->fd->bk_nextsize is thus the last "first of its size" chunk in the
chain, and may be tainted.
fwd->fd->bk_nextsize->fd_nextsize is thus the first chunk in the chain,
but relies on the tainted fwd->fd->bk_nextsize
If the user controls fwd->fd->bk_nextsize, and we dereference it, we're
reading from an attacker-chosen site. This is normally not a problem,
but it could be used as part of a cache attack (like rowhammer et al),
where reads are used to probe or corrupt the cache.
I think this is a risk we'll have to take as I can't see any other way
to get to the last first-in-size without iterating through the entire
chain, and we rely on these pointers being accurate anyway. An attempt
to poison the cache would, with this patch, cause the application to
exit.
So OK.
> victim->fd_nextsize = fwd->fd;
> victim->bk_nextsize = fwd->fd->bk_nextsize;
> fwd->fd->bk_nextsize = victim->bk_nextsize->fd_nextsize = victim;
Thanks for the analysis. I wasn't thinking about rowhammer.
> LGTM. Do you need someone to commit this on your behalf?
Yes please :)
Ben Kallus <benjamin.p.kallus.gr@dartmouth.edu> writes:
>> LGTM. Do you need someone to commit this on your behalf?
>
> Yes please :)
Done! Thanks!
@@ -4244,6 +4244,9 @@ _int_malloc (mstate av, size_t bytes)
fwd = bck;
bck = bck->bk;
+ if (__glibc_unlikely (fwd->fd->bk_nextsize->fd_nextsize != fwd->fd))
+ malloc_printerr ("malloc(): largebin double linked list corrupted (nextsize)");
+
victim->fd_nextsize = fwd->fd;
victim->bk_nextsize = fwd->fd->bk_nextsize;
fwd->fd->bk_nextsize = victim->bk_nextsize->fd_nextsize = victim;