| Message ID | 20220609234601.2026362-1-kpsingh@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [linux-next] security: Fix side effects of default BPF LSM hooks | expand |
On Thu, Jun 9, 2022 at 4:46 PM KP Singh <kpsingh@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? what am I missing?
On Fri, Jun 10, 2022 at 2:44 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Thu, Jun 9, 2022 at 4:46 PM KP Singh <kpsingh@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? > 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.
On Fri, Jun 10, 2022 at 2:55 AM KP Singh <kpsingh@kernel.org> wrote: > > On Fri, Jun 10, 2022 at 2:44 AM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Thu, Jun 9, 2022 at 4:46 PM KP Singh <kpsingh@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.
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@google.com> > Signed-off-by: KP Singh <kpsingh@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)
On 6/10/2022 11:50 AM, Casey Schaufler 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. What I failed to point out earlier is that you really want general LSM stacking for BPF to work the way you want it to. Reviewed-bys, Acked-bys and other participation in that effort would be most appreciated.
On Fri, Jun 10, 2022 at 8:50 PM Casey Schaufler <casey@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@google.com> > > Signed-off-by: KP Singh <kpsingh@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)
On Fri, Jun 10, 2022 at 9:00 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 6/10/2022 11:50 AM, Casey Schaufler 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. > > What I failed to point out earlier is that you really want general > LSM stacking for BPF to work the way you want it to. Reviewed-bys, Happy to take a look, but we should fix this bug independently though. > Acked-bys and other participation in that effort would be most > appreciated. >
On Fri, Jun 10, 2022 at 4:49 PM KP Singh <kpsingh@kernel.org> wrote: > > > > > 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. Agree that it's cleaner to use existing code like EOPNOTSUPP to indicate 'ignore this lsm'. Folks, reminder, please trim your replies.
On 6/10/2022 4:49 PM, KP Singh wrote: > On Fri, Jun 10, 2022 at 8:50 PM Casey Schaufler <casey@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. It would be really nice if there was sufficient consistency in the LSM infrastructure for this to make sense. The cases where a module supplies a hook but only cares about the data some of the time are rare, excepting for BPF. As I mention elsewhere, general stacking is what you need.
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 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); + 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; 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; 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; 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; 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; 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; } 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; } 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; 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; } - return rc; + return LSM_RET_DEFAULT(xfrm_state_pol_flow_match); } int security_xfrm_decode_session(struct sk_buff *skb, u32 *secid)
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. 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. Fixes: 520b7aa00d8c ("bpf: lsm: Initialize the BPF LSM hooks") Reported-by: Jann Horn <jannh@google.com> Signed-off-by: KP Singh <kpsingh@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(-)