On Wed, Jun 29, 2022 at 2:56 AM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > On Wed, Jun 29, 2022 at 10:11:19AM +0200, Christian Brauner wrote: > > On Tue, Jun 28, 2022 at 03:28:42PM -0700, Alexei Starovoitov wrote: > > > On Tue, Jun 28, 2022 at 10:52 AM KP Singh <kpsingh@xxxxxxxxxx> wrote: > > > > > > > > On Tue, Jun 28, 2022 at 7:33 PM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > > > > > > > > > On Tue, Jun 28, 2022 at 04:19:48PM +0000, KP Singh wrote: > > > > > > A simple test that adds an xattr on a copied /bin/ls and reads it back > > > > > > when the copied ls is executed. > > > > > > > > > > > > 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. > > > > > > > > So, If I understand correctly, we don't need xattr_permission (and > > > > other checks in > > > > vfs_getxattr) here as the BPF programs run as CAP_SYS_ADMIN. > > > > > > > > but... > > > > > > > > So, Is this bit what's missing then? > > > > > > > > error = vfs_getxattr(mnt_userns, d, kname, ctx->kvalue, ctx->size); > > > > if (error > 0) { > > > > if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) || > > > > (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0)) > > > > posix_acl_fix_xattr_to_user(mnt_userns, d_inode(d), > > > > ctx->kvalue, error); > > > > > > 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. > > > > So looking at: > > > > 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); > > > > if (xattr_sz <= 0) > > return; > > > > if (!bpf_strncmp(dir_xattr_value, sizeof(XATTR_VALUE), XATTR_VALUE)) > > result = 1; > > } > > > > 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. No one complains about the usage of EXPORT_SYMBOL(__vfs_getxattr) in the existing LSMs like selinux. 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.