[PATCH] malloc: Fix [BZ #22853] weak randomization on thread arenas.

[iripoll]

Hi Adhemerval,


   Basically there are two issues to consider:

1.- How much entropy shall be added to the arenas.
2.- Where the entropy is taken.


El 24/3/20 a las 15:58, Adhemerval Zanella escribió:
 >
 >
 > On 24/03/2020 11:40, Hector Marco wrote:
 >> Hello Adhemerval,
 >>
 >> Good to see you are processing this. Please let me share our view about
 >> the issue here:
 >>
 >>>
 >>> My main issue about using ASLR entropy is it provides different values
 >>> depending of the underlying architecture and kernel configuration. And
 >>> glibc still support some architecture that do not support ASLR at all.
 >>
 >> We don't see the problem here. Can you please elaborate a little bit
 >> more what's wrong? To our understanding (and what our patch does) if an
 >> architecture does not support ASLR then no randomization is applied.
 >
 > My understanding of this idea hardening is to randomize the arenas using
 > ASLR entropy and my point is to provide such hardening even when kernel
 > does not provide such ASLR.
 >

  The problem with the ASLR is that all the memory zones have to be randomized in order to be really 
effective. Therefore, if the libraries or thread stacks are not randomized, then there is no point 
to randomize the heaps of the threads.

 >>
 >>> And, as you pointed out, even for some ASLR enabled architectures the
 >>> entropy most kernels provides is low.
 >>
 >> The kernel entropy is not low. Actually the aliment is the one
 >> destroying the entropy but the entropy provided by the kernel is a high
 >> quality entropy one.
 >
 > My point is it might be considered low depending of the architecture and
 > kernel configuration, although it might not an issue once the heap
 > alignment is not taking in consideration (for instance i686 will provide
 > 8 bits, where 17 would be required to get a fully randomized placement
 > on the mmap area). So my question how randomized heap_info placement
 > do we really require here?
 >

   ASLR is extremely tricky and subtle issue. It may affect performance (caused by alignment or 
cache row collisions) and memory space (due to extra page table entries, the pagetable is less 
compact). Even more, sometimes the ASLR was used to improve the performance. For example, a few 
random bits were introduced in the thread stacks to improve performance.

   It would be a good exercise to implement the ASLR in the library, but it is a
far more complex issue, which requires a much deeper analysis.


   In summary, we think that it shall have (at least) the same entropy
than the less randomized memory object (the mmaps). This way, an
attacker can not take advantage of it. Which is the goal of the patch.

 >
 >>
 >>>
 >>> That's why I would prefer a platform-neutral solution.  As I pointed
 >>> out some bits from AT_RANDOM are still unused, and I don't see why
 >>> we can't use it here.
 >>>
 >>
 >> We don't thing so. The kernel provides 16 random bytes via AT_RANDOM and
 >> on 64-bit architectures those bytes are already used by the stack
 >> smashing protector and the pointer guard glibc. Please look at the
 >> sysdeps/generic/dl-osinfo.h file.
 >
 > I think you mean sysdeps/unix/sysv/linux/dl-osinfo.h, since it is the
 > one used for Linux.  And my idea was to use something similar of what
 > Florian has suggested some time ago [1] to use the AT_RANDOM as a feeder.
 >
 > [1] https://sourceware.org/legacy-ml/libc-alpha/2016-10/msg00531.html
 >

This is the second problem: Where the entropy comes from.

As you suggest, the entropy can be taken from a CSPRNG. But we shall test/consider
the time overhead.

But if the entropy is not "linked" with the original ASLR, then we will need to enable another 
feature to disable it. Let's see an example:

If I want to run a process without ASLR, now we can:
$ setarch x86_64 -R ./testing

But if we use tunables, then we will need to use something like:

$ GLIBC_TUNABLES=glibc.malloc.arena_randomization=0 setarch x86_64 -R ./testing

Which is possible, but it looks cumbersome and it is hard to explain why we have to disable arenas 
randomization. It may raise the question whether threads stacks can also be disabled or not and why.

   During the development of the patch we analyzed these options, and we discarded them because 
complexity.

   If you think that adding extra randomization bits to the arenas is necessary, then the tunables 
shall be the choice. But as we stated before, we think that it is not necessary.

 >>
 >> Therefore, we will be introducing a correlation (weakness) and attackers
 >> could use a partial info leak to bypass the canary (among other attacks).
 >>
 >> A different history would be to change how we are using the entropy from
 >> AT_RANDOM. As suggested by kernel developers, we could probably use it
 >> to feed a CSPRNG (note that we cannot claim that arc4random is an
 >> entropy source but an entropy collector or intermediary).
 >>
 >> We didn't go for this because the obvious overhead introduced. Right now
 >> our solution has zero overhead and it provides good entropy (a
 >> info leak cannot be used to leak anything else than what it is already
 >> known, the mmap area).
 >
 > Alright, my comments are not a blocker to this patch. What I would to
 > discuss if we should also randomize the arenas for environments that
 > do not provide ASLR (or limited entropy) and which would be best way
 > to do it.
 >

   Discussion is necessary and clarifying. Thanks.

   Regarding the test. The way to measure the entropy is very similar to the one used in the paxtest 
tool. Which is good enough for testing the correctness of the patch. But once the patch is 
integrated, then we don't think that the test is necessary any longer.

Regards.

 >>
 >>
 >>>>
 >>>> Also, remember that if the randomization of the arenas are
 >> independent of the rest of the memory layout, then there must be a
 >> mechanism to disable it. It is necessary for debugging.
 >>>
 >>> For such scenario we might disable randomization with a tunable.
 >>>
 >>
 >> Yes, we can but as you said, there are architectures that does not
 >> support ASLR and it sounds odd to disable part of the ASLR on
 >> architectures that have no ASLR at all.
 >>
 >>