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

[Wilco Dijkstra]

Hi,

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

The goal is to detect heap corruption cheaply and as early as possible.
Once an attacker can break the pointer swizzling, they can fake chunks that
pass every check we could throw at it. It is feasible to improve the pointer
swizzling to make this a bit harder, however if we wanted to make malloc
safer, we'd have to stop using data structures that are stored in chunks.

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

Tcache_put is used in several places on blocks that have been freed for a while
and were stored in different lists, so they could have been corrupted at that point.

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

You could remove checks on tcache_put, but then you delay detection of
corruption until the chunk gets allocated. If the block was aligned in tcache_put,
then tcache_get does not need to repeat it (for the first block).

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

tcache_get already does a check - what I'm saying is that we can improve it.

>> 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?

Basically change it to check the next field, like:

  if (__glibc_unlikely (!aligned_OK (REVEAL_PTR (e->next))))
    malloc_printerr ("malloc(): unaligned tcache chunk detected");

This works, is cheap and triggers in your testcase.

Cheers,
Wilco