On Fri, Sep 24, 2021 at 9:08 AM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
>
> On Thu, Sep 23, 2021 at 5:18 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> >
> > 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.
> >
> > In order to resolve the problem of an incorrect SELinux domain being
> > used in the lockdown check, this patch makes the decision to perform
> > all of the lockdown access control checks against the
> > SECINITSID_KERNEL domain. This is far from ideal, but it is what
> > we have available to us at this point in time.
> >
> > Fixes: 59438b46471a ("security,lockdown,selinux: implement SELinux lockdown")
> > Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx>
> >
> > --
> >
> > NOTES: While trivial, this patch is completely untested; I'm posting
> > this simply to see what comments there may be within the SELinux
> > community to such an approach as the current code is flawed and needs
> > to be corrected.
> > ---
> > security/selinux/hooks.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 6517f221d52c..4f016a49e3a6 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -7016,7 +7016,7 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
> > static int selinux_lockdown(enum lockdown_reason what)
> > {
> > struct common_audit_data ad;
> > - u32 sid = current_sid();
> > + u32 sid = SECINITSID_KERNEL;
> > int invalid_reason = (what <= LOCKDOWN_NONE) ||
> > (what == LOCKDOWN_INTEGRITY_MAX) ||
> > (what >= LOCKDOWN_CONFIDENTIALITY_MAX);
>
> Kind of begs the question of whether it is even worth keeping the check at all.
Yes, it does, especially considering that Linus seems to be rejecting
lockdown implementations that differ from the lockdown LSM. The only
argument I can see for keeping the SELinux lockdown check is if people
wanted to be able to include it as part of a policy analysis, although
given the limited nature of the control I'm not sure how useful that
would be in practice.
> This could potentially break an existing policy where lockdown has
> been defined and only allowed as needed but I suspect it is already
> allowed in Fedora and Android.
This was one of the reasons for the "untested!" warning. Like you, I
suspect the kernel domain has already been granted the necessary
permissions in deployed policies (I will be amazed if there are no
kernel threads which end up triggering a lockdown check), but even if
it did we need to make some sort of change to the existing code.
--
paul moore
www.paul-moore.com
