Re: [PATCH v3] lockdown,selinux: fix wrong subject in some SELinux lockdown checks
On Tue, Aug 31, 2021 at 2:58 PM Dan Williams wrote: > On Tue, Aug 31, 2021 at 6:53 AM Paul Moore wrote: > > On Tue, Aug 31, 2021 at 5:09 AM Ondrej Mosnacek wrote: > > > On Sat, Jun 19, 2021 at 12:18 AM Dan Williams > > > wrote: > > > > On Wed, Jun 16, 2021 at 1:51 AM Ondrej Mosnacek > > > > wrote: > > > > ... > > > > > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > > > > index 2acc6173da36..c1747b6555c7 100644 > > > > > --- a/drivers/cxl/mem.c > > > > > +++ b/drivers/cxl/mem.c > > > > > @@ -568,7 +568,7 @@ static bool cxl_mem_raw_command_allowed(u16 > > > > > opcode) > > > > > if (!IS_ENABLED(CONFIG_CXL_MEM_RAW_COMMANDS)) > > > > > return false; > > > > > > > > > > - if (security_locked_down(LOCKDOWN_NONE)) > > > > > + if (security_locked_down(current_cred(), LOCKDOWN_NONE)) > > > > > > > > Acked-by: Dan Williams > > > > > > > > ...however that usage looks wrong. The expectation is that if kernel > > > > integrity protections are enabled then raw command access should be > > > > disabled. So I think that should be equivalent to LOCKDOWN_PCI_ACCESS > > > > in terms of the command capabilities to filter. > > > > > > Yes, the LOCKDOWN_NONE seems wrong here... but it's a pre-existing bug > > > and I didn't want to go down yet another rabbit hole trying to fix it. > > > I'll look at this again once this patch is settled - it may indeed be > > > as simple as replacing LOCKDOWN_NONE with LOCKDOWN_PCI_ACCESS. > > > > At this point you should be well aware of my distaste for merging > > patches that have known bugs in them. Yes, this is a pre-existing > > condition, but it seems well within the scope of this work to address > > it as well. > > > > This isn't something that is going to get merged while the merge > > window is open, so at the very least you've got almost two weeks to > > sort this out - please do that. > > Yes, apologies, I should have sent the fix shortly after noticing the > problem. I'll get the CXL bug fix out of the way so Ondrej can move > this along. Thanks Dan. -- paul moore www.paul-moore.com
Re: [PATCH v3] lockdown,selinux: fix wrong subject in some SELinux lockdown checks
On Tue, Aug 31, 2021 at 6:53 AM Paul Moore wrote: > > On Tue, Aug 31, 2021 at 5:09 AM Ondrej Mosnacek wrote: > > On Sat, Jun 19, 2021 at 12:18 AM Dan Williams > > wrote: > > > On Wed, Jun 16, 2021 at 1:51 AM Ondrej Mosnacek > > > wrote: > > ... > > > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > > > index 2acc6173da36..c1747b6555c7 100644 > > > > --- a/drivers/cxl/mem.c > > > > +++ b/drivers/cxl/mem.c > > > > @@ -568,7 +568,7 @@ static bool cxl_mem_raw_command_allowed(u16 opcode) > > > > if (!IS_ENABLED(CONFIG_CXL_MEM_RAW_COMMANDS)) > > > > return false; > > > > > > > > - if (security_locked_down(LOCKDOWN_NONE)) > > > > + if (security_locked_down(current_cred(), LOCKDOWN_NONE)) > > > > > > Acked-by: Dan Williams > > > > > > ...however that usage looks wrong. The expectation is that if kernel > > > integrity protections are enabled then raw command access should be > > > disabled. So I think that should be equivalent to LOCKDOWN_PCI_ACCESS > > > in terms of the command capabilities to filter. > > > > Yes, the LOCKDOWN_NONE seems wrong here... but it's a pre-existing bug > > and I didn't want to go down yet another rabbit hole trying to fix it. > > I'll look at this again once this patch is settled - it may indeed be > > as simple as replacing LOCKDOWN_NONE with LOCKDOWN_PCI_ACCESS. > > At this point you should be well aware of my distaste for merging > patches that have known bugs in them. Yes, this is a pre-existing > condition, but it seems well within the scope of this work to address > it as well. > > This isn't something that is going to get merged while the merge > window is open, so at the very least you've got almost two weeks to > sort this out - please do that. Yes, apologies, I should have sent the fix shortly after noticing the problem. I'll get the CXL bug fix out of the way so Ondrej can move this along.
Re: [PATCH v3] lockdown,selinux: fix wrong subject in some SELinux lockdown checks
On Tue, Aug 31, 2021 at 5:09 AM Ondrej Mosnacek wrote: > On Sat, Jun 19, 2021 at 12:18 AM Dan Williams > wrote: > > On Wed, Jun 16, 2021 at 1:51 AM Ondrej Mosnacek wrote: ... > > > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c > > > index 2acc6173da36..c1747b6555c7 100644 > > > --- a/drivers/cxl/mem.c > > > +++ b/drivers/cxl/mem.c > > > @@ -568,7 +568,7 @@ static bool cxl_mem_raw_command_allowed(u16 opcode) > > > if (!IS_ENABLED(CONFIG_CXL_MEM_RAW_COMMANDS)) > > > return false; > > > > > > - if (security_locked_down(LOCKDOWN_NONE)) > > > + if (security_locked_down(current_cred(), LOCKDOWN_NONE)) > > > > Acked-by: Dan Williams > > > > ...however that usage looks wrong. The expectation is that if kernel > > integrity protections are enabled then raw command access should be > > disabled. So I think that should be equivalent to LOCKDOWN_PCI_ACCESS > > in terms of the command capabilities to filter. > > Yes, the LOCKDOWN_NONE seems wrong here... but it's a pre-existing bug > and I didn't want to go down yet another rabbit hole trying to fix it. > I'll look at this again once this patch is settled - it may indeed be > as simple as replacing LOCKDOWN_NONE with LOCKDOWN_PCI_ACCESS. At this point you should be well aware of my distaste for merging patches that have known bugs in them. Yes, this is a pre-existing condition, but it seems well within the scope of this work to address it as well. This isn't something that is going to get merged while the merge window is open, so at the very least you've got almost two weeks to sort this out - please do that. -- paul moore www.paul-moore.com
Re: [PATCH v3] lockdown,selinux: fix wrong subject in some SELinux lockdown checks
On Tue, Aug 31, 2021 at 5:08 AM Ondrej Mosnacek wrote: > Can we move this forward somehow, please? As mentioned previously, I can merge this via the SELinux tree but I need to see some ACKs from the other subsystems first, not to mention some resolution to the outstanding questions. -- paul moore www.paul-moore.com
Re: [PATCH v3] lockdown,selinux: fix wrong subject in some SELinux lockdown checks
On Sat, Jun 19, 2021 at 12:18 AM Dan Williams wrote:
> On Wed, Jun 16, 2021 at 1:51 AM Ondrej Mosnacek wrote:
> >
> > Commit 59438b46471a ("security,lockdown,selinux: implement SELinux
> > lockdown") added an implementation of the locked_down LSM hook to
> > SELinux, with the aim to restrict which domains are allowed to perform
> > operations that would breach lockdown.
> >
> > However, in several places the security_locked_down() hook is called in
> > situations where the current task isn't doing any action that would
> > directly breach lockdown, leading to SELinux checks that are basically
> > bogus.
> >
> > To fix this, add an explicit struct cred pointer argument to
> > security_lockdown() and define NULL as a special value to pass instead
> > of current_cred() in such situations. LSMs that take the subject
> > credentials into account can then fall back to some default or ignore
> > such calls altogether. In the SELinux lockdown hook implementation, use
> > SECINITSID_KERNEL in case the cred argument is NULL.
> >
> > Most of the callers are updated to pass current_cred() as the cred
> > pointer, thus maintaining the same behavior. The following callers are
> > modified to pass NULL as the cred pointer instead:
> > 1. arch/powerpc/xmon/xmon.c
> > Seems to be some interactive debugging facility. It appears that
> > the lockdown hook is called from interrupt context here, so it
> > should be more appropriate to request a global lockdown decision.
> > 2. fs/tracefs/inode.c:tracefs_create_file()
> > Here the call is used to prevent creating new tracefs entries when
> > the kernel is locked down. Assumes that locking down is one-way -
> > i.e. if the hook returns non-zero once, it will never return zero
> > again, thus no point in creating these files. Also, the hook is
> > often called by a module's init function when it is loaded by
> > userspace, where it doesn't make much sense to do a check against
> > the current task's creds, since the task itself doesn't actually
> > use the tracing functionality (i.e. doesn't breach lockdown), just
> > indirectly makes some new tracepoints available to whoever is
> > authorized to use them.
> > 3. net/xfrm/xfrm_user.c:copy_to_user_*()
> > Here a cryptographic secret is redacted based on the value returned
> > from the hook. There are two possible actions that may lead here:
> > a) A netlink message XFRM_MSG_GETSA with NLM_F_DUMP set - here the
> > task context is relevant, since the dumped data is sent back to
> > the current task.
> > b) When adding/deleting/updating an SA via XFRM_MSG_xxxSA, the
> > dumped SA is broadcasted to tasks subscribed to XFRM events -
> > here the current task context is not relevant as it doesn't
> > represent the tasks that could potentially see the secret.
> > It doesn't seem worth it to try to keep using the current task's
> > context in the a) case, since the eventual data leak can be
> > circumvented anyway via b), plus there is no way for the task to
> > indicate that it doesn't care about the actual key value, so the
> > check could generate a lot of "false alert" denials with SELinux.
> > Thus, let's pass NULL instead of current_cred() here faute de
> > mieux.
> >
> > Improvements-suggested-by: Casey Schaufler
> > Improvements-suggested-by: Paul Moore
> > Fixes: 59438b46471a ("security,lockdown,selinux: implement SELinux
> > lockdown")
> > Signed-off-by: Ondrej Mosnacek
> [..]
> > diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> > index 2acc6173da36..c1747b6555c7 100644
> > --- a/drivers/cxl/mem.c
> > +++ b/drivers/cxl/mem.c
> > @@ -568,7 +568,7 @@ static bool cxl_mem_raw_command_allowed(u16 opcode)
> > if (!IS_ENABLED(CONFIG_CXL_MEM_RAW_COMMANDS))
> > return false;
> >
> > - if (security_locked_down(LOCKDOWN_NONE))
> > + if (security_locked_down(current_cred(), LOCKDOWN_NONE))
>
> Acked-by: Dan Williams
>
> ...however that usage looks wrong. The expectation is that if kernel
> integrity protections are enabled then raw command access should be
> disabled. So I think that should be equivalent to LOCKDOWN_PCI_ACCESS
> in terms of the command capabilities to filter.
Yes, the LOCKDOWN_NONE seems wrong here... but it's a pre-existing bug
and I didn't want to go down yet another rabbit hole trying to fix it.
I'll look at this again once this patch is settled - it may indeed be
as simple as replacing LOCKDOWN_NONE with LOCKDOWN_PCI_ACCESS.
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
Re: [PATCH v3] lockdown,selinux: fix wrong subject in some SELinux lockdown checks
On Fri, Jun 18, 2021 at 5:40 AM Paul Moore wrote:
> On Wed, Jun 16, 2021 at 4:51 AM Ondrej Mosnacek wrote:
> >
> > Commit 59438b46471a ("security,lockdown,selinux: implement SELinux
> > lockdown") added an implementation of the locked_down LSM hook to
> > SELinux, with the aim to restrict which domains are allowed to perform
> > operations that would breach lockdown.
> >
> > However, in several places the security_locked_down() hook is called in
> > situations where the current task isn't doing any action that would
> > directly breach lockdown, leading to SELinux checks that are basically
> > bogus.
> >
> > To fix this, add an explicit struct cred pointer argument to
> > security_lockdown() and define NULL as a special value to pass instead
> > of current_cred() in such situations. LSMs that take the subject
> > credentials into account can then fall back to some default or ignore
> > such calls altogether. In the SELinux lockdown hook implementation, use
> > SECINITSID_KERNEL in case the cred argument is NULL.
> >
> > Most of the callers are updated to pass current_cred() as the cred
> > pointer, thus maintaining the same behavior. The following callers are
> > modified to pass NULL as the cred pointer instead:
> > 1. arch/powerpc/xmon/xmon.c
> > Seems to be some interactive debugging facility. It appears that
> > the lockdown hook is called from interrupt context here, so it
> > should be more appropriate to request a global lockdown decision.
> > 2. fs/tracefs/inode.c:tracefs_create_file()
> > Here the call is used to prevent creating new tracefs entries when
> > the kernel is locked down. Assumes that locking down is one-way -
> > i.e. if the hook returns non-zero once, it will never return zero
> > again, thus no point in creating these files. Also, the hook is
> > often called by a module's init function when it is loaded by
> > userspace, where it doesn't make much sense to do a check against
> > the current task's creds, since the task itself doesn't actually
> > use the tracing functionality (i.e. doesn't breach lockdown), just
> > indirectly makes some new tracepoints available to whoever is
> > authorized to use them.
> > 3. net/xfrm/xfrm_user.c:copy_to_user_*()
> > Here a cryptographic secret is redacted based on the value returned
> > from the hook. There are two possible actions that may lead here:
> > a) A netlink message XFRM_MSG_GETSA with NLM_F_DUMP set - here the
> > task context is relevant, since the dumped data is sent back to
> > the current task.
> > b) When adding/deleting/updating an SA via XFRM_MSG_xxxSA, the
> > dumped SA is broadcasted to tasks subscribed to XFRM events -
> > here the current task context is not relevant as it doesn't
> > represent the tasks that could potentially see the secret.
> > It doesn't seem worth it to try to keep using the current task's
> > context in the a) case, since the eventual data leak can be
> > circumvented anyway via b), plus there is no way for the task to
> > indicate that it doesn't care about the actual key value, so the
> > check could generate a lot of "false alert" denials with SELinux.
> > Thus, let's pass NULL instead of current_cred() here faute de
> > mieux.
> >
> > Improvements-suggested-by: Casey Schaufler
> > Improvements-suggested-by: Paul Moore
> > Fixes: 59438b46471a ("security,lockdown,selinux: implement SELinux
> > lockdown")
> > Signed-off-by: Ondrej Mosnacek
>
> This seems reasonable to me, but before I merge it into the SELinux
> tree I think it would be good to get some ACKs from the relevant
> subsystem folks. I don't believe we ever saw a response to the last
> question for the PPC folks, did we?
Can we move this forward somehow, please?
Quoting the yet-unanswered question from the v2 thread for convenience:
> > > The callers migrated to the new hook, passing NULL as cred:
> > > 1. arch/powerpc/xmon/xmon.c
[...]
> >
> > This definitely sounds like kernel_t based on the description above.
>
> Here I'm a little concerned that the hook might be called from some
> unusual interrupt, which is not masked by spin_lock_irqsave()... We
> ran into this with PMI (Platform Management Interrupt) before, see
> commit 5ae5fbd21079 ("powerpc/perf: Fix handling of privilege level
> checks in perf interrupt context"). While I can't see anything that
> would suggest something like this happening here, the whole thing is
> so foreign to me that I'm wary of making assumptions :)
>
> @Michael/PPC devs, can you confirm to us that xmon_is_locked_down() is
> only called from normal syscall/interrupt context (as opposed to
> something tricky like PMI)?
I strongly suspect the answer will be just "Of course it is, why would
you even ask such a silly question?", but please let's have it on
record so we can finally get this patch merged...
> > ---
> >
> > v3:
> > - add the c
Re: [PATCH v3] lockdown,selinux: fix wrong subject in some SELinux lockdown checks
On Sat, Jun 19, 2021 at 1:00 PM Thomas Gleixner wrote:
> On Wed, Jun 16 2021 at 10:51, Ondrej Mosnacek wrote:
> > diff --git a/arch/x86/mm/testmmiotrace.c b/arch/x86/mm/testmmiotrace.c
> > index bda73cb7a044..c43a13241ae8 100644
> > --- a/arch/x86/mm/testmmiotrace.c
> > +++ b/arch/x86/mm/testmmiotrace.c
> > @@ -116,7 +116,7 @@ static void do_test_bulk_ioremapping(void)
> > static int __init init(void)
> > {
> > unsigned long size = (read_far) ? (8 << 20) : (16 << 10);
> > - int ret = security_locked_down(LOCKDOWN_MMIOTRACE);
> > + int ret = security_locked_down(current_cred(), LOCKDOWN_MMIOTRACE);
>
> I have no real objection to those patches, but it strikes me odd that
> out of the 62 changed places 58 have 'current_cred()' and 4 have NULL as
> argument.
>
> I can't see why this would ever end up with anything else than
> current_cred() or NULL and NULL being the 'special' case. So why not
> having security_locked_down_no_cred() and make current_cred() implicit
> for security_locked_down() which avoids most of the churn and just makes
> the special cases special. I might be missing something though.
Unfortunately it is not uncommon for kernel subsystems to add, move,
or otherwise play around with LSM hooks without checking with the LSM
folks; generally this is okay, but there have been a few problems in
the past and I try to keep that in mind when we are introducing new
hooks or modifying existing ones. If we have two LSM hooks for
roughly the same control point it has the potential to cause
confusion, e.g. do I use the "normal" or the "no_cred" version? What
if I don't want to pass a credential, can I just use "no_cred"? My
thinking with the single, always-pass-a-cred function is that callers
don't have to worry about choosing from multiple, similar hooks and
they know they need to pass a cred which hopefully gets them thinking
about what cred is appropriate. It's not foolproof, but I believe the
single hook approach will be less prone to accidents ... or so I hope
:)
--
paul moore
www.paul-moore.com
Re: [PATCH v3] lockdown,selinux: fix wrong subject in some SELinux lockdown checks
On Wed, Jun 16, 2021 at 10:51:18AM +0200, Ondrej Mosnacek wrote:
> Commit 59438b46471a ("security,lockdown,selinux: implement SELinux
> lockdown") added an implementation of the locked_down LSM hook to
> SELinux, with the aim to restrict which domains are allowed to perform
> operations that would breach lockdown.
>
> However, in several places the security_locked_down() hook is called in
> situations where the current task isn't doing any action that would
> directly breach lockdown, leading to SELinux checks that are basically
> bogus.
>
> To fix this, add an explicit struct cred pointer argument to
> security_lockdown() and define NULL as a special value to pass instead
> of current_cred() in such situations. LSMs that take the subject
> credentials into account can then fall back to some default or ignore
> such calls altogether. In the SELinux lockdown hook implementation, use
> SECINITSID_KERNEL in case the cred argument is NULL.
>
> Most of the callers are updated to pass current_cred() as the cred
> pointer, thus maintaining the same behavior. The following callers are
> modified to pass NULL as the cred pointer instead:
> 1. arch/powerpc/xmon/xmon.c
> Seems to be some interactive debugging facility. It appears that
> the lockdown hook is called from interrupt context here, so it
> should be more appropriate to request a global lockdown decision.
> 2. fs/tracefs/inode.c:tracefs_create_file()
> Here the call is used to prevent creating new tracefs entries when
> the kernel is locked down. Assumes that locking down is one-way -
> i.e. if the hook returns non-zero once, it will never return zero
> again, thus no point in creating these files. Also, the hook is
> often called by a module's init function when it is loaded by
> userspace, where it doesn't make much sense to do a check against
> the current task's creds, since the task itself doesn't actually
> use the tracing functionality (i.e. doesn't breach lockdown), just
> indirectly makes some new tracepoints available to whoever is
> authorized to use them.
> 3. net/xfrm/xfrm_user.c:copy_to_user_*()
> Here a cryptographic secret is redacted based on the value returned
> from the hook. There are two possible actions that may lead here:
> a) A netlink message XFRM_MSG_GETSA with NLM_F_DUMP set - here the
> task context is relevant, since the dumped data is sent back to
> the current task.
> b) When adding/deleting/updating an SA via XFRM_MSG_xxxSA, the
> dumped SA is broadcasted to tasks subscribed to XFRM events -
> here the current task context is not relevant as it doesn't
> represent the tasks that could potentially see the secret.
> It doesn't seem worth it to try to keep using the current task's
> context in the a) case, since the eventual data leak can be
> circumvented anyway via b), plus there is no way for the task to
> indicate that it doesn't care about the actual key value, so the
> check could generate a lot of "false alert" denials with SELinux.
> Thus, let's pass NULL instead of current_cred() here faute de
> mieux.
>
> Improvements-suggested-by: Casey Schaufler
> Improvements-suggested-by: Paul Moore
> Fixes: 59438b46471a ("security,lockdown,selinux: implement SELinux lockdown")
> Signed-off-by: Ondrej Mosnacek
For the xfrm part:
Acked-by: Steffen Klassert
Re: [PATCH v3] lockdown, selinux: fix wrong subject in some SELinux lockdown checks
On Wed, Jun 16 2021 at 10:51, Ondrej Mosnacek wrote:
> diff --git a/arch/x86/mm/testmmiotrace.c b/arch/x86/mm/testmmiotrace.c
> index bda73cb7a044..c43a13241ae8 100644
> --- a/arch/x86/mm/testmmiotrace.c
> +++ b/arch/x86/mm/testmmiotrace.c
> @@ -116,7 +116,7 @@ static void do_test_bulk_ioremapping(void)
> static int __init init(void)
> {
> unsigned long size = (read_far) ? (8 << 20) : (16 << 10);
> - int ret = security_locked_down(LOCKDOWN_MMIOTRACE);
> + int ret = security_locked_down(current_cred(), LOCKDOWN_MMIOTRACE);
I have no real objection to those patches, but it strikes me odd that
out of the 62 changed places 58 have 'current_cred()' and 4 have NULL as
argument.
I can't see why this would ever end up with anything else than
current_cred() or NULL and NULL being the 'special' case. So why not
having security_locked_down_no_cred() and make current_cred() implicit
for security_locked_down() which avoids most of the churn and just makes
the special cases special. I might be missing something though.
Thanks,
tglx
Re: [PATCH v3] lockdown,selinux: fix wrong subject in some SELinux lockdown checks
On Wed, Jun 16, 2021 at 1:51 AM Ondrej Mosnacek wrote:
>
> Commit 59438b46471a ("security,lockdown,selinux: implement SELinux
> lockdown") added an implementation of the locked_down LSM hook to
> SELinux, with the aim to restrict which domains are allowed to perform
> operations that would breach lockdown.
>
> However, in several places the security_locked_down() hook is called in
> situations where the current task isn't doing any action that would
> directly breach lockdown, leading to SELinux checks that are basically
> bogus.
>
> To fix this, add an explicit struct cred pointer argument to
> security_lockdown() and define NULL as a special value to pass instead
> of current_cred() in such situations. LSMs that take the subject
> credentials into account can then fall back to some default or ignore
> such calls altogether. In the SELinux lockdown hook implementation, use
> SECINITSID_KERNEL in case the cred argument is NULL.
>
> Most of the callers are updated to pass current_cred() as the cred
> pointer, thus maintaining the same behavior. The following callers are
> modified to pass NULL as the cred pointer instead:
> 1. arch/powerpc/xmon/xmon.c
> Seems to be some interactive debugging facility. It appears that
> the lockdown hook is called from interrupt context here, so it
> should be more appropriate to request a global lockdown decision.
> 2. fs/tracefs/inode.c:tracefs_create_file()
> Here the call is used to prevent creating new tracefs entries when
> the kernel is locked down. Assumes that locking down is one-way -
> i.e. if the hook returns non-zero once, it will never return zero
> again, thus no point in creating these files. Also, the hook is
> often called by a module's init function when it is loaded by
> userspace, where it doesn't make much sense to do a check against
> the current task's creds, since the task itself doesn't actually
> use the tracing functionality (i.e. doesn't breach lockdown), just
> indirectly makes some new tracepoints available to whoever is
> authorized to use them.
> 3. net/xfrm/xfrm_user.c:copy_to_user_*()
> Here a cryptographic secret is redacted based on the value returned
> from the hook. There are two possible actions that may lead here:
> a) A netlink message XFRM_MSG_GETSA with NLM_F_DUMP set - here the
> task context is relevant, since the dumped data is sent back to
> the current task.
> b) When adding/deleting/updating an SA via XFRM_MSG_xxxSA, the
> dumped SA is broadcasted to tasks subscribed to XFRM events -
> here the current task context is not relevant as it doesn't
> represent the tasks that could potentially see the secret.
> It doesn't seem worth it to try to keep using the current task's
> context in the a) case, since the eventual data leak can be
> circumvented anyway via b), plus there is no way for the task to
> indicate that it doesn't care about the actual key value, so the
> check could generate a lot of "false alert" denials with SELinux.
> Thus, let's pass NULL instead of current_cred() here faute de
> mieux.
>
> Improvements-suggested-by: Casey Schaufler
> Improvements-suggested-by: Paul Moore
> Fixes: 59438b46471a ("security,lockdown,selinux: implement SELinux lockdown")
> Signed-off-by: Ondrej Mosnacek
[..]
> diff --git a/drivers/cxl/mem.c b/drivers/cxl/mem.c
> index 2acc6173da36..c1747b6555c7 100644
> --- a/drivers/cxl/mem.c
> +++ b/drivers/cxl/mem.c
> @@ -568,7 +568,7 @@ static bool cxl_mem_raw_command_allowed(u16 opcode)
> if (!IS_ENABLED(CONFIG_CXL_MEM_RAW_COMMANDS))
> return false;
>
> - if (security_locked_down(LOCKDOWN_NONE))
> + if (security_locked_down(current_cred(), LOCKDOWN_NONE))
Acked-by: Dan Williams
...however that usage looks wrong. The expectation is that if kernel
integrity protections are enabled then raw command access should be
disabled. So I think that should be equivalent to LOCKDOWN_PCI_ACCESS
in terms of the command capabilities to filter.
Re: [PATCH v3] lockdown,selinux: fix wrong subject in some SELinux lockdown checks
On Wed, Jun 16, 2021 at 4:51 AM Ondrej Mosnacek wrote:
>
> Commit 59438b46471a ("security,lockdown,selinux: implement SELinux
> lockdown") added an implementation of the locked_down LSM hook to
> SELinux, with the aim to restrict which domains are allowed to perform
> operations that would breach lockdown.
>
> However, in several places the security_locked_down() hook is called in
> situations where the current task isn't doing any action that would
> directly breach lockdown, leading to SELinux checks that are basically
> bogus.
>
> To fix this, add an explicit struct cred pointer argument to
> security_lockdown() and define NULL as a special value to pass instead
> of current_cred() in such situations. LSMs that take the subject
> credentials into account can then fall back to some default or ignore
> such calls altogether. In the SELinux lockdown hook implementation, use
> SECINITSID_KERNEL in case the cred argument is NULL.
>
> Most of the callers are updated to pass current_cred() as the cred
> pointer, thus maintaining the same behavior. The following callers are
> modified to pass NULL as the cred pointer instead:
> 1. arch/powerpc/xmon/xmon.c
> Seems to be some interactive debugging facility. It appears that
> the lockdown hook is called from interrupt context here, so it
> should be more appropriate to request a global lockdown decision.
> 2. fs/tracefs/inode.c:tracefs_create_file()
> Here the call is used to prevent creating new tracefs entries when
> the kernel is locked down. Assumes that locking down is one-way -
> i.e. if the hook returns non-zero once, it will never return zero
> again, thus no point in creating these files. Also, the hook is
> often called by a module's init function when it is loaded by
> userspace, where it doesn't make much sense to do a check against
> the current task's creds, since the task itself doesn't actually
> use the tracing functionality (i.e. doesn't breach lockdown), just
> indirectly makes some new tracepoints available to whoever is
> authorized to use them.
> 3. net/xfrm/xfrm_user.c:copy_to_user_*()
> Here a cryptographic secret is redacted based on the value returned
> from the hook. There are two possible actions that may lead here:
> a) A netlink message XFRM_MSG_GETSA with NLM_F_DUMP set - here the
> task context is relevant, since the dumped data is sent back to
> the current task.
> b) When adding/deleting/updating an SA via XFRM_MSG_xxxSA, the
> dumped SA is broadcasted to tasks subscribed to XFRM events -
> here the current task context is not relevant as it doesn't
> represent the tasks that could potentially see the secret.
> It doesn't seem worth it to try to keep using the current task's
> context in the a) case, since the eventual data leak can be
> circumvented anyway via b), plus there is no way for the task to
> indicate that it doesn't care about the actual key value, so the
> check could generate a lot of "false alert" denials with SELinux.
> Thus, let's pass NULL instead of current_cred() here faute de
> mieux.
>
> Improvements-suggested-by: Casey Schaufler
> Improvements-suggested-by: Paul Moore
> Fixes: 59438b46471a ("security,lockdown,selinux: implement SELinux lockdown")
> Signed-off-by: Ondrej Mosnacek
This seems reasonable to me, but before I merge it into the SELinux
tree I think it would be good to get some ACKs from the relevant
subsystem folks. I don't believe we ever saw a response to the last
question for the PPC folks, did we?
> ---
>
> v3:
> - add the cred argument to security_locked_down() and adapt all callers
> - keep using current_cred() in BPF, as the hook calls have been shifted
> to program load time (commit ff40e51043af ("bpf, lockdown, audit: Fix
> buggy SELinux lockdown permission checks"))
> - in SELinux, don't ignore hook calls where cred == NULL, but use
> SECINITSID_KERNEL as the subject instead
> - update explanations in the commit message
>
> v2: https://lore.kernel.org/lkml/20210517092006.803332-1-omosn...@redhat.com/
> - change to a single hook based on suggestions by Casey Schaufler
>
> v1: https://lore.kernel.org/lkml/20210507114048.138933-1-omosn...@redhat.com/
>
> arch/powerpc/xmon/xmon.c | 4 ++--
> arch/x86/kernel/ioport.c | 4 ++--
> arch/x86/kernel/msr.c| 4 ++--
> arch/x86/mm/testmmiotrace.c | 2 +-
> drivers/acpi/acpi_configfs.c | 2 +-
> drivers/acpi/custom_method.c | 2 +-
> drivers/acpi/osl.c | 3 ++-
> drivers/acpi/tables.c| 2 +-
> drivers/char/mem.c | 2 +-
> drivers/cxl/mem.c| 2 +-
> drivers/firmware/efi/efi.c | 2 +-
> drivers/firmware/efi/test/efi_test.c | 2 +-
> drivers/pci/pci-sysfs.c | 6 +++---
> drivers/pci/proc.c | 6 ++
[PATCH v3] lockdown, selinux: fix wrong subject in some SELinux lockdown checks
Commit 59438b46471a ("security,lockdown,selinux: implement SELinux
lockdown") added an implementation of the locked_down LSM hook to
SELinux, with the aim to restrict which domains are allowed to perform
operations that would breach lockdown.
However, in several places the security_locked_down() hook is called in
situations where the current task isn't doing any action that would
directly breach lockdown, leading to SELinux checks that are basically
bogus.
To fix this, add an explicit struct cred pointer argument to
security_lockdown() and define NULL as a special value to pass instead
of current_cred() in such situations. LSMs that take the subject
credentials into account can then fall back to some default or ignore
such calls altogether. In the SELinux lockdown hook implementation, use
SECINITSID_KERNEL in case the cred argument is NULL.
Most of the callers are updated to pass current_cred() as the cred
pointer, thus maintaining the same behavior. The following callers are
modified to pass NULL as the cred pointer instead:
1. arch/powerpc/xmon/xmon.c
Seems to be some interactive debugging facility. It appears that
the lockdown hook is called from interrupt context here, so it
should be more appropriate to request a global lockdown decision.
2. fs/tracefs/inode.c:tracefs_create_file()
Here the call is used to prevent creating new tracefs entries when
the kernel is locked down. Assumes that locking down is one-way -
i.e. if the hook returns non-zero once, it will never return zero
again, thus no point in creating these files. Also, the hook is
often called by a module's init function when it is loaded by
userspace, where it doesn't make much sense to do a check against
the current task's creds, since the task itself doesn't actually
use the tracing functionality (i.e. doesn't breach lockdown), just
indirectly makes some new tracepoints available to whoever is
authorized to use them.
3. net/xfrm/xfrm_user.c:copy_to_user_*()
Here a cryptographic secret is redacted based on the value returned
from the hook. There are two possible actions that may lead here:
a) A netlink message XFRM_MSG_GETSA with NLM_F_DUMP set - here the
task context is relevant, since the dumped data is sent back to
the current task.
b) When adding/deleting/updating an SA via XFRM_MSG_xxxSA, the
dumped SA is broadcasted to tasks subscribed to XFRM events -
here the current task context is not relevant as it doesn't
represent the tasks that could potentially see the secret.
It doesn't seem worth it to try to keep using the current task's
context in the a) case, since the eventual data leak can be
circumvented anyway via b), plus there is no way for the task to
indicate that it doesn't care about the actual key value, so the
check could generate a lot of "false alert" denials with SELinux.
Thus, let's pass NULL instead of current_cred() here faute de
mieux.
Improvements-suggested-by: Casey Schaufler
Improvements-suggested-by: Paul Moore
Fixes: 59438b46471a ("security,lockdown,selinux: implement SELinux lockdown")
Signed-off-by: Ondrej Mosnacek
---
v3:
- add the cred argument to security_locked_down() and adapt all callers
- keep using current_cred() in BPF, as the hook calls have been shifted
to program load time (commit ff40e51043af ("bpf, lockdown, audit: Fix
buggy SELinux lockdown permission checks"))
- in SELinux, don't ignore hook calls where cred == NULL, but use
SECINITSID_KERNEL as the subject instead
- update explanations in the commit message
v2: https://lore.kernel.org/lkml/20210517092006.803332-1-omosn...@redhat.com/
- change to a single hook based on suggestions by Casey Schaufler
v1: https://lore.kernel.org/lkml/20210507114048.138933-1-omosn...@redhat.com/
arch/powerpc/xmon/xmon.c | 4 ++--
arch/x86/kernel/ioport.c | 4 ++--
arch/x86/kernel/msr.c| 4 ++--
arch/x86/mm/testmmiotrace.c | 2 +-
drivers/acpi/acpi_configfs.c | 2 +-
drivers/acpi/custom_method.c | 2 +-
drivers/acpi/osl.c | 3 ++-
drivers/acpi/tables.c| 2 +-
drivers/char/mem.c | 2 +-
drivers/cxl/mem.c| 2 +-
drivers/firmware/efi/efi.c | 2 +-
drivers/firmware/efi/test/efi_test.c | 2 +-
drivers/pci/pci-sysfs.c | 6 +++---
drivers/pci/proc.c | 6 +++---
drivers/pci/syscall.c| 2 +-
drivers/pcmcia/cistpl.c | 2 +-
drivers/tty/serial/serial_core.c | 2 +-
fs/debugfs/file.c| 2 +-
fs/debugfs/inode.c | 2 +-
fs/proc/kcore.c | 2 +-
fs/tracefs/inode.c | 2 +-
include/linux/lsm_hook_defs.h| 2 +-
include/linux/lsm_hooks.h| 1 +
include/linux/security.h | 4 ++-
