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

Groups > linux.kernel > #1700511 > unrolled thread

[PATCH v4 00/15] exec: Use sane stack rlimit under secureexec

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 00/15] exec: Use sane stack rlimit under secureexec Kees Cook <keescook@chromium.org> - 2017-08-01 02:00 +0200
    [PATCH v4 02/15] exec: Correct comments about "point of no return" Kees Cook <keescook@chromium.org> - 2017-08-01 02:00 +0200
    [PATCH v4 10/15] exec: Use secureexec for setting dumpability Kees Cook <keescook@chromium.org> - 2017-08-01 02:00 +0200
      Re: [PATCH v4 10/15] exec: Use secureexec for setting dumpability James Morris <jmorris@namei.org> - 2017-08-01 02:50 +0200
    [PATCH v4 08/15] commoncap: Move cap_elevated calculation into bprm_set_creds Kees Cook <keescook@chromium.org> - 2017-08-01 02:00 +0200
      Re: [PATCH v4 08/15] commoncap: Move cap_elevated calculation into bprm_set_creds Andy Lutomirski <luto@kernel.org> - 2017-08-01 15:50 +0200
    [PATCH v4 11/15] exec: Use secureexec for clearing pdeath_signal Kees Cook <keescook@chromium.org> - 2017-08-01 02:00 +0200
    [PATCH v4 05/15] selinux: Refactor to remove bprm_secureexec hook Kees Cook <keescook@chromium.org> - 2017-08-01 02:00 +0200
      Re: [PATCH v4 05/15] selinux: Refactor to remove bprm_secureexec  hook James Morris <jmorris@namei.org> - 2017-08-01 02:50 +0200
      Re: [PATCH v4 05/15] selinux: Refactor to remove bprm_secureexec hook Andy Lutomirski <luto@kernel.org> - 2017-08-01 15:30 +0200
    [PATCH v4 09/15] LSM: drop bprm_secureexec hook Kees Cook <keescook@chromium.org> - 2017-08-01 02:00 +0200
    [PATCH v4 01/15] exec: Rename bprm->cred_prepared to called_set_creds Kees Cook <keescook@chromium.org> - 2017-08-01 02:00 +0200
    [PATCH v4 03/15] binfmt: Introduce secureexec flag Kees Cook <keescook@chromium.org> - 2017-08-01 02:00 +0200
      Re: [PATCH v4 03/15] binfmt: Introduce secureexec flag Kees Cook <keescook@chromium.org> - 2017-08-01 02:30 +0200
      Re: [PATCH v4 03/15] binfmt: Introduce secureexec flag James Morris <jmorris@namei.org> - 2017-08-01 02:50 +0200
    [PATCH v4 06/15] smack: Refactor to remove bprm_secureexec hook Kees Cook <keescook@chromium.org> - 2017-08-01 02:00 +0200
      Re: [PATCH v4 06/15] smack: Refactor to remove bprm_secureexec  hook James Morris <jmorris@namei.org> - 2017-08-01 02:50 +0200
      Re: [PATCH v4 06/15] smack: Refactor to remove bprm_secureexec hook Casey Schaufler <casey@schaufler-ca.com> - 2017-08-01 17:30 +0200
    [PATCH v4 07/15] commoncap: Refactor to remove bprm_secureexec hook Kees Cook <keescook@chromium.org> - 2017-08-01 02:00 +0200
    [PATCH v4 04/15] apparmor: Refactor to remove bprm_secureexec hook Kees Cook <keescook@chromium.org> - 2017-08-01 02:00 +0200
    [PATCH v4 13/15] exec: Consolidate dumpability logic Kees Cook <keescook@chromium.org> - 2017-08-01 02:00 +0200
    [PATCH v4 12/15] smack: Remove redundant pdeath_signal clearing Kees Cook <keescook@chromium.org> - 2017-08-01 02:00 +0200
      Re: [PATCH v4 12/15] smack: Remove redundant pdeath_signal  clearing James Morris <jmorris@namei.org> - 2017-08-01 03:00 +0200
      Re: [PATCH v4 12/15] smack: Remove redundant pdeath_signal clearing Casey Schaufler <casey@schaufler-ca.com> - 2017-08-01 17:30 +0200
    [PATCH v4 15/15] exec: Consolidate pdeath_signal clearing Kees Cook <keescook@chromium.org> - 2017-08-01 02:00 +0200
    Re: [PATCH v4 00/15] exec: Use sane stack rlimit under secureexec Kees Cook <keescook@chromium.org> - 2017-08-01 02:40 +0200
      Re: [PATCH v4 00/15] exec: Use sane stack rlimit under secureexec James Morris <jmorris@namei.org> - 2017-08-01 03:00 +0200
        Re: [PATCH v4 00/15] exec: Use sane stack rlimit under secureexec Kees Cook <keescook@chromium.org> - 2017-08-01 05:10 +0200
          Re: [PATCH v4 00/15] exec: Use sane stack rlimit under secureexec Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-01 07:20 +0200
            Re: [PATCH v4 00/15] exec: Use sane stack rlimit under secureexec Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-01 07:20 +0200
            Re: [PATCH v4 00/15] exec: Use sane stack rlimit under secureexec Kees Cook <keescook@chromium.org> - 2017-08-01 17:10 +0200
              Re: [PATCH v4 00/15] exec: Use sane stack rlimit under secureexec Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-01 22:20 +0200
                Re: [PATCH v4 00/15] exec: Use sane stack rlimit under secureexec Kees Cook <keescook@chromium.org> - 2017-08-01 23:10 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1700526 — [PATCH v4 13/15] exec: Consolidate dumpability logic

Since it's already valid to set dumpability in the early part of
setup_new_exec(), we can consolidate the logic into a single place.
The BINPRM_FLAGS_ENFORCE_NONDUMP is set during would_dump() calls
before setup_new_exec(), so its test is safe to move as well.

Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Serge Hallyn <serge@hallyn.com>
---
 fs/exec.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 708a72f93320..3ae8a40c587b 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1333,10 +1333,12 @@ void setup_new_exec(struct linux_binprm * bprm)
 
 	current->sas_ss_sp = current->sas_ss_size = 0;
 
-	if (!bprm->secureexec)
-		set_dumpable(current->mm, SUID_DUMP_USER);
-	else
+	/* Figure out dumpability. */
+	if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP ||
+	    bprm->secureexec)
 		set_dumpable(current->mm, suid_dumpable);
+	else
+		set_dumpable(current->mm, SUID_DUMP_USER);
 
 	arch_setup_new_exec();
 	perf_event_exec();
@@ -1350,9 +1352,6 @@ void setup_new_exec(struct linux_binprm * bprm)
 
 	if (bprm->secureexec) {
 		current->pdeath_signal = 0;
-	} else {
-		if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP)
-			set_dumpable(current->mm, suid_dumpable);
 	}
 
 	/* An exec changes our domain. We are no longer part of the thread
-- 
2.7.4

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

#1700527 — [PATCH v4 12/15] smack: Remove redundant pdeath_signal clearing

This removes the redundant pdeath_signal clearing in Smack: the check in
smack_bprm_committing_creds() matches the check in smack_bprm_set_creds()
(which used to be in the now-removed smack_bprm_securexec() hook) and
since secureexec is now being checked for clearing pdeath_signal, this
is redundant to the common exec code.

Cc: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Serge Hallyn <serge@hallyn.com>
---
 security/smack/smack_lsm.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 4f1967be3d20..4e33c650b224 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -957,20 +957,6 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
 	return 0;
 }
 
-/**
- * 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;
-}
-
 /*
  * Inode hooks
  */
@@ -4633,7 +4619,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(inode_alloc_security, smack_inode_alloc_security),
 	LSM_HOOK_INIT(inode_free_security, smack_inode_free_security),
-- 
2.7.4

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

#1700549 — Re: [PATCH v4 12/15] smack: Remove redundant pdeath_signal clearing

On Mon, 31 Jul 2017, Kees Cook wrote:

> This removes the redundant pdeath_signal clearing in Smack: the check in
> smack_bprm_committing_creds() matches the check in smack_bprm_set_creds()
> (which used to be in the now-removed smack_bprm_securexec() hook) and
> since secureexec is now being checked for clearing pdeath_signal, this
> is redundant to the common exec code.
> 
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> ---
>  security/smack/smack_lsm.c | 15 ---------------
>  1 file changed, 15 deletions(-)


Reviewed-by: James Morris <james.l.morris@oracle.com>


-- 
James Morris
<jmorris@namei.org>

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

#1701147 — Re: [PATCH v4 12/15] smack: Remove redundant pdeath_signal clearing

On 7/31/2017 4:51 PM, Kees Cook wrote:
> This removes the redundant pdeath_signal clearing in Smack: the check in
> smack_bprm_committing_creds() matches the check in smack_bprm_set_creds()
> (which used to be in the now-removed smack_bprm_securexec() hook) and
> since secureexec is now being checked for clearing pdeath_signal, this
> is redundant to the common exec code.
>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Acked-by: Serge Hallyn <serge@hallyn.com>

Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>

> ---
>  security/smack/smack_lsm.c | 15 ---------------
>  1 file changed, 15 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 4f1967be3d20..4e33c650b224 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -957,20 +957,6 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
>  	return 0;
>  }
>  
> -/**
> - * 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;
> -}
> -
>  /*
>   * Inode hooks
>   */
> @@ -4633,7 +4619,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(inode_alloc_security, smack_inode_alloc_security),
>  	LSM_HOOK_INIT(inode_free_security, smack_inode_free_security),

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

#1700528 — [PATCH v4 15/15] exec: Consolidate pdeath_signal clearing

Instead of an additional secureexec check for pdeath_signal, just move it
up into the initial secureexec test. Neither perf nor arch code touches
pdeath_signal, so the relocation shouldn't change anything.

Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Serge Hallyn <serge@hallyn.com>
---
 fs/exec.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index cddd2a2cbc1f..5a19912a4f53 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1330,6 +1330,9 @@ void setup_new_exec(struct linux_binprm * bprm)
 	bprm->secureexec |= bprm->cap_elevated;
 
 	if (bprm->secureexec) {
+		/* Make sure parent cannot signal privileged process. */
+		current->pdeath_signal = 0;
+
 		/*
 		 * For secureexec, reset the stack limit to sane default to
 		 * avoid bad behavior from the prior rlimits. This has to
@@ -1362,10 +1365,6 @@ void setup_new_exec(struct linux_binprm * bprm)
 	 */
 	current->mm->task_size = TASK_SIZE;
 
-	if (bprm->secureexec) {
-		current->pdeath_signal = 0;
-	}
-
 	/* An exec changes our domain. We are no longer part of the thread
 	   group */
 	current->self_exec_id++;
-- 
2.7.4

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

#1700537

Ugh, please ignore this v4. There's a typo that snuck in. I'll send a v5 soon...

-Kees

On Mon, Jul 31, 2017 at 4:51 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. To do this, we need to know the results of the
> bprm_secureexec hook before memory layouts. As it turns out, this
> can be made _mostly_ trivial by collapsing bprm_secureexec into
> bprm_set_creds.
>
> The LSMs using bprm_secureexec nearly always save state between
> bprm_set_creds and bprm_secureexec. In the face of multiple calls to
> bprm_set_creds (via prepare_binprm() calls from binfmt_script, etc),
> all LSMs except commoncap only pay attention to the first call, so
> that aligns well with collapsing bprm_secureexec into bprm_set_creds.
> The commoncaps, though, needs to check the _last_ bprm_set_creds, so
> this series just swaps one bprm flag for another (cap_effective is no
> longer needed to save state between bprm_set_creds and bprm_secureexec,
> but we do need to keep a separate state, so we add the cap_elevated flag).
>
> Once secureexec is available to setup_new_exec() before the memory
> layout, we can add an rlimit sanity-check for setuid execs. (With no
> need to clean up since we're past the point of no return.)
>
> Along the way, this fixes comments, renames a variable, and consolidates
> dumpability and pdeath_signal clearing, which includes some commit log
> archeology to examine the subtle differences between what we had and
> what we need.
>
> Several folks have looked at this already (thank you!) but I'd appreciate
> any other eyes on this to make sure it isn't broken in some special
> way. Looking at the diffstat, even after all my long comments, this is
> a net reduction in lines. :)
>
> Given this crosses a bunch of areas, I think this is likely best to go
> via the -mm tree, which is where nearly all of my prior exec work has
> lived too. It's also after rc2 at this point, so I'd be slightly nervous
> to see this land directly in Linus's tree, but I leave that decision up
> to Linus. :) Regardless, very little has changed between v3 and v4, so I
> think this is ready to go.
>
> Thanks!
>
> -Kees
>
> ----------------------------------------------------------------
> Kees Cook (15):
>       exec: Rename bprm->cred_prepared to called_set_creds
>       exec: Correct comments about "point of no return"
>       binfmt: Introduce secureexec flag
>       apparmor: Refactor to remove bprm_secureexec hook
>       selinux: Refactor to remove bprm_secureexec hook
>       smack: Refactor to remove bprm_secureexec hook
>       commoncap: Refactor to remove bprm_secureexec hook
>       commoncap: Move cap_elevated calculation into bprm_set_creds
>       LSM: drop bprm_secureexec hook
>       exec: Use secureexec for setting dumpability
>       exec: Use secureexec for clearing pdeath_signal
>       smack: Remove redundant pdeath_signal clearing
>       exec: Consolidate dumpability logic
>       exec: Use sane stack rlimit under secureexec
>       exec: Consolidate pdeath_signal clearing
>
>  fs/binfmt_elf.c                    |  2 +-
>  fs/binfmt_elf_fdpic.c              |  2 +-
>  fs/binfmt_flat.c                   |  2 +-
>  fs/exec.c                          | 56 ++++++++++++++++++++++++++++----------
>  include/linux/binfmts.h            | 24 ++++++++++++----
>  include/linux/lsm_hooks.h          | 14 ++++------
>  include/linux/security.h           |  7 -----
>  security/apparmor/domain.c         | 24 ++--------------
>  security/apparmor/include/domain.h |  1 -
>  security/apparmor/include/file.h   |  3 --
>  security/apparmor/lsm.c            |  1 -
>  security/commoncap.c               | 50 ++++++++--------------------------
>  security/security.c                |  5 ----
>  security/selinux/hooks.c           | 26 ++++--------------
>  security/smack/smack_lsm.c         | 34 ++---------------------
>  security/tomoyo/tomoyo.c           |  2 +-
>  16 files changed, 91 insertions(+), 162 deletions(-)
>
> v4:
> - add {Ack,Review,Test}ed-bys
> - reorder patches to move trivial refactoring to the front
> - move secureexec flag set earlier in the series to setup_new_exec(); amluto
>
> v3:
> - collapse brpm_secureexec into bprm_set_creds; ebiederm.
> - continue to improve various comments
>
> v2:
> - fix missed current_security() uses in LSMs.
> - research/consolidate dumpability setting logic
> - research/consolidate pdeath_signal clearing logic
> - split up logical steps a little more for easier review (and bisection)
> - fix some old broken comments
>
>



-- 
Kees Cook
Pixel Security

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

#1700552

On Mon, 31 Jul 2017, Kees Cook wrote:

> Ugh, please ignore this v4. There's a typo that snuck in. I'll send a v5 soon...

Aside from that, it's looking good. This touches a lot of stuff in 
security/ so it may make sense for it to go in via my tree.


-- 
James Morris
<jmorris@namei.org>

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

#1700600

On Mon, Jul 31, 2017 at 5:54 PM, James Morris <jmorris@namei.org> wrote:
> On Mon, 31 Jul 2017, Kees Cook wrote:
>
>> Ugh, please ignore this v4. There's a typo that snuck in. I'll send a v5 soon...
>
> Aside from that, it's looking good. This touches a lot of stuff in
> security/ so it may make sense for it to go in via my tree.

Yeah, I'm open to whatever. It's not clear where it should go, but if
you want to take it and Linus doesn't want it "early", that works for
me. Linus, Andrew, thoughts?

-Kees

-- 
Kees Cook
Pixel Security

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

#1700638

On Mon, Jul 31, 2017 at 8:03 PM, Kees Cook <keescook@chromium.org> wrote:
>
> Yeah, I'm open to whatever. It's not clear where it should go, but if
> you want to take it and Linus doesn't want it "early", that works for
> me. Linus, Andrew, thoughts?

I'd actually like this to go in separately from all the other security stuff.

And I just checked this on a separate branch, just because I wanted to
see what the overall diff was. There's a conflict with apparmor
already - the resolution looks fairly straightforward, but considering
the area this touches, it would probably be good that Kees keeps this
branch and verifies things like that.

                    Linus

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

#1700640

On Mon, Jul 31, 2017 at 10:11 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> And I just checked this on a separate branch, just because I wanted to
> see what the overall diff was. There's a conflict [..]

Side note: the overall patch looks fine to me. I like how it removes
complexity and code. I didn't test it (other than test *building* the
merge), so take that for what it's worth, but at least the patches
look sensible both on an individual level and as an end result.

                   Linus

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

#1701140

On Mon, Jul 31, 2017 at 10:11 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Jul 31, 2017 at 8:03 PM, Kees Cook <keescook@chromium.org> wrote:
>>
>> Yeah, I'm open to whatever. It's not clear where it should go, but if
>> you want to take it and Linus doesn't want it "early", that works for
>> me. Linus, Andrew, thoughts?
>
> I'd actually like this to go in separately from all the other security stuff.
>
> And I just checked this on a separate branch, just because I wanted to
> see what the overall diff was. There's a conflict with apparmor
> already - the resolution looks fairly straightforward, but considering
> the area this touches, it would probably be good that Kees keeps this
> branch and verifies things like that.

Do you want me to carry this for -next and send it as a distinct pull
request for v4.14?

-Kees

-- 
Kees Cook
Pixel Security

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

#1701427

On Tue, Aug 1, 2017 at 8:04 AM, Kees Cook <keescook@chromium.org> wrote:
>
> Do you want me to carry this for -next and send it as a distinct pull
> request for v4.14?

Yes, I think that would be preferred. I consider this a "execve()"
cleanup/change with implications for the security models rather than
the other way around, so I'd rather keep it separate, and you already
have a few other git trees so I think it makes sense to just treat it
as another of your git pulls next merge window.

                 Linus

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

#1701501

On Tue, Aug 1, 2017 at 1:19 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Tue, Aug 1, 2017 at 8:04 AM, Kees Cook <keescook@chromium.org> wrote:
>>
>> Do you want me to carry this for -next and send it as a distinct pull
>> request for v4.14?
>
> Yes, I think that would be preferred. I consider this a "execve()"
> cleanup/change with implications for the security models rather than
> the other way around, so I'd rather keep it separate, and you already
> have a few other git trees so I think it makes sense to just treat it
> as another of your git pulls next merge window.

Okay, sounds good. Thanks!

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web