[PATCH 0/1] Improved double free detection in the tcache.

[Wilco Dijkstra]

Hi David,

I believe this is the first time ever for a newly proposed security check that
doesn't significantly reduce performance!

> The previous double free detection didn’t account for an attacker to
> use a Poison Null Byte to change the size of a memory chunk
> is beeing sorted into.
> So that the check in 'tcache_double_free_verify' would pass
> even though it is a double free.

What do you mean with "poison null byte" here? Do you have a testcase to
show the attack? Ideally this should be part of the patch. One can certainly
overwrite the chunk header, but if you do so, you could also corrupt it in a
way that still passes all tests (eg. changing size so that one of the sizes fits
in tcache but the other does not).

> Alternatives Considered:
> - Store the size of a memory chunk in big endian and thus
>   the chunk size would not get overwritten because entrys in the
>   tcache are not that big.

I'm not sure how that would work. Are you talking about accidental off-by-1
write of a zero byte from the previous chunk into the next chunk header?
Why not larger overwrites?

> - Move the tcache_key before the actual memory chunk so that it
>   does not have to be checked at all, this would work better in generall
>  but also it would increase the memory usage.

On 64-bit targets you could add a 32 bit key in the chunk header.

>  static __attribute__ ((noinline)) void
> -tcache_double_free_verify (tcache_entry *e, size_t tc_idx)
> +tcache_double_free_verify (tcache_entry *e)
>  {
>    tcache_entry *tmp;
>    size_t cnt = 0;
>    LIBC_PROBE (memory_tcache_double_free, 2, e, tc_idx);

This won't build if you configure with --enable-systemtap.

-  for (tmp = tcache->entries[tc_idx];
-       tmp;
-       tmp = REVEAL_PTR (tmp->next), ++cnt)
+  for (size_t tc_idx = 0; tc_idx < TCACHE_MAX_BINS; ++tc_idx)

Basically this walks every entry in the tcache of the current thread, so
it would take 64 times longer assuming all bins are equally filled. If there
are only a few bins in actual use (as in typical applications), it's would be
less. Since we almost never call this function this is most likely OK.

     {
-      if (cnt >= mp_.tcache_count)
-	malloc_printerr ("free(): too many chunks detected in tcache");
-      if (__glibc_unlikely (!aligned_OK (tmp)))
-	malloc_printerr ("free(): unaligned chunk detected in tcache 2");
-      if (tmp == e)
-	malloc_printerr ("free(): double free detected in tcache 2");
-      /* If we get here, it was a coincidence.  We've wasted a
-	 few cycles, but don't abort.  */
+      for (tmp = tcache->entries[tc_idx];
+           tmp;
+           tmp = REVEAL_PTR (tmp->next), ++cnt)

I don't see how this could work. Surely you want to initialize cnt?

+        {
+          if (cnt >= mp_.tcache_count)
+    	malloc_printerr ("free(): too many chunks detected in tcache");
+          if (__glibc_unlikely (!aligned_OK (tmp)))
+    	malloc_printerr ("free(): unaligned chunk detected in tcache 2");
+          if (tmp == e)
+    	malloc_printerr ("free(): double free detected in tcache 2");
+          /* If we get here, it was a coincidence.  We've wasted a
+    	 few cycles, but don't abort.  */
+        }
     }
 }


Cheers,
Wilco