Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: Kees Cook <keescook@chromium.org>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: linux-kernel@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Oleg Nesterov <oleg@redhat.com>, Jann Horn <jannh@google.com>,
	Greg Ungerer <gerg@linux-m68k.org>, Rob Landley <rob@landley.net>,
	Bernd Edlinger <bernd.edlinger@hotmail.de>,
	linux-fsdevel@vger.kernel.org, Al Viro <viro@ZenIV.linux.org.uk>,
	Alexey Dobriyan <adobriyan@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Casey Schaufler <casey@schaufler-ca.com>,
	linux-security-module@vger.kernel.org,
	James Morris <jmorris@namei.org>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Andy Lutomirski <luto@amacapital.net>
Subject: Re: [PATCH 2/2] exec: Compute file based creds only once
Date: Fri, 29 May 2020 14:24:46 -0700
Message-ID: <202005291406.52E27AF8@keescook> (raw)
In-Reply-To: <871rn2r8m6.fsf_-_@x220.int.ebiederm.org>

On Fri, May 29, 2020 at 11:47:29AM -0500, Eric W. Biederman wrote:
> Move the computation of creds from prepare_binfmt into begin_new_exec
> so that the creds need only be computed once.  This is just code
> reorganization no semantic changes of any kind are made.
> 
> Moving the computation is safe.  I have looked through the kernel and
> verified none of the binfmts look at bprm->cred directly, and that
> there are no helpers that look at bprm->cred indirectly.  Which means
> that it is not a problem to compute the bprm->cred later in the
> execution flow as it is not used until it becomes current->cred.
> 
> A new function bprm_creds_from_file is added to contain the work that
> needs to be done.  bprm_creds_from_file first computes which file
> bprm->executable or most likely bprm->file that the bprm->creds
> will be computed from.
> 
> The funciton bprm_fill_uid is updated to receive the file instead of
> accessing bprm->file.  The now unnecessary work needed to reset the
> bprm->cred->euid, and bprm->cred->egid is removed from brpm_fill_uid.
> A small comment to document that bprm_fill_uid now only deals with the
> work to handle suid and sgid files.  The default case is already
> heandled by prepare_exec_creds.
> 
> The function security_bprm_repopulate_creds is renamed
> security_bprm_creds_from_file and now is explicitly passed the file
> from which to compute the creds.  The documentation of the
> bprm_creds_from_file security hook is updated to explain when the hook
> is called and what it needs to do.  The file is passed from
> cap_bprm_creds_from_file into get_file_caps so that the caps are
> computed for the appropriate file.  The now unnecessary work in
> cap_bprm_creds_from_file to reset the ambient capabilites has been
> removed.  A small comment to document that the work of
> cap_bprm_creds_from_file is to read capabilities from the files
> secureity attribute and derive capabilities from the fact the
> user had uid 0 has been added.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

This all looks good to me. Small notes below...

Reviewed-by: Kees Cook <keescook@chromium.org>

> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index cd3dd0afceb5..37bb3df751c6 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -44,18 +44,18 @@
>   *	request libc enable secure mode.
>   *	@bprm contains the linux_binprm structure.
>   *	Return 0 if the hook is successful and permission is granted.
> - * @bprm_repopulate_creds:
> - *	Assuming that the relevant bits of @bprm->cred->security have been
> - *	previously set, examine @bprm->file and regenerate them.  This is
> - *	so that the credentials derived from the interpreter the code is
> - *	actually going to run are used rather than credentials derived
> - *	from a script.  This done because the interpreter binary needs to
> - *	reopen script, and may end up opening something completely different.
> - *	This hook may also optionally check permissions (e.g. for
> - *	transitions between security domains).
> - *	The hook must set @bprm->active_secureexec to 1 if AT_SECURE should be set to
> + * @bprm_creds_from_file:
> + *	If @file is setpcap, suid, sgid or otherwise marked to change
> + *	privilege upon exec, update @bprm->cred to reflect that change.
> + *	This is called after finding the binary that will be executed.
> + *	without an interpreter.  This ensures that the credentials will not
> + *	be derived from a script that the binary will need to reopen, which
> + *	when reopend may end up being a completely different file.  This
> + *	hook may also optionally check permissions (e.g. for transitions
> + *	between security domains).
> + *	The hook must set @bprm->secureexec to 1 if AT_SECURE should be set to
>   *	request libc enable secure mode.
> - *	The hook must set @bprm->pf_per_clear to the personality flags that
> + *	The hook must set @bprm->per_clear to the personality flags that

Here and the other per_clear comment have language that doesn't quite
line up with how hooks should deal with the bits. They should not "set
it to" the personality flags they want clear, they need to "add the
bits" they want to see cleared. i.e I don't want something thinking
they're the only one touching per_clear, so they should never do:
	bprm->per_clear = PER_CLEAR_ON_SETID;
but always:
	bprm->per_clear |= PER_CLEAR_ON_SETID;

How about:

The hook must set @bprm->per_clear with any personality flag bits that

> diff --git a/security/commoncap.c b/security/commoncap.c

Not about this patch, but while looking through this file, I see:

int cap_bprm_set_creds(struct linux_binprm *bprm)
{
	...
	*capability manipulations*

        if (WARN_ON(!cap_ambient_invariant_ok(new)))
                return -EPERM;

        if (nonroot_raised_pE(new, old, root_uid, has_fcap)) {
                ret = audit_log_bprm_fcaps(bprm, new, old);
                if (ret < 0)
                        return ret;
        }

        new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);

        if (WARN_ON(!cap_ambient_invariant_ok(new)))
                return -EPERM;
	...
}

The cap_ambient_invariant_ok() test is needlessly repeated: it doesn't
examine securebits, and nonroot_raised_pE appears to have no
side-effects.

One of those can be dropped, yes?

-- 
Kees Cook

  reply index

Thread overview: 108+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <87h7wujhmz.fsf@x220.int.ebiederm.org>
     [not found] ` <87sgga6ze4.fsf@x220.int.ebiederm.org>
2020-05-09 19:40   ` [PATCH 0/5] exec: Control flow simplifications Eric W. Biederman
2020-05-09 19:40     ` [PATCH 1/5] exec: Call cap_bprm_set_creds directly from prepare_binprm Eric W. Biederman
2020-05-09 20:04       ` Linus Torvalds
2020-05-09 19:41     ` [PATCH 2/5] exec: Directly call security_bprm_set_creds from __do_execve_file Eric W. Biederman
2020-05-09 20:07       ` Linus Torvalds
2020-05-09 20:12         ` Eric W. Biederman
2020-05-09 20:19           ` Linus Torvalds
2020-05-11  3:15       ` Kees Cook
2020-05-11 16:52         ` Eric W. Biederman
2020-05-11 21:18           ` Kees Cook
2020-05-09 19:41     ` [PATCH 3/5] exec: Remove recursion from search_binary_handler Eric W. Biederman
2020-05-09 20:16       ` Linus Torvalds
2020-05-10  4:22       ` Tetsuo Handa
2020-05-10 19:38         ` Linus Torvalds
2020-05-11 14:33           ` Eric W. Biederman
2020-05-11 19:10             ` Rob Landley
2020-05-13 21:59               ` Eric W. Biederman
2020-05-14 18:46                 ` Rob Landley
2020-05-11 21:55             ` Kees Cook
2020-05-12 18:42               ` Eric W. Biederman
2020-05-12 19:25                 ` Kees Cook
2020-05-12 20:31                   ` Eric W. Biederman
2020-05-12 23:08                     ` Kees Cook
2020-05-12 23:47                       ` Kees Cook
2020-05-12 23:51                         ` Kees Cook
2020-05-14 14:56                           ` Eric W. Biederman
2020-05-14 16:56                             ` Casey Schaufler
2020-05-14 17:02                               ` Eric W. Biederman
2020-05-13  0:20                 ` Linus Torvalds
2020-05-13  2:39                   ` Rob Landley
2020-05-13 19:51                     ` Linus Torvalds
2020-05-14 16:49                   ` Eric W. Biederman
2020-05-09 19:42     ` [PATCH 4/5] exec: Allow load_misc_binary to call prepare_binfmt unconditionally Eric W. Biederman
2020-05-11 22:09       ` Kees Cook
2020-05-09 19:42     ` [PATCH 5/5] exec: Move the call of prepare_binprm into search_binary_handler Eric W. Biederman
2020-05-11 22:24       ` Kees Cook
2020-05-19  0:29     ` [PATCH v2 0/8] exec: Control flow simplifications Eric W. Biederman
2020-05-19  0:29       ` [PATCH v2 1/8] exec: Teach prepare_exec_creds how exec treats uids & gids Eric W. Biederman
2020-05-19 18:03         ` Kees Cook
2020-05-19 18:28           ` Linus Torvalds
2020-05-19 18:57             ` Eric W. Biederman
2020-05-19  0:30       ` [PATCH v2 2/8] exec: Factor security_bprm_creds_for_exec out of security_bprm_set_creds Eric W. Biederman
2020-05-19 15:34         ` Casey Schaufler
2020-05-19 18:10         ` Kees Cook
2020-05-19 21:28           ` James Morris
2020-05-19  0:31       ` [PATCH v2 3/8] exec: Convert security_bprm_set_creds into security_bprm_repopulate_creds Eric W. Biederman
2020-05-19 18:21         ` Kees Cook
2020-05-19 19:03           ` Eric W. Biederman
2020-05-19 19:14             ` Kees Cook
2020-05-20 20:22               ` Eric W. Biederman
2020-05-20 20:53                 ` Kees Cook
2020-05-19 21:52         ` James Morris
2020-05-20 12:40           ` Eric W. Biederman
2020-05-19  0:31       ` [PATCH v2 4/8] exec: Allow load_misc_binary to call prepare_binfmt unconditionally Eric W. Biederman
2020-05-19 18:27         ` Kees Cook
2020-05-19 19:08           ` Eric W. Biederman
2020-05-19 19:17             ` Kees Cook
2020-05-19  0:32       ` [PATCH v2 5/8] exec: Move the call of prepare_binprm into search_binary_handler Eric W. Biederman
2020-05-19 18:27         ` Kees Cook
2020-05-19 21:30         ` James Morris
2020-05-19  0:33       ` [PATCH v2 6/8] exec/binfmt_script: Don't modify bprm->buf and then return -ENOEXEC Eric W. Biederman
2020-05-19 19:08         ` Kees Cook
2020-05-19 19:19           ` Eric W. Biederman
2020-05-19  0:33       ` [PATCH v2 7/8] exec: Generic execfd support Eric W. Biederman
2020-05-19 19:46         ` Kees Cook
2020-05-19 19:54           ` Linus Torvalds
2020-05-19 20:20             ` Eric W. Biederman
2020-05-19 21:59         ` Rob Landley
2020-05-20 16:05           ` Eric W. Biederman
2020-05-21 22:50             ` Rob Landley
2020-05-22  3:28               ` Eric W. Biederman
2020-05-22  4:51                 ` Rob Landley
2020-05-22 13:35                   ` Eric W. Biederman
2020-05-19  0:34       ` [PATCH v2 8/8] exec: Remove recursion from search_binary_handler Eric W. Biederman
2020-05-19 20:37         ` Kees Cook
2020-05-19  1:25       ` [PATCH v2 0/8] exec: Control flow simplifications Linus Torvalds
2020-05-19 21:55       ` Kees Cook
2020-05-20 13:02         ` Eric W. Biederman
2020-05-20 22:12       ` Eric W. Biederman
2020-05-20 23:43         ` Kees Cook
2020-05-21 11:53           ` Eric W. Biederman
2020-05-28 15:38       ` [PATCH 0/11] exec: cred calculation simplifications Eric W. Biederman
2020-05-28 15:41         ` [PATCH 01/11] exec: Reduce bprm->per_clear to a single bit Eric W. Biederman
2020-05-28 19:04           ` Linus Torvalds
2020-05-28 19:17             ` Eric W. Biederman
2020-05-28 15:42         ` [PATCH 02/11] exec: Introduce active_per_clear the per file version of per_clear Eric W. Biederman
2020-05-28 19:05           ` Linus Torvalds
2020-05-28 15:42         ` [PATCH 03/11] exec: Compute file based creds only once Eric W. Biederman
2020-05-28 15:43         ` [PATCH 04/11] exec: Move uid/gid handling from creds_from_file into bprm_fill_uid Eric W. Biederman
2020-05-28 15:44         ` Eric W. Biederman
2020-05-28 15:44         ` [PATCH 05/11] exec: In bprm_fill_uid use CAP_SETGID to see if a gid change is safe Eric W. Biederman
2020-05-28 15:48         ` [PATCH 06/11] exec: Don't set secureexec when the uid or gid changes are abandoned Eric W. Biederman
2020-05-28 15:48         ` [PATCH 07/11] exec: Set saved, fs, and effective ids together in bprm_fill_uid Eric W. Biederman
2020-05-28 15:49         ` [PATCH 08/11] exec: In bprm_fill_uid remove unnecessary no new privs check Eric W. Biederman
2020-05-28 15:49         ` [PATCH 09/11] exec: In bprm_fill_uid only set per_clear when honoring suid or sgid Eric W. Biederman
2020-05-28 19:08           ` Linus Torvalds
2020-05-28 19:21             ` Eric W. Biederman
2020-05-28 15:50         ` [PATCH 10/11] exec: In bprm_fill_uid set secureexec at same time as per_clear Eric W. Biederman
2020-05-28 15:50         ` [PATCH 11/11] exec: Remove the label after_setid from bprm_fill_uid Eric W. Biederman
2020-05-29 16:45         ` [PATCH 0/2] exec: Remove the computation of bprm->cred Eric W. Biederman
2020-05-29 16:46           ` [PATCH 1/2] exec: Add a per bprm->file version of per_clear Eric W. Biederman
2020-05-29 21:06             ` Kees Cook
2020-05-30  3:23               ` Eric W. Biederman
2020-05-30  5:14                 ` Kees Cook
2020-05-29 16:47           ` [PATCH 2/2] exec: Compute file based creds only once Eric W. Biederman
2020-05-29 21:24             ` Kees Cook [this message]
2020-05-30  3:28               ` Eric W. Biederman
2020-05-30  5:18                 ` Kees Cook

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=202005291406.52E27AF8@keescook \
    --to=keescook@chromium.org \
    --cc=adobriyan@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=bernd.edlinger@hotmail.de \
    --cc=casey@schaufler-ca.com \
    --cc=ebiederm@xmission.com \
    --cc=gerg@linux-m68k.org \
    --cc=jannh@google.com \
    --cc=jmorris@namei.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=oleg@redhat.com \
    --cc=rob@landley.net \
    --cc=serge@hallyn.com \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@ZenIV.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git