Re: [PATCH] malloc/malloc.c: Mitigate null-byte overflow attacks

[Moritz Eckert <m dot eckert at cs dot ucsb dot edu>]

> > > I wonder if we should add a "size_is_sane()" macro to check for
> > > unreasonable sizes before we use them to compute pointers.
> > That sounds like a good idea to me. Would you prefer a separate macro
> > for prev_size and size that only gets the current chunk as a parameter or
> > a single macro that gets a parameter what to check for?
>
> I don't know, I was just wondering if there were some other way to
> determine that a size has been corrupted other than consistency checks.

Here would be my idea for a macro. Do you like that better?

The macro could also do the whole stepping backward in memory, as in 
"prev_chunk_secure(p)", which points p to the prev_chunk or
aborts otherwise.

Also would you prefer to keep the current check in unlink, or remove it 
entirely?


diff --git a/malloc/malloc.c b/malloc/malloc.c
index d3fcadd20e..73fc98d2c3 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -1426,6 +1426,12 @@ typedef struct malloc_chunk *mbinptr;
       }									      \
 }
 
+/* Check if prev_size is consistent with size*/
+#define prev_size_is_sane(p, prevsize) {				   \
+    if (__builtin_expect (prevsize !=  chunksize (p), 0))		      \
+        malloc_printerr ("corrupted size vs. prev_size");		      \
+}
+
 /*
    Indexing
 
@@ -4227,6 +4233,7 @@ _int_free (mstate av, mchunkptr p, int have_lock)
       prevsize = prev_size (p);
       size += prevsize;
       p = chunk_at_offset(p, -((long) prevsize));
+      prev_size_is_sane(p, prevsize)
       unlink(av, p, bck, fwd);
     }
 
@@ -4392,6 +4399,7 @@ static void malloc_consolidate(mstate av)
 	    prevsize = prev_size (p);
 	    size += prevsize;
 	    p = chunk_at_offset(p, -((long) prevsize));
+	    prev_size_is_sane(p, prevsize)
 	    unlink(av, p, bck, fwd);
 	  }