Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1683410 > unrolled thread

[PATCH 0/2] exec: Use sane stack rlimit for setuid exec

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] exec: Use sane stack rlimit for setuid exec Kees Cook <keescook@chromium.org> - 2017-07-07 22:00 +0200
    Re: [PATCH 0/2] exec: Use sane stack rlimit for setuid exec Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-07 22:10 +0200
      Re: [PATCH 0/2] exec: Use sane stack rlimit for setuid exec Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-07 22:20 +0200
        Re: [PATCH 0/2] exec: Use sane stack rlimit for setuid exec Kees Cook <keescook@chromium.org> - 2017-07-08 00:20 +0200
      Re: [PATCH 0/2] exec: Use sane stack rlimit for setuid exec Kees Cook <keescook@chromium.org> - 2017-07-08 00:20 +0200
        Re: [PATCH 0/2] exec: Use sane stack rlimit for setuid exec Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-08 00:50 +0200
      Re: [PATCH 0/2] exec: Use sane stack rlimit for setuid exec Kees Cook <keescook@chromium.org> - 2017-07-08 06:00 +0200
    Re: [PATCH 0/2] exec: Use sane stack rlimit for setuid exec Andy Lutomirski <luto@kernel.org> - 2017-07-08 00:00 +0200
      Re: [PATCH 0/2] exec: Use sane stack rlimit for setuid exec Kees Cook <keescook@chromium.org> - 2017-07-08 00:20 +0200

#1683410 — [PATCH 0/2] exec: Use sane stack rlimit for setuid exec

As discussed with Linus and Andy, we need to reset the stack rlimit
before we do memory layouts when execing a privilege-gaining (e.g.
setuid) program. This moves security_bprm_secureexec() earlier (with
required changes), and then lowers the stack limit when appropriate.

As a side-effect, dumpability is expanded to cover capabilities and
other LSM definitions of secureexec, and Smack can drop its special
handler for pdeath_signal clearing.

I'd appreciate some extra eyes on this to make sure this isn't
broken in some special way. I couldn't find anything that _depended_
on security_bprm_secureexec() being called late.

Thanks!

-Kees

[toc] | [next] | [standalone]

#1683414

On Fri, Jul 7, 2017 at 12:56 PM, Kees Cook <keescook@chromium.org> wrote:
> As discussed with Linus and Andy, we need to reset the stack rlimit
> before we do memory layouts when execing a privilege-gaining (e.g.
> setuid) program. This moves security_bprm_secureexec() earlier (with
> required changes), and then lowers the stack limit when appropriate.

Looks sane to me, and that first patch looks like a nice cleanup
regardless - the old semantics were insane.

But yes, we should have more people look at this, particular have the
security module people look at that first patch to make sure it is the
right thing to do for their policies, and make sure that everybody's
bprm_secureexec() function actually looks at the creds in the brmp,
not "current" (well, maybe they compare the two, which makes tons of
sense, and which the old  placement didn't sanely support).

It looks like Kees went through the security modules, but having the
people involved double-check is a good good idea.

                 Linus

[toc] | [prev] | [next] | [standalone]

#1683418

On Fri, Jul 7, 2017 at 1:04 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> It looks like Kees went through the security modules [..]

i take that back. It looks like Kees looked at smack, but not at
SElinux, for example.

selinux_bprm_secureexec() seems to just look at current_security(),
not at the new stuff in bprm at all.

Which would seem to be exactly the wrong thing to do, and is insane
(why pass in bprm at all?) but comes from the fact that we used to
call bprm_secureexec() in an insane place.

So I think this patch series is sadly broken - I think it does the
right thing, but the security modules definitely look like they need
to be updated for that right thing.

                    Linus

[toc] | [prev] | [next] | [standalone]

#1683442

On Fri, Jul 7, 2017 at 1:09 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jul 7, 2017 at 1:04 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> It looks like Kees went through the security modules [..]
>
> i take that back. It looks like Kees looked at smack, but not at
> SElinux, for example.

Well, I looked at the all but misthought about SELinux.

> selinux_bprm_secureexec() seems to just look at current_security(),
> not at the new stuff in bprm at all.

I was looking for cred. Yeah, I'll see what should happen here...

> Which would seem to be exactly the wrong thing to do, and is insane
> (why pass in bprm at all?) but comes from the fact that we used to
> call bprm_secureexec() in an insane place.
>
> So I think this patch series is sadly broken - I think it does the
> right thing, but the security modules definitely look like they need
> to be updated for that right thing.

Yeah, I'll rev this once SELinux is more clear...

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1683444

On Fri, Jul 7, 2017 at 1:04 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jul 7, 2017 at 12:56 PM, Kees Cook <keescook@chromium.org> wrote:
>> As discussed with Linus and Andy, we need to reset the stack rlimit
>> before we do memory layouts when execing a privilege-gaining (e.g.
>> setuid) program. This moves security_bprm_secureexec() earlier (with
>> required changes), and then lowers the stack limit when appropriate.
>
> Looks sane to me, and that first patch looks like a nice cleanup
> regardless - the old semantics were insane.

I wonder if we could collapse all the secureexec logic in
setup_new_exec. There are three places (?). I was shy to consolidate
those in this patch in case there were weird dependencies on
dumpability ordering. But I'll go see if I can clean those up too...

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1683450

On Fri, Jul 7, 2017 at 3:13 PM, Kees Cook <keescook@chromium.org> wrote:
>
> I wonder if we could collapse all the secureexec logic in
> setup_new_exec.

Probably.

Some of our insane calls back-and-forth between different layers are
due to people abstracting things out and trying very hard to keep old
(and bad) orderings without trying to really determine if they are the
right thing to do.

We *have* occasionally collapsed things when it became obvious just
how crazy things were, but not very often.

There's another thing that I _think_ should be cleaned up:

     install_exec_creds(bprm);

should also be moved into setup_new_exec().

It used to be at a different point in the load sequence, but we fixed
all that up in the ELF loader, but we kept it in the *callers* because
some of the old loaders have different sequences.

But it's quite likely that all the other loaders should be fixed to do
what ELF does. I think they currently have the odd old semantics that
they may load the binary using the old permissions, so a suid binary
needs to be readable by non-root users (which is just stupid).

But it's nasty nasty work to go through and check what subtle things
might change.

Which is why nobody ever does it ;(

                  Linus

[toc] | [prev] | [next] | [standalone]

#1683522

On Fri, Jul 7, 2017 at 1:04 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Fri, Jul 7, 2017 at 12:56 PM, Kees Cook <keescook@chromium.org> wrote:
>> As discussed with Linus and Andy, we need to reset the stack rlimit
>> before we do memory layouts when execing a privilege-gaining (e.g.
>> setuid) program. This moves security_bprm_secureexec() earlier (with
>> required changes), and then lowers the stack limit when appropriate.
>
> Looks sane to me, and that first patch looks like a nice cleanup
> regardless - the old semantics were insane.
>
> But yes, we should have more people look at this, particular have the
> security module people look at that first patch to make sure it is the
> right thing to do for their policies, and make sure that everybody's
> bprm_secureexec() function actually looks at the creds in the brmp,
> not "current" (well, maybe they compare the two, which makes tons of
> sense, and which the old  placement didn't sanely support).
>
> It looks like Kees went through the security modules, but having the
> people involved double-check is a good good idea.

Updated tree here, I'll send the series in email on Monday:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/setuid-rlimits/secureexec

This should fix the missed bprm->cred->security and breaks out each
step into logical pieces in case we need to sanely bisect.

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1683435

On Fri, Jul 7, 2017 at 12:56 PM, Kees Cook <keescook@chromium.org> wrote:
> As discussed with Linus and Andy, we need to reset the stack rlimit
> before we do memory layouts when execing a privilege-gaining (e.g.
> setuid) program. This moves security_bprm_secureexec() earlier (with
> required changes), and then lowers the stack limit when appropriate.

As I see it, there are two cases to harden:

1. Bad guy has a high rlimit and runs a setuid program with crazy
large arguments.  This is improved by this patch.  It's not entirely
clear to me exactly what problem is solved, though, except that the
rest of the exec code does not sanely check that we haven't used too
much stack.  How about putting a check later on to make sure that
we're not running low on stack rather than hoping we got the
arithmetic right?

2. Bad guy wants to trigger stack exhaustion in a setuid program at a
controlled location and thus sets a crazy low rlimit.  This isn't
addressed at all by this patch, but I assume it's what grsecurity was
trying to do.  FWIW, I seem to recall that a lot of setuid attacks use
intentionally weird rlimits to trigger unexpected signals.

--Andy

[toc] | [prev] | [next] | [standalone]

#1683443

On Fri, Jul 7, 2017 at 2:55 PM, Andy Lutomirski <luto@kernel.org> wrote:
> On Fri, Jul 7, 2017 at 12:56 PM, Kees Cook <keescook@chromium.org> wrote:
>> As discussed with Linus and Andy, we need to reset the stack rlimit
>> before we do memory layouts when execing a privilege-gaining (e.g.
>> setuid) program. This moves security_bprm_secureexec() earlier (with
>> required changes), and then lowers the stack limit when appropriate.
>
> As I see it, there are two cases to harden:
>
> 1. Bad guy has a high rlimit and runs a setuid program with crazy
> large arguments.  This is improved by this patch.  It's not entirely
> clear to me exactly what problem is solved, though, except that the
> rest of the exec code does not sanely check that we haven't used too
> much stack.  How about putting a check later on to make sure that
> we're not running low on stack rather than hoping we got the
> arithmetic right?

The rest of the exec uses a relatively fixed amount of space. (AT_*,
etc.) I didn't see any other dynamic stack usage, but maybe I missed
it?

>
> 2. Bad guy wants to trigger stack exhaustion in a setuid program at a
> controlled location and thus sets a crazy low rlimit.  This isn't
> addressed at all by this patch, but I assume it's what grsecurity was
> trying to do.  FWIW, I seem to recall that a lot of setuid attacks use
> intentionally weird rlimits to trigger unexpected signals.

It looks like they were protecting against 1:

        if (((!uid_eq(bprm->cred->euid, current_euid())) ||
(!gid_eq(bprm->cred->egid, current_egid()))) &&
            (old_rlim[RLIMIT_STACK].rlim_cur > (8 * 1024 * 1024)))
                current->signal->rlim[RLIMIT_STACK].rlim_cur = 8 * 1024 * 1024;

For 2, I think we need another examination of how things will fail
with too low a limit.

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web