[PATCH 8/8] elf: Scrub and reseed the AT_RANDOM bytes after deriving the guards (BZ 34197)

[DJ Delorie]

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> Once the pointer and stack guards have been derived from AT_RANDOM, scrub
> the bytes and refill them with new random data unrelated to the guards.
> On Linux, it uses getrandom syscall (as for tcache_key_initialize), and
> fallback to zero the memory if the syscall is not avaiable.
>
> This keeps AT_RANDOM useful to applications while ensuring those bytes no
> longer reveal the guards.
>
> The work is done by _dl_reseed_random, called once the guards are in place
> and before any ELF constructor can observe AT_RANDOM: in security_init for
> the dynamic loader and in __libc_start_main for statically linked programs.

> diff --git a/csu/libc-start.c b/csu/libc-start.c
> +  _dl_reseed_random (&_dl_random);

Ok.

> diff --git a/elf/rtld.c b/elf/rtld.c
> -  /* We do not need the _dl_random value anymore.  The less
> -     information we leave behind, the better, so clear the
> -     variable.  */

I see no reason to remove this comment; it's still valid.  If anything,
it should be expanded to include the new logic.  The other caller has no
comment, though...

> -  _dl_random = NULL;
> +  _dl_reseed_random (&_dl_random);

can _dl_reseed_random ever be called twice in the same program?  This
second one happens when we load audit modules, but you don't test that
case, and I wonder if calling it twice might result in NULL guards, or
mismatched ones...

> diff --git a/elf/tst-atrandom-scrub.c b/elf/tst-atrandom-scrub.c

> +/* The loader (security_init) and the static startup code (__libc_start_main)
> +   derive the stack and pointer guards from the AT_RANDOM bytes, scrub those
> +   bytes, and refill them with fresh entropy unrelated to the guards.  The
> +   AT_RANDOM entry is kept, so getauxval (AT_RANDOM) keeps returning 16 random
> +   bytes, but they no longer reveal the guards.  Check that neither guard can
> +   be reconstructed from AT_RANDOM and that no auxiliary vector entry holds a
> +   guard value.  */
> +
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/auxv.h>
> +
> +#include <stackguard-macros.h>
> +#include <tls.h>
> +#include <support/check.h>
> +
> +static int
> +do_test (void)
> +{
> +  uintptr_t stack_guard = STACK_CHK_GUARD;
> +  uintptr_t pointer_guard = POINTER_CHK_GUARD;
> +
> +  unsigned char *random = (unsigned char *) getauxval (AT_RANDOM);
> +  if (random == NULL)
> +    FAIL_UNSUPPORTED ("the kernel did not provide AT_RANDOM");
> +
> +  printf ("debug: stack guard   = %0*jx\n",
> +          (int) (2 * sizeof (uintptr_t)), (uintmax_t) stack_guard);
> +  printf ("debug: pointer guard = %0*jx\n",
> +          (int) (2 * sizeof (uintptr_t)), (uintmax_t) pointer_guard);
> +  printf ("debug: AT_RANDOM     = ");
> +  for (int i = 0; i < 16; i++)
> +    printf ("%02x", random[i]);
> +  printf ("\n");

Ok.

> +  /* Reconstruct the guards from the (reseeded) AT_RANDOM bytes the way the
> +     loader does and check that they no longer match the live guards.  */
> +  uintptr_t recovered_stack;
> +  memcpy (&recovered_stack, random, sizeof (recovered_stack));
> +#if __BYTE_ORDER == __LITTLE_ENDIAN
> +  recovered_stack &= ~(uintptr_t) 0xff;
> +#else
> +  recovered_stack &= ~((uintptr_t) 0xff << (8 * (sizeof (recovered_stack) - 1)));
> +#endif
> +  TEST_VERIFY (recovered_stack != stack_guard);
> +
> +  uintptr_t recovered_pointer;
> +  memcpy (&recovered_pointer, random + sizeof (uintptr_t),
> +          sizeof (recovered_pointer));
> +  TEST_VERIFY (recovered_pointer != pointer_guard);

Ok.

Should be verify that none of (stack_guard, pointer_guard, AT_RANDOM)
are all zeros?  Even assuming that the kernel can always give us more
random bits, we should ensure that they're being filled properly.

> diff --git a/sysdeps/generic/dl-reseed-random.h b/sysdeps/generic/dl-reseed-random.h
> +#include <string.h>
> +
> +static inline void __attribute__ ((always_inline))
> +_dl_reseed_random (void **dl_random)
> +{
> +  if (*dl_random == NULL)
> +    return;
> +  memset (*dl_random, '\0', 16);
> +  __asm__ __volatile__ ("" : : "r" (*dl_random) : "memory");
> +  *dl_random = NULL;
> +}
> +

Ok.

> diff --git a/sysdeps/unix/sysv/linux/dl-reseed-random.h b/sysdeps/unix/sysv/linux/dl-reseed-random.h
> +#include <string.h>
> +#include <not-cancel.h>
> +#include <sys/random.h>
> +
> +/* The stack and pointer guards have been derived from the 16 AT_RANDOM
> +   bytes pointed to by DL_RANDOM.  Scrub them first, so the guards cannot be
> +   recovered even if the refill below fails, then refill them with fresh
> +   entropy unrelated to the guards so that getauxval (AT_RANDOM) keeps
> +   returning random bytes.  */
> +static inline void __attribute__ ((always_inline))
> +_dl_reseed_random (void **dl_random)
> +{
> +  if (*dl_random == NULL)
> +    return;
> +  memset (*dl_random, '\0', 16);
> +  __asm__ __volatile__ ("" : : "r" (*dl_random) : "memory");
> +
> +  __getrandom_nocancel_nostatus_direct (*dl_random, 16, GRND_NONBLOCK);
> +  _dl_random = NULL;

Why access _dl_random directly here, and not &dl_random like the generic case?

IMHO This looks too much like a typo.