Re: [tip:x86/mm] x86/mmap, ASLR: Do not treat unlimited-stack tasks as legacy mmap

[Oleg Nesterov]

On 06/27, Jiri Kosina wrote:
>
> On Fri, 23 Jun 2017, Oleg Nesterov wrote:
>
> > > We added a heuristics to treat applications with RLIMIT_STACK configured
> > > to unlimited as legacy. This means:
> >
> > To me this also means a minor security problem. The comment above
> > PER_CLEAR_ON_SETID says "must be cleared upon setuid or setgid exec",
> > but if you do "ulimit -s unlimited" before suid exec then
> > ADDR_COMPAT_LAYOUT set by security checks will be ignored.
>
> Could you please be a bit more specific here?
>
> mmap_is_legacy() *first* checks for the ADDR_COMPAT_LAYOUT in the
> personality flags, and only then, if it's unset, RLIMIT_STACK comes to
> play.

Yes, and this means that even if ADDR_COMPAT_LAYOUT was cleared by

	current->personality &= ~bprm->per_clear;

in flush_old_exec() mmap_is_legacy() still returns true if
rlimit(STACK) == INFINITY.

IOW. Say, in case of suid exec bprm_fill_uid() sets
bprm->per_clear = PER_CLEAR_ON_SETID which includes ADDR_COMPAT_LAYOUT.
To me, this means that we do not want the legacy layout after suid exec,
but "ulimit -s unlimited" can be used to break the rule.



And let me quote my "rlimits && suid exec" email I sent some time before...

Imo RLIMITs are almost pointless security-wise, but now it seems to me they
can harm.

Say, the comment above PER_CLEAR_ON_SETID says "must be cleared upon setuid or
setgid exec" and this mask includes ADDR_COMPAT_LAYOUT. OK, this makes sense,
but this doesn't really work because you can just do "$ ulimit -s unlimited"
before suid exec and this will make mmap_is_legacy() return true. Of course,
only if rlim_max=RLIM_INFINITY, but afaik usually this is true.

Or you can lower RLIMIT_STACK to make suid app crash inside some "system
critical" section...

And even if we forget about the potential security impact, isn't it strange
that suid exec inherits RLIMITs from non-root process?

Perhaps it makes sense to reset RLIMITs on suid exec (say, if bprm->per_clear
is not zero) ? Yes, it is not clear how should we define SANE_RLIMITS_FOR_SUID,
and this should probably depend on sysctl, etc.


Oleg.