| Message ID | 871rng22dm.fsf_-_@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | exec: Control flow simplifications | expand |
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>
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
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
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; }
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(+)