On Sat, May 09, 2020 at 02:41:17PM -0500, Eric W. Biederman wrote: > > Now that security_bprm_set_creds is no longer responsible for calling > cap_bprm_set_creds, security_bprm_set_creds only does something for > the primary file that is being executed (not any interpreters it may > have). Therefore call security_bprm_set_creds from __do_execve_file, > instead of from prepare_binprm so that it is only called once, and > remove the now unnecessary called_set_creds field of struct binprm. > > Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx> > --- > fs/exec.c | 11 +++++------ > include/linux/binfmts.h | 6 ------ > security/apparmor/domain.c | 3 --- > security/selinux/hooks.c | 2 -- > security/smack/smack_lsm.c | 3 --- > security/tomoyo/tomoyo.c | 6 ------ > 6 files changed, 5 insertions(+), 26 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 765bfd51a546..635b5085050c 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1635,12 +1635,6 @@ int prepare_binprm(struct linux_binprm *bprm) > > bprm_fill_uid(bprm); > > - /* fill in binprm security blob */ > - retval = security_bprm_set_creds(bprm); > - if (retval) > - return retval; > - bprm->called_set_creds = 1; > - > retval = cap_bprm_set_creds(bprm); > if (retval) > return retval; > @@ -1858,6 +1852,11 @@ static int __do_execve_file(int fd, struct filename *filename, > if (retval < 0) > goto out; > > + /* fill in binprm security blob */ > + retval = security_bprm_set_creds(bprm); > + if (retval) > + goto out; > + > retval = prepare_binprm(bprm); > if (retval < 0) > goto out; > Here I go with a Sunday night review, so hopefully I'm thinking better than Friday night's review, but I *think* this patch is broken from the LSM sense of the world in that security_bprm_set_creds() is getting called _before_ the creds actually get fully set (in prepare_binprm() by the calls to bprm_fill_uid(), cap_bprm_set_creds(), and check_unsafe_exec()). As a specific example, see the setting of LSM_UNSAFE_NO_NEW_PRIVS in bprm->unsafe during check_unsafe_exec(), which must happen after bprm_fill_uid(bprm) and cap_bprm_set_creds(bprm), to have a "true" view of the execution privileges. Apparmor checks for this flag in its security_bprm_set_creds() hook. Similarly do selinux, smack, etc... The security_bprm_set_creds() boundary for LSM is to see the "final" state of the process privileges, and that needs to happen after bprm_fill_uid(), cap_bprm_set_creds(), and check_unsafe_exec() have all finished. So, as it stands, I don't think this will work, but perhaps it can still be rearranged to avoid the called_set_creds silliness. I'll look more this week... -Kees -- Kees Cook
