| Message ID | 87o8qkzrxp.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:31:14PM -0500, Eric W. Biederman wrote: > > Rename bprm->cap_elevated to bprm->active_secureexec and initialize it > in prepare_binprm instead of in cap_bprm_set_creds. Initializing > bprm->active_secureexec in prepare_binprm allows multiple > implementations of security_bprm_repopulate_creds to play nicely with > each other. > > Rename security_bprm_set_creds to security_bprm_reopulate_creds to > emphasize that this path recomputes part of bprm->cred. This > recomputation avoids the time of check vs time of use problems that > are inherent in unix #! interpreters. > > In short two renames and a move in the location of initializing > bprm->active_secureexec. I like this much better than the direct call to the capabilities hook. Thanks! Reviewed-by: Kees Cook <keescook@chromium.org> One nit is a bikeshed on the name "active_secureexec", since the word "active" isn't really associated with any other part of the binfmt logic. It's supposed to be "latest state from the binfmt loop", so instead of "active", I considered these words that I also didn't like: "current", "this", "recent", and "now". Is "latest" better than "active"? Probably not. > [...] > diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h > index d1217fcdedea..8605ab4a0f89 100644 > --- a/include/linux/binfmts.h > +++ b/include/linux/binfmts.h > @@ -27,10 +27,10 @@ struct linux_binprm { > unsigned long argmin; /* rlimit marker for copy_strings() */ > unsigned int > /* > - * True if most recent call to cap_bprm_set_creds > + * True if most recent call to security_bprm_set_creds > * resulted in elevated privileges. > */ > - cap_elevated:1, > + active_secureexec:1, Also, I'd like it if this comment could be made more verbose as well, for anyone trying to understand the binfmt execution flow for the first time. Perhaps: /* * Must be set True during the any call to * bprm_set_creds hook where the execution would * reuslt in elevated privileges. (The hook can be * called multiple times during nested interpreter * resolution across binfmt_script, binfmt_misc, etc). */
Kees Cook <keescook@chromium.org> writes: > On Mon, May 18, 2020 at 07:31:14PM -0500, Eric W. Biederman wrote: >> >> Rename bprm->cap_elevated to bprm->active_secureexec and initialize it >> in prepare_binprm instead of in cap_bprm_set_creds. Initializing >> bprm->active_secureexec in prepare_binprm allows multiple >> implementations of security_bprm_repopulate_creds to play nicely with >> each other. >> >> Rename security_bprm_set_creds to security_bprm_reopulate_creds to >> emphasize that this path recomputes part of bprm->cred. This >> recomputation avoids the time of check vs time of use problems that >> are inherent in unix #! interpreters. >> >> In short two renames and a move in the location of initializing >> bprm->active_secureexec. > > I like this much better than the direct call to the capabilities hook. > Thanks! > > Reviewed-by: Kees Cook <keescook@chromium.org> > > One nit is a bikeshed on the name "active_secureexec", since > the word "active" isn't really associated with any other part of the > binfmt logic. It's supposed to be "latest state from the binfmt loop", > so instead of "active", I considered these words that I also didn't > like: "current", "this", "recent", and "now". Is "latest" better than > "active"? Probably not. I had pretty much the same problem. Active at least conveys that it is still malleable and might change. >> [...] >> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h >> index d1217fcdedea..8605ab4a0f89 100644 >> --- a/include/linux/binfmts.h >> +++ b/include/linux/binfmts.h >> @@ -27,10 +27,10 @@ struct linux_binprm { >> unsigned long argmin; /* rlimit marker for copy_strings() */ >> unsigned int >> /* >> - * True if most recent call to cap_bprm_set_creds >> + * True if most recent call to security_bprm_set_creds >> * resulted in elevated privileges. >> */ >> - cap_elevated:1, >> + active_secureexec:1, > > Also, I'd like it if this comment could be made more verbose as well, for > anyone trying to understand the binfmt execution flow for the first time. > Perhaps: > > /* > * Must be set True during the any call to > * bprm_set_creds hook where the execution would > * reuslt in elevated privileges. (The hook can be > * called multiple times during nested interpreter > * resolution across binfmt_script, binfmt_misc, etc). > */ Well it is not during but after the call that it becomes true. I think most recent covers the case of multiple calls. I think having the loop explicitly in the code a few patches later makes it clear that there is a loop dealing with interpreters. Conciseness has a virtue in that it is easy to absorb. Seeing active says most recent and secureexec does not is enough to ask questions and look at the code. Eric
On Tue, May 19, 2020 at 02:03:23PM -0500, Eric W. Biederman wrote: > Kees Cook <keescook@chromium.org> writes: > > > On Mon, May 18, 2020 at 07:31:14PM -0500, Eric W. Biederman wrote: > >> [...] > >> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h > >> index d1217fcdedea..8605ab4a0f89 100644 > >> --- a/include/linux/binfmts.h > >> +++ b/include/linux/binfmts.h > >> @@ -27,10 +27,10 @@ struct linux_binprm { > >> unsigned long argmin; /* rlimit marker for copy_strings() */ > >> unsigned int > >> /* > >> - * True if most recent call to cap_bprm_set_creds > >> + * True if most recent call to security_bprm_set_creds > >> * resulted in elevated privileges. > >> */ > >> - cap_elevated:1, > >> + active_secureexec:1, > > > > Also, I'd like it if this comment could be made more verbose as well, for > > anyone trying to understand the binfmt execution flow for the first time. > > Perhaps: > > > > /* > > * Must be set True during the any call to > > * bprm_set_creds hook where the execution would > > * reuslt in elevated privileges. (The hook can be > > * called multiple times during nested interpreter > > * resolution across binfmt_script, binfmt_misc, etc). > > */ > Well it is not during but after the call that it becomes true. > I think most recent covers the case of multiple calls. I'm thinking of an LSM writing reading these comments to decide what they need to do to the flags, so it's a direction to them to set it to true if they have determined that privilege was gained. (Though in theory, this is all moot since only the commoncap hook cares.) > I think having the loop explicitly in the code a few patches > later makes it clear that there is a loop dealing with interpreters. > > Conciseness has a virtue in that it is easy to absorb. Seeing > active says most recent and secureexec does not is enough to ask > questions and look at the code. I still think a hint about the nature of nested exec resolution would be nice in here somewhere, especially given that this value is zeroed before each call to the hook.
On Mon, 18 May 2020, Eric W. Biederman wrote: > diff --git a/fs/exec.c b/fs/exec.c > index 9e70da47f8d9..8e3b93d51d31 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1366,7 +1366,7 @@ int begin_new_exec(struct linux_binprm * bprm) > * the final state of setuid/setgid/fscaps can be merged into the > * secureexec flag. > */ > - bprm->secureexec |= bprm->cap_elevated; > + bprm->secureexec |= bprm->active_secureexec; Which kernel tree are these patches for? Seems like begin_new_exec() is from a prerequisite patchset.
James Morris <jmorris@namei.org> writes: > On Mon, 18 May 2020, Eric W. Biederman wrote: > >> diff --git a/fs/exec.c b/fs/exec.c >> index 9e70da47f8d9..8e3b93d51d31 100644 >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -1366,7 +1366,7 @@ int begin_new_exec(struct linux_binprm * bprm) >> * the final state of setuid/setgid/fscaps can be merged into the >> * secureexec flag. >> */ >> - bprm->secureexec |= bprm->cap_elevated; >> + bprm->secureexec |= bprm->active_secureexec; > > Which kernel tree are these patches for? Seems like begin_new_exec() is > from a prerequisite patchset. The base is: git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git exec-next I should have mentioned. I am several round deep in cleaning up exec already. begin_new_exec is essentially forget_old_exec. Eric
Kees Cook <keescook@chromium.org> writes: > On Tue, May 19, 2020 at 02:03:23PM -0500, Eric W. Biederman wrote: >> Kees Cook <keescook@chromium.org> writes: >> >> > On Mon, May 18, 2020 at 07:31:14PM -0500, Eric W. Biederman wrote: >> >> [...] >> >> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h >> >> index d1217fcdedea..8605ab4a0f89 100644 >> >> --- a/include/linux/binfmts.h >> >> +++ b/include/linux/binfmts.h >> >> @@ -27,10 +27,10 @@ struct linux_binprm { >> >> unsigned long argmin; /* rlimit marker for copy_strings() */ >> >> unsigned int >> >> /* >> >> - * True if most recent call to cap_bprm_set_creds >> >> + * True if most recent call to security_bprm_set_creds >> >> * resulted in elevated privileges. >> >> */ >> >> - cap_elevated:1, >> >> + active_secureexec:1, >> > >> > Also, I'd like it if this comment could be made more verbose as well, for >> > anyone trying to understand the binfmt execution flow for the first time. >> > Perhaps: >> > >> > /* >> > * Must be set True during the any call to >> > * bprm_set_creds hook where the execution would >> > * reuslt in elevated privileges. (The hook can be >> > * called multiple times during nested interpreter >> > * resolution across binfmt_script, binfmt_misc, etc). >> > */ >> Well it is not during but after the call that it becomes true. >> I think most recent covers the case of multiple calls. > > I'm thinking of an LSM writing reading these comments to decide what > they need to do to the flags, so it's a direction to them to set it to > true if they have determined that privilege was gained. (Though in > theory, this is all moot since only the commoncap hook cares.) The comments for an LSM writer are in include/linux/lsm_hooks.h * @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 * request libc enable secure mode. * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. I hope that is detailed enough. I will leave the rest of the comments for the maintainer of the code. I really don't think we should duplicate the prescriptive comments in multiple locations. Eric
On Wed, May 20, 2020 at 03:22:38PM -0500, Eric W. Biederman wrote: > Kees Cook <keescook@chromium.org> writes: > > > On Tue, May 19, 2020 at 02:03:23PM -0500, Eric W. Biederman wrote: > >> Kees Cook <keescook@chromium.org> writes: > >> > >> > On Mon, May 18, 2020 at 07:31:14PM -0500, Eric W. Biederman wrote: > >> >> [...] > >> >> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h > >> >> index d1217fcdedea..8605ab4a0f89 100644 > >> >> --- a/include/linux/binfmts.h > >> >> +++ b/include/linux/binfmts.h > >> >> @@ -27,10 +27,10 @@ struct linux_binprm { > >> >> unsigned long argmin; /* rlimit marker for copy_strings() */ > >> >> unsigned int > >> >> /* > >> >> - * True if most recent call to cap_bprm_set_creds > >> >> + * True if most recent call to security_bprm_set_creds > >> >> * resulted in elevated privileges. > >> >> */ > >> >> - cap_elevated:1, > >> >> + active_secureexec:1, > >> > > >> > Also, I'd like it if this comment could be made more verbose as well, for > >> > anyone trying to understand the binfmt execution flow for the first time. > >> > Perhaps: > >> > > >> > /* > >> > * Must be set True during the any call to > >> > * bprm_set_creds hook where the execution would > >> > * reuslt in elevated privileges. (The hook can be > >> > * called multiple times during nested interpreter > >> > * resolution across binfmt_script, binfmt_misc, etc). > >> > */ > >> Well it is not during but after the call that it becomes true. > >> I think most recent covers the case of multiple calls. > > > > I'm thinking of an LSM writing reading these comments to decide what > > they need to do to the flags, so it's a direction to them to set it to > > true if they have determined that privilege was gained. (Though in > > theory, this is all moot since only the commoncap hook cares.) > > The comments for an LSM writer are in include/linux/lsm_hooks.h > > * @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 > * request libc enable secure mode. > * @bprm contains the linux_binprm structure. > * Return 0 if the hook is successful and permission is granted. > > I hope that is detailed enough. > > I will leave the rest of the comments for the maintainer of the code. > > I really don't think we should duplicate the prescriptive comments in > multiple locations. Okay, that's fair enough. Thanks!
diff --git a/fs/exec.c b/fs/exec.c index 9e70da47f8d9..8e3b93d51d31 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1366,7 +1366,7 @@ int begin_new_exec(struct linux_binprm * bprm) * the final state of setuid/setgid/fscaps can be merged into the * secureexec flag. */ - bprm->secureexec |= bprm->cap_elevated; + bprm->secureexec |= bprm->active_secureexec; if (bprm->secureexec) { /* Make sure parent cannot signal privileged process. */ @@ -1634,10 +1634,10 @@ int prepare_binprm(struct linux_binprm *bprm) int retval; loff_t pos = 0; + /* Recompute parts of bprm->cred based on bprm->file */ + bprm->active_secureexec = 0; bprm_fill_uid(bprm); - - /* fill in binprm security blob */ - retval = security_bprm_set_creds(bprm); + retval = security_bprm_repopulate_creds(bprm); if (retval) return retval; diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index d1217fcdedea..8605ab4a0f89 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -27,10 +27,10 @@ struct linux_binprm { unsigned long argmin; /* rlimit marker for copy_strings() */ unsigned int /* - * True if most recent call to cap_bprm_set_creds + * True if most recent call to security_bprm_set_creds * resulted in elevated privileges. */ - cap_elevated:1, + active_secureexec:1, /* * Set by bprm_creds_for_exec hook to indicate a * privilege-gaining exec has happened. Used to set diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index aab0695f41df..1e295ba12c0d 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -50,7 +50,7 @@ LSM_HOOK(int, 0, settime, const struct timespec64 *ts, const struct timezone *tz) LSM_HOOK(int, 0, vm_enough_memory, struct mm_struct *mm, long pages) LSM_HOOK(int, 0, bprm_creds_for_exec, struct linux_binprm *bprm) -LSM_HOOK(int, 0, bprm_set_creds, struct linux_binprm *bprm) +LSM_HOOK(int, 0, bprm_repopulate_creds, struct linux_binprm *bprm) LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm) LSM_HOOK(void, LSM_RET_VOID, bprm_committing_creds, struct linux_binprm *bprm) LSM_HOOK(void, LSM_RET_VOID, bprm_committed_creds, struct linux_binprm *bprm) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index c719af37df20..d618ecc4d660 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -44,7 +44,7 @@ * request libc enable secure mode. * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. - * @bprm_set_creds: + * @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 @@ -53,7 +53,7 @@ * 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->cap_elevated to 1 if AT_SECURE should be set to + * The hook must set @bprm->active_secureexec to 1 if AT_SECURE should be set to * request libc enable secure mode. * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. diff --git a/include/linux/security.h b/include/linux/security.h index 1bd7a6582775..d23f078eb589 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -140,7 +140,7 @@ extern int cap_capset(struct cred *new, const struct cred *old, const kernel_cap_t *effective, const kernel_cap_t *inheritable, const kernel_cap_t *permitted); -extern int cap_bprm_set_creds(struct linux_binprm *bprm); +extern int cap_bprm_repopulate_creds(struct linux_binprm *bprm); extern int cap_inode_setxattr(struct dentry *dentry, const char *name, const void *value, size_t size, int flags); extern int cap_inode_removexattr(struct dentry *dentry, const char *name); @@ -277,7 +277,7 @@ int security_syslog(int type); int security_settime64(const struct timespec64 *ts, const struct timezone *tz); int security_vm_enough_memory_mm(struct mm_struct *mm, long pages); int security_bprm_creds_for_exec(struct linux_binprm *bprm); -int security_bprm_set_creds(struct linux_binprm *bprm); +int security_bprm_repopulate_creds(struct linux_binprm *bprm); int security_bprm_check(struct linux_binprm *bprm); void security_bprm_committing_creds(struct linux_binprm *bprm); void security_bprm_committed_creds(struct linux_binprm *bprm); @@ -575,9 +575,9 @@ static inline int security_bprm_creds_for_exec(struct linux_binprm *bprm) return 0; } -static inline int security_bprm_set_creds(struct linux_binprm *bprm) +static inline int security_bprm_repopulate_creds(struct linux_binprm *bprm) { - return cap_bprm_set_creds(bprm); + return cap_bprm_repopluate_creds(bprm); } static inline int security_bprm_check(struct linux_binprm *bprm) diff --git a/security/commoncap.c b/security/commoncap.c index f4ee0ae106b2..045b5b80ea40 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -797,14 +797,14 @@ static inline bool nonroot_raised_pE(struct cred *new, const struct cred *old, } /** - * cap_bprm_set_creds - Set up the proposed credentials for execve(). + * cap_bprm_repopulate_creds - Set up the proposed credentials for execve(). * @bprm: The execution parameters, including the proposed creds * * Set up the proposed credentials for a new execution context being * constructed by execve(). The proposed creds in @bprm->cred is altered, * which won't take effect immediately. Returns 0 if successful, -ve on error. */ -int cap_bprm_set_creds(struct linux_binprm *bprm) +int cap_bprm_repopulate_creds(struct linux_binprm *bprm) { const struct cred *old = current_cred(); struct cred *new = bprm->cred; @@ -884,12 +884,11 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) return -EPERM; /* Check for privilege-elevated exec. */ - bprm->cap_elevated = 0; if (is_setid || (!__is_real(root_uid, new) && (effective || __cap_grew(permitted, ambient, new)))) - bprm->cap_elevated = 1; + bprm->active_secureexec = 1; return 0; } @@ -1346,7 +1345,7 @@ static struct security_hook_list capability_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(ptrace_traceme, cap_ptrace_traceme), 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_repopulate_creds, cap_bprm_repopulate_creds), LSM_HOOK_INIT(inode_need_killpriv, cap_inode_need_killpriv), LSM_HOOK_INIT(inode_killpriv, cap_inode_killpriv), LSM_HOOK_INIT(inode_getsecurity, cap_inode_getsecurity), diff --git a/security/security.c b/security/security.c index 4ee76a729f73..b890b7e2a765 100644 --- a/security/security.c +++ b/security/security.c @@ -828,9 +828,9 @@ int security_bprm_creds_for_exec(struct linux_binprm *bprm) return call_int_hook(bprm_creds_for_exec, 0, bprm); } -int security_bprm_set_creds(struct linux_binprm *bprm) +int security_bprm_repopulate_creds(struct linux_binprm *bprm) { - return call_int_hook(bprm_set_creds, 0, bprm); + return call_int_hook(bprm_repopulate_creds, 0, bprm); } int security_bprm_check(struct linux_binprm *bprm)
Rename bprm->cap_elevated to bprm->active_secureexec and initialize it in prepare_binprm instead of in cap_bprm_set_creds. Initializing bprm->active_secureexec in prepare_binprm allows multiple implementations of security_bprm_repopulate_creds to play nicely with each other. Rename security_bprm_set_creds to security_bprm_reopulate_creds to emphasize that this path recomputes part of bprm->cred. This recomputation avoids the time of check vs time of use problems that are inherent in unix #! interpreters. In short two renames and a move in the location of initializing bprm->active_secureexec. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- fs/exec.c | 8 ++++---- include/linux/binfmts.h | 4 ++-- include/linux/lsm_hook_defs.h | 2 +- include/linux/lsm_hooks.h | 4 ++-- include/linux/security.h | 8 ++++---- security/commoncap.c | 9 ++++----- security/security.c | 4 ++-- 7 files changed, 19 insertions(+), 20 deletions(-)