Re: [PATCH 2/2] fs, elf: drop MAP_FIXED from initial ET_DYN segment

[Kees Cook]

On Tue, Oct 17, 2017 at 2:04 AM, Michal Hocko <mhocko@xxxxxxxxxx> wrote:
> On Mon 16-10-17 12:38:19, Kees Cook wrote:
>> On Mon, Oct 16, 2017 at 11:43 AM, Michal Hocko <mhocko@xxxxxxxxxx> wrote:
>> > On Mon 16-10-17 09:44:31, Kees Cook wrote:
>> >> On Mon, Oct 16, 2017 at 6:44 AM, Michal Hocko <mhocko@xxxxxxxxxx> wrote:
>> >> > From: Michal Hocko <mhocko@xxxxxxxx>
>> >> >
>> >> > eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE") has added
>> >> > MAP_FIXED flag to the initial ET_DYN segment mapping which defines the
>> >> > randomized base for the PIE ELF segments. The thing is that MAP_FIXED
>> >> > shouldn't be really needed because the address is essentially random
>> >> > anyway. All other segments are mapped relatively to this base. elf_map
>> >> > makes sure that all segments will fit into the address space by
>> >> > enforcing total_mapping_size initial map.
>> >> >
>> >> > Why do we want to drop MAP_FIXED in the first place? Because it is error
>> >> > prone. If we happen to have an existing mapping in the requested range
>> >> > then we do not want to corrupt it silently. Without MAP_FIXED vm_mmap
>> >> > will simply fallback to another range. In reality there shouldn't be
>> >> > any conflicting mappings at this early exec stage so the mmap should
>> >> > succeed even without MAP_FIXED but subtle changes to the randomization
>> >> > can break this assumption so we should rather be careful here.
>> >> >
>> >> > Fixes: eab09532d400 ("binfmt_elf: use ELF_ET_DYN_BASE only for PIE")
>> >> > Signed-off-by: Michal Hocko <mhocko@xxxxxxxx>
>> >> > ---
>> >> >  fs/binfmt_elf.c | 1 -
>> >> >  1 file changed, 1 deletion(-)
>> >> >
>> >> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
>> >> > index 09456e2add18..244cc30dfa24 100644
>> >> > --- a/fs/binfmt_elf.c
>> >> > +++ b/fs/binfmt_elf.c
>> >> > @@ -988,7 +988,6 @@ static int load_elf_binary(struct linux_binprm *bprm)
>> >> >                                 load_bias = ELF_ET_DYN_BASE;
>> >> >                                 if (current->flags & PF_RANDOMIZE)
>> >> >                                         load_bias += arch_mmap_rnd();
>> >> > -                               elf_flags |= MAP_FIXED;
>> >>
>> >> If MAP_FIXED is being masked out in patch 1 (but used as a check for
>> >> correct position, I think this MAP_FIXED should _not_ be removed).
>> >> This provides for checking for the initial mapping. The failure mode
>> >> here would be to allow an attack to "push" a mapping away from some
>> >> overlapping region. This should not be allowed either: if the initial
>> >> mapping is "wrong", we should absolutely fail, otherwise we can be
>> >> introducing a silent reduction in PIE entropy.
>> >
>> > Do we really lose any entropy? We are using standard randomized mmap in
>> > that case. So we are randomized in either case. Are you worried that
>> > an attacker could tell the two cases and abuse some sort of offset2lib
>> > attack?
>>
>> Not in the regular case. I'm suggesting that what your changes are
>> preparing for is an _unknown_ way to collide mappings. In that case,
>> we should be as defensive as we know how. And if we were to remove
>> MAP_FIXED here, it would allow an attacker (with some future method)
>> to potentially collapse a range of ASLR for execution, since missing
>> MAP_FIXED here would silently move a mapping somewhere else. So we
>> should keep MAP_FIXED, as any collision would indicate an unknown
>> method of crashing an exec into something else.
>
> I am sorry but I do not follow. I could see how offset2lib would be a
> concern but you seem to be thinking about a different scenario. Could
> you be more specific please.

Sorry, I will try a specific example below.

> I am not insisting on this patch but it seems to me is just makes a
> recoverable state a failure.

Right, I understand you're trying to make it recoverable. I'm
suggesting that making it recoverable provides a way for an attack to
abuse it, and that what we'd be recovering from is a case we should
never ever see.

Consider the case where through some future bug/feature, it's possible
to put the stack at an arbitrary location during an exec. (We've
worked to fix that already, but who knows what the future holds either
through misfeatures or bugs.) If an attacker maps the stack over a
large portion of the PIE exec range, patch 2 will result in vmmap
searching out a location that isn't already allocated. This means that
instead of the PIE ASLR choosing from the entire possible range, it
will get limited to only the area where something isn't already
overlapping. This would give an attacker the ability to control the
PIE ASLR, possibly forcing it into a fixed location.

Without patch 2, an unexpected overlap will fail the exec, which is
_good_, because we should never see an overlap. If we do, something
has gone very wrong, and we shouldn't paper over that with a silent
loss of ASLR entropy.

-Kees

-- 
Kees Cook
Pixel Security