Re: [PATCH bpf-next v7 4/8] bpf: lsm: Implement attach, detach and execution

[Casey Schaufler <casey@schaufler-ca.com>]

On 3/27/2020 6:43 AM, Stephen Smalley wrote:
> On 3/27/20 8:41 AM, KP Singh wrote:
>> On 27-Mär 08:27, Stephen Smalley wrote:
>>> On 3/26/20 8:24 PM, James Morris wrote:
>>>> On Thu, 26 Mar 2020, KP Singh wrote:
>>>>
>>>>> +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
>>>>> +            const struct bpf_prog *prog)
>>>>> +{
>>>>> +    /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks
>>>>> +     */
>>>>> +    if (!capable(CAP_MAC_ADMIN))
>>>>> +        return -EPERM;
>>>>> +
>>>>
>>>> Stephen, can you confirm that your concerns around this are resolved
>>>> (IIRC, by SELinux implementing a bpf_prog callback) ?
>>>
>>> I guess the only residual concern I have is that CAP_MAC_ADMIN means
>>> something different to SELinux (ability to get/set file security contexts
>>> unknown to the currently loaded policy), so leaving the CAP_MAC_ADMIN check
>>> here (versus calling a new security hook here and checking CAP_MAC_ADMIN in
>>> the implementation of that hook for the modules that want that) conflates
>>> two very different things.  Prior to this patch, there are no users of
>>> CAP_MAC_ADMIN outside of individual security modules; it is only checked in
>>> module-specific logic within apparmor, safesetid, selinux, and smack, so the
>>> meaning was module-specific.
>>
>> As we had discussed, We do have a security hook as well:
>>
>> https://lore.kernel.org/bpf/20200324180652.GA11855@chromium.org/
>>
>> The bpf_prog hook which can check for BPF_PROG_TYPE_LSM and implement
>> module specific logic for LSM programs. I thougt that was okay?
>>
>> Kees was in favor of keeping the CAP_MAC_ADMIN check here:
>>
>> https://lore.kernel.org/bpf/202003241133.16C02BE5B@keescook
>>
>> If you feel strongly and Kees agrees, we can remove the CAP_MAC_ADMIN
>> check here, but given that we already have a security hook that meets
>> the requirements, we probably don't need another one.
>
> I would favor removing the CAP_MAC_ADMIN check here, and implementing it in a bpf_prog hook for Smack and AppArmor if they want that.  SELinux would implement its own check in its existing bpf_prog hook.
>
The whole notion of one security module calling into another for permission
to do something still gives me the heebee jeebees, but if more nimble minds
than mine think this is a good idea I won't nack it.