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

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

[Kees Cook]

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

[PATCH 1/2] exec: Move security_bprm_secureexec() earlier

[Kees Cook]

There are several places where exec needs to know if a privilege-gain
has happened. Right now it only checks uid/euid differences, though
capabilities could have been elevated too. This moves the secureexec
check ahead of committing credentials, and retains the value for later
examination. A resulting redundant case of clearing pdeath_signal is
also removed from Smack, and commoncap is updated to examine bprm->cred
instead of current->cred.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/binfmt_elf.c            |  2 +-
 fs/binfmt_elf_fdpic.c      |  2 +-
 fs/exec.c                  | 15 ++++++++++-----
 include/linux/binfmts.h    |  3 ++-
 include/linux/lsm_hooks.h  |  3 ++-
 security/commoncap.c       |  4 +---
 security/smack/smack_lsm.c | 15 ---------------
 7 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 5075fd5c62c8..7f6ec4dac13d 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -254,7 +254,7 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
 	NEW_AUX_ENT(AT_EUID, from_kuid_munged(cred->user_ns, cred->euid));
 	NEW_AUX_ENT(AT_GID, from_kgid_munged(cred->user_ns, cred->gid));
 	NEW_AUX_ENT(AT_EGID, from_kgid_munged(cred->user_ns, cred->egid));
- 	NEW_AUX_ENT(AT_SECURE, security_bprm_secureexec(bprm));
+	NEW_AUX_ENT(AT_SECURE, bprm->secureexec);
 	NEW_AUX_ENT(AT_RANDOM, (elf_addr_t)(unsigned long)u_rand_bytes);
 #ifdef ELF_HWCAP2
 	NEW_AUX_ENT(AT_HWCAP2, ELF_HWCAP2);
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index cf93a4fad012..5aa9199dfb13 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -650,7 +650,7 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm,
 	NEW_AUX_ENT(AT_EUID,	(elf_addr_t) from_kuid_munged(cred->user_ns, cred->euid));
 	NEW_AUX_ENT(AT_GID,	(elf_addr_t) from_kgid_munged(cred->user_ns, cred->gid));
 	NEW_AUX_ENT(AT_EGID,	(elf_addr_t) from_kgid_munged(cred->user_ns, cred->egid));
-	NEW_AUX_ENT(AT_SECURE,	security_bprm_secureexec(bprm));
+	NEW_AUX_ENT(AT_SECURE,	bprm->secureexec);
 	NEW_AUX_ENT(AT_EXECFN,	bprm->exec);
 
 #ifdef ARCH_DLINFO
diff --git a/fs/exec.c b/fs/exec.c
index ddca2cf15f71..1e8d647d8e7c 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1330,12 +1330,18 @@ EXPORT_SYMBOL(would_dump);
 
 void setup_new_exec(struct linux_binprm * bprm)
 {
+	/* This is the point of no return */
+
+	if (security_bprm_secureexec(bprm)) {
+		/* Record for AT_SECURE. */
+		bprm->secureexec = 1;
+	}
+
 	arch_pick_mmap_layout(current->mm);
 
-	/* This is the point of no return */
 	current->sas_ss_sp = current->sas_ss_size = 0;
 
-	if (uid_eq(current_euid(), current_uid()) && gid_eq(current_egid(), current_gid()))
+	if (!bprm->secureexec)
 		set_dumpable(current->mm, SUID_DUMP_USER);
 	else
 		set_dumpable(current->mm, suid_dumpable);
@@ -1350,9 +1356,8 @@ void setup_new_exec(struct linux_binprm * bprm)
 	 */
 	current->mm->task_size = TASK_SIZE;
 
-	/* install the new credentials */
-	if (!uid_eq(bprm->cred->uid, current_euid()) ||
-	    !gid_eq(bprm->cred->gid, current_egid())) {
+	/* prepare to install the new credentials */
+	if (bprm->secureexec) {
 		current->pdeath_signal = 0;
 	} else {
 		if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP)
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 05488da3aee9..1afaa303cad0 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -27,9 +27,10 @@ struct linux_binprm {
 	unsigned int
 		cred_prepared:1,/* true if creds already prepared (multiple
 				 * preps happen for interpreters) */
-		cap_effective:1;/* true if has elevated effective capabilities,
+		cap_effective:1,/* true if has elevated effective capabilities,
 				 * false if not; except for init which inherits
 				 * its parent's caps anyway */
+		secureexec:1;	/* true when gaining privileges */
 #ifdef __alpha__
 	unsigned int taso:1;
 #endif
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 080f34e66017..d1bd24fb4a33 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -72,7 +72,8 @@
  *	Return a boolean value (0 or 1) indicating whether a "secure exec"
  *	is required.  The flag is passed in the auxiliary table
  *	on the initial stack to the ELF interpreter to indicate whether libc
- *	should enable secure mode.
+ *	should enable secure mode. Called before bprm_committing_creds(),
+ *	so pending credentials are in @bprm->cred.
  *	@bprm contains the linux_binprm structure.
  *
  * Security hooks for filesystem operations.
diff --git a/security/commoncap.c b/security/commoncap.c
index 7abebd782d5e..482d3aac2fc6 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -624,12 +624,10 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
  * Determine whether a secure execution is required, return 1 if it is, and 0
  * if it is not.
  *
- * The credentials have been committed by this point, and so are no longer
- * available through @bprm->cred.
  */
 int cap_bprm_secureexec(struct linux_binprm *bprm)
 {
-	const struct cred *cred = current_cred();
+	const struct cred *cred = bprm->cred;
 	kuid_t root_uid = make_kuid(cred->user_ns, 0);
 
 	if (!uid_eq(cred->uid, root_uid)) {
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 658f5d8c7e76..0c89afcb24c4 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -954,20 +954,6 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
 }
 
 /**
- * smack_bprm_committing_creds - Prepare to install the new credentials
- * from bprm.
- *
- * @bprm: binprm for exec
- */
-static void smack_bprm_committing_creds(struct linux_binprm *bprm)
-{
-	struct task_smack *bsp = bprm->cred->security;
-
-	if (bsp->smk_task != bsp->smk_forked)
-		current->pdeath_signal = 0;
-}
-
-/**
  * smack_bprm_secureexec - Return the decision to use secureexec.
  * @bprm: binprm for exec
  *
@@ -4645,7 +4631,6 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(sb_parse_opts_str, smack_parse_opts_str),
 
 	LSM_HOOK_INIT(bprm_set_creds, smack_bprm_set_creds),
-	LSM_HOOK_INIT(bprm_committing_creds, smack_bprm_committing_creds),
 	LSM_HOOK_INIT(bprm_secureexec, smack_bprm_secureexec),
 
 	LSM_HOOK_INIT(inode_alloc_security, smack_inode_alloc_security),
-- 
2.7.4

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

[Kees Cook]

Before memory layout selection and credentials having been updated,
reset stack rlimit to something sane for setuid execs to avoid having
the caller having control over memory layouts.

$ ulimit -s
8192
$ ulimit -s unlimited
$ /bin/sh -c 'ulimit -s'
unlimited
$ sudo /bin/sh -c 'ulimit -s'
8192

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/exec.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/fs/exec.c b/fs/exec.c
index 1e8d647d8e7c..2b072cf79f6d 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1335,6 +1335,16 @@ void setup_new_exec(struct linux_binprm * bprm)
 	if (security_bprm_secureexec(bprm)) {
 		/* Record for AT_SECURE. */
 		bprm->secureexec = 1;
+
+		/*
+		 * If this is a setuid execution, reset the stack limit to
+		 * sane default to avoid bad behavior from the prior rlimits.
+		 * This has to happen before arch_pick_mmap_layout(), which
+		 * examines RLIMIT_STACK, but after the point of not return
+		 * to avoid cleaning up the change on failure.
+		 */
+		if (current->signal->rlim[RLIMIT_STACK].rlim_cur > _STK_LIM)
+			current->signal->rlim[RLIMIT_STACK].rlim_cur = _STK_LIM;
 	}
 
 	arch_pick_mmap_layout(current->mm);
-- 
2.7.4

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

[Linus Torvalds]

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

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

[Linus Torvalds]

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

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

[Kees Cook]

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

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

[Kees Cook]

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

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

[Linus Torvalds]

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

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

[Kees Cook]

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

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

[Andy Lutomirski]

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

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

[Kees Cook]

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