On Fri, Sep 24, 2021 at 5:12 PM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
> On Fri, Sep 24, 2021 at 10:22 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> >
> > 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.
>
> Looks like Fedora policy allowed both permissions unconditionally (no
> boolean) to all unconfined domains.
> So SECINITSID_KERNEL checks will pass but are rather pointless unless
> Fedora decides to define separate
> integrity/confidentiality rules and wrap them each with a boolean
> (e.g. allow_kernel_integrity_violation,
> allow_kernel_confidentiality_violation) so that an admin can disable
> them to enforce lockdown independently
> of the lockdown module.
>
> Android policy allows all domains :lockdown confidentiality but
> prohibits (neverallow) integrity permission from
> being allowed on user (production) builds. They do allow apps
> :lockdown integrity on debug builds for debugfs
> kcov usage, so that rule would need to be fixed if switching to always
> using SECINITSID_KERNEL or the checks will
> start failing.
>
> Did all the issues around invoking audit from arbitrary contexts in
> which security_locked_down() is called get sorted?
> If not, we'll still have that as a potential problem if permission is
> denied or an auditallow rule is defined on lockdown.
>
> Can we get Linux distro and Android folks to speak as to whether they
> consider the check in this reduced form to still be useful or whether
> we should just remove it altogether?
I would vote for just removing it rather than basically duplicating
the Lockdown LSM functionality. It likely wouldn't even be complete as
a couple of the security_locked_down() calls happen at __init time,
where SELinux wouldn't get a chance to deny them (no policy loaded
yet).
(Well... I *tried* to make this thing work, but it really feels like
too much hassle to reasonably tweak all the existing callers to
fulfill the SELinux expectations to be worth it. I admit I'll be kind
of relieved to see the lockdown class go - it brought me nothing but
pain :) It would be a nice feature if done right, but that would have
to be a new patch that somehow deals with all the intricacies...
Either someone finds enough motivation to do it, or it just shouldn't
be there, IMHO.)
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
