Re: [PATCH] malloc: harden removal from unsorted list

Re: [PATCH] malloc: harden removal from unsorted list

[DJ Delorie]

Franois Goichon <fgoichon@google.com> writes:
> Gentle ping - anything required on my end here (e.g. pull request)?

I'm OK with it.  Florian?  Any further comments?

Re: [PATCH] malloc: harden removal from unsorted list

[Florian Weimer]

On 03/12/2018 07:44 PM, DJ Delorie wrote:
> 
> Franois Goichon <fgoichon@google.com> writes:
>> Gentle ping - anything required on my end here (e.g. pull request)?
> 
> I'm OK with it.  Florian?  Any further comments?

The latest commit looks okay.  Can you commit it in François's name?

Thanks,
Florian

Re: [PATCH] malloc: harden removal from unsorted list

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
>>> Gentle ping - anything required on my end here (e.g. pull request)?
>> 
>> I'm OK with it.  Florian?  Any further comments?
>
> The latest commit looks okay.  Can you commit it in François's name?

Done.

Probably should have a bz associated with it too, though.  And a test
case, eventually.

Re: [PATCH] malloc: harden removal from unsorted list

[Florian Weimer]

On 03/14/2018 09:47 PM, DJ Delorie wrote:
> Florian Weimer <fweimer@redhat.com> writes:
>>>> Gentle ping - anything required on my end here (e.g. pull request)?
>>>
>>> I'm OK with it.  Florian?  Any further comments?
>>
>> The latest commit looks okay.  Can you commit it in François's name?
> 
> Done.
> 
> Probably should have a bz associated with it too, though.  And a test
> case, eventually.

It's not user-visibile, so it doesn't need a bug.

Hardening test cases are intrinsically difficult to write and probably 
of little value.  You need to make sure that change not merely breaks 
one specific case, but entire exploitation strategy.  Otherwise, why 
bother with adding this kind of hardening?

Thanks,
Florian

Re: [PATCH] malloc: harden removal from unsorted list

[François Goichon]

Gentle ping - is anything required on my end here (e.g. pull request)?

Re: [PATCH] malloc: harden removal from unsorted list

[Francois Goichon]

	* malloc/malloc.c (_int_malloc): Added check before removing from
	unsorted list.
---
  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);

Re: [PATCH] malloc: harden removal from unsorted list

[DJ Delorie]

LGTM.  ChangeLog?

Francois Goichon <fgoichon@google.com> writes:
> 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);

Re: [PATCH] malloc: harden removal from unsorted list

[Francois Goichon]

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

Re: [PATCH] malloc: harden removal from unsorted list

[Florian Weimer]

* 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.

Re: [PATCH] malloc: harden removal from unsorted list

[DJ Delorie]

While I have no problems with the patch itself (i.e. LGTM), I have two
side notes:

1. We probably could use better error messages now that we have three
numbered ones.  ERROR 458104 - CONSULT MANUAL.

2. At what point do these attack vectors move from "something a hacker
could do" to "something a developer could do" ?  If a hacker can do this
type of computations, a hacker can do pretty much whatever they want
already.  Given how critical malloc performance is to applications these
days, we need to be careful to not harden against things that are only
possible through the developer's malice.

[PATCH] malloc: harden removal from unsorted list

[Francois Goichon]

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

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;