[v2,1/8] exec: Teach prepare_exec_creds how exec treats uids & gids
diff mbox series

Message ID 871rng22dm.fsf_-_@x220.int.ebiederm.org
State New
Headers show
Series
  • exec: Control flow simplifications
Related show

Commit Message

Eric W. Biederman May 19, 2020, 12:29 a.m. UTC
It is almost possible to use the result of prepare_exec_creds with no
modifications during exec.  Update prepare_exec_creds to initialize
the suid and the fsuid to the euid, and the sgid and the fsgid to the
egid.  This is all that is needed to handle the common case of exec
when nothing special like a setuid exec is happening.

That this preserves the existing behavior of exec can be verified
by examing bprm_fill_uid and cap_bprm_set_creds.

This change makes it clear that the later parts of exec that
update bprm->cred are just need to handle special cases such
as setuid exec and change of domains.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 kernel/cred.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Kees Cook May 19, 2020, 6:03 p.m. UTC | #1
On Mon, May 18, 2020 at 07:29:41PM -0500, Eric W. Biederman wrote:
> 
> It is almost possible to use the result of prepare_exec_creds with no
> modifications during exec.  Update prepare_exec_creds to initialize
> the suid and the fsuid to the euid, and the sgid and the fsgid to the
> egid.  This is all that is needed to handle the common case of exec
> when nothing special like a setuid exec is happening.
> 
> That this preserves the existing behavior of exec can be verified
> by examing bprm_fill_uid and cap_bprm_set_creds.

Yup, agreed.

> This change makes it clear that the later parts of exec that
> update bprm->cred are just need to handle special cases such
> as setuid exec and change of domains.

One question, though: why add this, since the repeat calling of the caps
LSM hook will do this? Is there a call ordering change here, or is this
just to make the new LSM hook more robust?

Regardless, this looks correct, if perhaps redundant. :)

Reviewed-by: Kees Cook <keescook@chromium.org>
Linus Torvalds May 19, 2020, 6:28 p.m. UTC | #2
On Tue, May 19, 2020 at 11:03 AM Kees Cook <keescook@chromium.org> wrote:
>
> One question, though: why add this, since the repeat calling of the caps
> LSM hook will do this?

I assume it's for the "preserve_creds" case where we don't even end up
setting creds at all.

Yeah, at some point we'll hit a bprm handler that doesn't set
'preserve_creds', and it all does get set in the end, but that's not
statically all that obvious.

I think it makes sense to initialize as much as possible from the
generic code, and rely as little as possible on what the binfmt
handlers end up actually doing.

              Linus
Eric W. Biederman May 19, 2020, 6:57 p.m. UTC | #3
Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Tue, May 19, 2020 at 11:03 AM Kees Cook <keescook@chromium.org> wrote:
>>
>> One question, though: why add this, since the repeat calling of the caps
>> LSM hook will do this?
>
> I assume it's for the "preserve_creds" case where we don't even end up
> setting creds at all.
>
> Yeah, at some point we'll hit a bprm handler that doesn't set
> 'preserve_creds', and it all does get set in the end, but that's not
> statically all that obvious.
>
> I think it makes sense to initialize as much as possible from the
> generic code, and rely as little as possible on what the binfmt
> handlers end up actually doing.

Where this initially came from was I was looking at how to clean up the
case of no_new_privs/ptrace of a suid executable when we don't have
enough permissions.   Just being able to create creds that kept
everything as they were looked very useful and there was just this one
little bit missing.

I included the change to prepare_exec_creds in this patchset to
emphasize that neither security_bprm_creds_for_exec nor
security_bprm_repopulate_creds need to do anything if there is nothing
special going on.

At the very least that helps me think through what the LSMs are required
to do, and what those hooks are for.  AKA privilege changing execs.

So I was thinking rely on the LSMs as little as possible rather than
rely on the binfmt handlers as little as possible.  But it is the same
idea.

And yes it makes everything easier to analyze if everything starts off
in a known good state.

Eric

Patch
diff mbox series

diff --git a/kernel/cred.c b/kernel/cred.c
index 71a792616917..421b1149c651 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -315,6 +315,9 @@  struct cred *prepare_exec_creds(void)
 	new->process_keyring = NULL;
 #endif
 
+	new->suid = new->fsuid = new->euid;
+	new->sgid = new->fsgid = new->egid;
+
 	return new;
 }