| Message ID | 87k11kzyjm.fsf_-_@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | exec: Control flow simplifications | expand |
On Sat, May 9, 2020 at 12:44 PM Eric W. Biederman <ebiederm@xmission.com> 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. Ahh, good, this patch removes the 'called_set_creds' logic from the security subsystems. So it does half of what I asked for: please also just rename that "security_bprm_set_creds()" to be "security_primary_bprm_set_creds()" so that the change of semantics also shows up that way. And so that there is no confusion about the fact that "cap_bprm_set_creds()" has absolutely nothing to do with "security_bprm_set_creds()" any more. Linus
Linus Torvalds <torvalds@linux-foundation.org> writes: > On Sat, May 9, 2020 at 12:44 PM Eric W. Biederman <ebiederm@xmission.com> 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. > > Ahh, good, this patch removes the 'called_set_creds' logic from the > security subsystems. > > So it does half of what I asked for: please also just rename that > "security_bprm_set_creds()" to be "security_primary_bprm_set_creds()" > so that the change of semantics also shows up that way. > > And so that there is no confusion about the fact that > "cap_bprm_set_creds()" has absolutely nothing to do with > "security_bprm_set_creds()" any more. I agree something needs to be renamed, to remove confusion. I am off for a nap now, and tomorrow is Mother's day so I probably won't be back to this seriously until Monday. But please disect these patches and I will address any problems. Eric
On Sat, May 9, 2020 at 1:15 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > I agree something needs to be renamed, to remove confusion. Yeah, the alternative is to rename the capability version. I don't care much which way it goes, although I do think it's best to call out explicitly that the security hook functions get only the "primary" executable brpm info. Which is why I'd prefer to just rename all those low-level security cases. It makes for a slightly bigger patch, but I think it makes for better readability, and makes it explicit that that hook is literally just for the primary executable, not for the interpreter or whatever. Linus
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@xmission.com> > --- > 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 <keescook@chromium.org> writes: > 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@xmission.com> >> --- >> 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... I think you are getting prepare_binprm confused with prepare_bprm_creds. Understandable given the similarity of their names. > 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... If you look at the flow of the code in __do_execve_file before this change it is: prepare_bprm_creds() check_unsafe_exec() ... prepare_binprm() bprm_file_uid() bprm->cred->euid = current_euid() bprm->cred->egid = current_egid() security_bprm_set_creds() for_each_lsm() lsm->bprm_set_creds() if (called_set_creds) return; ... bprm->called_set_creds = 1; ... exec_binprm() search_binary_handler() security_bprm_check() tomoyo_bprm_check_security() ima_bprm_check() load_script() prepare_binprm() /* called_set_creds already == 1 */ bprm_file_uid() security_bprm_set_creds() for_each_lsm() lsm->bprm_set_creds() if (called_set_creds) return; ... search_binary_handler() security_bprm_check_security() load_elf_binary() ... setup_new_exec ... Assuming you are executing a shell script. Now bprm_file_uid is written with the assumption that it will be called multiple times and it reinitializes all of it's variables each time. As you can see in above the implementations of bprm_set_creds() only really execute before called_set_creds is set, aka the first time. They in no way see the final state. Further when I looked as those hooks they were not looking at the values set by bprm_file_uid at all. There were busy with the values their they needed to set in that hook for their particular lsm. So while in theory I can see the danger of moving above bprm_file_uid I don't see anything in practice that would be a problem. Further by moving the call of security_bprm_set_creds out of prepare_binprm int __do_execve_file just before the call of prepare_binprm I am just moving the call above binprm_fill_uid and nothing else. So I think you just confused prepare_bprm_creds with prepare_binprm. As most of your criticisms appear valid in that case. Can you take a second look? Thank you, Eric
On Mon, May 11, 2020 at 11:52:41AM -0500, Eric W. Biederman wrote: > Kees Cook <keescook@chromium.org> writes: > > > 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@xmission.com> > >> --- > >> 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... > > I think you are getting prepare_binprm confused with prepare_bprm_creds. > Understandable given the similarity of their names. I fixated on a bad example, having confused myself about when check_unsafe_exec() happens. My original concern (with the bad example) was that the LSM is having security_bprm_set_creds() called before the new cred in bprm->cred has been initialized with all the correct uid/gid, caps, and associated flags. But anything associated with capabilities should be confined to the commoncap LSM, though there is "leakage" into the uid/gid states and some bprm state (more on this later). That said, as you also found, I can't find any LSM that examines those fields of the cred (I had stopped this research last night when I saw check_unsafe_exec() and confused myself); they're all looking at other bprm state not associated with caps and uid changes (file, unsafe_exec, security field of new cred, etc). So that's very good! That means we've actually kept a bright line between things here -- whew. > > 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... > > If you look at the flow of the code in __do_execve_file before this > change it is: > > prepare_bprm_creds() > check_unsafe_exec() > > ... > > prepare_binprm() > bprm_file_uid() (bprm_fill_uid(), but yes) > bprm->cred->euid = current_euid() > bprm->cred->egid = current_egid() > security_bprm_set_creds() > for_each_lsm() > lsm->bprm_set_creds() > if (called_set_creds) > return; > ... > bprm->called_set_creds = 1; > ... > > exec_binprm() > search_binary_handler() > security_bprm_check() > tomoyo_bprm_check_security() > ima_bprm_check() > load_script() > prepare_binprm() > /* called_set_creds already == 1 */ > bprm_file_uid() > security_bprm_set_creds() > for_each_lsm() > lsm->bprm_set_creds() > if (called_set_creds) > return; > ... > search_binary_handler() > security_bprm_check_security() > load_elf_binary() > ... > setup_new_exec > ... > > > Assuming you are executing a shell script. > > Now bprm_file_uid is written with the assumption that it will be called > multiple times and it reinitializes all of it's variables each time. Right -- and the same is true for cap_bprm_set_creds() (in that it needs to be run multiple times and depends on the work done in bprm_fill_uid()). If we encounter a future use-case for having other LSMs call out here multiple time, we can introduce a new LSM hook. > As you can see in above the implementations of bprm_set_creds() only > really execute before called_set_creds is set, aka the first time. > They in no way see the final state. > > Further when I looked as those hooks they were not looking at the values > set by bprm_file_uid at all. There were busy with the values their > they needed to set in that hook for their particular lsm. Agreed (though I'd love some other LSM eyes on this conclusion). > So while in theory I can see the danger of moving above bprm_file_uid > I don't see anything in practice that would be a problem. > > Further by moving the call of security_bprm_set_creds out of > prepare_binprm int __do_execve_file just before the call of > prepare_binprm I am just moving the call above binprm_fill_uid > and nothing else. > > So I think you just confused prepare_bprm_creds with prepare_binprm. > As most of your criticisms appear valid in that case. Can you take a > second look? So, in earlier attempts to clean up code near all this, I removed the LSM's bprm_secureexec hook, which only commoncap was using to impart details about privilege elevation. I switched the semantics to having LSMs set bprm->secureexec to true (but never to zero). Since commoncap's idea of "was I elevated?" might repeatedly change, I had to store its results "privately" in the bprm, which got us cap_elevated (in 46d98eb4e1d2): c425e189ffd7 ("binfmt: Introduce secureexec flag") 993b3ab0642e ("apparmor: Refactor to remove bprm_secureexec hook") 62874c3adf70 ("selinux: Refactor to remove bprm_secureexec hook") 46d98eb4e1d2 ("commoncap: Refactor to remove bprm_secureexec hook") ee67ae7ef6ff ("commoncap: Move cap_elevated calculation into bprm_set_creds") 2af622802696 ("LSM: drop bprm_secureexec hook") So, given the special-case nature of capabilities here, this does seem to be the right choice (assuming we're not missing something in the other LSMs). As such, I think the comment for cap_elevated needs to be updated to reflect the change to function call flow, and to specify it cannot be used by the other LSMs. Maybe something like: /* * True if most recent call to cap_bprm_set_creds() * (due to multiple prepare_binprm() calls from the * binfmt_script/misc handlers) resulted in elevated * privileges. This is used internally by fs/exec.c * to set bprm->secureexec. */ cap_elevated:1, And that brings us to naming. Whee. I think we should make the following name changes: bprm_fill_uid -> bprm_establish_privileges cap_bprm_set_creds -> cap_establish_privileges Finally, I think we should update the comment on bprm_set_creds (which, actually, I think is the correct name now) to something like: * @bprm_set_creds: * Save security information in the @bprm->cred->security field, * typically based on information about the bprm->file, for later * use during the @bprm_committing_creds hook. Specifically * the credentials themselves (uid, gid, etc), are not finalized * yet and must not be examined until the @bprm_committing_creds * hook. * This hook is called once, after the creds structure has been * allocated. * The hook must set @bprm->secureexec to 1 if a "secure exec" * has happened as a result of this hook call. The flag is used to * indicate the need for a sanitized execution environment, and is * also passed in the ELF auxiliary table on the initial stack to * indicate whether libc should enable secure mode. * This hook may also optionally check LSM-specific permissions * (e.g. for transitions between security domains). * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. -Kees
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; diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 1b48e2154766..42f760acfc2c 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -26,12 +26,6 @@ struct linux_binprm { unsigned long p; /* current top of mem */ unsigned long argmin; /* rlimit marker for copy_strings() */ unsigned int - /* - * True after the bprm_set_creds hook has been called once - * (multiple calls can be made via prepare_binprm() for - * binfmt_script/misc). - */ - called_set_creds:1, /* * True if most recent call to the commoncaps bprm_set_creds * hook (due to multiple prepare_binprm() calls from the diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 6ceb74e0f789..61b9181a9e1f 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -875,9 +875,6 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) file_inode(bprm->file)->i_mode }; - if (bprm->called_set_creds) - return 0; - ctx = task_ctx(current); AA_BUG(!cred_label(bprm->cred)); AA_BUG(!ctx); diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 0b4e32161b77..ff3e1be53da5 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2297,8 +2297,6 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm) /* SELinux context only depends on initial program or script and not * the script interpreter */ - if (bprm->called_set_creds) - return 0; old_tsec = selinux_cred(current_cred()); new_tsec = selinux_cred(bprm->cred); diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 8c61d175e195..bd1967730fec 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -904,9 +904,6 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm) struct superblock_smack *sbsp; int rc; - if (bprm->called_set_creds) - return 0; - isp = smack_inode(inode); if (isp->smk_task == NULL || isp->smk_task == bsp->smk_task) return 0; diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c index 716c92ec941a..d965ce80a7fb 100644 --- a/security/tomoyo/tomoyo.c +++ b/security/tomoyo/tomoyo.c @@ -71,12 +71,6 @@ static void tomoyo_bprm_committed_creds(struct linux_binprm *bprm) */ static int tomoyo_bprm_set_creds(struct linux_binprm *bprm) { - /* - * Do only if this function is called for the first time of an execve - * operation. - */ - if (bprm->called_set_creds) - return 0; /* * Load policy if /sbin/tomoyo-init exists and /sbin/init is requested * for the first time.
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@xmission.com> --- 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(-)