[PATCH] malloc: Add integrity check to largebin nextsizes

[PATCH] malloc: Add integrity check to largebin nextsizes

[Ben Kallus]

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(+)

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;
-- 
2.48.1

Re: [PATCH] malloc: Add integrity check to largebin nextsizes

[DJ Delorie]

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;

Re: [PATCH] malloc: Add integrity check to largebin nextsizes

[Ben Kallus]

Thanks for the analysis. I wasn't thinking about rowhammer.

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

Yes please :)

Re: [PATCH] malloc: Add integrity check to largebin nextsizes

[DJ Delorie]

Ben Kallus <benjamin.p.kallus.gr@dartmouth.edu> writes:
>> LGTM.  Do you need someone to commit this on your behalf?
>
> Yes please :)

Done!  Thanks!