Re: [PATCH] mm: larger stack guard gap, between vmas

Kees Cook
2017-07-05T23:50:19Z

On Wed, Jul 5, 2017 at 10:23 AM, Andy Lutomirski <luto@kernel.org> wrote:
> Right. But I think the approach that we're all taking here is a bit
> nutty. We all realize that this issue is a longstanding *GCC* bug
> [1], but we're acting like it's a Big Deal (tm) kernel bug that Must
> Be Fixed (tm) and therefore is allowed to break ABI. My security hat
> is normally pretty hard-line, but I think it may be time to call BS.
>
> Imagine if Kees had sent some symlink hardening patch that was
> default-on and broke a stock distro. Or if I had sent a vsyscall
> hardening patch that broke real code. It would get reverted right
> away, probably along with a diatribe about how we should have known
> better. I think this stack gap stuff is the same thing. It's not a
> security fix -- it's a hardening patch.
>
> Looking at it that way, I think a new inherited-on-exec flag is nucking futs.
>
> I'm starting to think that the right approach is to mostly revert all
> this stuff (the execve fixes are fine). Then start over and think
> about it as hardening. I would suggest the following approach:
>
> - The stack gap is one page, just like it's been for years.
> - As a hardening feature, if the stack would expand within 64k or
> whatever of a non-MAP_FIXED mapping, refuse to expand it. (This might
> have to be a non-hinted mapping, not just a non-MAP_FIXED mapping.)
> The idea being that, if you deliberately place a mapping under the
> stack, you know what you're doing. If you're like LibreOffice and do
> something daft and are thus exploitable, you're on your own.
> - As a hardening measure, don't let mmap without MAP_FIXED position
> something within 64k or whatever of the bottom of the stack unless a
> MAP_FIXED mapping is between them.
>
> And that's all. It's not like a 64k gap actually fixes these bugs for
> real -- it just makes them harder to exploit.
>
> [1] The code that GCC generates for char buf[bug number] and alloca()
> is flat-out wrong. Everyone who's ever thought about it all all knows
> it and has known about it for years, but no one cared to fix it.

As part of that should we put restrictions on the environment of
set*id exec too? Part of the risks demonstrated by Qualys was that
allowing a privilege-elevating binary to inherit rlimits can have lead
to the nasty memory layout side-effects. That would fall into the
"hardening" bucket as well. And if it turns out there is some set*id
binary out there that can't run with "only", e.g., 128MB of stack, we
can make it configurable...

-Kees

--
Kees Cook
Pixel Security