Re: [PATCH 2/2] x86/mmap: handle worst-case heap randomization in mmap_base

[Kees Cook]

On Wed, Mar 13, 2019 at 3:58 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Tue, Mar 12, 2019 at 10:33 AM Ali Saidi <alisaidi@xxxxxxxxxx> wrote:
> >
> > Increase mmap_base by the worst-case brk randomization so that
> > the stack and heap remain apart.
> >
> > In Linux 4.13 a change was committed that special cased the kernel ELF
> > loader when the loader is invoked directly (eab09532d400; binfmt_elf: use
> > ELF_ET_DYN_BASE only for PIE). Generally, the loader isnât invoked
> > directly and this issue is limited to cases where it is, (e.g to set a
> > non-inheritable LD_LIBRARY_PATH, testing new versions of the loader). In
> > those rare cases, the loader doesn't take into account the amount of brk
> > randomization that will be applied by arch_randomize_brk(). This can
> > lead to the stack and heap being arbitrarily close to each other.
>
> In the case of using the loader directly, brk (so helpfully identified
> as "[heap]") is allocated with the _loader_ not the binary. For
> example, with ASLR entirely disabled, you can see this more clearly:
>
> $ /bin/cat /proc/self/maps
> 555555554000-55555555c000 r-xp 00000000 fd:02 34603015
>   /bin/cat
> 55555575b000-55555575c000 r--p 00007000 fd:02 34603015
>   /bin/cat
> 55555575c000-55555575d000 rw-p 00008000 fd:02 34603015
>   /bin/cat
> 55555575d000-55555577e000 rw-p 00000000 00:00 0                          [heap]
> ...
> 7ffff7ff7000-7ffff7ffa000 r--p 00000000 00:00 0                          [vvar]
> 7ffff7ffa000-7ffff7ffc000 r-xp 00000000 00:00 0                          [vdso]
> 7ffff7ffc000-7ffff7ffd000 r--p 00027000 fd:02 49287483
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7ffd000-7ffff7ffe000 rw-p 00028000 fd:02 49287483
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7ffe000-7ffff7fff000 rw-p 00000000 00:00 0
> 7ffffffde000-7ffffffff000 rw-p 00000000 00:00 0                          [stack]
>
> $ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
> ...
> 7ffff7bcc000-7ffff7bd4000 r-xp 00000000 fd:02 34603015
>   /bin/cat
> 7ffff7bd4000-7ffff7dd3000 ---p 00008000 fd:02 34603015
>   /bin/cat
> 7ffff7dd3000-7ffff7dd4000 r--p 00007000 fd:02 34603015
>   /bin/cat
> 7ffff7dd4000-7ffff7dd5000 rw-p 00008000 fd:02 34603015
>   /bin/cat
> 7ffff7dd5000-7ffff7dfc000 r-xp 00000000 fd:02 49287483
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7fb2000-7ffff7fd6000 rw-p 00000000 00:00 0
> 7ffff7ff7000-7ffff7ffa000 r--p 00000000 00:00 0                          [vvar]
> 7ffff7ffa000-7ffff7ffc000 r-xp 00000000 00:00 0                          [vdso]
> 7ffff7ffc000-7ffff7ffd000 r--p 00027000 fd:02 49287483
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7ffd000-7ffff7ffe000 rw-p 00028000 fd:02 49287483
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7ffff7ffe000-7ffff8020000 rw-p 00000000 00:00 0                          [heap]
> 7ffffffde000-7ffffffff000 rw-p 00000000 00:00 0                          [stack]
>
> So I think changing this globally isn't the right solution (normally
> brk is between text and mmap). Adjusting the mmap base padding means
> we lose even more memory space. Perhaps it would be better if brk
> allocation would be placed before the mmap region (i.e. use
> ELF_ET_DYN_BASE). This seems to work for me:
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 7d09d125f148..cdaa33f4a3ef 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1131,6 +1131,15 @@ static int load_elf_binary(struct linux_binprm *bprm)
>         current->mm->end_data = end_data;
>         current->mm->start_stack = bprm->p;
>
> +       /*
> +        * When executing a loader directly (ET_DYN without Interp), move
> +        * the brk area out of the mmap region (since it grows up, and may
> +        * collide early with the stack growing down), and into the unused
> +        * ELF_ET_DYN_BASE region.
> +        */
> +       if (!elf_interpreter)
> +               current->mm->brk = current->mm->start_brk = ELF_ET_DYN_BASE;
> +
>         if ((current->flags & PF_RANDOMIZE) && (randomize_va_space > 1)) {
>                 current->mm->brk = current->mm->start_brk =
>                         arch_randomize_brk(current->mm);
>
> $ /lib/x86_64-linux-gnu/ld-2.27.so /bin/cat /proc/self/maps
> 555556de3000-555556e04000 rw-p 00000000 00:00 0                          [heap]
> 7f8467da9000-7f8467f90000 r-xp 00000000 fd:01 399295
>   /lib/x86_64-linux-gnu/libc-2.27.so
> ...
> 7f846819a000-7f84681a2000 r-xp 00000000 fd:01 263229
>   /bin/cat
> ...
> 7f84685cb000-7f84685cc000 rw-p 00028000 fd:01 399286
>   /lib/x86_64-linux-gnu/ld-2.27.so
> 7f84685cc000-7f84685cd000 rw-p 00000000 00:00 0
> 7ffce68f8000-7ffce6919000 rw-p 00000000 00:00 0                          [stack]
> 7ffce69f0000-7ffce69f3000 r--p 00000000 00:00 0                          [vvar]
> 7ffce69f3000-7ffce69f4000 r-xp 00000000 00:00 0                          [vdso]
>
> Does anyone see problems with this? (Note that ET_EXEC base is
> 0x400000, so no collision there...)

Adding some more people to CC... what do people think about this
moving of the brk to ELF_ET_DYN_BASE in this corner-case? Anything
that worked before should still work (i.e. the ultimately-launched
binary already had the brk very far from its text, so this should be
no different from a COMPAT_BRK standpoint). The only risk I see here
is that if someone started to suddenly depend on the entire memory
space above the mmap region being available when launching binaries
via a direct loader execs... which seems highly unlikely, I'd hope:
this would mean a binary would not work when execed normally.

-- 
Kees Cook