On Thu, Jun 30, 2022 at 3:26 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> On Thu, Jun 30, 2022 at 02:21:56PM +0200, KP Singh wrote:
> > On Thu, Jun 30, 2022 at 1:45 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> > >
> > > On Wed, Jun 29, 2022 at 08:02:50PM -0700, Alexei Starovoitov wrote:
> > > > On Wed, Jun 29, 2022 at 2:56 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> >
> > [...]
> >
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx>
> > > > > > > > > > ---
> > > > > > > > > > .../testing/selftests/bpf/prog_tests/xattr.c | 54 +++++++++++++++++++
> > > > > > > >
> > > > > > > > [...]
> > > > > > > >
> > > > > > > > > > +SEC("lsm.s/bprm_committed_creds")
> > > > > > > > > > +void BPF_PROG(bprm_cc, struct linux_binprm *bprm)
> > > > > > > > > > +{
> > > > > > > > > > + struct task_struct *current = bpf_get_current_task_btf();
> > > > > > > > > > + char dir_xattr_value[64] = {0};
> > > > > > > > > > + int xattr_sz = 0;
> > > > > > > > > > +
> > > > > > > > > > + xattr_sz = bpf_getxattr(bprm->file->f_path.dentry,
> > > > > > > > > > + bprm->file->f_path.dentry->d_inode, XATTR_NAME,
> > > > > > > > > > + dir_xattr_value, 64);
> > > > > > > > >
> > > > > > > > > Yeah, this isn't right. You're not accounting for the caller's userns
> > > > > > > > > nor for the idmapped mount. If this is supposed to work you will need a
> > > > > > > > > variant of vfs_getxattr() that takes the mount's idmapping into account
> > > > > > > > > afaict. See what needs to happen after do_getxattr().
> > > > > > > >
> > > > > > > > Thanks for taking a look.
> > > > > > > >
> >
> > [...]
> >
> > > > > > >
> > > > > > > That will not be correct.
> > > > > > > posix_acl_fix_xattr_to_user checking current_user_ns()
> > > > > > > is checking random tasks that happen to be running
> > > > > > > when lsm hook got invoked.
> > > > > > >
> > > > > > > KP,
> > > > > > > we probably have to document clearly that neither 'current*'
> > > > > > > should not be used here.
> > > > > > > xattr_permission also makes little sense in this context.
> > > > > > > If anything it can be a different kfunc if there is a use case,
> > > > > > > but I don't see it yet.
> > > > > > > bpf-lsm prog calling __vfs_getxattr is just like other lsm-s that
> > > > > > > call it directly. It's the kernel that is doing its security thing.
> > > > > >
> > > > > > Right, but LSMs usually only retrieve their own xattr namespace (ima,
> > > > > > selinux, smack) or they calculate hashes for xattrs based on the raw
> > > > > > filesystem xattr values (evm).
> > > > > >
> > > > > > But this new bpf_getxattr() is different. It allows to retrieve _any_
> > > > > > xattr in any security hook it can be attached to. So someone can write a
> > > > > > bpf program that retrieves filesystem capabilites or posix acls. And
> > > > > > these are xattrs that require higher-level vfs involvement to be
> > > > > > sensible in most contexts.
> > > > > >
> >
> > [...]
> >
> > > > > >
> > > > > > This hooks a bpf-lsm program to the security_bprm_committed_creds()
> > > > > > hook. It then retrieves the extended attributes of the file to be
> > > > > > executed. The hook currently always retrieves the raw filesystem values.
> > > > > >
> > > > > > But for example any XATTR_NAME_CAPS filesystem capabilities that
> > > > > > might've been stored will be taken into account during exec. And both
> > > > > > the idmapping of the mount and the caller matter when determing whether
> > > > > > they are used or not.
> > > > > >
> > > > > > But the current implementation of bpf_getxattr() just ignores both. It
> > > > > > will always retrieve the raw filesystem values. So if one invokes this
> > > > > > hook they're not actually retrieving the values as they are seen by
> > > > > > fs/exec.c. And I'm wondering why that is ok? And even if this is ok for
> > > > > > some use-cases it might very well become a security issue in others if
> > > > > > access decisions are always based on the raw values.
> > > > > >
> > > > > > I'm not well-versed in this so bear with me, please.
> > > > >
> > > > > If this is really just about retrieving the "security.bpf" xattr and no
> > > > > other xattr then the bpf_getxattr() variant should somehow hard-code
> > > > > that to ensure that no other xattrs can be retrieved, imho.
> > > >
> > > > All of these restrictions look very artificial to me.
> > > > Especially the part "might very well become a security issue"
> > > > just doesn't click.
> > > > We're talking about bpf-lsm progs here that implement security.
> > > > Can somebody implement a poor bpf-lsm that doesn't enforce
> > > > any actual security? Sure. It's a code.
> > >
> > > The point is that with the current implementation of bpf_getxattr() you
> > > are able to retrieve any xattrs and we have way less control over a
> > > bpf-lsm program than we do over selinux which a simple git grep
> > > __vfs_getxattr() is all we need.
> > >
> > > The thing is that with bpf_getxattr() as it stands it is currently
> > > impossible to retrieve xattr values - specifically filesystem
> > > capabilities and posix acls - and see them exactly like the code you're
> > > trying to supervise is. And that seems very strange from a security
> > > perspective. So if someone were to write
> > >
> > > SEC("lsm.s/bprm_creds_from_file")
> > > void BPF_PROG(bprm_cc, struct linux_binprm *bprm)
> > > {
> > > struct task_struct *current = bpf_get_current_task_btf();
> > >
> > > xattr_sz = bpf_getxattr(bprm->file->f_path.dentry,
> > > bprm->file->f_path.dentry->d_inode,
> > > XATTR_NAME_POSIX_ACL_ACCESS, ..);
> > > // or
> > > xattr_sz = bpf_getxattr(bprm->file->f_path.dentry,
> > > bprm->file->f_path.dentry->d_inode,
> > > XATTR_NAME_CAPS, ..);
> > >
> > > }
> > >
> > > they'd get the raw nscaps and the raw xattrs back. But now, as just a
> > > tiny example, the nscaps->rootuid and the ->e_id fields in the posix
> > > ACLs make zero sense in this context.
> > >
> > > And what's more there's no way for the bpf-lsm program to turn them into
> > > something that makes sense in the context of the hook they are retrieved
> > > in. It lacks all the necessary helpers to do so afaict.
> > >
> > > > No one complains about the usage of EXPORT_SYMBOL(__vfs_getxattr)
> > > > in the existing LSMs like selinux.
> > >
> > > Selinux only cares about its own xattr namespace. It doesn't retrieve
> > > fscaps or posix acls and it's not possible to write selinux programs
> > > that do so. With the bpf-lsm that's very much possible.
> > >
> > > And if we'd notice selinux would start retrieving random xattrs we'd ask
> > > the same questions we do here.
> > >
> > > > No one complains about its usage in out of tree LSMs.
> > > > Is that a security issue? Of course not.
> > > > __vfs_getxattr is a kernel mechanism that LSMs use to implement
> > > > the security features they need.
> > > > __vfs_getxattr as kfunc here is pretty much the same as EXPORT_SYMBOL
> > > > with a big difference that it's EXPORT_SYMBOL_GPL.
> > > > BPF land doesn't have an equivalent of non-gpl export and is not going
> > > > to get one.
> >
> > I want to reiterate what Alexei is saying here:
> >
> > *Please* consider this as a simple wrapper around __vfs_getxattr
> > with a limited attach surface and extra verification checks and
> > and nothing else.
> >
> > What you are saying is __vfs_getxattr does not make sense in some
> > contexts. But kernel modules can still use it right?
> >
> > The user is implementing an LSM, if they chose to do things that don't make
> > sense, then they can surely cause a lot more harm:
> >
> > SEC("lsm/bprm_check_security")
> > int BPF_PROG(bprm_check, struct linux_binprm *bprm)
> > {
> > return -EPERM;
> > }
> >
> > >
> > > This discussion would probably be a lot shorter if this series were sent
> > > with a proper explanation of how this supposed to work and what it's
> > > used for.
> >
> > It's currently scoped to BPF LSM (albeit limited to LSM for now)
> > but it won't just be used in LSM programs but some (allow-listed)
> > tracing programs too.
> >
> > We want to leave the flexibility to the implementer of the LSM hooks. If the
> > implementer choses to retrieve posix_acl_* we can also expose
> > posix_acl_fix_xattr_to_user or a different kfunc that adds this logic too
> > but that would be a separate kfunc (and a separate use-case).
>
> No, sorry. That's what I feared and that's why I think this low-level
> exposure of __vfs_getxattr() is wrong:
> The posix_acl_fix_xattr_*() helpers, as well as the helpers like
> get_file_caps() will not be exported. We're not going to export that
I don't want to expose them and I don't want any others to be
exposed either.
> deeply internal vfs machinery. So I would NACK that. If you want that -
> and that's what I'm saying here - you need to encapsulate this into your
> vfs_*xattr() helper that you can call from your kfuncs.
It seems like __vfs_getxattr is already exposed and does the wrong thing in
some contexts, why can't we just "fix" __vfs_getxattr then?
- KP
>
> >
> > >
> > > A series without a cover letter and no detailed explanation in the
> > > commit messages makes it quite hard to understand whether what is asked
> > > can be acked or not.
> >
> > As I mentioned in
> >
> > https://lore.kernel.org/bpf/CACYkzJ70uqVJr5EnM0i03Lu+zkuSsXOXcOLQoUS6HZPqH=skpQ@xxxxxxxxxxxxxx/T/#m74f32bae800a97d5c2caf08cee4199d3ba48d76c
> >
> > I will resend with a cover letter that has more details.
>
> Thank you!
>
> >
> > >
> > > I'm just adding Serge and Casey to double-check here as the LSM stuff is
> > > more up their alley. I can just look at this from the perspective of a
> > > vfs person.
> > >
> > > If you have your eBPF meeting thing I'm also happy to jump on there next
> >
> > Sure, we can discuss this during BPF office hours next week.
>
> Sounds good.
