diff mbox series

malloc: Add integrity check to largebin nextsizes

Message ID 20250214053454.2346370-1-benjamin.p.kallus.gr@dartmouth.edu (mailing list archive)
State Committed
Commit 4cf2d869367e3813c6c8f662915dedb1f3830c53
Delegated to: DJ Delorie
Headers
Series 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

Ben Kallus Feb. 14, 2025, 5:34 a.m. UTC 
  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

DJ Delorie Feb. 24, 2025, 9:45 p.m. UTC | #1 
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;
Ben Kallus Feb. 24, 2025, 10:04 p.m. UTC | #2 
Thanks for the analysis. I wasn't thinking about rowhammer.

> LGTM.  Do you need someone to commit this on your behalf?

Yes please :)
DJ Delorie March 4, 2025, 3:09 a.m. UTC | #3 
Ben Kallus <benjamin.p.kallus.gr@dartmouth.edu> writes:
>> LGTM.  Do you need someone to commit this on your behalf?
>
> Yes please :)

Done!  Thanks!
diff mbox series

Patch

diff --git a/malloc/malloc.c b/malloc/malloc.c
index dcac903e2a..931ca48112 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -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;