[PATCH] [PATCH] malloc: check tcache mem size in tcache_get_n to avoid arbitrary mem allocation

[dbgbgtf]

Hi!

> On AArch64 the alignment check does usually trigger on that final malloc in the test
> without your patch, so it is capable of detecting this particular corruption. That seems
> to be an issue with the testcase, however it's true the existing checks aren't consistent.

Sorry for that silly mistake in testcase.
I fixed it by adding a small check to make sure the target address
is correctly aligned, which hacker could do the same way to avoid alignment check.

So you can see the current check won't be enough.Hacker can get
arbitrary mem allocation from tcache only if the target address is
correctly aligned, which I don't really think is a good idea.

> Currently tcache_put doesn't always guarantee the chunk is aligned,
> but we could trivially ensure that by moving the alignment check inside it

Putting check in `tcache_put` could be a bad idea.
I mean, in my testcase, changing the `ptr[0]->fd` happens after free().

So `tcache_put` usually won't be able to check the changing of `ptr[0]->fd`.
But the good news is you can putting chunks into tcache without any checks.
Just like I remove the checks in fastbin smashing.

> I believe that this is the best way forward - rather than just
> adding random checks on some paths, we should be maintaining clear invariants on
> all paths.

What about putting the checks in `tcache_get`, because checks in
`tcache_put` can be bypassed

> Changing tcache_get_n to check the next pointer ensures your testcase passes without
> needing additional checks.

Could you explain how the check is carried out?

Cheers,
dbgbgtf

Signed-off-by: dbgbgtf <dudududuMaxVer@gmail.com>
---
 malloc/malloc.c                     |  7 +--
 malloc/tst-tcache-arbitrary-alloc.c | 69 +++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 5 deletions(-)
 create mode 100644 malloc/tst-tcache-arbitrary-alloc.c

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 23b9306a19..e2341f2da8 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3186,6 +3186,8 @@ tcache_get_n (size_t tc_idx, tcache_entry **ep)
 
   if (__glibc_unlikely (!aligned_OK (e)))
     malloc_printerr ("malloc(): unaligned tcache chunk detected");
+  if (__glibc_unlikely (tc_idx != csize2tidx(chunksize_nomask (mem2chunk(e)))))
+    malloc_printerr ("malloc(): tcache mem size vs request size");
 
   if (ep == &(tcache->entries[tc_idx]))
       *ep = REVEAL_PTR (e->next);
@@ -3993,11 +3995,6 @@ _int_malloc (mstate av, size_t bytes)
 		  while (tcache->counts[tc_idx] < mp_.tcache_count
 			 && (tc_victim = *fb) != NULL)
 		    {
-		      if (__glibc_unlikely (misaligned_chunk (tc_victim)))
-			malloc_printerr ("malloc(): unaligned fastbin chunk detected 3");
-		      size_t victim_tc_idx = csize2tidx (chunksize (tc_victim));
-		      if (__glibc_unlikely (tc_idx != victim_tc_idx))
-			malloc_printerr ("malloc(): chunk size mismatch in fastbin");
 		      if (SINGLE_THREAD_P)
 			*fb = REVEAL_PTR (tc_victim->fd);
 		      else
diff --git a/malloc/tst-tcache-arbitrary-alloc.c b/malloc/tst-tcache-arbitrary-alloc.c
new file mode 100644
index 0000000000..c37fb0cb18
--- /dev/null
+++ b/malloc/tst-tcache-arbitrary-alloc.c
@@ -0,0 +1,69 @@
+#include <stdbool.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+#define INTERNAL_SIZE_T size_t
+
+#define mem2chunk(p) ((void *)((char *)(p) - CHUNK_HDR_SZ))
+#define SIZE_SZ (sizeof (INTERNAL_SIZE_T))
+#define CHUNK_HDR_SZ (2 * SIZE_SZ)
+
+struct malloc_chunk
+{
+
+  INTERNAL_SIZE_T mchunk_prev_size; /* Size of previous chunk (if free).  */
+  INTERNAL_SIZE_T mchunk_size;      /* Size in bytes, including overhead. */
+
+  struct malloc_chunk *fd; /* double links -- used only if free. */
+  struct malloc_chunk *bk;
+
+  /* Only used for large blocks: pointer to next larger size.  */
+  struct malloc_chunk *fd_nextsize; /* double links -- used only if free. */
+  struct malloc_chunk *bk_nextsize;
+};
+
+typedef struct malloc_chunk *mchunkptr;
+
+int
+main ()
+{
+  void *ptr[4];
+  ptr[0] = malloc (0x100);
+  ptr[1] = malloc (0x100);
+  // malloc aleast two chunk at the same size
+
+  free (ptr[1]);
+  free (ptr[0]);
+  // free the second one, and the the first one
+  // the tcachebin will be tcache->ptr[0]->ptr[1]
+
+  ptr[0] = mem2chunk (ptr[0]);
+
+  // checking if ptr[2] address is correctly aligned like 0x7fff ffff fff0
+  // if ptr[2] is not correctly aligned, use ptr[3] instead
+  // which should be a correctly aligned address
+  bool is_wrong_align = (INTERNAL_SIZE_T)&ptr[2] & 0x8;
+  if (is_wrong_align)
+    {
+      printf ("target address at: %p\n", &ptr[3]);
+      ((mchunkptr)ptr[0])->fd = (mchunkptr)(((INTERNAL_SIZE_T)ptr[0] >> 12)
+                                            ^ (INTERNAL_SIZE_T)(&ptr[3]));
+    }
+  else
+    {
+      printf ("target address at: %p\n", &ptr[2]);
+      ((mchunkptr)ptr[0])->fd = (mchunkptr)(((INTERNAL_SIZE_T)ptr[0] >> 12)
+                                            ^ (INTERNAL_SIZE_T)(&ptr[2]));
+    }
+  // if we change the ptr[0]->fd
+  // the bin will be tcache->ptr[0]->anywhere
+  // I use ptr[2] or ptr[3] address as example
+
+  ptr[1] = malloc (0x100);
+  ptr[0] = malloc (0x100);
+  // and the malloc them, take the tcachebin out
+  // to see if we have target address
+
+  printf ("ptr[0] point at: %p", ptr[0]);
+  // so we get target address now
+}
-- 
2.49.0