On Mon, May 11, 2020 at 11:52:41AM -0500, Eric W. Biederman wrote:
> Kees Cook <keescook@xxxxxxxxxxxx> 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@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...
>
> 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
--
Kees Cook
