| Message ID | 163292547664.17566.8479687865641275719.stgit@olly (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: remove the SELinux lockdown implementation | expand |
On Wed, Sep 29, 2021 at 10:24 AM Paul Moore <paul@paul-moore.com> wrote: > > NOTE: This patch intentionally omits any "Fixes:" metadata or stable > tagging since it removes a SELinux access control check; while > removing the control point is the right thing to do moving forward, > removing it in stable kernels could be seen as a regression. > > The original SELinux lockdown implementation in 59438b46471a > ("security,lockdown,selinux: implement SELinux lockdown") used the > current task's credentials as both the subject and object in the > SELinux lockdown hook, selinux_lockdown(). Unfortunately that > proved to be incorrect in a number of cases as the core kernel was > calling the LSM lockdown hook in places where the credentials from > the "current" task_struct were not the correct credentials to use > in the SELinux access check. > > Attempts were made to resolve this by adding a credential pointer > to the LSM lockdown hook as well as suggesting that the single hook > be split into two: one for user tasks, one for kernel tasks; however > neither approach was deemed acceptable by Linus. Faced with the > prospect of either changing the subj/obj in the access check to a > constant context (likely the kernel's label) or removing the SELinux > lockdown check entirely, the SELinux community decided that removing > the lockdown check was preferable. > > The supporting changes to the general LSM layer are left intact, this > patch only removes the SELinux implementation. > > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > security/selinux/hooks.c | 30 ------------------------------ > security/selinux/include/classmap.h | 2 -- > 2 files changed, 32 deletions(-) A quick note regarding the selinux-testsuite: the lockdown related tests fail, unsurprisingly, but everything else succeeds. I'm going to give this patch 24 hours or so before merging, but once merged we should probably consider just removing the lockdown tests from the selinux-testsuite.
On Wed, Sep 29, 2021 at 4:24 PM Paul Moore <paul@paul-moore.com> wrote: > NOTE: This patch intentionally omits any "Fixes:" metadata or stable > tagging since it removes a SELinux access control check; while > removing the control point is the right thing to do moving forward, > removing it in stable kernels could be seen as a regression. > > The original SELinux lockdown implementation in 59438b46471a > ("security,lockdown,selinux: implement SELinux lockdown") used the > current task's credentials as both the subject and object in the > SELinux lockdown hook, selinux_lockdown(). Unfortunately that > proved to be incorrect in a number of cases as the core kernel was > calling the LSM lockdown hook in places where the credentials from > the "current" task_struct were not the correct credentials to use > in the SELinux access check. > > Attempts were made to resolve this by adding a credential pointer > to the LSM lockdown hook as well as suggesting that the single hook > be split into two: one for user tasks, one for kernel tasks; however > neither approach was deemed acceptable by Linus. Faced with the > prospect of either changing the subj/obj in the access check to a > constant context (likely the kernel's label) or removing the SELinux > lockdown check entirely, the SELinux community decided that removing > the lockdown check was preferable. > > The supporting changes to the general LSM layer are left intact, this > patch only removes the SELinux implementation. > > Signed-off-by: Paul Moore <paul@paul-moore.com> I would probably also remove LSM_AUDIT_DATA_LOCKDOWN, but I don't care enough to argue about it :) Acked-by: Ondrej Mosnacek <omosnace@redhat.com> /me goes off to prepare a patch to remove the lockdown test from the testsuite... > --- > security/selinux/hooks.c | 30 ------------------------------ > security/selinux/include/classmap.h | 2 -- > 2 files changed, 32 deletions(-) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 6517f221d52c..11ebbbd65823 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -7013,34 +7013,6 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux) > } > #endif > > -static int selinux_lockdown(enum lockdown_reason what) > -{ > - struct common_audit_data ad; > - u32 sid = current_sid(); > - int invalid_reason = (what <= LOCKDOWN_NONE) || > - (what == LOCKDOWN_INTEGRITY_MAX) || > - (what >= LOCKDOWN_CONFIDENTIALITY_MAX); > - > - if (WARN(invalid_reason, "Invalid lockdown reason")) { > - audit_log(audit_context(), > - GFP_ATOMIC, AUDIT_SELINUX_ERR, > - "lockdown_reason=invalid"); > - return -EINVAL; > - } > - > - ad.type = LSM_AUDIT_DATA_LOCKDOWN; > - ad.u.reason = what; > - > - if (what <= LOCKDOWN_INTEGRITY_MAX) > - return avc_has_perm(&selinux_state, > - sid, sid, SECCLASS_LOCKDOWN, > - LOCKDOWN__INTEGRITY, &ad); > - else > - return avc_has_perm(&selinux_state, > - sid, sid, SECCLASS_LOCKDOWN, > - LOCKDOWN__CONFIDENTIALITY, &ad); > -} > - > struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = { > .lbs_cred = sizeof(struct task_security_struct), > .lbs_file = sizeof(struct file_security_struct), > @@ -7349,8 +7321,6 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(perf_event_write, selinux_perf_event_write), > #endif > > - LSM_HOOK_INIT(locked_down, selinux_lockdown), > - > /* > * PUT "CLONING" (ACCESSING + ALLOCATING) HOOKS HERE > */ > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > index 084757ff4390..9859787e8e61 100644 > --- a/security/selinux/include/classmap.h > +++ b/security/selinux/include/classmap.h > @@ -250,8 +250,6 @@ struct security_class_mapping secclass_map[] = { > { COMMON_SOCK_PERMS, NULL } }, > { "perf_event", > { "open", "cpu", "kernel", "tracepoint", "read", "write", NULL } }, > - { "lockdown", > - { "integrity", "confidentiality", NULL } }, > { "anon_inode", > { COMMON_FILE_PERMS, NULL } }, > { NULL } > -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.
On Thu, Sep 30, 2021 at 4:32 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Wed, Sep 29, 2021 at 4:24 PM Paul Moore <paul@paul-moore.com> wrote: > > NOTE: This patch intentionally omits any "Fixes:" metadata or stable > > tagging since it removes a SELinux access control check; while > > removing the control point is the right thing to do moving forward, > > removing it in stable kernels could be seen as a regression. > > > > The original SELinux lockdown implementation in 59438b46471a > > ("security,lockdown,selinux: implement SELinux lockdown") used the > > current task's credentials as both the subject and object in the > > SELinux lockdown hook, selinux_lockdown(). Unfortunately that > > proved to be incorrect in a number of cases as the core kernel was > > calling the LSM lockdown hook in places where the credentials from > > the "current" task_struct were not the correct credentials to use > > in the SELinux access check. > > > > Attempts were made to resolve this by adding a credential pointer > > to the LSM lockdown hook as well as suggesting that the single hook > > be split into two: one for user tasks, one for kernel tasks; however > > neither approach was deemed acceptable by Linus. Faced with the > > prospect of either changing the subj/obj in the access check to a > > constant context (likely the kernel's label) or removing the SELinux > > lockdown check entirely, the SELinux community decided that removing > > the lockdown check was preferable. > > > > The supporting changes to the general LSM layer are left intact, this > > patch only removes the SELinux implementation. > > > > Signed-off-by: Paul Moore <paul@paul-moore.com> > > I would probably also remove LSM_AUDIT_DATA_LOCKDOWN, but I don't care > enough to argue about it :) As mentioned in the commit description, that was intentional. I wanted to keep the removal of the SELinux hook implementation separate from any core LSM changes. At some point in the future we can consider dropping the, rather small, core LSM changes. However it is my opinion that if we are going to do that we should move the lockdown LSM functionality out of the LSM and into the core kernel. If Linus is effectively only going to allow a single lockdown security model I feel the lockdown calls shouldn't be part of the LSM. Regardless, this patch is now merged into selinux/next.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 6517f221d52c..11ebbbd65823 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -7013,34 +7013,6 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux) } #endif -static int selinux_lockdown(enum lockdown_reason what) -{ - struct common_audit_data ad; - u32 sid = current_sid(); - int invalid_reason = (what <= LOCKDOWN_NONE) || - (what == LOCKDOWN_INTEGRITY_MAX) || - (what >= LOCKDOWN_CONFIDENTIALITY_MAX); - - if (WARN(invalid_reason, "Invalid lockdown reason")) { - audit_log(audit_context(), - GFP_ATOMIC, AUDIT_SELINUX_ERR, - "lockdown_reason=invalid"); - return -EINVAL; - } - - ad.type = LSM_AUDIT_DATA_LOCKDOWN; - ad.u.reason = what; - - if (what <= LOCKDOWN_INTEGRITY_MAX) - return avc_has_perm(&selinux_state, - sid, sid, SECCLASS_LOCKDOWN, - LOCKDOWN__INTEGRITY, &ad); - else - return avc_has_perm(&selinux_state, - sid, sid, SECCLASS_LOCKDOWN, - LOCKDOWN__CONFIDENTIALITY, &ad); -} - struct lsm_blob_sizes selinux_blob_sizes __lsm_ro_after_init = { .lbs_cred = sizeof(struct task_security_struct), .lbs_file = sizeof(struct file_security_struct), @@ -7349,8 +7321,6 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(perf_event_write, selinux_perf_event_write), #endif - LSM_HOOK_INIT(locked_down, selinux_lockdown), - /* * PUT "CLONING" (ACCESSING + ALLOCATING) HOOKS HERE */ diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index 084757ff4390..9859787e8e61 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -250,8 +250,6 @@ struct security_class_mapping secclass_map[] = { { COMMON_SOCK_PERMS, NULL } }, { "perf_event", { "open", "cpu", "kernel", "tracepoint", "read", "write", NULL } }, - { "lockdown", - { "integrity", "confidentiality", NULL } }, { "anon_inode", { COMMON_FILE_PERMS, NULL } }, { NULL }
NOTE: This patch intentionally omits any "Fixes:" metadata or stable tagging since it removes a SELinux access control check; while removing the control point is the right thing to do moving forward, removing it in stable kernels could be seen as a regression. The original SELinux lockdown implementation in 59438b46471a ("security,lockdown,selinux: implement SELinux lockdown") used the current task's credentials as both the subject and object in the SELinux lockdown hook, selinux_lockdown(). Unfortunately that proved to be incorrect in a number of cases as the core kernel was calling the LSM lockdown hook in places where the credentials from the "current" task_struct were not the correct credentials to use in the SELinux access check. Attempts were made to resolve this by adding a credential pointer to the LSM lockdown hook as well as suggesting that the single hook be split into two: one for user tasks, one for kernel tasks; however neither approach was deemed acceptable by Linus. Faced with the prospect of either changing the subj/obj in the access check to a constant context (likely the kernel's label) or removing the SELinux lockdown check entirely, the SELinux community decided that removing the lockdown check was preferable. The supporting changes to the general LSM layer are left intact, this patch only removes the SELinux implementation. Signed-off-by: Paul Moore <paul@paul-moore.com> --- security/selinux/hooks.c | 30 ------------------------------ security/selinux/include/classmap.h | 2 -- 2 files changed, 32 deletions(-)