All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] selinux: use SECINITSID_KERNEL as the subj/obj in the lockdown hook
@ 2021-09-23 21:18 Paul Moore
  2021-09-24 13:08 ` Stephen Smalley
  0 siblings, 1 reply; 9+ messages in thread
From: Paul Moore @ 2021-09-23 21:18 UTC (permalink / raw)
  To: selinux

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@paul-moore.com>

--

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);


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] selinux: use SECINITSID_KERNEL as the subj/obj in the lockdown hook
  2021-09-23 21:18 [RFC PATCH] selinux: use SECINITSID_KERNEL as the subj/obj in the lockdown hook Paul Moore
@ 2021-09-24 13:08 ` Stephen Smalley
  2021-09-24 14:22   ` Paul Moore
  0 siblings, 1 reply; 9+ messages in thread
From: Stephen Smalley @ 2021-09-24 13:08 UTC (permalink / raw)
  To: Paul Moore; +Cc: SElinux list

On Thu, Sep 23, 2021 at 5:18 PM Paul Moore <paul@paul-moore.com> 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@paul-moore.com>
>
> --
>
> 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.
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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] selinux: use SECINITSID_KERNEL as the subj/obj in the lockdown hook
  2021-09-24 13:08 ` Stephen Smalley
@ 2021-09-24 14:22   ` Paul Moore
  2021-09-24 15:12     ` Stephen Smalley
  0 siblings, 1 reply; 9+ messages in thread
From: Paul Moore @ 2021-09-24 14:22 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: SElinux list

On Fri, Sep 24, 2021 at 9:08 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Thu, Sep 23, 2021 at 5:18 PM Paul Moore <paul@paul-moore.com> 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@paul-moore.com>
> >
> > --
> >
> > 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] selinux: use SECINITSID_KERNEL as the subj/obj in the lockdown hook
  2021-09-24 14:22   ` Paul Moore
@ 2021-09-24 15:12     ` Stephen Smalley
  2021-09-24 16:38       ` Ondrej Mosnacek
                         ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Stephen Smalley @ 2021-09-24 15:12 UTC (permalink / raw)
  To: Paul Moore, Ondrej Mosnacek, Jeffrey Vander Stoep; +Cc: SElinux list

On Fri, Sep 24, 2021 at 10:22 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Sep 24, 2021 at 9:08 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Thu, Sep 23, 2021 at 5:18 PM Paul Moore <paul@paul-moore.com> 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@paul-moore.com>
> > >
> > > --
> > >
> > > 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?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] selinux: use SECINITSID_KERNEL as the subj/obj in the lockdown hook
  2021-09-24 15:12     ` Stephen Smalley
@ 2021-09-24 16:38       ` Ondrej Mosnacek
  2021-09-24 19:10         ` Paul Moore
  2021-09-24 19:03       ` Paul Moore
  2021-09-25 21:07       ` Chris PeBenito
  2 siblings, 1 reply; 9+ messages in thread
From: Ondrej Mosnacek @ 2021-09-24 16:38 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: Paul Moore, Jeffrey Vander Stoep, SElinux list

On Fri, Sep 24, 2021 at 5:12 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Fri, Sep 24, 2021 at 10:22 AM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Fri, Sep 24, 2021 at 9:08 AM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
> > > On Thu, Sep 23, 2021 at 5:18 PM Paul Moore <paul@paul-moore.com> 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@paul-moore.com>
> > > >
> > > > --
> > > >
> > > > 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.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] selinux: use SECINITSID_KERNEL as the subj/obj in the lockdown hook
  2021-09-24 15:12     ` Stephen Smalley
  2021-09-24 16:38       ` Ondrej Mosnacek
@ 2021-09-24 19:03       ` Paul Moore
  2021-09-25 21:07       ` Chris PeBenito
  2 siblings, 0 replies; 9+ messages in thread
From: Paul Moore @ 2021-09-24 19:03 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: Ondrej Mosnacek, Jeffrey Vander Stoep, SElinux list

On Fri, Sep 24, 2021 at 11:12 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> 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.

Thanks Stephen.

> 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.

I believe the only issue was the eBPF code and that was resolved in a
separate patch that is already upstream.

> 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?

Yes, that's probably going to be the deal breaker.  However as the day
goes on I'm growing more fond of just ripping out that SELinux hook
and being done with it.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] selinux: use SECINITSID_KERNEL as the subj/obj in the lockdown hook
  2021-09-24 16:38       ` Ondrej Mosnacek
@ 2021-09-24 19:10         ` Paul Moore
  0 siblings, 0 replies; 9+ messages in thread
From: Paul Moore @ 2021-09-24 19:10 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: Stephen Smalley, Jeffrey Vander Stoep, SElinux list

On Fri, Sep 24, 2021 at 12:38 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> I would vote for just removing it rather than basically duplicating
> the Lockdown LSM functionality.

/me records the vote

> (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.)

I'm really only willing to entertain one of two options here: 1) stick
to a lockdown-esque, always kernel_t hook or 2) remove the SELinux
hook implementation.  Anyone who sends me a patch doing something else
is likely going to see it NACK'd as soon as I check my email.  Maybe
I'll soften my stance on this in a year or two, but I *really* don't
like having email exchanges with Linus like what I had this week.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] selinux: use SECINITSID_KERNEL as the subj/obj in the lockdown hook
  2021-09-24 15:12     ` Stephen Smalley
  2021-09-24 16:38       ` Ondrej Mosnacek
  2021-09-24 19:03       ` Paul Moore
@ 2021-09-25 21:07       ` Chris PeBenito
  2021-09-27 14:07         ` Paul Moore
  2 siblings, 1 reply; 9+ messages in thread
From: Chris PeBenito @ 2021-09-25 21:07 UTC (permalink / raw)
  To: Stephen Smalley, Paul Moore, Ondrej Mosnacek, Jeffrey Vander Stoep
  Cc: SElinux list

On 9/24/21 11:12 AM, Stephen Smalley wrote:
> On Fri, Sep 24, 2021 at 10:22 AM Paul Moore <paul@paul-moore.com> wrote:
>>> On Thu, Sep 23, 2021 at 5:18 PM Paul Moore <paul@paul-moore.com> 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.

> 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?

FWIW, I think the check should be removed.

-- 
Chris PeBenito

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [RFC PATCH] selinux: use SECINITSID_KERNEL as the subj/obj in the lockdown hook
  2021-09-25 21:07       ` Chris PeBenito
@ 2021-09-27 14:07         ` Paul Moore
  0 siblings, 0 replies; 9+ messages in thread
From: Paul Moore @ 2021-09-27 14:07 UTC (permalink / raw)
  To: Chris PeBenito, Jeffrey Vander Stoep
  Cc: Stephen Smalley, Ondrej Mosnacek, SElinux list

On Sat, Sep 25, 2021 at 5:07 PM Chris PeBenito <pebenito@ieee.org> wrote:
> On 9/24/21 11:12 AM, Stephen Smalley wrote:
> > On Fri, Sep 24, 2021 at 10:22 AM Paul Moore <paul@paul-moore.com> wrote:
> >>> On Thu, Sep 23, 2021 at 5:18 PM Paul Moore <paul@paul-moore.com> 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.
>
> > 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?
>
> FWIW, I think the check should be removed.

/me punches another voting card

Thanks Chris.  Unless we hear a rather compelling case from the
Android folks I think we've got our answer.

Jeff, or any of the other Android folks, now is the time to speak up
on this.  If I don't hear from any of you guys within the next few
days I think we'll rip out the SELinux lockdown hook.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-09-27 14:07 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-23 21:18 [RFC PATCH] selinux: use SECINITSID_KERNEL as the subj/obj in the lockdown hook Paul Moore
2021-09-24 13:08 ` Stephen Smalley
2021-09-24 14:22   ` Paul Moore
2021-09-24 15:12     ` Stephen Smalley
2021-09-24 16:38       ` Ondrej Mosnacek
2021-09-24 19:10         ` Paul Moore
2021-09-24 19:03       ` Paul Moore
2021-09-25 21:07       ` Chris PeBenito
2021-09-27 14:07         ` Paul Moore

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.