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

[Roberto Sassu]

On Thu, 2022-10-27 at 12:39 +0200, 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.
> 2. A verifier function that checks return values of LSM
> hooks.
> 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.

I looked at security.c:

/*
 * SELinux and Smack integrate the cap call,
 * so assume that all LSMs supplying this call do so.
 */

Other than checking the return value, probably we should also wrap
bpf_lsm_inode_{set,remove}xattr() to do the capability check, right?

Roberto