[PATCH linux-next] security: Fix side effects of default BPF LSM hooks

[KP Singh]

On Fri, Jun 10, 2022 at 2:55 AM KP Singh <kpsingh at kernel.org> wrote:
>
> On Fri, Jun 10, 2022 at 2:44 AM Alexei Starovoitov
> <alexei.starovoitov at gmail.com> wrote:
> >
> > On Thu, Jun 9, 2022 at 4:46 PM KP Singh <kpsingh at kernel.org> wrote:
> > >
> > > BPF LSM currently has a default implementation for each LSM hooks which
> > > return a default value defined in include/linux/lsm_hook_defs.h. These
> > > hooks should have no functional effect when there is no BPF program
> > > loaded to implement the hook logic.
> > >
> > > Some LSM hooks treat any return value of the hook as policy decision
> > > which results in destructive side effects.
> > >
> > > This issue and the effects were reported to me by Jann Horn:
> > >
> > > For a system configured with CONFIG_BPF_LSM and the bpf lsm is enabled
> > > (via lsm= or CONFIG_LSM) an unprivileged user can vandalize the system
> > > by removing the security.capability xattrs from binaries, preventing
> > > them from working normally:
> > >
> > > $ getfattr -d -m- /bin/ping
> > > getfattr: Removing leading '/' from absolute path names
> > > security.capability=0sAQAAAgAgAAAAAAAAAAAAAAAAAAA=
> > >
> > > $ setfattr -x security.capability /bin/ping
> > > $ getfattr -d -m- /bin/ping
> > > $ ping 1.2.3.4
> > > $ ping google.com
> > > $ echo $?
> > > 2
> > >
> > > The above reproduces with:
> > >
> > > cat /sys/kernel/security/lsm
> > > capability,apparmor,bpf
> >
> > Why is this bpf related?
> > apparmor doesn't have that hook,
> > while capability returns 0.
> > So bpf's default==0 doesn't change the situation.
> >
> > Just
> > cat /sys/kernel/security/lsm
> > capability
> >
> > would reproduce the issue?

Just to clarify, when one just has:

cat /sys/kernel/security/lsm
capability

call_int_hook would return the IRC (i.e 1) which would lead the code
to the capability check. (i.e. cap_inode_removexattr)

ret = call_int_hook(inode_removexattr, 1, mnt_userns, dentry, name);
if (ret == 1)
    ret = cap_inode_removexattr(mnt_userns, dentry, name);

cap_inode_removexattr restricts setting security.* xattrs to only CAP_SYS_ADMIN.

Now, since BPF's hook returns 0 here, the capability check is skipped.

But then again, this is just one of the hooks which has the issue.




> > what am I missing?
>
> capability does not define the inode_removexattr LSM hook:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/commoncap.c#n1449
>
> It's only when the return value of the hook is 1, it checks for
> cap_inode_removexattr.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/security.c#n1408
>
> Only 3 LSMs define the hook (bpf, smack and selinux):
>
> fgrep -R LSM_HOOK_INIT *  | grep inode_removexattr
> selinux/hooks.c: LSM_HOOK_INIT(inode_removexattr, selinux_inode_removexattr),
> smack/smack_lsm.c: LSM_HOOK_INIT(inode_removexattr, smack_inode_removexattr),
>
> The BPF LSM default hooks intend to provide no side-effects when the
> LSM is enabled and
> for the hooks that the patch updates, there is a side-effect.