diff mbox

malloc/malloc.c: Mitigate null-byte overflow attacks

Message ID 82b19ef6-f0e6-ab77-5dad-ea9bba34e98c@cs.ucsb.edu
State Committed, archived
Headers

Commit Message

Moritz Eckert Jan. 15, 2018, 7:56 p.m. UTC 
  Hi,

I gave this another thought and to make things simpler propose a slight 
change in the patch that would keep the old check and just additionally 
checks the prev_size<->size before traversing back in memory.
That way, we'll just add an easy efficient way of catching the 
poison_null_byte, without any other implications.

Thanks,
Moritz

Comments

Florian Weimer Aug. 16, 2018, 3:09 p.m. UTC | #1 
On 01/15/2018 08:56 PM, Moritz Eckert wrote:
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index f5aafd2c05..d6ebfafd9a 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -4288,6 +4288,8 @@ _int_free (mstate av, mchunkptr p, int have_lock)
>         prevsize = prev_size (p);
>         size += prevsize;
>         p = chunk_at_offset(p, -((long) prevsize));
> +      if (__builtin_expect (chunksize(p) != prevsize, 0))
> +        malloc_printerr ("corrupted size vs. prev_size");
>         unlink(av, p, bck, fwd);
>       }
>   
> @@ -4449,6 +4451,8 @@ static void malloc_consolidate(mstate av)
>   	  prevsize = prev_size (p);
>   	  size += prevsize;
>   	  p = chunk_at_offset(p, -((long) prevsize));
> +	  if (__builtin_expect (chunksize(p) != prevsize, 0))
> +	    malloc_printerr ("corrupted size vs. prev_size");
>   	  unlink(av, p, bck, fwd);
>   	}

I think it would make sense to have different error messages in both 
cases, to make crash reports more meaningful.  And __glibc_unlikely 
should be used.

Otherwise, it looks good to me.  DJ, could you commit this in Moritz' 
name with a proper changelog entry?  It does not raise to the threshold 
of requiring copyright assignment.

Thanks,
Florian
diff mbox

Patch

diff --git a/malloc/malloc.c b/malloc/malloc.c
index f5aafd2c05..d6ebfafd9a 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -4288,6 +4288,8 @@  _int_free (mstate av, mchunkptr p, int have_lock)
       prevsize = prev_size (p);
       size += prevsize;
       p = chunk_at_offset(p, -((long) prevsize));
+      if (__builtin_expect (chunksize(p) != prevsize, 0))
+        malloc_printerr ("corrupted size vs. prev_size");
       unlink(av, p, bck, fwd);
     }
 
@@ -4449,6 +4451,8 @@  static void malloc_consolidate(mstate av)
 	  prevsize = prev_size (p);
 	  size += prevsize;
 	  p = chunk_at_offset(p, -((long) prevsize));
+	  if (__builtin_expect (chunksize(p) != prevsize, 0))
+	    malloc_printerr ("corrupted size vs. prev_size");
 	  unlink(av, p, bck, fwd);
 	}