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

[Roberto Sassu]

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.

Roberto