[PATCH linux-next] security: Fix side effects of default BPF LSM hooks
Casey Schaufler
casey at schaufler-ca.com
Fri Jun 10 18:50:50 UTC 2022
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?'
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