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

Groups > linux.kernel > #1690768 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 00/15] exec: Use sane stack rlimit under secureexec Kees Cook <keescook@chromium.org> - 2017-07-19 00:30 +0200
    [PATCH v3 05/15] smack: Refactor to remove bprm_secureexec hook Kees Cook <keescook@chromium.org> - 2017-07-19 00:30 +0200
      Re: [PATCH v3 05/15] smack: Refactor to remove bprm_secureexec hook Kees Cook <keescook@chromium.org> - 2017-07-26 06:00 +0200
        Re: [PATCH v3 05/15] smack: Refactor to remove bprm_secureexec hook Casey Schaufler <casey@schaufler-ca.com> - 2017-07-26 20:00 +0200
    [PATCH v3 12/15] smack: Remove redundant pdeath_signal clearing Kees Cook <keescook@chromium.org> - 2017-07-19 00:30 +0200
    [PATCH v3 14/15] exec: Use sane stack rlimit under secureexec Kees Cook <keescook@chromium.org> - 2017-07-19 00:30 +0200
      Re: [PATCH v3 14/15] exec: Use sane stack rlimit under secureexec James Morris <jmorris@namei.org> - 2017-07-19 11:50 +0200
    [PATCH v3 11/15] exec: Use secureexec for clearing pdeath_signal Kees Cook <keescook@chromium.org> - 2017-07-19 00:30 +0200
    [PATCH v3 06/15] commoncap: Refactor to remove bprm_secureexec hook Kees Cook <keescook@chromium.org> - 2017-07-19 00:30 +0200
      Re: [PATCH v3 06/15] commoncap: Refactor to remove bprm_secureexec hook Andy Lutomirski <luto@kernel.org> - 2017-07-19 03:20 +0200
        Re: [PATCH v3 06/15] commoncap: Refactor to remove bprm_secureexec hook Kees Cook <keescook@chromium.org> - 2017-07-19 06:50 +0200
        Re: [PATCH v3 06/15] commoncap: Refactor to remove bprm_secureexec hook Andy Lutomirski <luto@kernel.org> - 2017-07-20 07:00 +0200
          Re: [PATCH v3 06/15] commoncap: Refactor to remove bprm_secureexec hook Kees Cook <keescook@chromium.org> - 2017-08-01 00:50 +0200
            Re: [PATCH v3 06/15] commoncap: Refactor to remove bprm_secureexec hook Andy Lutomirski <luto@kernel.org> - 2017-08-01 15:20 +0200
      Re: [PATCH v3 06/15] commoncap: Refactor to remove bprm_secureexec  hook James Morris <jmorris@namei.org> - 2017-07-19 11:30 +0200
    [PATCH v3 01/15] binfmt: Introduce secureexec flag Kees Cook <keescook@chromium.org> - 2017-07-19 00:30 +0200
      Re: [PATCH v3 01/15] binfmt: Introduce secureexec flag John Johansen <john.johansen@canonical.com> - 2017-07-19 02:10 +0200
      Re: [PATCH v3 01/15] binfmt: Introduce secureexec flag Andy Lutomirski <luto@kernel.org> - 2017-07-19 03:10 +0200
    [PATCH v3 15/15] exec: Consolidate pdeath_signal clearing Kees Cook <keescook@chromium.org> - 2017-07-19 00:30 +0200
    [PATCH v3 13/15] exec: Consolidate dumpability logic Kees Cook <keescook@chromium.org> - 2017-07-19 00:30 +0200
    [PATCH v3 07/15] commoncap: Move cap_elevated calculation into bprm_set_creds Kees Cook <keescook@chromium.org> - 2017-07-19 00:30 +0200
      Re: [PATCH v3 07/15] commoncap: Move cap_elevated calculation into bprm_set_creds Andy Lutomirski <luto@kernel.org> - 2017-07-19 04:00 +0200
      Re: [PATCH v3 07/15] commoncap: Move cap_elevated calculation into  bprm_set_creds James Morris <jmorris@namei.org> - 2017-07-19 11:30 +0200
    [PATCH v3 10/15] exec: Use secureexec for setting dumpability Kees Cook <keescook@chromium.org> - 2017-07-19 00:30 +0200
      Re: [PATCH v3 10/15] exec: Use secureexec for setting dumpability Kees Cook <keescook@chromium.org> - 2017-07-26 06:00 +0200
    [PATCH v3 04/15] selinux: Refactor to remove bprm_secureexec hook Kees Cook <keescook@chromium.org> - 2017-07-19 00:40 +0200
      Re: [PATCH v3 04/15] selinux: Refactor to remove bprm_secureexec hook Paul Moore <paul@paul-moore.com> - 2017-07-20 02:10 +0200
        Re: [PATCH v3 04/15] selinux: Refactor to remove bprm_secureexec hook Paul Moore <paul@paul-moore.com> - 2017-07-20 02:20 +0200
          Re: [PATCH v3 04/15] selinux: Refactor to remove bprm_secureexec hook Kees Cook <keescook@chromium.org> - 2017-07-20 03:40 +0200
            Re: [PATCH v3 04/15] selinux: Refactor to remove bprm_secureexec hook Paul Moore <paul@paul-moore.com> - 2017-07-20 15:50 +0200
              Re: [PATCH v3 04/15] selinux: Refactor to remove bprm_secureexec hook Kees Cook <keescook@chromium.org> - 2017-07-20 19:10 +0200
                Re: [PATCH v3 04/15] selinux: Refactor to remove bprm_secureexec hook Paul Moore <paul@paul-moore.com> - 2017-07-20 22:50 +0200
                  Re: [PATCH v3 04/15] selinux: Refactor to remove bprm_secureexec hook Paul Moore <paul@paul-moore.com> - 2017-07-21 17:50 +0200
                    Re: [PATCH v3 04/15] selinux: Refactor to remove bprm_secureexec hook Kees Cook <keescook@chromium.org> - 2017-07-21 19:40 +0200
                      Re: [PATCH v3 04/15] selinux: Refactor to remove bprm_secureexec hook Paul Moore <paul@paul-moore.com> - 2017-07-21 21:20 +0200
    [PATCH v3 02/15] exec: Rename bprm->cred_prepared to called_set_creds Kees Cook <keescook@chromium.org> - 2017-07-19 00:40 +0200
      Re: [PATCH v3 02/15] exec: Rename bprm->cred_prepared to  called_set_creds John Johansen <john.johansen@canonical.com> - 2017-07-19 02:10 +0200
      Re: [PATCH v3 02/15] exec: Rename bprm->cred_prepared to called_set_creds Andy Lutomirski <luto@kernel.org> - 2017-07-19 03:10 +0200
        Re: [PATCH v3 02/15] exec: Rename bprm->cred_prepared to called_set_creds Kees Cook <keescook@chromium.org> - 2017-07-19 06:50 +0200
      Re: [PATCH v3 02/15] exec: Rename bprm->cred_prepared to  called_set_creds James Morris <jmorris@namei.org> - 2017-07-19 11:30 +0200
      Re: [PATCH v3 02/15] exec: Rename bprm->cred_prepared to called_set_creds Paul Moore <paul@paul-moore.com> - 2017-07-20 02:00 +0200
    Re: [PATCH v3 00/15] exec: Use sane stack rlimit under secureexec Linus Torvalds <torvalds@linux-foundation.org> - 2017-07-19 01:10 +0200
    Re: [PATCH v3 00/15] exec: Use sane stack rlimit under secureexec "Serge E. Hallyn" <serge@hallyn.com> - 2017-07-19 05:30 +0200
      Re: [PATCH v3 00/15] exec: Use sane stack rlimit under secureexec Kees Cook <keescook@chromium.org> - 2017-07-19 07:30 +0200

Page 1 of 3  [1] 2 3  Next page →

#1690768 — [PATCH v3 00/15] exec: Use sane stack rlimit under secureexec

This series has grown... :P

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.

I'd appreciate some extra eyes on this to make sure this 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.

Thanks!

-Kees
----------------------------------------------------------------
Kees Cook (15):
      binfmt: Introduce secureexec flag
      exec: Rename bprm->cred_prepared to called_set_creds
      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: Correct comments about "point of no return"
      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(-)

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

[toc] | [next] | [standalone]

#1690769 — [PATCH v3 05/15] smack: Refactor to remove bprm_secureexec hook

The Smack bprm_secureexec hook can be merged with the bprm_set_creds
hook since it's dealing with the same information, and all of the details
are finalized during the first call to the bprm_set_creds hook via
prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored
via bprm->called_set_creds).

Here, the test can just happen at the end of the bprm_set_creds hook,
and the bprm_secureexec hook can be dropped.

Cc: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 security/smack/smack_lsm.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 7d4b2e221124..4f1967be3d20 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -950,6 +950,10 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
 	bsp->smk_task = isp->smk_task;
 	bprm->per_clear |= PER_CLEAR_ON_SETID;
 
+	/* Decide if this is a secure exec. */
+	if (bsp->smk_task != bsp->smk_forked)
+		bprm->secureexec = 1;
+
 	return 0;
 }
 
@@ -967,22 +971,6 @@ static void smack_bprm_committing_creds(struct linux_binprm *bprm)
 		current->pdeath_signal = 0;
 }
 
-/**
- * smack_bprm_secureexec - Return the decision to use secureexec.
- * @bprm: binprm for exec
- *
- * Returns 0 on success.
- */
-static int smack_bprm_secureexec(struct linux_binprm *bprm)
-{
-	struct task_smack *tsp = current_security();
-
-	if (tsp->smk_task != tsp->smk_forked)
-		return 1;
-
-	return 0;
-}
-
 /*
  * Inode hooks
  */
@@ -4646,7 +4634,6 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
 
 	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),
 	LSM_HOOK_INIT(inode_free_security, smack_inode_free_security),
-- 
2.7.4

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

#1696761 — Re: [PATCH v3 05/15] smack: Refactor to remove bprm_secureexec hook

On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <keescook@chromium.org> wrote:
> The Smack bprm_secureexec hook can be merged with the bprm_set_creds
> hook since it's dealing with the same information, and all of the details
> are finalized during the first call to the bprm_set_creds hook via
> prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored
> via bprm->called_set_creds).
>
> Here, the test can just happen at the end of the bprm_set_creds hook,
> and the bprm_secureexec hook can be dropped.
>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>

How does this look to you, Casey? I've only got a few unreviewed
patches in this series. Two touch Smack. :)

Thanks!

-Kees

> ---
>  security/smack/smack_lsm.c | 21 ++++-----------------
>  1 file changed, 4 insertions(+), 17 deletions(-)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 7d4b2e221124..4f1967be3d20 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -950,6 +950,10 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
>         bsp->smk_task = isp->smk_task;
>         bprm->per_clear |= PER_CLEAR_ON_SETID;
>
> +       /* Decide if this is a secure exec. */
> +       if (bsp->smk_task != bsp->smk_forked)
> +               bprm->secureexec = 1;
> +
>         return 0;
>  }
>
> @@ -967,22 +971,6 @@ static void smack_bprm_committing_creds(struct linux_binprm *bprm)
>                 current->pdeath_signal = 0;
>  }
>
> -/**
> - * smack_bprm_secureexec - Return the decision to use secureexec.
> - * @bprm: binprm for exec
> - *
> - * Returns 0 on success.
> - */
> -static int smack_bprm_secureexec(struct linux_binprm *bprm)
> -{
> -       struct task_smack *tsp = current_security();
> -
> -       if (tsp->smk_task != tsp->smk_forked)
> -               return 1;
> -
> -       return 0;
> -}
> -
>  /*
>   * Inode hooks
>   */
> @@ -4646,7 +4634,6 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
>
>         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),
>         LSM_HOOK_INIT(inode_free_security, smack_inode_free_security),
> --
> 2.7.4
>



-- 
Kees Cook
Pixel Security

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

#1697456 — Re: [PATCH v3 05/15] smack: Refactor to remove bprm_secureexec hook

On 7/25/2017 8:58 PM, Kees Cook wrote:
> On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <keescook@chromium.org> wrote:
>> The Smack bprm_secureexec hook can be merged with the bprm_set_creds
>> hook since it's dealing with the same information, and all of the details
>> are finalized during the first call to the bprm_set_creds hook via
>> prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored
>> via bprm->called_set_creds).
>>
>> Here, the test can just happen at the end of the bprm_set_creds hook,
>> and the bprm_secureexec hook can be dropped.
>>
>> Cc: Casey Schaufler <casey@schaufler-ca.com>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
> How does this look to you, Casey? I've only got a few unreviewed
> patches in this series. Two touch Smack. :)

The eyes don't see any problems, but I haven't had a chance
to try it out.

>
> Thanks!
>
> -Kees
>
>> ---
>>  security/smack/smack_lsm.c | 21 ++++-----------------
>>  1 file changed, 4 insertions(+), 17 deletions(-)
>>
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index 7d4b2e221124..4f1967be3d20 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -950,6 +950,10 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm)
>>         bsp->smk_task = isp->smk_task;
>>         bprm->per_clear |= PER_CLEAR_ON_SETID;
>>
>> +       /* Decide if this is a secure exec. */
>> +       if (bsp->smk_task != bsp->smk_forked)
>> +               bprm->secureexec = 1;
>> +
>>         return 0;
>>  }
>>
>> @@ -967,22 +971,6 @@ static void smack_bprm_committing_creds(struct linux_binprm *bprm)
>>                 current->pdeath_signal = 0;
>>  }
>>
>> -/**
>> - * smack_bprm_secureexec - Return the decision to use secureexec.
>> - * @bprm: binprm for exec
>> - *
>> - * Returns 0 on success.
>> - */
>> -static int smack_bprm_secureexec(struct linux_binprm *bprm)
>> -{
>> -       struct task_smack *tsp = current_security();
>> -
>> -       if (tsp->smk_task != tsp->smk_forked)
>> -               return 1;
>> -
>> -       return 0;
>> -}
>> -
>>  /*
>>   * Inode hooks
>>   */
>> @@ -4646,7 +4634,6 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
>>
>>         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),
>>         LSM_HOOK_INIT(inode_free_security, smack_inode_free_security),
>> --
>> 2.7.4
>>
>
>


.

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

#1690770 — [PATCH v3 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>
---
 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]

#1690771 — [PATCH v3 14/15] exec: Use sane stack rlimit under secureexec

For a secureexec, before memory layout selection has happened, reset the
stack rlimit to something sane to avoid the caller having control over
the resulting layouts.

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

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/exec.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/exec.c b/fs/exec.c
index 45418d6ec583..4227c1e56566 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1349,6 +1349,18 @@ void setup_new_exec(struct linux_binprm * bprm)
 	 */
 	bprm->secureexec |= bprm->cap_elevated;
 
+	if (bprm->secureexec) {
+		/*
+		 * For secureexec, 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 no return to avoid
+		 * needing to clean 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);
 
 	current->sas_ss_sp = current->sas_ss_size = 0;
-- 
2.7.4

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

#1691230 — Re: [PATCH v3 14/15] exec: Use sane stack rlimit under secureexec

On Tue, 18 Jul 2017, Kees Cook wrote:

> For a secureexec, before memory layout selection has happened, reset the
> stack rlimit to something sane to avoid the caller having control over
> the resulting layouts.
> 
> $ ulimit -s
> 8192
> $ ulimit -s unlimited
> $ /bin/sh -c 'ulimit -s'
> unlimited
> $ sudo /bin/sh -c 'ulimit -s'
> 8192
> 
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Kees Cook <keescook@chromium.org>


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


-- 
James Morris
<jmorris@namei.org>

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

#1690772 — [PATCH v3 11/15] exec: Use secureexec for clearing pdeath_signal

Like dumpability, clearing pdeath_signal happens both in setup_new_exec()
and later in commit_creds(). The test in setup_new_exec() is different
from all other privilege comparisons, though: it is checking the new cred
(bprm) uid vs the old cred (current) euid. This appears to be a bug,
introduced by commit a6f76f23d297 ("CRED: Make execve() take advantage of
copy-on-write credentials"):

-       if (bprm->e_uid != current_euid() ||
-           bprm->e_gid != current_egid()) {
-               set_dumpable(current->mm, suid_dumpable);
+       /* install the new credentials */
+       if (bprm->cred->uid != current_euid() ||
+           bprm->cred->gid != current_egid()) {

It was bprm euid vs current euid (and egids), but the effective got
dropped. Nothing in the exec flow changes bprm->cred->uid (nor gid).
The call traces are:

	prepare_bprm_creds()
	    prepare_exec_creds()
	        prepare_creds()
	            memcpy(new_creds, old_creds, ...)
	            security_prepare_creds() (unimplemented by commoncap)
	...
	prepare_binprm()
	    bprm_fill_uid()
	        resets euid/egid to current euid/egid
	        sets euid/egid on bprm based on set*id file bits
	    security_bprm_set_creds()
		cap_bprm_set_creds()
		        handle all caps-based manipulations

so this test is effectively a test of current_uid() vs current_euid(),
which is wrong, just like the prior dumpability tests were wrong.

The commit log says "Clear pdeath_signal and set dumpable on
certain circumstances that may not be covered by commit_creds()." This
may be meaning the earlier old euid vs new euid (and egid) test that
got changed.

Luckily, as with dumpability, this is all masked by commit_creds()
which performs old/new euid and egid tests and clears pdeath_signal.

And again, like dumpability, we should include LSM secureexec logic for
pdeath_signal clearing. For example, Smack goes out of its way to clear
pdeath_signal when it finds a secureexec condition.

Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/exec.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 5241c8f25f5d..81793a193d92 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1368,8 +1368,7 @@ void setup_new_exec(struct linux_binprm * bprm)
 	 */
 	current->mm->task_size = TASK_SIZE;
 
-	if (!uid_eq(bprm->cred->uid, current_euid()) ||
-	    !gid_eq(bprm->cred->gid, current_egid())) {
+	if (bprm->secureexec) {
 		current->pdeath_signal = 0;
 	} else {
 		if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP)
-- 
2.7.4

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

#1690773 — [PATCH v3 06/15] commoncap: Refactor to remove bprm_secureexec hook

The commoncap implementation of the bprm_secureexec hook is the only LSM
that depends on the final call to its bprm_set_creds hook (since it may
be called for multiple files, it ignores bprm->called_set_creds). As a
result, it cannot safely _clear_ bprm->secureexec since other LSMs may
have set it.  Instead, remove the bprm_secureexec hook by introducing a
new flag to bprm specific to commoncap: cap_elevated. This is similar to
cap_effective, but that is used for a specific subset of elevated
privileges, and exists solely to track state from bprm_set_creds to
bprm_secureexec. As such, it will be removed in the next patch.

Here, set the new bprm->cap_elevated flag when setuid/setgid has happened
from bprm_fill_uid() or fscapabilities have been prepared. This temporarily
moves the bprm_secureexec hook to a static inline. The helper will be
removed in the next patch; this makes the step easier to review and bisect,
since this does not introduce any changes to inputs nor outputs to the
"elevated privileges" calculation.

The new flag is merged with the bprm->secureexec flag in setup_new_exec()
since this marks the end of any further prepare_binprm() calls.

Cc: Serge Hallyn <serge@hallyn.com>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/exec.c               |  7 +++++++
 include/linux/binfmts.h |  7 +++++++
 security/commoncap.c    | 12 ++++++++----
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 925c85a45d97..53ffa739fb58 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1330,6 +1330,13 @@ EXPORT_SYMBOL(would_dump);
 
 void setup_new_exec(struct linux_binprm * bprm)
 {
+	/*
+	 * Once here, prepare_binrpm() will not be called any more, so
+	 * the final state of setuid/setgid/fscaps can be merged into the
+	 * secureexec flag.
+	 */
+	bprm->secureexec |= bprm->cap_elevated;
+
 	arch_pick_mmap_layout(current->mm);
 
 	/* This is the point of no return */
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 36be5a67517a..a82f5edf23f9 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -35,6 +35,13 @@ struct linux_binprm {
 				 * false if not; except for init which inherits
 				 * its parent's caps anyway */
 		/*
+		 * True if most recent call to the commoncaps bprm_set_creds
+		 * hook (due to multiple prepare_binprm() calls from the
+		 * binfmt_script/misc handlers) resulted in elevated
+		 * privileges.
+		 */
+		cap_elevated:1,
+		/*
 		 * Set by bprm_set_creds hook to indicate a privilege-gaining
 		 * exec has happened. Used to sanitize execution environment
 		 * and to set AT_SECURE auxv for glibc.
diff --git a/security/commoncap.c b/security/commoncap.c
index 7abebd782d5e..abb6050c8083 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -481,6 +481,8 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
 	return rc;
 }
 
+static int is_secureexec(struct linux_binprm *bprm);
+
 /**
  * cap_bprm_set_creds - Set up the proposed credentials for execve().
  * @bprm: The execution parameters, including the proposed creds
@@ -614,11 +616,14 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 	if (WARN_ON(!cap_ambient_invariant_ok(new)))
 		return -EPERM;
 
+	/* Check for privilege-elevated exec. */
+	bprm->cap_elevated = is_secureexec(bprm);
+
 	return 0;
 }
 
 /**
- * cap_bprm_secureexec - Determine whether a secure execution is required
+ * is_secureexec - Determine whether a secure execution is required
  * @bprm: The execution parameters
  *
  * Determine whether a secure execution is required, return 1 if it is, and 0
@@ -627,9 +632,9 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
  * 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)
+static int is_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)) {
@@ -1079,7 +1084,6 @@ struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(capget, cap_capget),
 	LSM_HOOK_INIT(capset, cap_capset),
 	LSM_HOOK_INIT(bprm_set_creds, cap_bprm_set_creds),
-	LSM_HOOK_INIT(bprm_secureexec, cap_bprm_secureexec),
 	LSM_HOOK_INIT(inode_need_killpriv, cap_inode_need_killpriv),
 	LSM_HOOK_INIT(inode_killpriv, cap_inode_killpriv),
 	LSM_HOOK_INIT(mmap_addr, cap_mmap_addr),
-- 
2.7.4

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

#1690871 — Re: [PATCH v3 06/15] commoncap: Refactor to remove bprm_secureexec hook

On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <keescook@chromium.org> wrote:
> The commoncap implementation of the bprm_secureexec hook is the only LSM
> that depends on the final call to its bprm_set_creds hook (since it may
> be called for multiple files, it ignores bprm->called_set_creds). As a
> result, it cannot safely _clear_ bprm->secureexec since other LSMs may
> have set it.  Instead, remove the bprm_secureexec hook by introducing a
> new flag to bprm specific to commoncap: cap_elevated. This is similar to
> cap_effective, but that is used for a specific subset of elevated
> privileges, and exists solely to track state from bprm_set_creds to
> bprm_secureexec. As such, it will be removed in the next patch.
>
> Here, set the new bprm->cap_elevated flag when setuid/setgid has happened
> from bprm_fill_uid() or fscapabilities have been prepared. This temporarily
> moves the bprm_secureexec hook to a static inline. The helper will be
> removed in the next patch; this makes the step easier to review and bisect,
> since this does not introduce any changes to inputs nor outputs to the
> "elevated privileges" calculation.
>
> The new flag is merged with the bprm->secureexec flag in setup_new_exec()
> since this marks the end of any further prepare_binprm() calls.

Reviewed-by: Andy Lutomirski <luto@kernel.org>

with the redundant caveat that...

> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1330,6 +1330,13 @@ EXPORT_SYMBOL(would_dump);
>
>  void setup_new_exec(struct linux_binprm * bprm)
>  {
> +       /*
> +        * Once here, prepare_binrpm() will not be called any more, so
> +        * the final state of setuid/setgid/fscaps can be merged into the
> +        * secureexec flag.
> +        */
> +       bprm->secureexec |= bprm->cap_elevated;
> +

...the weird placement of the other assignments to bprm->secureexec
makes this exceedingly confusing.

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

#1690966 — Re: [PATCH v3 06/15] commoncap: Refactor to remove bprm_secureexec hook

On Tue, Jul 18, 2017 at 6:10 PM, Andy Lutomirski <luto@kernel.org> wrote:
> On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <keescook@chromium.org> wrote:
>> The commoncap implementation of the bprm_secureexec hook is the only LSM
>> that depends on the final call to its bprm_set_creds hook (since it may
>> be called for multiple files, it ignores bprm->called_set_creds). As a
>> result, it cannot safely _clear_ bprm->secureexec since other LSMs may
>> have set it.  Instead, remove the bprm_secureexec hook by introducing a
>> new flag to bprm specific to commoncap: cap_elevated. This is similar to
>> cap_effective, but that is used for a specific subset of elevated
>> privileges, and exists solely to track state from bprm_set_creds to
>> bprm_secureexec. As such, it will be removed in the next patch.
>>
>> Here, set the new bprm->cap_elevated flag when setuid/setgid has happened
>> from bprm_fill_uid() or fscapabilities have been prepared. This temporarily
>> moves the bprm_secureexec hook to a static inline. The helper will be
>> removed in the next patch; this makes the step easier to review and bisect,
>> since this does not introduce any changes to inputs nor outputs to the
>> "elevated privileges" calculation.
>>
>> The new flag is merged with the bprm->secureexec flag in setup_new_exec()
>> since this marks the end of any further prepare_binprm() calls.
>
> Reviewed-by: Andy Lutomirski <luto@kernel.org>
>
> with the redundant caveat that...
>
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -1330,6 +1330,13 @@ EXPORT_SYMBOL(would_dump);
>>
>>  void setup_new_exec(struct linux_binprm * bprm)
>>  {
>> +       /*
>> +        * Once here, prepare_binrpm() will not be called any more, so
>> +        * the final state of setuid/setgid/fscaps can be merged into the
>> +        * secureexec flag.
>> +        */
>> +       bprm->secureexec |= bprm->cap_elevated;
>> +
>
> ...the weird placement of the other assignments to bprm->secureexec
> makes this exceedingly confusing.

Any thoughts on how I could improve this? The main take-away is that
commoncap's secureexec is special, and this was the cleanest way I
could find to deal with it...

-Kees

-- 
Kees Cook
Pixel Security

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

#1692418 — Re: [PATCH v3 06/15] commoncap: Refactor to remove bprm_secureexec hook

On Tue, Jul 18, 2017 at 6:10 PM, Andy Lutomirski <luto@kernel.org> wrote:
> On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <keescook@chromium.org> wrote:
>> The commoncap implementation of the bprm_secureexec hook is the only LSM
>> that depends on the final call to its bprm_set_creds hook (since it may
>> be called for multiple files, it ignores bprm->called_set_creds). As a
>> result, it cannot safely _clear_ bprm->secureexec since other LSMs may
>> have set it.  Instead, remove the bprm_secureexec hook by introducing a
>> new flag to bprm specific to commoncap: cap_elevated. This is similar to
>> cap_effective, but that is used for a specific subset of elevated
>> privileges, and exists solely to track state from bprm_set_creds to
>> bprm_secureexec. As such, it will be removed in the next patch.
>>
>> Here, set the new bprm->cap_elevated flag when setuid/setgid has happened
>> from bprm_fill_uid() or fscapabilities have been prepared. This temporarily
>> moves the bprm_secureexec hook to a static inline. The helper will be
>> removed in the next patch; this makes the step easier to review and bisect,
>> since this does not introduce any changes to inputs nor outputs to the
>> "elevated privileges" calculation.
>>
>> The new flag is merged with the bprm->secureexec flag in setup_new_exec()
>> since this marks the end of any further prepare_binprm() calls.
>
> Reviewed-by: Andy Lutomirski <luto@kernel.org>
>
> with the redundant caveat that...
>
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -1330,6 +1330,13 @@ EXPORT_SYMBOL(would_dump);
>>
>>  void setup_new_exec(struct linux_binprm * bprm)
>>  {
>> +       /*
>> +        * Once here, prepare_binrpm() will not be called any more, so
>> +        * the final state of setuid/setgid/fscaps can be merged into the
>> +        * secureexec flag.
>> +        */
>> +       bprm->secureexec |= bprm->cap_elevated;
>> +
>
> ...the weird placement of the other assignments to bprm->secureexec
> makes this exceedingly confusing.

Can you just put the bprm->secureexec |=
security_bprm_secureexec(bprm); assignment in prepare_binprm() right
after security_bprm_set_creds()? This would make patch 1 make sense
and make this make sense too, I think.  Or is there some reason why it
wouldn't work?  If the latter, I think the patch descriptions and
comments should maybe be fixed up.

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

#1700455 — Re: [PATCH v3 06/15] commoncap: Refactor to remove bprm_secureexec hook

On Wed, Jul 19, 2017 at 9:53 PM, Andy Lutomirski <luto@kernel.org> wrote:
> On Tue, Jul 18, 2017 at 6:10 PM, Andy Lutomirski <luto@kernel.org> wrote:
>> On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <keescook@chromium.org> wrote:
>>> The commoncap implementation of the bprm_secureexec hook is the only LSM
>>> that depends on the final call to its bprm_set_creds hook (since it may
>>> be called for multiple files, it ignores bprm->called_set_creds). As a
>>> result, it cannot safely _clear_ bprm->secureexec since other LSMs may
>>> have set it.  Instead, remove the bprm_secureexec hook by introducing a
>>> new flag to bprm specific to commoncap: cap_elevated. This is similar to
>>> cap_effective, but that is used for a specific subset of elevated
>>> privileges, and exists solely to track state from bprm_set_creds to
>>> bprm_secureexec. As such, it will be removed in the next patch.
>>>
>>> Here, set the new bprm->cap_elevated flag when setuid/setgid has happened
>>> from bprm_fill_uid() or fscapabilities have been prepared. This temporarily
>>> moves the bprm_secureexec hook to a static inline. The helper will be
>>> removed in the next patch; this makes the step easier to review and bisect,
>>> since this does not introduce any changes to inputs nor outputs to the
>>> "elevated privileges" calculation.
>>>
>>> The new flag is merged with the bprm->secureexec flag in setup_new_exec()
>>> since this marks the end of any further prepare_binprm() calls.
>>
>> Reviewed-by: Andy Lutomirski <luto@kernel.org>
>>
>> with the redundant caveat that...
>>
>>> --- a/fs/exec.c
>>> +++ b/fs/exec.c
>>> @@ -1330,6 +1330,13 @@ EXPORT_SYMBOL(would_dump);
>>>
>>>  void setup_new_exec(struct linux_binprm * bprm)
>>>  {
>>> +       /*
>>> +        * Once here, prepare_binrpm() will not be called any more, so
>>> +        * the final state of setuid/setgid/fscaps can be merged into the
>>> +        * secureexec flag.
>>> +        */
>>> +       bprm->secureexec |= bprm->cap_elevated;
>>> +
>>
>> ...the weird placement of the other assignments to bprm->secureexec
>> makes this exceedingly confusing.
>
> Can you just put the bprm->secureexec |=
> security_bprm_secureexec(bprm); assignment in prepare_binprm() right
> after security_bprm_set_creds()? This would make patch 1 make sense
> and make this make sense too, I think.  Or is there some reason why it
> wouldn't work?  If the latter, I think the patch descriptions and
> comments should maybe be fixed up.

Yeah, I'll make this change for the next version. It makes things a
little less ugly in the series. In this version I was trying to focus
on eliminating the LSM hook instead of first moving it (to
setup_new_exec()) and then moving it a second time (to the
bprm_set_creds() hook).

Have you had a chance to review the later consolidation patches? So
far no one else has reviewed those. (David, any chance you have some
time too?) I'd love to get at least some Reviewed-bys for them...

-Kees

-- 
Kees Cook
Pixel Security

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

#1701019 — Re: [PATCH v3 06/15] commoncap: Refactor to remove bprm_secureexec hook

On Mon, Jul 31, 2017 at 3:43 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Jul 19, 2017 at 9:53 PM, Andy Lutomirski <luto@kernel.org> wrote:
>> On Tue, Jul 18, 2017 at 6:10 PM, Andy Lutomirski <luto@kernel.org> wrote:
>>> On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <keescook@chromium.org> wrote:
>>>> The commoncap implementation of the bprm_secureexec hook is the only LSM
>>>> that depends on the final call to its bprm_set_creds hook (since it may
>>>> be called for multiple files, it ignores bprm->called_set_creds). As a
>>>> result, it cannot safely _clear_ bprm->secureexec since other LSMs may
>>>> have set it.  Instead, remove the bprm_secureexec hook by introducing a
>>>> new flag to bprm specific to commoncap: cap_elevated. This is similar to
>>>> cap_effective, but that is used for a specific subset of elevated
>>>> privileges, and exists solely to track state from bprm_set_creds to
>>>> bprm_secureexec. As such, it will be removed in the next patch.
>>>>
>>>> Here, set the new bprm->cap_elevated flag when setuid/setgid has happened
>>>> from bprm_fill_uid() or fscapabilities have been prepared. This temporarily
>>>> moves the bprm_secureexec hook to a static inline. The helper will be
>>>> removed in the next patch; this makes the step easier to review and bisect,
>>>> since this does not introduce any changes to inputs nor outputs to the
>>>> "elevated privileges" calculation.
>>>>
>>>> The new flag is merged with the bprm->secureexec flag in setup_new_exec()
>>>> since this marks the end of any further prepare_binprm() calls.
>>>
>>> Reviewed-by: Andy Lutomirski <luto@kernel.org>
>>>
>>> with the redundant caveat that...
>>>
>>>> --- a/fs/exec.c
>>>> +++ b/fs/exec.c
>>>> @@ -1330,6 +1330,13 @@ EXPORT_SYMBOL(would_dump);
>>>>
>>>>  void setup_new_exec(struct linux_binprm * bprm)
>>>>  {
>>>> +       /*
>>>> +        * Once here, prepare_binrpm() will not be called any more, so
>>>> +        * the final state of setuid/setgid/fscaps can be merged into the
>>>> +        * secureexec flag.
>>>> +        */
>>>> +       bprm->secureexec |= bprm->cap_elevated;
>>>> +
>>>
>>> ...the weird placement of the other assignments to bprm->secureexec
>>> makes this exceedingly confusing.
>>
>> Can you just put the bprm->secureexec |=
>> security_bprm_secureexec(bprm); assignment in prepare_binprm() right
>> after security_bprm_set_creds()? This would make patch 1 make sense
>> and make this make sense too, I think.  Or is there some reason why it
>> wouldn't work?  If the latter, I think the patch descriptions and
>> comments should maybe be fixed up.
>
> Yeah, I'll make this change for the next version. It makes things a
> little less ugly in the series. In this version I was trying to focus
> on eliminating the LSM hook instead of first moving it (to
> setup_new_exec()) and then moving it a second time (to the
> bprm_set_creds() hook).
>
> Have you had a chance to review the later consolidation patches? So
> far no one else has reviewed those. (David, any chance you have some
> time too?) I'd love to get at least some Reviewed-bys for them...

I looked briefly.  I'll try to look more closely tomorrow.

--Andy

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

#1691174 — Re: [PATCH v3 06/15] commoncap: Refactor to remove bprm_secureexec hook

On Tue, 18 Jul 2017, Kees Cook wrote:

> The commoncap implementation of the bprm_secureexec hook is the only LSM
> that depends on the final call to its bprm_set_creds hook (since it may
> be called for multiple files, it ignores bprm->called_set_creds). As a
> result, it cannot safely _clear_ bprm->secureexec since other LSMs may
> have set it.  Instead, remove the bprm_secureexec hook by introducing a
> new flag to bprm specific to commoncap: cap_elevated. This is similar to
> cap_effective, but that is used for a specific subset of elevated
> privileges, and exists solely to track state from bprm_set_creds to
> bprm_secureexec. As such, it will be removed in the next patch.
> 
> Here, set the new bprm->cap_elevated flag when setuid/setgid has happened
> from bprm_fill_uid() or fscapabilities have been prepared. This temporarily
> moves the bprm_secureexec hook to a static inline. The helper will be
> removed in the next patch; this makes the step easier to review and bisect,
> since this does not introduce any changes to inputs nor outputs to the
> "elevated privileges" calculation.
> 
> The new flag is merged with the bprm->secureexec flag in setup_new_exec()
> since this marks the end of any further prepare_binprm() calls.
> 
> Cc: Serge Hallyn <serge@hallyn.com>
> Cc: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Kees Cook <keescook@chromium.org>


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


-- 
James Morris
<jmorris@namei.org>

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

#1690777 — [PATCH v3 01/15] binfmt: Introduce secureexec flag

The bprm_secureexec hook can be moved earlier. Right now, it is called
during create_elf_tables(), via load_binary(), via search_binary_handler(),
via exec_binprm(). Nearly all (see exception below) state used by
bprm_secureexec is created during the bprm_set_creds hook, called from
prepare_binprm().

For all LSMs (except commoncaps described next), only the first execution
of bprm_set_creds takes any effect (they all check bprm->cred_prepared which
prepare_binprm() sets after the first call to the bprm_set_creds hook).
However, all these LSMs also only do anything with bprm_secureexec when
they detected a secure state during their first run of bprm_set_creds.
Therefore, it is functionally identical to move the detection into
bprm_set_creds, since the results from secureexec here only need to be
based on the first call to the LSM's bprm_set_creds hook.

The single exception is that the commoncaps secureexec hook also examines
euid/uid and egid/gid differences which are controlled by bprm_fill_uid(),
via prepare_binprm(), which can be called multiple times (e.g.
binfmt_script, binfmt_misc), and may clear the euid/egid for the final
load (i.e. the script interpreter). However, while commoncaps specifically
ignores bprm->cred_prepared, and runs its bprm_set_creds hook each time
prepare_binprm() may get called, it needs to base the secureexec decision
on the final call to bprm_set_creds. As a result, it will need special
handling.

To begin this refactoring, this adds the secureexec flag to the bprm
struct, which will eventually be used in place of the LSM hook.

Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/binfmt_elf.c         | 3 ++-
 fs/binfmt_elf_fdpic.c   | 3 ++-
 include/linux/binfmts.h | 8 +++++++-
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 5075fd5c62c8..991e4de3515f 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -254,7 +254,8 @@ 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));
+	bprm->secureexec |= 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..c88b35d4a6b3 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -650,7 +650,8 @@ 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));
+	bprm->secureexec |= security_bprm_secureexec(bprm);
+	NEW_AUX_ENT(AT_SECURE,	bprm->secureexec);
 	NEW_AUX_ENT(AT_EXECFN,	bprm->exec);
 
 #ifdef ARCH_DLINFO
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 05488da3aee9..9508b5f83c7e 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -27,9 +27,15 @@ 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 */
+		/*
+		 * Set by bprm_set_creds hook to indicate a privilege-gaining
+		 * exec has happened. Used to sanitize execution environment
+		 * and to set AT_SECURE auxv for glibc.
+		 */
+		secureexec:1;
 #ifdef __alpha__
 	unsigned int taso:1;
 #endif
-- 
2.7.4

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

#1690843 — Re: [PATCH v3 01/15] binfmt: Introduce secureexec flag

On 07/18/2017 03:25 PM, Kees Cook wrote:
> The bprm_secureexec hook can be moved earlier. Right now, it is called
> during create_elf_tables(), via load_binary(), via search_binary_handler(),
> via exec_binprm(). Nearly all (see exception below) state used by
> bprm_secureexec is created during the bprm_set_creds hook, called from
> prepare_binprm().
> 
> For all LSMs (except commoncaps described next), only the first execution
> of bprm_set_creds takes any effect (they all check bprm->cred_prepared which
> prepare_binprm() sets after the first call to the bprm_set_creds hook).
> However, all these LSMs also only do anything with bprm_secureexec when
> they detected a secure state during their first run of bprm_set_creds.
> Therefore, it is functionally identical to move the detection into
> bprm_set_creds, since the results from secureexec here only need to be
> based on the first call to the LSM's bprm_set_creds hook.
> 
> The single exception is that the commoncaps secureexec hook also examines
> euid/uid and egid/gid differences which are controlled by bprm_fill_uid(),
> via prepare_binprm(), which can be called multiple times (e.g.
> binfmt_script, binfmt_misc), and may clear the euid/egid for the final
> load (i.e. the script interpreter). However, while commoncaps specifically
> ignores bprm->cred_prepared, and runs its bprm_set_creds hook each time
> prepare_binprm() may get called, it needs to base the secureexec decision
> on the final call to bprm_set_creds. As a result, it will need special
> handling.
> 
> To begin this refactoring, this adds the secureexec flag to the bprm
> struct, which will eventually be used in place of the LSM hook.
> 
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>

looks good

Reviewed-by: John Johansen <john.johansen@canonical.com>

> ---
>  fs/binfmt_elf.c         | 3 ++-
>  fs/binfmt_elf_fdpic.c   | 3 ++-
>  include/linux/binfmts.h | 8 +++++++-
>  3 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 5075fd5c62c8..991e4de3515f 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -254,7 +254,8 @@ 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));
> +	bprm->secureexec |= 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..c88b35d4a6b3 100644
> --- a/fs/binfmt_elf_fdpic.c
> +++ b/fs/binfmt_elf_fdpic.c
> @@ -650,7 +650,8 @@ 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));
> +	bprm->secureexec |= security_bprm_secureexec(bprm);
> +	NEW_AUX_ENT(AT_SECURE,	bprm->secureexec);
>  	NEW_AUX_ENT(AT_EXECFN,	bprm->exec);
>  
>  #ifdef ARCH_DLINFO
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index 05488da3aee9..9508b5f83c7e 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -27,9 +27,15 @@ 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 */
> +		/*
> +		 * Set by bprm_set_creds hook to indicate a privilege-gaining
> +		 * exec has happened. Used to sanitize execution environment
> +		 * and to set AT_SECURE auxv for glibc.
> +		 */
> +		secureexec:1;
>  #ifdef __alpha__
>  	unsigned int taso:1;
>  #endif
> 

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

#1690864 — Re: [PATCH v3 01/15] binfmt: Introduce secureexec flag

On Tue, Jul 18, 2017 at 3:25 PM, Kees Cook <keescook@chromium.org> wrote:
> To begin this refactoring, this adds the secureexec flag to the bprm
> struct, which will eventually be used in place of the LSM hook.

I'm very confused.  See below.  Maybe a later patch will unconfuse me.

>
> Cc: David Howells <dhowells@redhat.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  fs/binfmt_elf.c         | 3 ++-
>  fs/binfmt_elf_fdpic.c   | 3 ++-
>  include/linux/binfmts.h | 8 +++++++-
>  3 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 5075fd5c62c8..991e4de3515f 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -254,7 +254,8 @@ 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));
> +       bprm->secureexec |= security_bprm_secureexec(bprm);
> +       NEW_AUX_ENT(AT_SECURE, bprm->secureexec);

This is ->load_binary ...

> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index 05488da3aee9..9508b5f83c7e 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -27,9 +27,15 @@ 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 */
> +               /*
> +                * Set by bprm_set_creds hook to indicate a privilege-gaining
> +                * exec has happened. Used to sanitize execution environment
> +                * and to set AT_SECURE auxv for glibc.
> +                */

... which is not bprm_set_creds().

What am I missing here?

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

#1690778 — [PATCH v3 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>
---
 fs/exec.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 4227c1e56566..9525ac15b897 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1350,6 +1350,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
@@ -1382,10 +1385,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]

#1690779 — [PATCH v3 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>
---
 fs/exec.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 81793a193d92..45418d6ec583 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1353,10 +1353,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();
@@ -1370,9 +1372,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]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web