diff mbox series

malloc: harden removal from unsorted list

Message ID 9b74b50e-66a6-3321-5668-d89b35a21cd8@google.com
State New
Headers show
Series malloc: harden removal from unsorted list | expand

Commit Message

Francois Goichon Feb. 23, 2018, 12:17 p.m. UTC 
There isn't any linked list check when getting chunks out of the 
unsorted list which allows an attacker to write &av->top 
(unsorted_chunks chunk) almost anywhere (e.g. into a bin of their 
choosing to get it out in a subsequent allocation), with a relatively 
simple corruption:

--
   // Initial state
   long * c = malloc(0x100);
   malloc(0x100);
   free(c);

   // Corruption: point c->bk to bin used in alloc after next
   c[1] += (0x110 >> 4)*8*2 - 8;
   malloc(0x100);
   c = malloc(0x100);  // &av->top
--

Controlling bins is likely to be enough to gain code execution by 
overwriting farther ahead in .data or .bss (e.g. __free_hook).

Tested on i386 and x86_64.


	* malloc/malloc.c (_int_malloc): Added check before removing from
	unsorted list.

---
  malloc/malloc.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Florian Weimer Feb. 25, 2018, 3:15 p.m. UTC | #1 
* Francois Goichon:

> -		    malloc_printerr ("malloc(): corrupted unsorted chunks");
> +		    malloc_printerr ("malloc(): corrupted unsorted chunks 2");

I suggest not to change the existing messages, so that we can
interpret them without closely looking at the glibc version.  I think
that's more useful than keeping the messages in source code order.
Francois Goichon Feb. 26, 2018, 10:48 a.m. UTC | #2 
On the exploitability of this, the += was just there to explicitly point
out the targeted bin, but an actual exploitation would more likely look
at  overwriting the least significant byte(s), or the full address if in
a position to leak a free or uninitialized chunk beforehand.

I believe this is in line with other attacks that malloc tries to
prevent, i.e. where controlling a single pointer is sufficient to get
significant control over the application's behavior. I agree it's a fine
line though and that it's not malloc's role to stop exploitation of edge
cases where the attacker has an unreasonable amount of control.

Happy to try and put numbers on the performance impact of this change
if it's a concern here - if yes is there a benchmark that would be best
suited to this usecase?

Rolled back the existing messages as suggested:


---
  malloc/malloc.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 58f9acd4d1..fd1a263e9e 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3775,6 +3775,8 @@ _int_malloc (mstate av, size_t bytes)
              }

            /* remove from unsorted list */
+          if (__glibc_unlikely (bck->fd != victim))
+            malloc_printerr ("malloc(): corrupted unsorted chunks 3");
            unsorted_chunks (av)->bk = bck;
            bck->fd = unsorted_chunks (av);
diff mbox series

Patch

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 58f9acd4d1..17d373eca9 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3775,6 +3775,8 @@  _int_malloc (mstate av, size_t bytes)
              }

            /* remove from unsorted list */
+          if (__glibc_unlikely (bck->fd != victim))
+            malloc_printerr ("malloc(): corrupted unsorted chunks");
            unsorted_chunks (av)->bk = bck;
            bck->fd = unsorted_chunks (av);

@@ -3941,7 +3943,7 @@  _int_malloc (mstate av, size_t bytes)
                    bck = unsorted_chunks (av);
                    fwd = bck->fd;
  		  if (__glibc_unlikely (fwd->bk != bck))
-		    malloc_printerr ("malloc(): corrupted unsorted chunks");
+		    malloc_printerr ("malloc(): corrupted unsorted chunks 2");
                    remainder->bk = bck;
                    remainder->fd = fwd;
                    bck->fd = remainder;
@@ -4045,7 +4047,7 @@  _int_malloc (mstate av, size_t bytes)
                    bck = unsorted_chunks (av);
                    fwd = bck->fd;
  		  if (__glibc_unlikely (fwd->bk != bck))
-		    malloc_printerr ("malloc(): corrupted unsorted chunks 2");
+		    malloc_printerr ("malloc(): corrupted unsorted chunks 3");
                    remainder->bk = bck;
                    remainder->fd = fwd;
                    bck->fd = remainder;