Re: [PATCH v2 2/8] exec: Move security_bprm_secureexec() earlier

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

Kees Cook <keescook@chromium.org> writes:

> On Mon, Jul 10, 2017 at 1:57 AM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>> Kees Cook <keescook@chromium.org> writes:
>>
>>> There are several places where exec needs to know if a privilege-gain has
>>> happened. These should be using the results of security_bprm_secureexec()
>>> but it is getting (needlessly) called very late.
>>
>> It is hard to tell at a glance but I believe this introduces a
>> regression.
>>
>> cap_bprm_set_creds is currently called before cap_bprm_secureexec and
>> it has a number of cases such as no_new_privs and ptrace that can result
>> in some of the precomputed credential changes not happening.
>>
>> Without accounting for that I believe your cap_bprm_securexec now
>> returns a postive value too early.
>
> It's still before cap_bprm_secureexec. cap_brpm_set_creds() is in
> prepare_binprm(), which is well before exec_binprm() and it's eventual
> call to setup_new_exec().

Good point.  I didn't double check and the set in the name had me
thinking it was setting the creds on current.

Is there any reason we need a second security hook?  It feels like we
should be able to just fold the secureexec hook into the set_creds hook.

The two are so interrelated I fear that having them separate only
encourages them to diverge in trivial ways as it is easy to forget about
some detail or other.

Certainly having them called from different functions seems wrong.  If
we know enough in prepare_binprm we know enough.

Eric