[PATCH linux-next] security: Fix side effects of default BPF LSM hooks
KP Singh
kpsingh at kernel.org
Fri Jun 10 23:49:27 UTC 2022
On Fri, Jun 10, 2022 at 8:50 PM Casey Schaufler <casey at schaufler-ca.com> wrote:
>
> On 6/9/2022 4:46 PM, KP Singh 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
> >
> > But not with SELinux as SELinux does the required check in its LSM hook:
> >
> > cat /sys/kernel/security/lsm
> > capability,selinux,bpf
> >
> > In this case security_inode_removexattr() calls
> > call_int_hook(inode_removexattr, 1, mnt_userns, dentry, name), which
> > expects a return value of 1 to mean "no LSM hooks hit" and 0 is
> > supposed to mean "the LSM decided to permit the access and checked
> > cap_inode_removexattr"
> >
> > There are other security hooks that are similarly effected.
>
> This shouldn't come as a surprise.
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2281257.html
> is just one place where this sort of issue has been discussed.
>
> > In order to reliably fix this issue and also allow LSM Hooks and BPF
> > programs which implement hook logic to choose to not make a decision
> > in certain conditions (e.g. when BPF programs are used for auditing),
> > introduce a special return value LSM_HOOK_NO_EFFECT which can be used
> > by the hook to indicate to the framework that it does not intend to
> > make a decision.
>
> The LSM infrastructure already has a convention of returning
> -EOPNOTSUPP for this condition. Why add another value to check?'
This is not the case in call_int_hook currently.
If we can update the LSM infra to imply that -EOPNOTSUPP means
that the hook iteration can continue as that implies "no decision"
this would be okay as well.
> If you really want LSM_HOOK_NO_EFFECT you need to convert all the
> hooks, in both the infrastructure and the modules, that use
> -EOPNOTSUPP as well as what you have below.
>
> > Fixes: 520b7aa00d8c ("bpf: lsm: Initialize the BPF LSM hooks")
> > Reported-by: Jann Horn <jannh at google.com>
> > Signed-off-by: KP Singh <kpsingh at kernel.org>
> > ---
> > include/linux/lsm_hooks.h | 6 +++
> > kernel/bpf/bpf_lsm.c | 14 +++++--
> > security/security.c | 78 +++++++++++++++++++++++++++++----------
> > 3 files changed, 75 insertions(+), 23 deletions(-)
> >
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 91c8146649f5..fcf3c2c57127 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -1576,6 +1576,12 @@
> > * thread (IORING_SETUP_SQPOLL).
> > *
> > */
> > +
> > +/*
> > + * If an LSM hook choses to make no decision. i.e. it's only for auditing or
> > + * a default hook like the BPF LSM hooks, it can return LSM_HOOK_NO_EFFECT.
> > + */
> > + #define LSM_HOOK_NO_EFFECT -INT_MAX
>
> How are you going to ensure this will never, ever conflict with
> a legitimate error code? At the very least this needs to be in errno.h,
> not here.
>
> > union security_list_options {
> > #define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__);
> > #include "lsm_hook_defs.h"
> > diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> > index c1351df9f7ee..52f2fc493c57 100644
> > --- a/kernel/bpf/bpf_lsm.c
> > +++ b/kernel/bpf/bpf_lsm.c
> > @@ -20,12 +20,18 @@
> > /* For every LSM hook that allows attachment of BPF programs, declare a nop
> > * function where a BPF program can be attached.
> > */
> > -#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
> > -noinline RET bpf_lsm_##NAME(__VA_ARGS__) \
> > -{ \
> > - return DEFAULT; \
> > +#define DEFINE_LSM_HOOK_void(NAME, ...) \
> > +noinline void bpf_lsm_##NAME(__VA_ARGS__) {}
> > +
> > +#define DEFINE_LSM_HOOK_int(NAME, ...) \
> > +noinline int bpf_lsm_##NAME(__VA_ARGS__) \
> > +{ \
> > + return LSM_HOOK_NO_EFFECT; \
> > }
> >
> > +#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
> > + DEFINE_LSM_HOOK_##RET(NAME, __VA_ARGS__)
> > +
> > #include <linux/lsm_hook_defs.h>
> > #undef LSM_HOOK
> >
> > diff --git a/security/security.c b/security/security.c
> > index 188b8f782220..3c1b0b00c4a7 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -734,11 +734,16 @@ static int lsm_superblock_alloc(struct super_block *sb)
> >
> > #define call_int_hook(FUNC, IRC, ...) ({ \
> > int RC = IRC; \
> > + int THISRC; \
> > + \
> > do { \
> > struct security_hook_list *P; \
> > \
> > hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> > - RC = P->hook.FUNC(__VA_ARGS__); \
> > + THISRC = P->hook.FUNC(__VA_ARGS__); \
> > + if (THISRC == LSM_HOOK_NO_EFFECT) \
> > + continue; \
> > + RC = THISRC; \
> > if (RC != 0) \
> > break; \
> > } \
> > @@ -831,7 +836,7 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> > {
> > struct security_hook_list *hp;
> > int cap_sys_admin = 1;
> > - int rc;
> > + int rc, thisrc;
> >
> > /*
> > * The module will respond with a positive value if
> > @@ -841,7 +846,11 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> > * thinks it should not be set it won't.
> > */
> > hlist_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) {
> > - rc = hp->hook.vm_enough_memory(mm, pages);
>
> This could be a case where BPF is already broken.
> To match the documented interface the module should return
> 1 if it doesn't care.
>
> > + thisrc = hp->hook.vm_enough_memory(mm, pages);
> > + if (thisrc == LSM_HOOK_NO_EFFECT)
> > + continue;
> > + rc = thisrc;
> > +
> > if (rc <= 0) {
> > cap_sys_admin = 0;
> > break;
> > @@ -895,6 +904,8 @@ int security_fs_context_parse_param(struct fs_context *fc,
> > hlist_for_each_entry(hp, &security_hook_heads.fs_context_parse_param,
> > list) {
> > trc = hp->hook.fs_context_parse_param(fc, param);
> > + if (trc == LSM_HOOK_NO_EFFECT)
> > + continue;
>
> OK, so the LSM convention has to take a back seat to the mount
> convention in this case. The BPF hook should adhere to that convention
> and return -ENOPARAM when it doesn't care.
>
> > if (trc == 0)
> > rc = 0;
> > else if (trc != -ENOPARAM)
> > @@ -1063,14 +1074,17 @@ int security_dentry_init_security(struct dentry *dentry, int mode,
> > u32 *ctxlen)
> > {
> > struct security_hook_list *hp;
> > - int rc;
> > + int rc, thisrc;
> >
> > /*
> > * Only one module will provide a security context.
> > */
> > hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security, list) {
> > - rc = hp->hook.dentry_init_security(dentry, mode, name,
> > - xattr_name, ctx, ctxlen);
> > + thisrc = hp->hook.dentry_init_security(dentry, mode, name,
> > + xattr_name, ctx, ctxlen);
> > + if (thisrc == LSM_HOOK_NO_EFFECT)
> > + continue;
> > + rc = thisrc;
>
> The BPF module should return LSM_RET_DEFAULT(dentry_init_security),
> which I believe is -EOPNOTSUPP. BTW, if BPF ever supplies a hook for
> this that does something I expect there will be tears.
>
> > if (rc != LSM_RET_DEFAULT(dentry_init_security))
> > return rc;
> > }
> > @@ -1430,7 +1444,7 @@ int security_inode_getsecurity(struct user_namespace *mnt_userns,
> > void **buffer, bool alloc)
> > {
> > struct security_hook_list *hp;
> > - int rc;
> > + int rc, thisrc;
> >
> > if (unlikely(IS_PRIVATE(inode)))
> > return LSM_RET_DEFAULT(inode_getsecurity);
> > @@ -1438,7 +1452,10 @@ int security_inode_getsecurity(struct user_namespace *mnt_userns,
> > * Only one module will provide an attribute with a given name.
> > */
> > hlist_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) {
> > - rc = hp->hook.inode_getsecurity(mnt_userns, inode, name, buffer, alloc);
> > + thisrc = hp->hook.inode_getsecurity(mnt_userns, inode, name, buffer, alloc);
> > + if (thisrc == LSM_HOOK_NO_EFFECT)
> > + continue;
> > + rc = thisrc;
>
> The BPF module should return LSM_RET_DEFAULT(inode_getsecurity).
>
> > if (rc != LSM_RET_DEFAULT(inode_getsecurity))
> > return rc;
> > }
> > @@ -1448,7 +1465,7 @@ int security_inode_getsecurity(struct user_namespace *mnt_userns,
> > int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags)
> > {
> > struct security_hook_list *hp;
> > - int rc;
> > + int rc, thisrc;
> >
> > if (unlikely(IS_PRIVATE(inode)))
> > return LSM_RET_DEFAULT(inode_setsecurity);
> > @@ -1456,8 +1473,11 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
> > * Only one module will provide an attribute with a given name.
> > */
> > hlist_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) {
> > - rc = hp->hook.inode_setsecurity(inode, name, value, size,
> > - flags);
> > + thisrc = hp->hook.inode_setsecurity(inode, name, value, size,
> > + flags);
> > + if (thisrc == LSM_HOOK_NO_EFFECT)
> > + continue;
> > + rc = thisrc;
>
> Again.
>
> > if (rc != LSM_RET_DEFAULT(inode_setsecurity))
> > return rc;
> > }
> > @@ -1486,7 +1506,7 @@ EXPORT_SYMBOL(security_inode_copy_up);
> > int security_inode_copy_up_xattr(const char *name)
> > {
> > struct security_hook_list *hp;
> > - int rc;
> > + int rc, thisrc;
> >
> > /*
> > * The implementation can return 0 (accept the xattr), 1 (discard the
> > @@ -1495,7 +1515,10 @@ int security_inode_copy_up_xattr(const char *name)
> > */
> > hlist_for_each_entry(hp,
> > &security_hook_heads.inode_copy_up_xattr, list) {
> > - rc = hp->hook.inode_copy_up_xattr(name);
> > + thisrc = hp->hook.inode_copy_up_xattr(name);
> > + if (thisrc == LSM_HOOK_NO_EFFECT)
> > + continue;
> > + rc = thisrc;
>
> Again.
>
> > if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr))
> > return rc;
> > }
> > @@ -1889,6 +1912,8 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> >
> > hlist_for_each_entry(hp, &security_hook_heads.task_prctl, list) {
> > thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
> > + if (thisrc == LSM_HOOK_NO_EFFECT)
> > + continue;
> > if (thisrc != LSM_RET_DEFAULT(task_prctl)) {
> > rc = thisrc;
> > if (thisrc != 0)
> > @@ -2055,11 +2080,16 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> > char **value)
> > {
> > struct security_hook_list *hp;
> > + int rc;
> >
> > hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
> > if (lsm != NULL && strcmp(lsm, hp->lsm))
> > continue;
> > - return hp->hook.getprocattr(p, name, value);
> > + rc = hp->hook.getprocattr(p, name, value);
> > + if (rc == LSM_HOOK_NO_EFFECT)
> > + continue;
> > + else
> > + return rc;
>
> This shouldn't be necessary as the LSM is explicitly called out.
>
> > }
> > return LSM_RET_DEFAULT(getprocattr);
> > }
> > @@ -2068,11 +2098,16 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> > size_t size)
> > {
> > struct security_hook_list *hp;
> > + int rc;
> >
> > hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
> > if (lsm != NULL && strcmp(lsm, hp->lsm))
> > continue;
> > - return hp->hook.setprocattr(name, value, size);
> > + rc = hp->hook.setprocattr(name, value, size);
> > + if (rc == LSM_HOOK_NO_EFFECT)
> > + continue;
> > + else
> > + return rc;
>
> Same here.
>
> > }
> > return LSM_RET_DEFAULT(setprocattr);
> > }
> > @@ -2091,14 +2126,17 @@ EXPORT_SYMBOL(security_ismaclabel);
> > int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
> > {
> > struct security_hook_list *hp;
> > - int rc;
> > + int rc, thisrc;
> >
> > /*
> > * Currently, only one LSM can implement secid_to_secctx (i.e this
> > * LSM hook is not "stackable").
> > */
> > hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
> > - rc = hp->hook.secid_to_secctx(secid, secdata, seclen);
> > + thisrc = hp->hook.secid_to_secctx(secid, secdata, seclen);
> > + if (thisrc == LSM_HOOK_NO_EFFECT)
> > + continue;
> > + rc = thisrc;
>
> As with dentry_init, you're playing with fire if BPF expects to use this hook.
>
> > if (rc != LSM_RET_DEFAULT(secid_to_secctx))
> > return rc;
> > }
> > @@ -2509,9 +2547,11 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
> > hlist_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match,
> > list) {
> > rc = hp->hook.xfrm_state_pol_flow_match(x, xp, flic);
> > - break;
> > + if (rc == LSM_HOOK_NO_EFFECT)
> > + continue;
> > + return rc;
>
> This is an odd duck hook, and SELinux will be broken if BPF supplied a real hook.
>
> > }
> > - return rc;
> > + return LSM_RET_DEFAULT(xfrm_state_pol_flow_match);
> > }
> >
> > int security_xfrm_decode_session(struct sk_buff *skb, u32 *secid)
More information about the Linux-security-module-archive
mailing list