Re: [RFC][PATCH] bpf: Check xattr name/value pair from bpf_lsm_inode_init_security()

[Casey Schaufler]

On 10/27/2022 3:39 AM, KP Singh wrote:
> On Wed, Oct 26, 2022 at 7:14 PM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
>> On Wed, Oct 26, 2022 at 1:42 AM Roberto Sassu
>> <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
>>> On 10/26/2022 8:37 AM, Alexei Starovoitov wrote:
>>>> On Tue, Oct 25, 2022 at 7:58 AM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>>>>> On 10/25/2022 12:43 AM, Roberto Sassu wrote:
>>>>>> On Mon, 2022-10-24 at 19:13 -0700, Alexei Starovoitov wrote:
>>>>>>> I'm looking at security_inode_init_security() and it is indeed messy.
>>>>>>> Per file system initxattrs callback that processes kmalloc-ed
>>>>>>> strings.
>>>>>>> Yikes.
>>>>>>>
>>>>>>> In the short term we should denylist inode_init_security hook to
>>>>>>> disallow attaching bpf-lsm there. set/getxattr should be done
>>>>>>> through kfuncs instead of such kmalloc-a-string hack.
>>>>>> Inode_init_security is an example. It could be that the other hooks are
>>>>>> affected too. What happens if they get arbitrary positive values too?
>>>>> TL;DR - Things will go cattywampus.
>>>>>
>>>>> The LSM infrastructure is an interface that has "grown organically",
>>>>> and isn't necessarily consistent in what it requires of the security
>>>>> module implementations. There are cases where it assumes that the
>>>>> security module hooks are well behaved, as you've discovered. I have
>>>>> no small amount of fear that someone is going to provide an eBPF
>>>>> program for security_secid_to_secctx(). There has been an assumption,
>>>>> oft stated, that all security modules are going to be reviewed as
>>>>> part of the upstream process. The review process ought to catch hooks
>>>>> that return unacceptable values. Alas, we've lost that with BPF.
>>>>>
>>>>> It would take a(nother) major overhaul of the LSM infrastructure to
>>>>> make it safe against hooks that are not well behaved. From what I have
>>>>> seen so far it wouldn't be easy/convenient/performant to do it in the
>>>>> BPF security module either. I personally think that BPF needs to
>>>>> ensure that the eBPF implementations don't return inappropriate values,
>>>>> but I understand why that is problematic.
>>>> That's an accurate statement. Thank you.
>>>>
>>>> Going back to the original question...
>>>> We fix bugs when we discover them.
>>>> Regardless of the subsystem they belong to.
>>>> No finger pointing.
>>> I'm concerned about the following situation:
>>>
>>> struct <something> *function()
>>> {
>>>
>>>         ret = security_*();
>>>         if (ret)
>>>                 return ERR_PTR(ret);
>>>
>>> }
>>>
>>> int caller()
>>> {
>>>         ptr = function()
>>>         if (IS_ERR(ptr)
>>>                 goto out;
>>>
>>>         <use of invalid pointer>
>>> }
>>>
>>> I quickly found an occurrence of this:
>>>
>>> static int lookup_one_common()
>>> {
>>>
>>> [...]
>>>
>>>         return inode_permission();
>>> }
>>>
>>> struct dentry *try_lookup_one_len()
>>> {
>>>
>>> [...]
>>>
>>>          err = lookup_one_common(&init_user_ns, name, base, len, &this);
>>>          if (err)
>>>                  return ERR_PTR(err);
>>>
>>>
>>> Unfortunately, attaching to inode_permission causes the kernel
>>> to crash immediately (it does not happen with negative return
>>> values).
>>>
>>> So, I think the fix should be broader, and not limited to the
>>> inode_init_security hook. Will try to see how it can be fixed.
>> I see. Let's restrict bpf-lsm return values to IS_ERR_VALUE.
>> Trivial verifier change.
> Thanks, yes this indeed is an issue. We need to do a few things:
>
> 1. Restrict some hooks that we know the BPF LSM will never need.

It might be difficult to identify which hooks will never be useful
in a general purpose programming system like BPF. I do suggest that,
if at all possible, you restrict any hook that uses or provides a
secid. That will take out the bulk of the "dangerous" hooks.

> 2. A verifier function that checks return values of LSM
> hooks.

That would be grand.

> For most LSK hooks IS_ERR_VALUE is fine, however, there are some hooks
> like *xattr hooks that use a return value of 1 to indicate a
> capability check is required which might need special handling.

The exceptions are pretty rare, and I don't see a reason why
we couldn't "normalize", or at least more clearly document the
outliers.