Re: [PATCH 2/5] exec: Directly call security_bprm_set_creds from __do_execve_file

[ebiederm@xmission.com (Eric W. Biederman)]

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