Kees Cook <keescook@xxxxxxxxxxxx> writes:
> On Fri, May 29, 2020 at 11:47:29AM -0500, Eric W. Biederman wrote:
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index cd3dd0afceb5..37bb3df751c6 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -44,18 +44,18 @@
>> * request libc enable secure mode.
>> - * The hook must set @bprm->pf_per_clear to the personality flags that
>> + * The hook must set @bprm->per_clear to the personality flags that
>
> Here and the other per_clear comment have language that doesn't quite
> line up with how hooks should deal with the bits. They should not "set
> it to" the personality flags they want clear, they need to "add the
> bits" they want to see cleared. i.e I don't want something thinking
> they're the only one touching per_clear, so they should never do:
> bprm->per_clear = PER_CLEAR_ON_SETID;
> but always:
> bprm->per_clear |= PER_CLEAR_ON_SETID;
>
> How about:
>
> The hook must set @bprm->per_clear with any personality flag bits that
Sounds good:
The range-diff winds up being:
1: c9258ef4879b ! 1: a7868323c263 exec: Add a per bprm->file version of per_clear
@@ Commit message
History Tree: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
Fixes: 1bb0fa189c6a ("[PATCH] NX: clean up legacy binary support")
+ Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
## fs/exec.c ##
@@ include/linux/lsm_hooks.h
* transitions between security domains).
* The hook must set @bprm->active_secureexec to 1 if AT_SECURE should be set to
* request libc enable secure mode.
-+ * The hook must set @bprm->pf_per_clear to the personality flags that
-+ * should be cleared from current->personality.
++ * The hook must add to @bprm->pf_per_clear any personality flags that
++ * should be cleared from current->personality.
* @bprm contains the linux_binprm structure.
* Return 0 if the hook is successful and permission is granted.
* @bprm_check_security:
2: e6f20c69b96e ! 2: 56305aa9b6fa exec: Compute file based creds only once
@@ Commit message
secureity attribute and derive capabilities from the fact the
user had uid 0 has been added.
+ Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
## fs/binfmt_misc.c ##
@@ include/linux/lsm_hooks.h
+ * between security domains).
+ * The hook must set @bprm->secureexec to 1 if AT_SECURE should be set to
* request libc enable secure mode.
-- * The hook must set @bprm->pf_per_clear to the personality flags that
-+ * The hook must set @bprm->per_clear to the personality flags that
- * should be cleared from current->personality.
+- * The hook must add to @bprm->pf_per_clear any personality flags that
++ * The hook must add to @bprm->per_clear any personality flags that
+ * should be cleared from current->personality.
* @bprm contains the linux_binprm structure.
* Return 0 if the hook is successful and permission is granted.
>> diff --git a/security/commoncap.c b/security/commoncap.c
>
> Not about this patch, but while looking through this file, I see:
>
> int cap_bprm_set_creds(struct linux_binprm *bprm)
> {
> ...
> *capability manipulations*
>
> if (WARN_ON(!cap_ambient_invariant_ok(new)))
> return -EPERM;
>
> if (nonroot_raised_pE(new, old, root_uid, has_fcap)) {
> ret = audit_log_bprm_fcaps(bprm, new, old);
> if (ret < 0)
> return ret;
> }
>
> new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
>
> if (WARN_ON(!cap_ambient_invariant_ok(new)))
> return -EPERM;
> ...
> }
>
> The cap_ambient_invariant_ok() test is needlessly repeated: it doesn't
> examine securebits, and nonroot_raised_pE appears to have no
> side-effects.
>
> One of those can be dropped, yes?
That is what it looks like to me.
I am hoping to take a deep dive into this function after I finish with
bprm_fill_uid (the patches that were dropped).
My brain bends on little details like is_setid not testing if the
excutable was suid or sgid, but instead is testing something close but
unrelated.
I hope that when the dust clears the function can become a
straightforward implementation of the capability equations.
We will see.
Eric
