* Re: [PATCH 0/1] ima: check control characters in policy path
From: James Bottomley @ 2021-08-14 12:47 UTC (permalink / raw)
To: Tianxing Zhang, zohar
Cc: linux-integrity, linux-security-module, linux-kernel
In-Reply-To: <20210814081356.293-1-anakinzhang96@gmail.com>
On Sat, 2021-08-14 at 16:13 +0800, Tianxing Zhang wrote:
> Hi,
>
> IMA policy can be updated with /sys/kernel/security/ima/policy
> interface when CONFIG_IMA_WRITE_POLICY is set. However, kernel does
> not check the file path carefully. It only checks if the path has '/'
> prefix.
>
> When a policy file path contains control characters like '\r' or
> '\b', invalid error messages can be printed to overwrite system
> messages.
This doesn't sound like a good idea: filesystems accept control
characters in names, so the IMA file policy has to be able to specify
them. We can debate whether filesystems should do this, but while they
do IMA has to as well.
> For example:
>
> $ echo -e "/\rtest invalid path: ddddddddddddddddddddd" >
> /sys/kernel/security/ima/policy
> $ dmesg
> test invalid path: ddddddddddddddddddddd (-2)
>
> After adding this patch, we'll be able to throw out error message:
>
> $ echo -e "/\rtest invalid path: ddddddddddddddddddddd" >
> /sys/kernel/security/ima/policy
> -bash: echo: write error: Invalid argument
> $ dmesg
> [ 11.684004] ima: invalid path (control characters are not allowed)
> [ 11.684071] ima: policy update failed
>
> Any suggestions would be appreciated, thank you.
I don't quite understand what you think the problem is. Only root can
write IMA policies so no-one other than a legitimate administrator can
use bogus paths like the above. If the problem is producing a bogus
log message, we do have several IMA messages that print out
measured/appraised file names ... they would be vulnerable to this
since a generic user could have created them with control character
containg file names, and your proposed patch wouldn't fix that.
Wouldn't a better solution be to have a file name print that expands
the unprintable characters?
James
^ permalink raw reply
* [PATCH 1/1] ima: check control characters in policy file path
From: Tianxing Zhang @ 2021-08-14 8:27 UTC (permalink / raw)
To: zohar; +Cc: linux-integrity, linux-security-module, linux-kernel,
Tianxing Zhang
When a policy file path contains control characters like '\r' or '\b',
invalid error messages can be printed to overwrite system messages:
$ echo -e "/\rtest 12345678" > /sys/kernel/security/ima/policy
This patch rejects policy paths with control characters.
Signed-off-by: Tianxing Zhang <anakinzhang96@gmail.com>
---
security/integrity/ima/ima_fs.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 3d8e9d5db5aa..e6daa138de89 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -316,6 +316,7 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
{
char *data;
ssize_t result;
+ int i;
if (datalen >= PAGE_SIZE)
datalen = PAGE_SIZE - 1;
@@ -331,6 +332,14 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
goto out;
}
+ for (i = 0; data[i] != '\n' && data[i] != '\0'; i++) {
+ if (iscntrl(data[i])) {
+ pr_err_once("file path with no control characters required\n");
+ result = -EINVAL;
+ goto out_free;
+ }
+ }
+
result = mutex_lock_interruptible(&ima_write_mutex);
if (result < 0)
goto out_free;
--
2.25.1
^ permalink raw reply related
* [PATCH 0/1] ima: check control characters in policy path
From: Tianxing Zhang @ 2021-08-14 8:13 UTC (permalink / raw)
To: zohar; +Cc: linux-integrity, linux-security-module, linux-kernel,
Tianxing Zhang
Hi,
IMA policy can be updated with /sys/kernel/security/ima/policy interface when
CONFIG_IMA_WRITE_POLICY is set. However, kernel does not check the file path
carefully. It only checks if the path has '/' prefix.
When a policy file path contains control characters like '\r' or '\b',
invalid error messages can be printed to overwrite system messages.
For example:
$ echo -e "/\rtest invalid path: ddddddddddddddddddddd" > /sys/kernel/security/ima/policy
$ dmesg
test invalid path: ddddddddddddddddddddd (-2)
After adding this patch, we'll be able to throw out error message:
$ echo -e "/\rtest invalid path: ddddddddddddddddddddd" > /sys/kernel/security/ima/policy
-bash: echo: write error: Invalid argument
$ dmesg
[ 11.684004] ima: invalid path (control characters are not allowed)
[ 11.684071] ima: policy update failed
Any suggestions would be appreciated, thank you.
Tianxing Zhang (1):
ima: check control characters in policy file path
security/integrity/ima/ima_fs.c | 9 +++++++++
1 file changed, 9 insertions(+)
--
2.25.1
^ permalink raw reply
* Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
From: Casey Schaufler @ 2021-08-13 21:47 UTC (permalink / raw)
To: Paul Moore
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley, linux-kernel, netdev, Casey Schaufler
In-Reply-To: <CAHC9VhQxG+LXxgtczhH=yVdeh9mTO+Xhe=TeQ4eihjtkQ2=3Fw@mail.gmail.com>
On 8/13/2021 1:43 PM, Paul Moore wrote:
> On Fri, Aug 13, 2021 at 2:48 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 8/13/2021 8:31 AM, Paul Moore wrote:
>>> On Thu, Aug 12, 2021 at 6:38 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 8/12/2021 1:59 PM, Paul Moore wrote:
>>>>> On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>>> Create a new audit record type to contain the subject information
>>>>>> when there are multiple security modules that require such data.
>>> ...
>>>
>>>>> The local
>>>>> audit context is a hack that is made necessary by the fact that we
>>>>> have to audit things which happen outside the scope of an executing
>>>>> task, e.g. the netfilter audit hooks, it should *never* be used when
>>>>> there is a valid task_struct.
>>>> In the existing audit code a "current context" is only needed for
>>>> syscall events, so that's the only case where it's allocated. Would
>>>> you suggest that I track down the non-syscall events that include
>>>> subj= fields and add allocate a "current context" for them? I looked
>>>> into doing that, and it wouldn't be simple.
>>> This is why the "local context" was created. Prior to these stacking
>>> additions, and the audit container ID work, we never needed to group
>>> multiple audit records outside of a syscall context into a single
>>> audit event so passing a NULL context into audit_log_start() was
>>> reasonable. The local context was designed as a way to generate a
>>> context for use in a local function scope to group multiple records,
>>> however, for reasons I'll get to below I'm now wondering if the local
>>> context approach is really workable ...
>> I haven't found a place where it didn't work. What is the concern?
> The concern is that use of a local context can destroy any hopes of
> linking with other related records, e.g. SYSCALL and PATH records, to
> form a single cohesive event. If the current task_struct is valid for
> a given function invocation then we *really* should be using current's
> audit_context.
>
> However, based on our discussion here it would seem that we may have
> some issues where current->audit_context is not being managed
> correctly. I'm not surprised, but I will admit to being disappointed.
I'd believe that with syscall audit being a special case for other reasons
the multiple record situation got taken care of on a case-by-case basis
and no one really paid much attention to generality. It's understandable.
>>> What does your audit config look like? Both the kernel command line
>>> and the output of 'auditctl -l' would be helpful.
>> On the fedora system:
>>
>> BOOT_IMAGE=(hd0,gpt2)/vmlinuz-5.14.0-rc5stack+
>> root=/dev/mapper/fedora-root ro resume=/dev/mapper/fedora-swap
>> rd.lvm.lv=fedora/root rd.lvm.lv=fedora/swap lsm.debug
>>
>> -a always,exit -F arch=b64 -S bpf -F key=testsuite-1628714321-EtlWIphW
>>
>> On the Ubuntu system:
>>
>> BOOT_IMAGE=/boot/vmlinuz-5.14.0-rc1stack+
>> root=UUID=39c25777-d413-4c2e-948c-dfa2bf259049 ro lsm.debug
>>
>> No rules
> The Fedora system looks to have some audit-testsuite leftovers, but
> that shouldn't have an impact on what we are discussing; in both cases
> I would expect current->audit_context to be allocated and non-NULL.
As would I.
>>> I'm beginning to suspect that you have the default
>>> we-build-audit-into-the-kernel-because-product-management-said-we-have-to-but-we-don't-actually-enable-it-at-runtime
>>> audit configuration that is de rigueur for many distros these days.
>> Yes, but I've also fiddled about with it so as to get better event coverage.
>> I've run the audit-testsuite, which has got to fiddle about with the audit
>> configuration.
> Yes, it looks like my hunch was wrong.
>
>>> If that is the case, there are many cases where you would not see a
>>> NULL current->audit_context simply because the config never allocated
>>> one, see kernel/auditsc.c:audit_alloc().
>> I assume you mean that I *would* see a NULL current->audit_context
>> in the "event not enabled" case.
> Yep, typo.
>
>>> Regardless, assuming that is the case we probably need to find an
>>> alternative to the local context approach as it currently works. For
>>> reasons we already talked about, we don't want to use a local
>>> audit_context if there is the possibility for a proper
>>> current->audit_context, but we need to do *something* so that we can
>>> group these multiple events into a single record.
>> I tried a couple things, but neither was satisfactory.
>>
>>> Since this is just occurring to me now I need a bit more time to think
>>> on possible solutions - all good ideas are welcome - but the first
>>> thing that pops into my head is that we need to augment
>>> audit_log_end() to potentially generated additional, associated
>>> records similar to what we do on syscall exit in audit_log_exit().
>> I looked into that. You need a place to save the timestamp
>> that doesn't disappear. That's the role the audit_context plays
>> now.
> Yes, I've spent a few hours staring at the poorly planned struct that
> is audit_context ;)
>
> Regardless, the obvious place for such a thing is audit_buffer; we can
> stash whatever we need in there.
I had considered doing that, but was afraid that moving the timestamp
out of the audit_context might have dire consequences.
>>> Of
>>> course the audit_log_end() changes would be much more limited than
>>> audit_log_exit(), just the LSM subject and audit container ID info,
>>> and even then we might want to limit that to cases where the ab->ctx
>>> value is NULL and let audit_log_exit() handle it otherwise. We may
>>> need to store the event type in the audit_buffer during
>>> audit_log_start() so that we can later use that in audit_log_end() to
>>> determine what additional records are needed.
>>>
>>> Regardless, let's figure out why all your current->audit_context
>>> values are NULL
>> That's what's maddening, and why I implemented audit_alloc_for_lsm().
>> They aren't all NULL. Sometimes current->audit_context is NULL,
>> sometimes it isn't, for the same event. I thought it might be a
>> question of the netlink interface being treated specially, but
>> that doesn't explain all the cases.
> Your netlink changes are exactly what made me think, "this is
> obviously wrong", but now I'm wondering if a previously held
> assumption of "current is valid and points to the calling process" in
> the case of the kernel servicing netlink messages sent from userspace.
If that's the case the subject data in the audit record is going
to be bogus. From what I've seen that data appears to be correct.
> Or rather, perhaps that assumption is still true but something is
> causing current->audit_context to be NULL in that case.
I can imagine someone deciding not to set up audit_context in
situations like netlink because they knew that nothing following
that would be a syscall event. I've been looking into the audit
userspace and there are assumptions like that all over the place.
> Friday the 13th indeed.
I've been banging my head against this for a couple months.
My biggest fear is that I may have learned enough about the
audit system to make useful contributions.
^ permalink raw reply
* Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
From: Paul Moore @ 2021-08-13 20:43 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley, linux-kernel, netdev
In-Reply-To: <fba1a123-d6e5-dcb0-3d49-f60b26f65b29@schaufler-ca.com>
On Fri, Aug 13, 2021 at 2:48 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 8/13/2021 8:31 AM, Paul Moore wrote:
> > On Thu, Aug 12, 2021 at 6:38 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 8/12/2021 1:59 PM, Paul Moore wrote:
> >>> On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >>>> Create a new audit record type to contain the subject information
> >>>> when there are multiple security modules that require such data.
> > ...
> >
> >>> The local
> >>> audit context is a hack that is made necessary by the fact that we
> >>> have to audit things which happen outside the scope of an executing
> >>> task, e.g. the netfilter audit hooks, it should *never* be used when
> >>> there is a valid task_struct.
> >> In the existing audit code a "current context" is only needed for
> >> syscall events, so that's the only case where it's allocated. Would
> >> you suggest that I track down the non-syscall events that include
> >> subj= fields and add allocate a "current context" for them? I looked
> >> into doing that, and it wouldn't be simple.
> >
> > This is why the "local context" was created. Prior to these stacking
> > additions, and the audit container ID work, we never needed to group
> > multiple audit records outside of a syscall context into a single
> > audit event so passing a NULL context into audit_log_start() was
> > reasonable. The local context was designed as a way to generate a
> > context for use in a local function scope to group multiple records,
> > however, for reasons I'll get to below I'm now wondering if the local
> > context approach is really workable ...
>
> I haven't found a place where it didn't work. What is the concern?
The concern is that use of a local context can destroy any hopes of
linking with other related records, e.g. SYSCALL and PATH records, to
form a single cohesive event. If the current task_struct is valid for
a given function invocation then we *really* should be using current's
audit_context.
However, based on our discussion here it would seem that we may have
some issues where current->audit_context is not being managed
correctly. I'm not surprised, but I will admit to being disappointed.
> > What does your audit config look like? Both the kernel command line
> > and the output of 'auditctl -l' would be helpful.
>
> On the fedora system:
>
> BOOT_IMAGE=(hd0,gpt2)/vmlinuz-5.14.0-rc5stack+
> root=/dev/mapper/fedora-root ro resume=/dev/mapper/fedora-swap
> rd.lvm.lv=fedora/root rd.lvm.lv=fedora/swap lsm.debug
>
> -a always,exit -F arch=b64 -S bpf -F key=testsuite-1628714321-EtlWIphW
>
> On the Ubuntu system:
>
> BOOT_IMAGE=/boot/vmlinuz-5.14.0-rc1stack+
> root=UUID=39c25777-d413-4c2e-948c-dfa2bf259049 ro lsm.debug
>
> No rules
The Fedora system looks to have some audit-testsuite leftovers, but
that shouldn't have an impact on what we are discussing; in both cases
I would expect current->audit_context to be allocated and non-NULL.
> > I'm beginning to suspect that you have the default
> > we-build-audit-into-the-kernel-because-product-management-said-we-have-to-but-we-don't-actually-enable-it-at-runtime
> > audit configuration that is de rigueur for many distros these days.
>
> Yes, but I've also fiddled about with it so as to get better event coverage.
> I've run the audit-testsuite, which has got to fiddle about with the audit
> configuration.
Yes, it looks like my hunch was wrong.
> > If that is the case, there are many cases where you would not see a
> > NULL current->audit_context simply because the config never allocated
> > one, see kernel/auditsc.c:audit_alloc().
>
> I assume you mean that I *would* see a NULL current->audit_context
> in the "event not enabled" case.
Yep, typo.
> > Regardless, assuming that is the case we probably need to find an
> > alternative to the local context approach as it currently works. For
> > reasons we already talked about, we don't want to use a local
> > audit_context if there is the possibility for a proper
> > current->audit_context, but we need to do *something* so that we can
> > group these multiple events into a single record.
>
> I tried a couple things, but neither was satisfactory.
>
> > Since this is just occurring to me now I need a bit more time to think
> > on possible solutions - all good ideas are welcome - but the first
> > thing that pops into my head is that we need to augment
> > audit_log_end() to potentially generated additional, associated
> > records similar to what we do on syscall exit in audit_log_exit().
>
> I looked into that. You need a place to save the timestamp
> that doesn't disappear. That's the role the audit_context plays
> now.
Yes, I've spent a few hours staring at the poorly planned struct that
is audit_context ;)
Regardless, the obvious place for such a thing is audit_buffer; we can
stash whatever we need in there.
> > Of
> > course the audit_log_end() changes would be much more limited than
> > audit_log_exit(), just the LSM subject and audit container ID info,
> > and even then we might want to limit that to cases where the ab->ctx
> > value is NULL and let audit_log_exit() handle it otherwise. We may
> > need to store the event type in the audit_buffer during
> > audit_log_start() so that we can later use that in audit_log_end() to
> > determine what additional records are needed.
> >
> > Regardless, let's figure out why all your current->audit_context
> > values are NULL
>
> That's what's maddening, and why I implemented audit_alloc_for_lsm().
> They aren't all NULL. Sometimes current->audit_context is NULL,
> sometimes it isn't, for the same event. I thought it might be a
> question of the netlink interface being treated specially, but
> that doesn't explain all the cases.
Your netlink changes are exactly what made me think, "this is
obviously wrong", but now I'm wondering if a previously held
assumption of "current is valid and points to the calling process" in
the case of the kernel servicing netlink messages sent from userspace.
Or rather, perhaps that assumption is still true but something is
causing current->audit_context to be NULL in that case.
Friday the 13th indeed.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 1/1] NAX LSM: Add initial support support
From: Igor Zhbanov @ 2021-08-13 20:10 UTC (permalink / raw)
To: THOBY Simon
Cc: Igor Zhbanov, linux-integrity, linux-security-module, Mimi Zohar
In-Reply-To: <d97d7fdb-1676-9670-6cf5-2427f780ec6f@viveris.fr>
Hi Simon,
ср, 28 июл. 2021 г. в 13:19, THOBY Simon <Simon.THOBY@viveris.fr>:
> The kernel style guide mandates that all variables are only defined at the beggining of the
> function, and not at the beggining of any block like C89.
I've checked the kernel coding style and couldn't find anything about
variables definition
place. Could you, please, point me to the requirement?
Scoping variables declaration makes the code better and more secure.
Besides, I've checked the kernel sources and see many examples of that.
Thank you.
^ permalink raw reply
* Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
From: Casey Schaufler @ 2021-08-13 18:48 UTC (permalink / raw)
To: Paul Moore
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley, linux-kernel, netdev, Casey Schaufler
In-Reply-To: <CAHC9VhSSASAL1mVwDo1VS3HcEF7Yb3LTTaoajEtq1HsA-8R+xQ@mail.gmail.com>
On 8/13/2021 8:31 AM, Paul Moore wrote:
> On Thu, Aug 12, 2021 at 6:38 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 8/12/2021 1:59 PM, Paul Moore wrote:
>>> On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> Create a new audit record type to contain the subject information
>>>> when there are multiple security modules that require such data.
> ...
>
>>> The local
>>> audit context is a hack that is made necessary by the fact that we
>>> have to audit things which happen outside the scope of an executing
>>> task, e.g. the netfilter audit hooks, it should *never* be used when
>>> there is a valid task_struct.
>> In the existing audit code a "current context" is only needed for
>> syscall events, so that's the only case where it's allocated. Would
>> you suggest that I track down the non-syscall events that include
>> subj= fields and add allocate a "current context" for them? I looked
>> into doing that, and it wouldn't be simple.
> This is why the "local context" was created. Prior to these stacking
> additions, and the audit container ID work, we never needed to group
> multiple audit records outside of a syscall context into a single
> audit event so passing a NULL context into audit_log_start() was
> reasonable. The local context was designed as a way to generate a
> context for use in a local function scope to group multiple records,
> however, for reasons I'll get to below I'm now wondering if the local
> context approach is really workable ...
I haven't found a place where it didn't work. What is the concern?
>>> Hopefully that makes sense?
>> Yes, it makes sense. Methinks you may believe that the current context
>> is available more regularly than it actually is.
>>
>> I instrumented the audit event functions with:
>>
>> WARN_ONCE(audit_context, "%s has context\n", __func__);
>> WARN_ONCE(!audit_context, "%s lacks context\n", __func__);
>>
>> I only used local contexts where the 2nd WARN_ONCE was hit.
> What does your audit config look like? Both the kernel command line
> and the output of 'auditctl -l' would be helpful.
On the fedora system:
BOOT_IMAGE=(hd0,gpt2)/vmlinuz-5.14.0-rc5stack+
root=/dev/mapper/fedora-root ro resume=/dev/mapper/fedora-swap
rd.lvm.lv=fedora/root rd.lvm.lv=fedora/swap lsm.debug
-a always,exit -F arch=b64 -S bpf -F key=testsuite-1628714321-EtlWIphW
On the Ubuntu system:
BOOT_IMAGE=/boot/vmlinuz-5.14.0-rc1stack+
root=UUID=39c25777-d413-4c2e-948c-dfa2bf259049 ro lsm.debug
No rules
> I'm beginning to suspect that you have the default
> we-build-audit-into-the-kernel-because-product-management-said-we-have-to-but-we-don't-actually-enable-it-at-runtime
> audit configuration that is de rigueur for many distros these days.
Yes, but I've also fiddled about with it so as to get better event coverage.
I've run the audit-testsuite, which has got to fiddle about with the audit
configuration.
> If that is the case, there are many cases where you would not see a
> NULL current->audit_context simply because the config never allocated
> one, see kernel/auditsc.c:audit_alloc().
I assume you mean that I *would* see a NULL current->audit_context
in the "event not enabled" case.
> If that is the case, I'm
> honestly a little surprised we didn't realize that earlier, especially
> given all the work/testing that Richard has done with the audit
> container ID bits, but then again he surely had a proper audit config
> during his testing so it wouldn't have appeared.
>
> Good times.
Indeed.
> Regardless, assuming that is the case we probably need to find an
> alternative to the local context approach as it currently works. For
> reasons we already talked about, we don't want to use a local
> audit_context if there is the possibility for a proper
> current->audit_context, but we need to do *something* so that we can
> group these multiple events into a single record.
I tried a couple things, but neither was satisfactory.
> Since this is just occurring to me now I need a bit more time to think
> on possible solutions - all good ideas are welcome - but the first
> thing that pops into my head is that we need to augment
> audit_log_end() to potentially generated additional, associated
> records similar to what we do on syscall exit in audit_log_exit().
I looked into that. You need a place to save the timestamp
that doesn't disappear. That's the role the audit_context plays
now.
> Of
> course the audit_log_end() changes would be much more limited than
> audit_log_exit(), just the LSM subject and audit container ID info,
> and even then we might want to limit that to cases where the ab->ctx
> value is NULL and let audit_log_exit() handle it otherwise. We may
> need to store the event type in the audit_buffer during
> audit_log_start() so that we can later use that in audit_log_end() to
> determine what additional records are needed.
>
> Regardless, let's figure out why all your current->audit_context
> values are NULL
That's what's maddening, and why I implemented audit_alloc_for_lsm().
They aren't all NULL. Sometimes current->audit_context is NULL,
sometimes it isn't, for the same event. I thought it might be a
question of the netlink interface being treated specially, but
that doesn't explain all the cases.
> first (report back on your audit config please), I may
> be worrying about a hypothetical that isn't real.
>
^ permalink raw reply
* Re: [PATCH v3 01/14] integrity: Introduce a Linux keyring for the Machine Owner Key (MOK)
From: Nayna @ 2021-08-13 18:26 UTC (permalink / raw)
To: Jarkko Sakkinen, Eric Snowberg
Cc: keyrings, linux-integrity, Mimi Zohar, dhowells, dwmw2, herbert,
davem, jmorris, serge, keescook, gregkh, torvalds, scott.branden,
weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
linux-kernel, linux-crypto, linux-security-module,
James.Bottomley, pjones, glin, konrad.wilk
In-Reply-To: <20210812185853.p5mgsgrftgwvt5fx@kernel.org>
On 8/12/21 2:58 PM, Jarkko Sakkinen wrote:
> On Wed, Aug 11, 2021 at 10:18:42PM -0400, Eric Snowberg wrote:
>> Many UEFI Linux distributions boot using shim. The UEFI shim provides
>> what is called Machine Owner Keys (MOK). Shim uses both the UEFI Secure
>> Boot DB and MOK keys to validate the next step in the boot chain. The
>> MOK facility can be used to import user generated keys. These keys can
>> be used to sign an end-users development kernel build. When Linux
>> boots, both UEFI Secure Boot DB and MOK keys get loaded in the Linux
>> .platform keyring.
>>
>> Add a new Linux keyring called .mok. This keyring shall contain just
> I would consider ".machine" instead. It holds MOK keys but is not a
> MOK key.
I agree with changing the name.
I believe the underlying source from where CA keys are loaded might vary
based on the architecture (".mok" is UEFI specific.). The key part is
that this new keyring should contain only CA keys which can be later
used to vouch for user keys loaded onto IMA or secondary keyring at
runtime. It would be good to have a "ca" in the name, like .xxxx-ca,
where xxxx can be machine, owner, or system. I prefer .system-ca.
Thanks & Regards,
- Nayna
^ permalink raw reply
* Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
From: Paul Moore @ 2021-08-13 15:31 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley, linux-kernel, netdev
In-Reply-To: <ace9d273-3560-3631-33fa-7421a165b038@schaufler-ca.com>
On Thu, Aug 12, 2021 at 6:38 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 8/12/2021 1:59 PM, Paul Moore wrote:
> > On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> Create a new audit record type to contain the subject information
> >> when there are multiple security modules that require such data.
...
> > The local
> > audit context is a hack that is made necessary by the fact that we
> > have to audit things which happen outside the scope of an executing
> > task, e.g. the netfilter audit hooks, it should *never* be used when
> > there is a valid task_struct.
>
> In the existing audit code a "current context" is only needed for
> syscall events, so that's the only case where it's allocated. Would
> you suggest that I track down the non-syscall events that include
> subj= fields and add allocate a "current context" for them? I looked
> into doing that, and it wouldn't be simple.
This is why the "local context" was created. Prior to these stacking
additions, and the audit container ID work, we never needed to group
multiple audit records outside of a syscall context into a single
audit event so passing a NULL context into audit_log_start() was
reasonable. The local context was designed as a way to generate a
context for use in a local function scope to group multiple records,
however, for reasons I'll get to below I'm now wondering if the local
context approach is really workable ...
> > Hopefully that makes sense?
>
> Yes, it makes sense. Methinks you may believe that the current context
> is available more regularly than it actually is.
>
> I instrumented the audit event functions with:
>
> WARN_ONCE(audit_context, "%s has context\n", __func__);
> WARN_ONCE(!audit_context, "%s lacks context\n", __func__);
>
> I only used local contexts where the 2nd WARN_ONCE was hit.
What does your audit config look like? Both the kernel command line
and the output of 'auditctl -l' would be helpful.
I'm beginning to suspect that you have the default
we-build-audit-into-the-kernel-because-product-management-said-we-have-to-but-we-don't-actually-enable-it-at-runtime
audit configuration that is de rigueur for many distros these days.
If that is the case, there are many cases where you would not see a
NULL current->audit_context simply because the config never allocated
one, see kernel/auditsc.c:audit_alloc(). If that is the case, I'm
honestly a little surprised we didn't realize that earlier, especially
given all the work/testing that Richard has done with the audit
container ID bits, but then again he surely had a proper audit config
during his testing so it wouldn't have appeared.
Good times.
Regardless, assuming that is the case we probably need to find an
alternative to the local context approach as it currently works. For
reasons we already talked about, we don't want to use a local
audit_context if there is the possibility for a proper
current->audit_context, but we need to do *something* so that we can
group these multiple events into a single record.
Since this is just occurring to me now I need a bit more time to think
on possible solutions - all good ideas are welcome - but the first
thing that pops into my head is that we need to augment
audit_log_end() to potentially generated additional, associated
records similar to what we do on syscall exit in audit_log_exit(). Of
course the audit_log_end() changes would be much more limited than
audit_log_exit(), just the LSM subject and audit container ID info,
and even then we might want to limit that to cases where the ab->ctx
value is NULL and let audit_log_exit() handle it otherwise. We may
need to store the event type in the audit_buffer during
audit_log_start() so that we can later use that in audit_log_end() to
determine what additional records are needed.
Regardless, let's figure out why all your current->audit_context
values are NULL first (report back on your audit config please), I may
be worrying about a hypothetical that isn't real.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 3/3] virt: Add sev_secret module to expose confidential computing secrets
From: Andrew Scull @ 2021-08-13 13:05 UTC (permalink / raw)
To: Dov Murik
Cc: linux-efi, Borislav Petkov, Ashish Kalra, Brijesh Singh,
Tom Lendacky, Ard Biesheuvel, James Morris, Serge E. Hallyn,
Andi Kleen, Dr. David Alan Gilbert, James Bottomley,
Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
linux-security-module, linux-kernel
In-Reply-To: <20210809190157.279332-4-dovmurik@linux.ibm.com>
On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote:
> The new sev_secret module exposes the confidential computing (coco)
> secret area via securityfs interface.
>
> When the module is loaded (and securityfs is mounted, typically under
> /sys/kernel/security), a "coco/sev_secret" directory is created in
> securityfs. In it, a file is created for each secret entry. The name
> of each such file is the GUID of the secret entry, and its content is
> the secret data.
>
> This allows applications running in a confidential computing setting to
> read secrets provided by the guest owner via a secure secret injection
> mechanism (such as AMD SEV's LAUNCH_SECRET command).
>
> Removing (unlinking) files in the "coco/sev_secret" directory will zero
> out the secret in memory, and remove the filesystem entry. If the
> module is removed and loaded again, that secret will not appear in the
> filesystem.
We've also been looking into a similar secret mechanism recently in the
context of Android and protected KVM [1]. Our secrets would come from a
different source, likely described as a reserved-memory node in the DT,
but would need to be exposed to userspace in the same way as the SEV
secrets. Originally I tried using a character device, but this approach
with securityfs feels neater to me.
We're also looking to pass secrets from the bootloader to Linux, outside
of any virtualization or confidential compute context (at least a far as
I have understood the meaning of the term). Again, this feels like it
would be exposed to userspace in the same way.
It would be good to be able to share the parts that would be common. I
expect that would mean the operations for a secret file and for a
directory of secrets at a minimum. But it might also influence the paths
in securityfs; I see, looking back, that the "coco" directory was added
since the RFC but would a generalized "secret" subsystem make sense? Or
would it be preferable for each case to define their own path?
[1] -- https://lwn.net/Articles/836693/
> +static int sev_secret_unlink(struct inode *dir, struct dentry *dentry)
> +{
> + struct sev_secret *s = sev_secret_get();
> + struct inode *inode = d_inode(dentry);
> + struct secret_entry *e = (struct secret_entry *)inode->i_private;
> + int i;
> +
> + if (e) {
> + /* Zero out the secret data */
> + memzero_explicit(e->data, secret_entry_data_len(e));
Would there be a benefit in flushing these zeros?
> + e->guid = NULL_GUID;
> + }
> +
> + inode->i_private = NULL;
> +
> + for (i = 0; i < SEV_SECRET_NUM_FILES; i++)
> + if (s->fs_files[i] == dentry)
> + s->fs_files[i] = NULL;
> +
> + /*
> + * securityfs_remove tries to lock the directory's inode, but we reach
> + * the unlink callback when it's already locked
> + */
> + inode_unlock(dir);
> + securityfs_remove(dentry);
> + inode_lock(dir);
> +
> + return 0;
> +}
^ permalink raw reply
* Re: [PATCH 1/1] NAX LSM: Add initial support support
From: THOBY Simon @ 2021-08-13 8:23 UTC (permalink / raw)
To: Igor Zhbanov
Cc: Igor Zhbanov, linux-integrity, linux-security-module, Mimi Zohar
In-Reply-To: <CAEUiM9MCWyRTZyuV1KGDamib34Z0r_vJUWasVGb8wLFH04ynqQ@mail.gmail.com>
Hi Igor,
On 8/13/21 10:05 AM, Igor Zhbanov wrote:
> Hi Simon,
>
>> Yes, what I meant was that maybe you could just declare it at the beginning of the function,
>> and not use it at all in the sysctl table. Because as I see it, you only use allowed_caps_hex in the sysctl
>> table to copy the string to that temporary (variable), and its use is limited to that one function.
>>
>> Instead of:
>>
>> + if ((error = proc_dostring(table, write, buffer, lenp, ppos)))
>> + return error;
> ...
>> You could probably get away with something like:
> ...
>> + strncpy(allowed_caps_hex, buffer, ALLOWED_CAPS_HEX_LEN + 1);
>
> proc_dostring() is more than simple strncpy(). It is handling offsets too.
> I.e. if a user will try to write not from the starting position. But
> I've seen that some
> functions simply create an instance of struct ctl_table, fill it and
> call needed function.
Oh you're right, I assumed the sysctls write always had to be written from position zero,
but I just learned of 'sysctl_writes_strict': even though by default the kernel forbid
writes at another offset than zero or partial writes on sysctl files, users can enable
a more permissive behavior like 'SYSCTL_WRITES_LEGACY'.
Sorry about that.
>
> Thanks.
>
Thanks,
Simon
^ permalink raw reply
* Re: [PATCH 1/1] NAX LSM: Add initial support support
From: THOBY Simon @ 2021-08-13 8:08 UTC (permalink / raw)
To: Igor Zhbanov
Cc: Igor Zhbanov, linux-integrity, linux-security-module, Mimi Zohar
In-Reply-To: <CAEUiM9ObZD=miina1NsP8Ohtv=byO=33Kp2XaeF8_RB_R_uC1Q@mail.gmail.com>
Hi Igor,
On 8/12/21 6:43 PM, Igor Zhbanov wrote:
> Hi Simon,
>
> Thanks for thorough review. Will post the corrected version soon.
>
>>> @@ -278,11 +279,11 @@ endchoice
>>>
>>> config LSM
>>> string "Ordered list of enabled LSMs"
>>> - default "landlock,lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
>>> - default "landlock,lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
>>> - default "landlock,lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
>>> - default "landlock,lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
>>> - default "landlock,lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"
>>> + default "nax,landlock,lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
>>> + default "nax,landlock,lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
>>> + default "nax,landlock,lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
>>> + default "nax,landlock,lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
>>> + default "nax,landlock,lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"
>>
>> I don't know the policy with regard to new LSMs, but enabling this one by default is maybe a bit violent?
>> That said, considering the default mode is SECURITY_NAX_MODE_LOG, this would just log kernel messages and
>> not break existing systems, so this shouldn't be a problem.
>
> I've just oriended on landlock and lockdown. They are handled in the similar
> way. But, yes, by default NAX module doesn't block anything.
> If you suggest not to do that, I can remove it.
If other LSMs are also enabled by default when added to the kernel, and keeping the idea that the default behavior
is warning-only (warning in a rate-limited fashion, as you rightfully did, so people running JIT engines as root
don't get their kernel log flooded with warnings), then I have no objection to that change.
>
>>> +__setup("nax_mode=", setup_mode);
>>> +
>>> +static int __init setup_quiet(char *str)
>>> +{
>>> + unsigned long val;
>>> +
>>> + if (!locked && !kstrtoul(str, 0, &val))
>>> + quiet = val ? 1 : 0;
>>
>> The order of the kernel parameters will have an impact on NAX behavior.
>>
>> "nax_mode=1 nax_locked=1" and "nax_locked=1 nax_mode=1" will end up with the same behavior.
>> in the first case the mode is enforced, in the second case it isn't (well unless you changed
>> the kernel configuration from the default) and it can't be enabled later either.
>>
>> Is that desired?
>
> Yes. The idea is that on boot you can set-up the proper options then block
> further changing (especially turning the module off).
> Initially it was implemented for sysctl parameters, so, you can change
> something in init-scripts, then lock. Then, I've extended it to command-line
> parameters.
> I can ignore "locked" flag in setup_* functions to defer locking to sysctl
> parsing. (Unless our command-line is glued by the bootloader from several
> parts, so we want to lock it as early as possible...).
>
I may have badly made my point (especially considering I made a lot of typos when writing
that mail, for which I would like to apologize).
My sentence
"nax_mode=1 nax_locked=1" and "nax_locked=1 nax_mode=1" will end up with the same behavior.
lacked the "not" word, and both options will NOT have the same behavior.
What I wanted to say was that I think both parameter should have the same behavior, and that
the ordering of the options shouldn't impact the end result, because order-dependent options
may confuse users.
For the matter of have a kernel commandline being the result of concatenations from multiple
sources, I think that if any attacker is able to alter part of the command line, they can
already write 'lsm=' to it and completely disable NAX, thus I'm not sure 'nax_locked' should
impact other setup_* functions.
I believe keeping the nax_locked parameter, but not checking for the 'locked' status in the other setup_*
functions should be enought, as sysctls writes will still be protected by the 'locked' variable.
> Thanks.
>
Thanks,
Simon
^ permalink raw reply
* Re: [PATCH 1/1] NAX LSM: Add initial support support
From: Igor Zhbanov @ 2021-08-13 8:05 UTC (permalink / raw)
To: THOBY Simon
Cc: Igor Zhbanov, linux-integrity, linux-security-module, Mimi Zohar
In-Reply-To: <7642c670-55d9-9c30-40a9-15aba27503da@viveris.fr>
Hi Simon,
> Yes, what I meant was that maybe you could just declare it at the beginning of the function,
> and not use it at all in the sysctl table. Because as I see it, you only use allowed_caps_hex in the sysctl
> table to copy the string to that temporary (variable), and its use is limited to that one function.
>
> Instead of:
>
> + if ((error = proc_dostring(table, write, buffer, lenp, ppos)))
> + return error;
...
> You could probably get away with something like:
...
>+ strncpy(allowed_caps_hex, buffer, ALLOWED_CAPS_HEX_LEN + 1);
proc_dostring() is more than simple strncpy(). It is handling offsets too.
I.e. if a user will try to write not from the starting position. But
I've seen that some
functions simply create an instance of struct ctl_table, fill it and
call needed function.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/1] NAX LSM: Add initial support support
From: THOBY Simon @ 2021-08-13 7:47 UTC (permalink / raw)
To: Igor Zhbanov
Cc: Igor Zhbanov, linux-integrity, linux-security-module, Mimi Zohar
In-Reply-To: <CAEUiM9PRv+WhALQb-1im2_oZzAOvWzrMFrgn5xX9sU1K6DJ6+w@mail.gmail.com>
Hi Igor,
On 8/12/21 10:24 PM, Igor Zhbanov wrote:
> Hi Simon,
>
> ср, 28 июл. 2021 г. в 13:19, THOBY Simon <Simon.THOBY@viveris.fr>:
>> Nit: considering that allowed_caps_hex is only used in this function, and that it represents a small amount of
>> stack space, couldn't you define it directly in the function?
>>
>>> + len = strlen(allowed_caps_hex);
>
> It is also used as sysctl parameter input buffer in nax_sysctl_table[]
> for "allowed_caps" parameter.
>
Yes, what I meant was that maybe you could just declare it at the beginning of the function,
and not use it at all in the sysctl table. Because as I see it, you only use allowed_caps_hex in the sysctl
table to copy the string to that temporary (variable), and its use is limited to that one function.
Instead of:
+ if ((error = proc_dostring(table, write, buffer, lenp, ppos)))
+ return error;
You could probably get away with something like:
> +static int
> +nax_dostring(struct ctl_table *table, int write, void *buffer,
> + size_t *lenp, loff_t *ppos)
> +{
+ int error;
+ char allowed_caps_hex[ALLOWED_CAPS_HEX_LEN + 1];
[...]
+ len = strlen(allowed_caps_hex);
+ if (len > ALLOWED_CAPS_HEX_LEN)
+ return -EINVAL;
+ strncpy(allowed_caps_hex, buffer, ALLOWED_CAPS_HEX_LEN + 1);
I have to admit that having nearly no kernel development experience, I'm not
sure there is any guidance on the matter (maybe there is even guidance that recommend
preferring what your original patch does to what I suggested) but I think it is
unnecessary to have a variable accessible to the whole file but being used to hold (small)
temporary data for a single function.
In any case, I would appreciate comments for other reviewers if what I'm saying is completely
wrong, so as to not mislead you in doing adverse changes.
Thanks,
Simon
^ permalink raw reply
* Re: [PATCH v3 01/14] integrity: Introduce a Linux keyring for the Machine Owner Key (MOK)
From: Mimi Zohar @ 2021-08-13 0:35 UTC (permalink / raw)
To: Eric Snowberg
Cc: keyrings, linux-integrity, David Howells, David Woodhouse,
Herbert Xu, David S . Miller, Jarkko Sakkinen, James Morris,
Serge E . Hallyn, keescook, gregkh, torvalds, scott.branden,
weiyongjun1, nayna, ebiggers, ardb, Lakshmi Ramasubramanian,
lszubowi, linux-kernel, linux-crypto, linux-security-module,
James Bottomley, pjones, konrad.wilk@oracle.com
In-Reply-To: <915366E7-0B86-4F38-8AFD-EDA5FC1916D5@oracle.com>
On Thu, 2021-08-12 at 16:36 -0600, Eric Snowberg wrote:
> > On Aug 12, 2021, at 3:31 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Wed, 2021-08-11 at 22:18 -0400, Eric Snowberg wrote:
> >> Many UEFI Linux distributions boot using shim. The UEFI shim provides
> >> what is called Machine Owner Keys (MOK). Shim uses both the UEFI Secure
> >> Boot DB and MOK keys to validate the next step in the boot chain. The
> >> MOK facility can be used to import user generated keys. These keys can
> >> be used to sign an end-users development kernel build. When Linux
> >> boots, both UEFI Secure Boot DB and MOK keys get loaded in the Linux
> >> .platform keyring.
> >>
> >> Add a new Linux keyring called .mok. This keyring shall contain just
> >> MOK keys and not the remaining keys in the platform keyring. This new
> >> .mok keyring will be used in follow on patches. Unlike keys in the
> >> platform keyring, keys contained in the .mok keyring will be trusted
> >> within the kernel if the end-user has chosen to do so.
> >>
> >> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> >> ---
> >> v1: Initial version
> >> v2: Removed destory keyring code
> >> v3: Unmodified from v2
> >> ---
> >> security/integrity/Makefile | 3 ++-
> >> security/integrity/digsig.c | 1 +
> >> security/integrity/integrity.h | 3 ++-
> >> .../integrity/platform_certs/mok_keyring.c | 21 +++++++++++++++++++
> >> 4 files changed, 26 insertions(+), 2 deletions(-)
> >> create mode 100644 security/integrity/platform_certs/mok_keyring.c
> >>
> >> diff --git a/security/integrity/Makefile b/security/integrity/Makefile
> >> index 7ee39d66cf16..8e2e98cba1f6 100644
> >> --- a/security/integrity/Makefile
> >> +++ b/security/integrity/Makefile
> >> @@ -9,7 +9,8 @@ integrity-y := iint.o
> >> integrity-$(CONFIG_INTEGRITY_AUDIT) += integrity_audit.o
> >> integrity-$(CONFIG_INTEGRITY_SIGNATURE) += digsig.o
> >> integrity-$(CONFIG_INTEGRITY_ASYMMETRIC_KEYS) += digsig_asymmetric.o
> >> -integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += platform_certs/platform_keyring.o
> >> +integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += platform_certs/platform_keyring.o \
> >> + platform_certs/mok_keyring.o
> >> integrity-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/efi_parser.o \
> >> platform_certs/load_uefi.o \
> >> platform_certs/keyring_handler.o
> >> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> >> index 3b06a01bd0fd..e07334504ef1 100644
> >> --- a/security/integrity/digsig.c
> >> +++ b/security/integrity/digsig.c
> >> @@ -30,6 +30,7 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
> >> ".ima",
> >> #endif
> >> ".platform",
> >> + ".mok",
> >> };
> >>
> >> #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> >> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> >> index 547425c20e11..e0e17ccba2e6 100644
> >> --- a/security/integrity/integrity.h
> >> +++ b/security/integrity/integrity.h
> >> @@ -151,7 +151,8 @@ int integrity_kernel_read(struct file *file, loff_t offset,
> >> #define INTEGRITY_KEYRING_EVM 0
> >> #define INTEGRITY_KEYRING_IMA 1
> >> #define INTEGRITY_KEYRING_PLATFORM 2
> >> -#define INTEGRITY_KEYRING_MAX 3
> >> +#define INTEGRITY_KEYRING_MOK 3
> >> +#define INTEGRITY_KEYRING_MAX 4
> >>
> >> extern struct dentry *integrity_dir;
> >>
> >> diff --git a/security/integrity/platform_certs/mok_keyring.c b/security/integrity/platform_certs/mok_keyring.c
> >> new file mode 100644
> >> index 000000000000..b1ee45b77731
> >> --- /dev/null
> >> +++ b/security/integrity/platform_certs/mok_keyring.c
> >> @@ -0,0 +1,21 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * MOK keyring routines.
> >> + *
> >> + * Copyright (c) 2021, Oracle and/or its affiliates.
> >> + */
> >> +
> >> +#include "../integrity.h"
> >> +
> >> +static __init int mok_keyring_init(void)
> >> +{
> >> + int rc;
> >> +
> >> + rc = integrity_init_keyring(INTEGRITY_KEYRING_MOK);
> >> + if (rc)
> >> + return rc;
> >> +
> >> + pr_notice("MOK Keyring initialized\n");
> >> + return 0;
> >> +}
> >> +device_initcall(mok_keyring_init);
> >
> > The ordering of the patches in this patch set is not quite
> > right.
>
> I will work on reordering the patches in the next round.
>
> > Please first introduce the new keyring with the new Kconfig,
> > new restriction, and loading the keys onto the new keyring. Introduce
> > the builitin_secondary_and_ca_trusted restriction and linking the new
> > keyring to the secondary keyring. Only after everything is in place,
> > define and use the UEFI mok variable(s).
> >
> > Originally, I asked you to "Separate each **logical change** into a
> > separate patch." After re-ordering the patches, see if merging some of
> > them together now makes sense.
>
> I’ll see if merging some of them together makes sense.
>
> With the new Kconfig option, should the default be 'y' or ’n' when the secondary
> is defined? Thanks.
It definitely needs to be opt in. Please make it 'n'.
Mimi
^ permalink raw reply
* Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
From: Casey Schaufler @ 2021-08-12 22:38 UTC (permalink / raw)
To: Paul Moore
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley, linux-kernel, netdev, Casey Schaufler
In-Reply-To: <CAHC9VhTj2OJ7E6+iSBLNZaiPK-16UY0zSFJikpz+teef3JOosg@mail.gmail.com>
On 8/12/2021 1:59 PM, Paul Moore wrote:
> On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> Create a new audit record type to contain the subject information
>> when there are multiple security modules that require such data.
>> This record is linked with the same timestamp and serial number
>> using the audit_alloc_local() mechanism.
>> The record is produced only in cases where there is more than one
>> security module with a process "context".
>> In cases where this record is produced the subj= fields of
>> other records in the audit event will be set to "subj=?".
>>
>> An example of the MAC_TASK_CONTEXTS (1420) record is:
>>
>> type=UNKNOWN[1420]
>> msg=audit(1600880931.832:113)
>> subj_apparmor="=unconfined"
>> subj_smack="_"
>>
>> There will be a subj_$LSM= entry for each security module
>> LSM that supports the secid_to_secctx and secctx_to_secid
>> hooks. The BPF security module implements secid/secctx
>> translation hooks, so it has to be considered to provide a
>> secctx even though it may not actually do so.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> To: paul@paul-moore.com
>> To: linux-audit@redhat.com
>> To: rgb@redhat.com
>> Cc: netdev@vger.kernel.org
>> ---
>> drivers/android/binder.c | 2 +-
>> include/linux/audit.h | 16 +++++
>> include/linux/security.h | 16 ++++-
>> include/net/netlabel.h | 2 +-
>> include/net/scm.h | 2 +-
>> include/net/xfrm.h | 13 +++-
>> include/uapi/linux/audit.h | 1 +
>> kernel/audit.c | 90 +++++++++++++++++++------
>> kernel/auditfilter.c | 5 +-
>> kernel/auditsc.c | 27 ++++++--
>> net/ipv4/ip_sockglue.c | 2 +-
>> net/netfilter/nf_conntrack_netlink.c | 4 +-
>> net/netfilter/nf_conntrack_standalone.c | 2 +-
>> net/netfilter/nfnetlink_queue.c | 2 +-
>> net/netlabel/netlabel_unlabeled.c | 21 +++---
>> net/netlabel/netlabel_user.c | 14 ++--
>> net/netlabel/netlabel_user.h | 6 +-
>> net/xfrm/xfrm_policy.c | 8 ++-
>> net/xfrm/xfrm_state.c | 18 +++--
>> security/integrity/ima/ima_api.c | 6 +-
>> security/integrity/integrity_audit.c | 5 +-
>> security/security.c | 46 ++++++++-----
>> security/smack/smackfs.c | 3 +-
>> 23 files changed, 221 insertions(+), 90 deletions(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index 2c3a2348a144..3520caa0260c 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -2722,7 +2722,7 @@ static void binder_transaction(struct binder_proc *proc,
>> * case well anyway.
>> */
>> security_task_getsecid_obj(proc->tsk, &blob);
>> - ret = security_secid_to_secctx(&blob, &lsmctx);
>> + ret = security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_DISPLAY);
>> if (ret) {
>> return_error = BR_FAILED_REPLY;
>> return_error_param = ret;
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index 97cd7471e572..85eb87f6f92d 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -291,6 +291,7 @@ extern int audit_alloc(struct task_struct *task);
>> extern void __audit_free(struct task_struct *task);
>> extern struct audit_context *audit_alloc_local(gfp_t gfpflags);
>> extern void audit_free_context(struct audit_context *context);
>> +extern void audit_free_local(struct audit_context *context);
>> extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
>> unsigned long a2, unsigned long a3);
>> extern void __audit_syscall_exit(int ret_success, long ret_value);
>> @@ -386,6 +387,19 @@ static inline void audit_ptrace(struct task_struct *t)
>> __audit_ptrace(t);
>> }
>>
>> +static inline struct audit_context *audit_alloc_for_lsm(gfp_t gfp)
>> +{
>> + struct audit_context *context = audit_context();
>> +
>> + if (context)
>> + return context;
>> +
>> + if (lsm_multiple_contexts())
>> + return audit_alloc_local(gfp);
>> +
>> + return NULL;
>> +}
> We don't want to do this, at least not as it is written above. We
> shouldn't have a function which abstracts away the creation of a local
> audit_context. Usage of a local audit_context should be explicit in
> the caller, and if the caller's execution context is ambiguous enough
> that it can require both a task_struct audit_context and a local
> audit_context then we need to handle that on a case-by-case basis.
> Hiding it like this is guaranteed to cause problems later.
OK. Please understand that *every case* where I've used audit_alloc_for_lsm()
is a case where I have *verified* that context may be NULL. I will make
the change.
> I probably did a poor job of explaining what a local context is during
> the last patchset; I'll try to do a better job here but also let me
> say this as clear as I can ... if the "current" task struct is valid
> for a given code path, *never* use a local audit context.
I probably did a poor job of demonstrating that I never use a local
context where there's a valid current context.
> The local
> audit context is a hack that is made necessary by the fact that we
> have to audit things which happen outside the scope of an executing
> task, e.g. the netfilter audit hooks, it should *never* be used when
> there is a valid task_struct.
In the existing audit code a "current context" is only needed for
syscall events, so that's the only case where it's allocated. Would
you suggest that I track down the non-syscall events that include
subj= fields and add allocate a "current context" for them? I looked
into doing that, and it wouldn't be simple.
> It's the audit_context which helps bind multiple audit records into a
> single event, creating a new, "local" audit_context destroys that
> binding
... if there's a current context. There often isn't. That's why I'm
using a local context. There is not a "current" context available.
> as audit records created using that local audit_context have a
> different timestamp from those records created using the current
> task_struct's audit_context.
(Weeps) I only introduce a local context where there is no current
context available, so this is never a problem.
> Hopefully that makes sense?
Yes, it makes sense. Methinks you may believe that the current context
is available more regularly than it actually is.
I instrumented the audit event functions with:
WARN_ONCE(audit_context, "%s has context\n", __func__);
WARN_ONCE(!audit_context, "%s lacks context\n", __func__);
I only used local contexts where the 2nd WARN_ONCE was hit.
>> /* Private API (for audit.c only) */
>> extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
>> extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, umode_t mode);
>> @@ -560,6 +574,8 @@ extern int audit_signals;
>> }
>> static inline void audit_free_context(struct audit_context *context)
>> { }
>> +static inline void audit_free_local(struct audit_context *context)
>> +{ }
>> static inline int audit_alloc(struct task_struct *task)
>> {
>> return 0;
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 3e9743118fb9..b3cf68cf2bd6 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -182,6 +182,8 @@ struct lsmblob {
>> #define LSMBLOB_INVALID -1 /* Not a valid LSM slot number */
>> #define LSMBLOB_NEEDED -2 /* Slot requested on initialization */
>> #define LSMBLOB_NOT_NEEDED -3 /* Slot not requested */
>> +#define LSMBLOB_DISPLAY -4 /* Use the "display" slot */
>> +#define LSMBLOB_FIRST -5 /* Use the default "display" slot */
>>
>> /**
>> * lsmblob_init - initialize an lsmblob structure
>> @@ -248,6 +250,15 @@ static inline u32 lsmblob_value(const struct lsmblob *blob)
>> return 0;
>> }
>>
>> +static inline bool lsm_multiple_contexts(void)
>> +{
>> +#ifdef CONFIG_SECURITY
>> + return lsm_slot_to_name(1) != NULL;
>> +#else
>> + return false;
>> +#endif
>> +}
>> +
>> /* These functions are in security/commoncap.c */
>> extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
>> int cap, unsigned int opts);
>> @@ -578,7 +589,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>> size_t size);
>> int security_netlink_send(struct sock *sk, struct sk_buff *skb);
>> int security_ismaclabel(const char *name);
>> -int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp);
>> +int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp,
>> + int display);
>> int security_secctx_to_secid(const char *secdata, u32 seclen,
>> struct lsmblob *blob);
>> void security_release_secctx(struct lsmcontext *cp);
>> @@ -1433,7 +1445,7 @@ static inline int security_ismaclabel(const char *name)
>> }
>>
>> static inline int security_secid_to_secctx(struct lsmblob *blob,
>> - struct lsmcontext *cp)
>> + struct lsmcontext *cp, int display)
>> {
>> return -EOPNOTSUPP;
>> }
>> diff --git a/include/net/netlabel.h b/include/net/netlabel.h
>> index 73fc25b4042b..216cb1ffc8f0 100644
>> --- a/include/net/netlabel.h
>> +++ b/include/net/netlabel.h
>> @@ -97,7 +97,7 @@ struct calipso_doi;
>>
>> /* NetLabel audit information */
>> struct netlbl_audit {
>> - u32 secid;
>> + struct lsmblob lsmdata;
>> kuid_t loginuid;
>> unsigned int sessionid;
>> };
> This chunk seems lost here, does it belong in another patch?
Probably. I am getting a touch of patch-rot showing up.
>> diff --git a/include/net/scm.h b/include/net/scm.h
>> index b77a52f93389..f4d567d4885e 100644
>> --- a/include/net/scm.h
>> +++ b/include/net/scm.h
>> @@ -101,7 +101,7 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
>> * and the infrastructure will know which it is.
>> */
>> lsmblob_init(&lb, scm->secid);
>> - err = security_secid_to_secctx(&lb, &context);
>> + err = security_secid_to_secctx(&lb, &context, LSMBLOB_DISPLAY);
> Misplaced code change?
Same here.
>> if (!err) {
>> put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, context.len,
>> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
>> index cbff7c2a9724..a10fa01f7bf4 100644
>> --- a/include/net/xfrm.h
>> +++ b/include/net/xfrm.h
>> @@ -660,13 +660,22 @@ struct xfrm_spi_skb_cb {
>> #define XFRM_SPI_SKB_CB(__skb) ((struct xfrm_spi_skb_cb *)&((__skb)->cb[0]))
>>
>> #ifdef CONFIG_AUDITSYSCALL
>> -static inline struct audit_buffer *xfrm_audit_start(const char *op)
>> +static inline struct audit_buffer *xfrm_audit_start(const char *op,
>> + struct audit_context **lac)
>> {
>> + struct audit_context *context;
>> struct audit_buffer *audit_buf = NULL;
>>
>> if (audit_enabled == AUDIT_OFF)
>> return NULL;
>> - audit_buf = audit_log_start(audit_context(), GFP_ATOMIC,
>> + context = audit_context();
>> + if (lac != NULL) {
>> + if (lsm_multiple_contexts() && context == NULL)
>> + context = audit_alloc_local(GFP_ATOMIC);
>> + *lac = context;
>> + }
>> +
>> + audit_buf = audit_log_start(context, GFP_ATOMIC,
>> AUDIT_MAC_IPSEC_EVENT);
>> if (audit_buf == NULL)
>> return NULL;
> Related to the other comments around local audit_contexts, we don't
> want to do this; use the existing audit_context, @lac in this case, so
> that this audit record is bound to the other associated records into a
> single audit event (all have the same timestamp).
Hmm. This is clearly a problem. Looks like a change came in
that I didn't see.
>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index 841123390d41..cba63789a164 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -386,10 +386,12 @@ void audit_log_lost(const char *message)
>> static int audit_log_config_change(char *function_name, u32 new, u32 old,
>> int allow_changes)
>> {
>> + struct audit_context *context;
>> struct audit_buffer *ab;
>> int rc = 0;
>>
>> - ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
>> + context = audit_alloc_for_lsm(GFP_KERNEL);
>> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> We shouldn't need to do this for the reasons discussed up near the top
> of this email (and elsewhere as well).
Here and elsewhere, I only put audit_alloc_for_lsm() in because there
are cases where audit_context() returns NULL. Really.
>
> I'm going to refrain from commenting on the other uses of
> audit_alloc_for_lsm() in this patch unless there is something unique
> to the code path, so if you don't see me comment about a use of
> audit_alloc_for_lsm() you can assume it should be removed and the
> existing audit_context used instead.
>
>> @@ -399,6 +401,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
>> allow_changes = 0; /* Something weird, deny request */
>> audit_log_format(ab, " res=%d", allow_changes);
>> audit_log_end(ab);
>> + audit_free_local(context);
> See my comment directly above regarding usage of
> audit_alloc_for_lsm(), it obviously applies here too.
>
>> @@ -2128,6 +2136,36 @@ void audit_log_key(struct audit_buffer *ab, char *key)
>> audit_log_format(ab, "(null)");
>> }
>>
>> +static void audit_log_lsm(struct audit_context *context, struct lsmblob *blob)
> See my note below about moving this into audit_log_task_context(),
Either works for me. This seemed consistent with the rest of the audit
code, but I'm happy to change it if you like that better.
> but
> if we really need to keep this as a separate function, let's consider
> changing the name to something which indicates that it logs the LSM
> data as *subject* fields. How about audit_log_lsm_subj()?
>
>> +{
>> + struct audit_buffer *ab;
>> + struct lsmcontext lsmdata;
>> + bool sep = false;
>> + int error;
>> + int i;
>> +
>> + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_MAC_TASK_CONTEXTS);
>> + if (!ab)
>> + return; /* audit_panic or being filtered */
>> +
>> + for (i = 0; i < LSMBLOB_ENTRIES; i++) {
>> + if (blob->secid[i] == 0)
>> + continue;
>> + error = security_secid_to_secctx(blob, &lsmdata, i);
>> + if (error && error != -EINVAL) {
>> + audit_panic("error in audit_log_lsm");
>> + return;
>> + }
>> +
>> + audit_log_format(ab, "%ssubj_%s=\"%s\"", sep ? " " : "",
>> + lsm_slot_to_name(i), lsmdata.context);
>> + sep = true;
> Since @i starts at zero, you can get rid of @sep by replacing it with @i:
>
> audit_log_format(ab, ..., (i ? " " : ""), ...);
Clever.
>
>> + security_release_secctx(&lsmdata);
>> + }
>> + audit_log_end(ab);
>> +}
>> @@ -2138,7 +2176,18 @@ int audit_log_task_context(struct audit_buffer *ab)
>> if (!lsmblob_is_set(&blob))
>> return 0;
>>
>> - error = security_secid_to_secctx(&blob, &context);
>> + /*
>> + * If there is more than one security module that has a
>> + * subject "context" it's necessary to put the subject data
>> + * into a separate record to maintain compatibility.
>> + */
>> + if (lsm_multiple_contexts()) {
>> + audit_log_format(ab, " subj=?");
>> + audit_log_lsm(ab->ctx, &blob);
>> + return 0;
>> + }
>> +
>> + error = security_secid_to_secctx(&blob, &context, LSMBLOB_FIRST);
> Instead of the lsm_multiple_contexts() case bailing on the rest of the
> function with a return inside the if-block, let's made the code a bit
> more robust by organizing it like this:
Sure, why not?
>
> int audit_log_task_context(ab)
> {
> /* common stuff at the start */
>
> if (lsm_multiple_contexts()) {
> /* multi-LSM stuff */
> } else {
> /* single LSM stuff */
> }
>
> /* common stuff at the end */
> }
>
> ... it also may make sense to just move the body of audit_log_lsm()
> into audit_log_task_context() and do away with audit_log_lsm()
> entirely; is it ever going to be called from anywhere else?
>
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index 0e58a3ab56f5..01fdcbf468c0 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -993,12 +993,11 @@ struct audit_context *audit_alloc_local(gfp_t gfpflags)
>> context = audit_alloc_context(AUDIT_STATE_BUILD, gfpflags);
>> if (!context) {
>> audit_log_lost("out of memory in audit_alloc_local");
>> - goto out;
>> + return NULL;
>> }
>> context->serial = audit_serial();
>> ktime_get_coarse_real_ts64(&context->ctime);
>> context->local = true;
>> -out:
>> return context;
>> }
> This chunk should be moved to 21/25 when audit_alloc_local() is first defined.
True. I was trying to minimize the change to the original audit_alloc_local()
patch on the assumption that it was coming in for other reasons, but that hasn't
happened.
>> @@ -1019,6 +1018,13 @@ void audit_free_context(struct audit_context *context)
>> }
>> EXPORT_SYMBOL(audit_free_context);
>>
>> +void audit_free_local(struct audit_context *context)
>> +{
>> + if (context && context->local)
>> + audit_free_context(context);
>> +}
>> +EXPORT_SYMBOL(audit_free_local);
> If this is strictly necessary, and I don't think it is, it should also
> be moved to patch 21/25 with the original definition of a local
> audit_context. However, there really should be no reason why we have
> to distinguish between a proper and local audtit_context when it comes
> to free'ing the memory, just call audit_free_context() in both cases.
>
>> @@ -1036,7 +1042,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
>> from_kuid(&init_user_ns, auid),
>> from_kuid(&init_user_ns, uid), sessionid);
>> if (lsmblob_is_set(blob)) {
>> - if (security_secid_to_secctx(blob, &lsmctx)) {
>> + if (security_secid_to_secctx(blob, &lsmctx, LSMBLOB_FIRST)) {
> Misplaced code change?
>
> Actually, there are a lot of these below, I'm not going to comment on
> all of them as I think you get the idea ... and I very well may be
> wrong so I'll save you all of my wrongness in that case :)
>
>> diff --git a/security/security.c b/security/security.c
>> index cb359e185d1a..5d7fd982f84a 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2309,7 +2309,7 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>> hlist_for_each_entry(hp, &security_hook_heads.setprocattr,
>> list) {
>> rc = hp->hook.setprocattr(name, value, size);
>> - if (rc < 0)
>> + if (rc < 0 && rc != -EINVAL)
>> return rc;
>> }
> This really looks misplaced ... ?
Yeah, you're not the first person to notice these bits of patch-rot.
I have some clean-up to do.
Thank you for the review.
^ permalink raw reply
* Re: [PATCH v3 01/14] integrity: Introduce a Linux keyring for the Machine Owner Key (MOK)
From: Eric Snowberg @ 2021-08-12 22:36 UTC (permalink / raw)
To: Mimi Zohar
Cc: keyrings, linux-integrity, David Howells, David Woodhouse,
Herbert Xu, David S . Miller, Jarkko Sakkinen, James Morris,
Serge E . Hallyn, keescook, gregkh, torvalds, scott.branden,
weiyongjun1, nayna, ebiggers, ardb, Lakshmi Ramasubramanian,
lszubowi, linux-kernel, linux-crypto, linux-security-module,
James Bottomley, pjones, konrad.wilk@oracle.com
In-Reply-To: <34b12d8d47564a182f0a29a9592e203b17ccdd69.camel@linux.ibm.com>
> On Aug 12, 2021, at 3:31 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Wed, 2021-08-11 at 22:18 -0400, Eric Snowberg wrote:
>> Many UEFI Linux distributions boot using shim. The UEFI shim provides
>> what is called Machine Owner Keys (MOK). Shim uses both the UEFI Secure
>> Boot DB and MOK keys to validate the next step in the boot chain. The
>> MOK facility can be used to import user generated keys. These keys can
>> be used to sign an end-users development kernel build. When Linux
>> boots, both UEFI Secure Boot DB and MOK keys get loaded in the Linux
>> .platform keyring.
>>
>> Add a new Linux keyring called .mok. This keyring shall contain just
>> MOK keys and not the remaining keys in the platform keyring. This new
>> .mok keyring will be used in follow on patches. Unlike keys in the
>> platform keyring, keys contained in the .mok keyring will be trusted
>> within the kernel if the end-user has chosen to do so.
>>
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>> ---
>> v1: Initial version
>> v2: Removed destory keyring code
>> v3: Unmodified from v2
>> ---
>> security/integrity/Makefile | 3 ++-
>> security/integrity/digsig.c | 1 +
>> security/integrity/integrity.h | 3 ++-
>> .../integrity/platform_certs/mok_keyring.c | 21 +++++++++++++++++++
>> 4 files changed, 26 insertions(+), 2 deletions(-)
>> create mode 100644 security/integrity/platform_certs/mok_keyring.c
>>
>> diff --git a/security/integrity/Makefile b/security/integrity/Makefile
>> index 7ee39d66cf16..8e2e98cba1f6 100644
>> --- a/security/integrity/Makefile
>> +++ b/security/integrity/Makefile
>> @@ -9,7 +9,8 @@ integrity-y := iint.o
>> integrity-$(CONFIG_INTEGRITY_AUDIT) += integrity_audit.o
>> integrity-$(CONFIG_INTEGRITY_SIGNATURE) += digsig.o
>> integrity-$(CONFIG_INTEGRITY_ASYMMETRIC_KEYS) += digsig_asymmetric.o
>> -integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += platform_certs/platform_keyring.o
>> +integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += platform_certs/platform_keyring.o \
>> + platform_certs/mok_keyring.o
>> integrity-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/efi_parser.o \
>> platform_certs/load_uefi.o \
>> platform_certs/keyring_handler.o
>> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
>> index 3b06a01bd0fd..e07334504ef1 100644
>> --- a/security/integrity/digsig.c
>> +++ b/security/integrity/digsig.c
>> @@ -30,6 +30,7 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
>> ".ima",
>> #endif
>> ".platform",
>> + ".mok",
>> };
>>
>> #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
>> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
>> index 547425c20e11..e0e17ccba2e6 100644
>> --- a/security/integrity/integrity.h
>> +++ b/security/integrity/integrity.h
>> @@ -151,7 +151,8 @@ int integrity_kernel_read(struct file *file, loff_t offset,
>> #define INTEGRITY_KEYRING_EVM 0
>> #define INTEGRITY_KEYRING_IMA 1
>> #define INTEGRITY_KEYRING_PLATFORM 2
>> -#define INTEGRITY_KEYRING_MAX 3
>> +#define INTEGRITY_KEYRING_MOK 3
>> +#define INTEGRITY_KEYRING_MAX 4
>>
>> extern struct dentry *integrity_dir;
>>
>> diff --git a/security/integrity/platform_certs/mok_keyring.c b/security/integrity/platform_certs/mok_keyring.c
>> new file mode 100644
>> index 000000000000..b1ee45b77731
>> --- /dev/null
>> +++ b/security/integrity/platform_certs/mok_keyring.c
>> @@ -0,0 +1,21 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * MOK keyring routines.
>> + *
>> + * Copyright (c) 2021, Oracle and/or its affiliates.
>> + */
>> +
>> +#include "../integrity.h"
>> +
>> +static __init int mok_keyring_init(void)
>> +{
>> + int rc;
>> +
>> + rc = integrity_init_keyring(INTEGRITY_KEYRING_MOK);
>> + if (rc)
>> + return rc;
>> +
>> + pr_notice("MOK Keyring initialized\n");
>> + return 0;
>> +}
>> +device_initcall(mok_keyring_init);
>
> The ordering of the patches in this patch set is not quite
> right.
I will work on reordering the patches in the next round.
> Please first introduce the new keyring with the new Kconfig,
> new restriction, and loading the keys onto the new keyring. Introduce
> the builitin_secondary_and_ca_trusted restriction and linking the new
> keyring to the secondary keyring. Only after everything is in place,
> define and use the UEFI mok variable(s).
>
> Originally, I asked you to "Separate each **logical change** into a
> separate patch." After re-ordering the patches, see if merging some of
> them together now makes sense.
I’ll see if merging some of them together makes sense.
With the new Kconfig option, should the default be 'y' or ’n' when the secondary
is defined? Thanks.
^ permalink raw reply
* Re: [PATCH v3 01/14] integrity: Introduce a Linux keyring for the Machine Owner Key (MOK)
From: Eric Snowberg @ 2021-08-12 22:16 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: keyrings, linux-integrity, Mimi Zohar, David Howells,
David Woodhouse, Herbert Xu, David S . Miller, James Morris,
Serge E . Hallyn, keescook, gregkh, torvalds, scott.branden,
weiyongjun1, nayna, ebiggers, ardb, Lakshmi Ramasubramanian,
lszubowi, linux-kernel, linux-crypto, linux-security-module,
James.Bottomley, pjones, konrad.wilk@oracle.com
In-Reply-To: <20210812185853.p5mgsgrftgwvt5fx@kernel.org>
> On Aug 12, 2021, at 12:58 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Wed, Aug 11, 2021 at 10:18:42PM -0400, Eric Snowberg wrote:
>> Many UEFI Linux distributions boot using shim. The UEFI shim provides
>> what is called Machine Owner Keys (MOK). Shim uses both the UEFI Secure
>> Boot DB and MOK keys to validate the next step in the boot chain. The
>> MOK facility can be used to import user generated keys. These keys can
>> be used to sign an end-users development kernel build. When Linux
>> boots, both UEFI Secure Boot DB and MOK keys get loaded in the Linux
>> .platform keyring.
>>
>> Add a new Linux keyring called .mok. This keyring shall contain just
>
> I would consider ".machine" instead. It holds MOK keys but is not a
> MOK key.
I’m open to renaming it to anything that you and the other maintainers
feel would be appropriate. I just want to make sure there is an agreement
on the new name before I make the change. Thanks.
^ permalink raw reply
* Re: [PATCH v3 10/14] KEYS: change link restriction for secondary to also trust mok
From: Mimi Zohar @ 2021-08-12 22:14 UTC (permalink / raw)
To: Eric Snowberg
Cc: keyrings, linux-integrity, David Howells, David Woodhouse,
Herbert Xu, David S . Miller, Jarkko Sakkinen, James Morris,
Serge E . Hallyn, keescook, gregkh, torvalds, scott.branden,
weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
linux-kernel, linux-crypto, linux-security-module,
James.Bottomley, pjones, glin, konrad.wilk@oracle.com
In-Reply-To: <F792F96B-C482-4F28-A984-A6B5F49C3A56@oracle.com>
On Thu, 2021-08-12 at 16:10 -0600, Eric Snowberg wrote:
> > On Aug 12, 2021, at 1:46 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Wed, 2021-08-11 at 22:18 -0400, Eric Snowberg wrote:
> >> With the introduction of the mok keyring, the end-user may choose to
> >> trust Machine Owner Keys (MOK) within the kernel. If they have chosen to
> >> trust them, the .mok keyring will contain these keys. If not, the mok
> >> keyring will always be empty. Update the restriction check to allow the
> >> secondary trusted keyring to also trust mok keys.
> >>
> >> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> >> ---
> >> v3: Initial version
> >> ---
> >> certs/system_keyring.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> >> index cb773e09ea67..8cc19a1ff051 100644
> >> --- a/certs/system_keyring.c
> >> +++ b/certs/system_keyring.c
> >> @@ -110,7 +110,7 @@ static __init struct key_restriction *get_builtin_and_secondary_restriction(void
> >> if (!restriction)
> >> panic("Can't allocate secondary trusted keyring restriction\n");
> >>
> >> - restriction->check = restrict_link_by_builtin_and_secondary_trusted;
> >> + restriction->check = restrict_link_by_builtin_secondary_and_ca_trusted;
> >>
> >> return restriction;
> >> }
> >
> > Not everyone needs to build a generic kernel, like the distros. As
> > previously discussed, not everyone is willing to trust the new MOK
> > keyring nor the UEFI variable for enabling it. For those environments,
> > they should be able to totally disable the MOK keyring.
> >
> > Please define a Kconfig similar to "CONFIG_SECONDARY_TRUSTED_KEYRING"
> > for MOK. The "restriction" would be based on the new Kconfig being
> > enabled.
>
> Yes, I can add that. Currently there is a Kconfig to enable the secondary
> and another for IMA to trust the secondary. Would you like to see two new
> Kconfig options added? One that allows the secondary to use the mok as a new
> trust source and another for IMA to trust the mok keyring. Or a single Kconfig
> that handles both? Thanks.
A single Kconfig option for enabling the new keyring should be fine.
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v3 10/14] KEYS: change link restriction for secondary to also trust mok
From: Eric Snowberg @ 2021-08-12 22:10 UTC (permalink / raw)
To: Mimi Zohar
Cc: keyrings, linux-integrity, David Howells, David Woodhouse,
Herbert Xu, David S . Miller, Jarkko Sakkinen, James Morris,
Serge E . Hallyn, keescook, gregkh, torvalds, scott.branden,
weiyongjun1, nayna, ebiggers, ardb, nramas, lszubowi,
linux-kernel, linux-crypto, linux-security-module,
James.Bottomley, pjones, glin, konrad.wilk@oracle.com
In-Reply-To: <58e6e03b69b8fadfb854669086020c633837be88.camel@linux.ibm.com>
> On Aug 12, 2021, at 1:46 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> Hi Eric,
>
> On Wed, 2021-08-11 at 22:18 -0400, Eric Snowberg wrote:
>> With the introduction of the mok keyring, the end-user may choose to
>> trust Machine Owner Keys (MOK) within the kernel. If they have chosen to
>> trust them, the .mok keyring will contain these keys. If not, the mok
>> keyring will always be empty. Update the restriction check to allow the
>> secondary trusted keyring to also trust mok keys.
>>
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>> ---
>> v3: Initial version
>> ---
>> certs/system_keyring.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
>> index cb773e09ea67..8cc19a1ff051 100644
>> --- a/certs/system_keyring.c
>> +++ b/certs/system_keyring.c
>> @@ -110,7 +110,7 @@ static __init struct key_restriction *get_builtin_and_secondary_restriction(void
>> if (!restriction)
>> panic("Can't allocate secondary trusted keyring restriction\n");
>>
>> - restriction->check = restrict_link_by_builtin_and_secondary_trusted;
>> + restriction->check = restrict_link_by_builtin_secondary_and_ca_trusted;
>>
>> return restriction;
>> }
>
> Not everyone needs to build a generic kernel, like the distros. As
> previously discussed, not everyone is willing to trust the new MOK
> keyring nor the UEFI variable for enabling it. For those environments,
> they should be able to totally disable the MOK keyring.
>
> Please define a Kconfig similar to "CONFIG_SECONDARY_TRUSTED_KEYRING"
> for MOK. The "restriction" would be based on the new Kconfig being
> enabled.
Yes, I can add that. Currently there is a Kconfig to enable the secondary
and another for IMA to trust the secondary. Would you like to see two new
Kconfig options added? One that allows the secondary to use the mok as a new
trust source and another for IMA to trust the mok keyring. Or a single Kconfig
that handles both? Thanks.
^ permalink raw reply
* Re: [PATCH v3 04/14] integrity: add add_to_mok_keyring
From: Eric Snowberg @ 2021-08-12 22:04 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: keyrings, linux-integrity, Mimi Zohar, David Howells,
David Woodhouse, Herbert Xu, David S . Miller, James Morris,
Serge E . Hallyn, keescook, gregkh, torvalds, scott.branden,
weiyongjun1, nayna, ebiggers, ardb, Lakshmi Ramasubramanian,
lszubowi, linux-kernel, linux-crypto, linux-security-module,
James.Bottomley, pjones, glin, konrad.wilk@oracle.com
In-Reply-To: <20210812193245.yev4gyeuxrfwqfty@kernel.org>
> On Aug 12, 2021, at 1:32 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Wed, Aug 11, 2021 at 10:18:45PM -0400, Eric Snowberg wrote:
>> Add the ability to load Machine Owner Key (MOK) keys to the mok keyring.
>> If the permissions do not allow the key to be added to the mok keyring
>> this is not an error, add it to the platform keyring instead.
>
> Should state why it isn't an error for clarity.
I’ll add that in the next round, thanks.
^ permalink raw reply
* Re: [PATCH v3 01/14] integrity: Introduce a Linux keyring for the Machine Owner Key (MOK)
From: Mimi Zohar @ 2021-08-12 21:31 UTC (permalink / raw)
To: Eric Snowberg, keyrings, linux-integrity, dhowells, dwmw2,
herbert, davem, jarkko, jmorris, serge
Cc: keescook, gregkh, torvalds, scott.branden, weiyongjun1, nayna,
ebiggers, ardb, nramas, lszubowi, linux-kernel, linux-crypto,
linux-security-module, James.Bottomley, pjones, glin, konrad.wilk
In-Reply-To: <20210812021855.3083178-2-eric.snowberg@oracle.com>
Hi Eric,
On Wed, 2021-08-11 at 22:18 -0400, Eric Snowberg wrote:
> Many UEFI Linux distributions boot using shim. The UEFI shim provides
> what is called Machine Owner Keys (MOK). Shim uses both the UEFI Secure
> Boot DB and MOK keys to validate the next step in the boot chain. The
> MOK facility can be used to import user generated keys. These keys can
> be used to sign an end-users development kernel build. When Linux
> boots, both UEFI Secure Boot DB and MOK keys get loaded in the Linux
> .platform keyring.
>
> Add a new Linux keyring called .mok. This keyring shall contain just
> MOK keys and not the remaining keys in the platform keyring. This new
> .mok keyring will be used in follow on patches. Unlike keys in the
> platform keyring, keys contained in the .mok keyring will be trusted
> within the kernel if the end-user has chosen to do so.
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
> v1: Initial version
> v2: Removed destory keyring code
> v3: Unmodified from v2
> ---
> security/integrity/Makefile | 3 ++-
> security/integrity/digsig.c | 1 +
> security/integrity/integrity.h | 3 ++-
> .../integrity/platform_certs/mok_keyring.c | 21 +++++++++++++++++++
> 4 files changed, 26 insertions(+), 2 deletions(-)
> create mode 100644 security/integrity/platform_certs/mok_keyring.c
>
> diff --git a/security/integrity/Makefile b/security/integrity/Makefile
> index 7ee39d66cf16..8e2e98cba1f6 100644
> --- a/security/integrity/Makefile
> +++ b/security/integrity/Makefile
> @@ -9,7 +9,8 @@ integrity-y := iint.o
> integrity-$(CONFIG_INTEGRITY_AUDIT) += integrity_audit.o
> integrity-$(CONFIG_INTEGRITY_SIGNATURE) += digsig.o
> integrity-$(CONFIG_INTEGRITY_ASYMMETRIC_KEYS) += digsig_asymmetric.o
> -integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += platform_certs/platform_keyring.o
> +integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += platform_certs/platform_keyring.o \
> + platform_certs/mok_keyring.o
> integrity-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/efi_parser.o \
> platform_certs/load_uefi.o \
> platform_certs/keyring_handler.o
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 3b06a01bd0fd..e07334504ef1 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -30,6 +30,7 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
> ".ima",
> #endif
> ".platform",
> + ".mok",
> };
>
> #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 547425c20e11..e0e17ccba2e6 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -151,7 +151,8 @@ int integrity_kernel_read(struct file *file, loff_t offset,
> #define INTEGRITY_KEYRING_EVM 0
> #define INTEGRITY_KEYRING_IMA 1
> #define INTEGRITY_KEYRING_PLATFORM 2
> -#define INTEGRITY_KEYRING_MAX 3
> +#define INTEGRITY_KEYRING_MOK 3
> +#define INTEGRITY_KEYRING_MAX 4
>
> extern struct dentry *integrity_dir;
>
> diff --git a/security/integrity/platform_certs/mok_keyring.c b/security/integrity/platform_certs/mok_keyring.c
> new file mode 100644
> index 000000000000..b1ee45b77731
> --- /dev/null
> +++ b/security/integrity/platform_certs/mok_keyring.c
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MOK keyring routines.
> + *
> + * Copyright (c) 2021, Oracle and/or its affiliates.
> + */
> +
> +#include "../integrity.h"
> +
> +static __init int mok_keyring_init(void)
> +{
> + int rc;
> +
> + rc = integrity_init_keyring(INTEGRITY_KEYRING_MOK);
> + if (rc)
> + return rc;
> +
> + pr_notice("MOK Keyring initialized\n");
> + return 0;
> +}
> +device_initcall(mok_keyring_init);
The ordering of the patches in this patch set is not quite
right. Please first introduce the new keyring with the new Kconfig,
new restriction, and loading the keys onto the new keyring. Introduce
the builitin_secondary_and_ca_trusted restriction and linking the new
keyring to the secondary keyring. Only after everything is in place,
define and use the UEFI mok variable(s).
Originally, I asked you to "Separate each **logical change** into a
separate patch." After re-ordering the patches, see if merging some of
them together now makes sense.
thanks,
Mimi
^ permalink raw reply
* Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM attributes
From: Paul Moore @ 2021-08-12 20:59 UTC (permalink / raw)
To: Casey Schaufler
Cc: casey.schaufler, James Morris, linux-security-module, selinux,
linux-audit, keescook, john.johansen, penguin-kernel,
Stephen Smalley, linux-kernel, netdev
In-Reply-To: <20210722004758.12371-23-casey@schaufler-ca.com>
On Wed, Jul 21, 2021 at 9:12 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Create a new audit record type to contain the subject information
> when there are multiple security modules that require such data.
> This record is linked with the same timestamp and serial number
> using the audit_alloc_local() mechanism.
> The record is produced only in cases where there is more than one
> security module with a process "context".
> In cases where this record is produced the subj= fields of
> other records in the audit event will be set to "subj=?".
>
> An example of the MAC_TASK_CONTEXTS (1420) record is:
>
> type=UNKNOWN[1420]
> msg=audit(1600880931.832:113)
> subj_apparmor="=unconfined"
> subj_smack="_"
>
> There will be a subj_$LSM= entry for each security module
> LSM that supports the secid_to_secctx and secctx_to_secid
> hooks. The BPF security module implements secid/secctx
> translation hooks, so it has to be considered to provide a
> secctx even though it may not actually do so.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> To: paul@paul-moore.com
> To: linux-audit@redhat.com
> To: rgb@redhat.com
> Cc: netdev@vger.kernel.org
> ---
> drivers/android/binder.c | 2 +-
> include/linux/audit.h | 16 +++++
> include/linux/security.h | 16 ++++-
> include/net/netlabel.h | 2 +-
> include/net/scm.h | 2 +-
> include/net/xfrm.h | 13 +++-
> include/uapi/linux/audit.h | 1 +
> kernel/audit.c | 90 +++++++++++++++++++------
> kernel/auditfilter.c | 5 +-
> kernel/auditsc.c | 27 ++++++--
> net/ipv4/ip_sockglue.c | 2 +-
> net/netfilter/nf_conntrack_netlink.c | 4 +-
> net/netfilter/nf_conntrack_standalone.c | 2 +-
> net/netfilter/nfnetlink_queue.c | 2 +-
> net/netlabel/netlabel_unlabeled.c | 21 +++---
> net/netlabel/netlabel_user.c | 14 ++--
> net/netlabel/netlabel_user.h | 6 +-
> net/xfrm/xfrm_policy.c | 8 ++-
> net/xfrm/xfrm_state.c | 18 +++--
> security/integrity/ima/ima_api.c | 6 +-
> security/integrity/integrity_audit.c | 5 +-
> security/security.c | 46 ++++++++-----
> security/smack/smackfs.c | 3 +-
> 23 files changed, 221 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 2c3a2348a144..3520caa0260c 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2722,7 +2722,7 @@ static void binder_transaction(struct binder_proc *proc,
> * case well anyway.
> */
> security_task_getsecid_obj(proc->tsk, &blob);
> - ret = security_secid_to_secctx(&blob, &lsmctx);
> + ret = security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_DISPLAY);
> if (ret) {
> return_error = BR_FAILED_REPLY;
> return_error_param = ret;
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 97cd7471e572..85eb87f6f92d 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -291,6 +291,7 @@ extern int audit_alloc(struct task_struct *task);
> extern void __audit_free(struct task_struct *task);
> extern struct audit_context *audit_alloc_local(gfp_t gfpflags);
> extern void audit_free_context(struct audit_context *context);
> +extern void audit_free_local(struct audit_context *context);
> extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
> unsigned long a2, unsigned long a3);
> extern void __audit_syscall_exit(int ret_success, long ret_value);
> @@ -386,6 +387,19 @@ static inline void audit_ptrace(struct task_struct *t)
> __audit_ptrace(t);
> }
>
> +static inline struct audit_context *audit_alloc_for_lsm(gfp_t gfp)
> +{
> + struct audit_context *context = audit_context();
> +
> + if (context)
> + return context;
> +
> + if (lsm_multiple_contexts())
> + return audit_alloc_local(gfp);
> +
> + return NULL;
> +}
We don't want to do this, at least not as it is written above. We
shouldn't have a function which abstracts away the creation of a local
audit_context. Usage of a local audit_context should be explicit in
the caller, and if the caller's execution context is ambiguous enough
that it can require both a task_struct audit_context and a local
audit_context then we need to handle that on a case-by-case basis.
Hiding it like this is guaranteed to cause problems later.
I probably did a poor job of explaining what a local context is during
the last patchset; I'll try to do a better job here but also let me
say this as clear as I can ... if the "current" task struct is valid
for a given code path, *never* use a local audit context. The local
audit context is a hack that is made necessary by the fact that we
have to audit things which happen outside the scope of an executing
task, e.g. the netfilter audit hooks, it should *never* be used when
there is a valid task_struct.
It's the audit_context which helps bind multiple audit records into a
single event, creating a new, "local" audit_context destroys that
binding as audit records created using that local audit_context have a
different timestamp from those records created using the current
task_struct's audit_context.
Hopefully that makes sense?
> /* Private API (for audit.c only) */
> extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
> extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, umode_t mode);
> @@ -560,6 +574,8 @@ extern int audit_signals;
> }
> static inline void audit_free_context(struct audit_context *context)
> { }
> +static inline void audit_free_local(struct audit_context *context)
> +{ }
> static inline int audit_alloc(struct task_struct *task)
> {
> return 0;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 3e9743118fb9..b3cf68cf2bd6 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -182,6 +182,8 @@ struct lsmblob {
> #define LSMBLOB_INVALID -1 /* Not a valid LSM slot number */
> #define LSMBLOB_NEEDED -2 /* Slot requested on initialization */
> #define LSMBLOB_NOT_NEEDED -3 /* Slot not requested */
> +#define LSMBLOB_DISPLAY -4 /* Use the "display" slot */
> +#define LSMBLOB_FIRST -5 /* Use the default "display" slot */
>
> /**
> * lsmblob_init - initialize an lsmblob structure
> @@ -248,6 +250,15 @@ static inline u32 lsmblob_value(const struct lsmblob *blob)
> return 0;
> }
>
> +static inline bool lsm_multiple_contexts(void)
> +{
> +#ifdef CONFIG_SECURITY
> + return lsm_slot_to_name(1) != NULL;
> +#else
> + return false;
> +#endif
> +}
> +
> /* These functions are in security/commoncap.c */
> extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
> int cap, unsigned int opts);
> @@ -578,7 +589,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> size_t size);
> int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> int security_ismaclabel(const char *name);
> -int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp);
> +int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp,
> + int display);
> int security_secctx_to_secid(const char *secdata, u32 seclen,
> struct lsmblob *blob);
> void security_release_secctx(struct lsmcontext *cp);
> @@ -1433,7 +1445,7 @@ static inline int security_ismaclabel(const char *name)
> }
>
> static inline int security_secid_to_secctx(struct lsmblob *blob,
> - struct lsmcontext *cp)
> + struct lsmcontext *cp, int display)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/include/net/netlabel.h b/include/net/netlabel.h
> index 73fc25b4042b..216cb1ffc8f0 100644
> --- a/include/net/netlabel.h
> +++ b/include/net/netlabel.h
> @@ -97,7 +97,7 @@ struct calipso_doi;
>
> /* NetLabel audit information */
> struct netlbl_audit {
> - u32 secid;
> + struct lsmblob lsmdata;
> kuid_t loginuid;
> unsigned int sessionid;
> };
This chunk seems lost here, does it belong in another patch?
> diff --git a/include/net/scm.h b/include/net/scm.h
> index b77a52f93389..f4d567d4885e 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -101,7 +101,7 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
> * and the infrastructure will know which it is.
> */
> lsmblob_init(&lb, scm->secid);
> - err = security_secid_to_secctx(&lb, &context);
> + err = security_secid_to_secctx(&lb, &context, LSMBLOB_DISPLAY);
Misplaced code change?
> if (!err) {
> put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, context.len,
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index cbff7c2a9724..a10fa01f7bf4 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -660,13 +660,22 @@ struct xfrm_spi_skb_cb {
> #define XFRM_SPI_SKB_CB(__skb) ((struct xfrm_spi_skb_cb *)&((__skb)->cb[0]))
>
> #ifdef CONFIG_AUDITSYSCALL
> -static inline struct audit_buffer *xfrm_audit_start(const char *op)
> +static inline struct audit_buffer *xfrm_audit_start(const char *op,
> + struct audit_context **lac)
> {
> + struct audit_context *context;
> struct audit_buffer *audit_buf = NULL;
>
> if (audit_enabled == AUDIT_OFF)
> return NULL;
> - audit_buf = audit_log_start(audit_context(), GFP_ATOMIC,
> + context = audit_context();
> + if (lac != NULL) {
> + if (lsm_multiple_contexts() && context == NULL)
> + context = audit_alloc_local(GFP_ATOMIC);
> + *lac = context;
> + }
> +
> + audit_buf = audit_log_start(context, GFP_ATOMIC,
> AUDIT_MAC_IPSEC_EVENT);
> if (audit_buf == NULL)
> return NULL;
Related to the other comments around local audit_contexts, we don't
want to do this; use the existing audit_context, @lac in this case, so
that this audit record is bound to the other associated records into a
single audit event (all have the same timestamp).
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 841123390d41..cba63789a164 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -386,10 +386,12 @@ void audit_log_lost(const char *message)
> static int audit_log_config_change(char *function_name, u32 new, u32 old,
> int allow_changes)
> {
> + struct audit_context *context;
> struct audit_buffer *ab;
> int rc = 0;
>
> - ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> + context = audit_alloc_for_lsm(GFP_KERNEL);
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
We shouldn't need to do this for the reasons discussed up near the top
of this email (and elsewhere as well).
I'm going to refrain from commenting on the other uses of
audit_alloc_for_lsm() in this patch unless there is something unique
to the code path, so if you don't see me comment about a use of
audit_alloc_for_lsm() you can assume it should be removed and the
existing audit_context used instead.
> @@ -399,6 +401,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
> allow_changes = 0; /* Something weird, deny request */
> audit_log_format(ab, " res=%d", allow_changes);
> audit_log_end(ab);
> + audit_free_local(context);
See my comment directly above regarding usage of
audit_alloc_for_lsm(), it obviously applies here too.
> @@ -2128,6 +2136,36 @@ void audit_log_key(struct audit_buffer *ab, char *key)
> audit_log_format(ab, "(null)");
> }
>
> +static void audit_log_lsm(struct audit_context *context, struct lsmblob *blob)
See my note below about moving this into audit_log_task_context(), but
if we really need to keep this as a separate function, let's consider
changing the name to something which indicates that it logs the LSM
data as *subject* fields. How about audit_log_lsm_subj()?
> +{
> + struct audit_buffer *ab;
> + struct lsmcontext lsmdata;
> + bool sep = false;
> + int error;
> + int i;
> +
> + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_MAC_TASK_CONTEXTS);
> + if (!ab)
> + return; /* audit_panic or being filtered */
> +
> + for (i = 0; i < LSMBLOB_ENTRIES; i++) {
> + if (blob->secid[i] == 0)
> + continue;
> + error = security_secid_to_secctx(blob, &lsmdata, i);
> + if (error && error != -EINVAL) {
> + audit_panic("error in audit_log_lsm");
> + return;
> + }
> +
> + audit_log_format(ab, "%ssubj_%s=\"%s\"", sep ? " " : "",
> + lsm_slot_to_name(i), lsmdata.context);
> + sep = true;
Since @i starts at zero, you can get rid of @sep by replacing it with @i:
audit_log_format(ab, ..., (i ? " " : ""), ...);
> + security_release_secctx(&lsmdata);
> + }
> + audit_log_end(ab);
> +}
> @@ -2138,7 +2176,18 @@ int audit_log_task_context(struct audit_buffer *ab)
> if (!lsmblob_is_set(&blob))
> return 0;
>
> - error = security_secid_to_secctx(&blob, &context);
> + /*
> + * If there is more than one security module that has a
> + * subject "context" it's necessary to put the subject data
> + * into a separate record to maintain compatibility.
> + */
> + if (lsm_multiple_contexts()) {
> + audit_log_format(ab, " subj=?");
> + audit_log_lsm(ab->ctx, &blob);
> + return 0;
> + }
> +
> + error = security_secid_to_secctx(&blob, &context, LSMBLOB_FIRST);
Instead of the lsm_multiple_contexts() case bailing on the rest of the
function with a return inside the if-block, let's made the code a bit
more robust by organizing it like this:
int audit_log_task_context(ab)
{
/* common stuff at the start */
if (lsm_multiple_contexts()) {
/* multi-LSM stuff */
} else {
/* single LSM stuff */
}
/* common stuff at the end */
}
... it also may make sense to just move the body of audit_log_lsm()
into audit_log_task_context() and do away with audit_log_lsm()
entirely; is it ever going to be called from anywhere else?
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 0e58a3ab56f5..01fdcbf468c0 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -993,12 +993,11 @@ struct audit_context *audit_alloc_local(gfp_t gfpflags)
> context = audit_alloc_context(AUDIT_STATE_BUILD, gfpflags);
> if (!context) {
> audit_log_lost("out of memory in audit_alloc_local");
> - goto out;
> + return NULL;
> }
> context->serial = audit_serial();
> ktime_get_coarse_real_ts64(&context->ctime);
> context->local = true;
> -out:
> return context;
> }
This chunk should be moved to 21/25 when audit_alloc_local() is first defined.
> @@ -1019,6 +1018,13 @@ void audit_free_context(struct audit_context *context)
> }
> EXPORT_SYMBOL(audit_free_context);
>
> +void audit_free_local(struct audit_context *context)
> +{
> + if (context && context->local)
> + audit_free_context(context);
> +}
> +EXPORT_SYMBOL(audit_free_local);
If this is strictly necessary, and I don't think it is, it should also
be moved to patch 21/25 with the original definition of a local
audit_context. However, there really should be no reason why we have
to distinguish between a proper and local audtit_context when it comes
to free'ing the memory, just call audit_free_context() in both cases.
> @@ -1036,7 +1042,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> from_kuid(&init_user_ns, auid),
> from_kuid(&init_user_ns, uid), sessionid);
> if (lsmblob_is_set(blob)) {
> - if (security_secid_to_secctx(blob, &lsmctx)) {
> + if (security_secid_to_secctx(blob, &lsmctx, LSMBLOB_FIRST)) {
Misplaced code change?
Actually, there are a lot of these below, I'm not going to comment on
all of them as I think you get the idea ... and I very well may be
wrong so I'll save you all of my wrongness in that case :)
> diff --git a/security/security.c b/security/security.c
> index cb359e185d1a..5d7fd982f84a 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2309,7 +2309,7 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> hlist_for_each_entry(hp, &security_hook_heads.setprocattr,
> list) {
> rc = hp->hook.setprocattr(name, value, size);
> - if (rc < 0)
> + if (rc < 0 && rc != -EINVAL)
> return rc;
> }
This really looks misplaced ... ?
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 1/1] NAX LSM: Add initial support support
From: Igor Zhbanov @ 2021-08-12 20:24 UTC (permalink / raw)
To: THOBY Simon
Cc: Igor Zhbanov, linux-integrity, linux-security-module, Mimi Zohar
In-Reply-To: <d97d7fdb-1676-9670-6cf5-2427f780ec6f@viveris.fr>
Hi Simon,
ср, 28 июл. 2021 г. в 13:19, THOBY Simon <Simon.THOBY@viveris.fr>:
> Nit: considering that allowed_caps_hex is only used in this function, and that it represents a small amount of
> stack space, couldn't you define it directly in the function?
>
> > + len = strlen(allowed_caps_hex);
It is also used as sysctl parameter input buffer in nax_sysctl_table[]
for "allowed_caps" parameter.
^ permalink raw reply
* Re: [PATCH v4 2/2] tpm: ibmvtpm: Rename tpm_process_cmd to tpm_status and define flag
From: Jarkko Sakkinen @ 2021-08-12 19:48 UTC (permalink / raw)
To: Stefan Berger
Cc: Stefan Berger, nasastry, linux-integrity, linux-security-module,
linux-kernel, Nayna Jain, George Wilson
In-Reply-To: <4eff0296-78da-52b6-322d-56e0f9d78dc2@linux.ibm.com>
On Wed, Aug 11, 2021 at 08:15:14AM -0400, Stefan Berger wrote:
>
> On 8/10/21 10:10 PM, Jarkko Sakkinen wrote:
> > On Tue, Aug 10, 2021 at 09:50:55PM -0400, Stefan Berger wrote:
> > > On 8/10/21 1:58 PM, Jarkko Sakkinen wrote:
> > > > On Mon, Aug 09, 2021 at 03:21:59PM -0400, Stefan Berger wrote:
> > > > > From: Stefan Berger <stefanb@linux.ibm.com>
> > > > >
> > > > > Rename the field tpm_processing_cmd to tpm_status in ibmvtpm_dev and set
> > > > > the TPM_STATUS_BUSY flag while the vTPM is busy processing a command.
> > > > >
> > > > >
> > > > > default:
> > > > > diff --git a/drivers/char/tpm/tpm_ibmvtpm.h b/drivers/char/tpm/tpm_ibmvtpm.h
> > > > > index 51198b137461..252f1cccdfc5 100644
> > > > > --- a/drivers/char/tpm/tpm_ibmvtpm.h
> > > > > +++ b/drivers/char/tpm/tpm_ibmvtpm.h
> > > > > @@ -41,7 +41,8 @@ struct ibmvtpm_dev {
> > > > > wait_queue_head_t wq;
> > > > > u16 res_len;
> > > > > u32 vtpm_version;
> > > > > - u8 tpm_processing_cmd;
> > > > > + u8 tpm_status;
> > > > > +#define TPM_STATUS_BUSY (1 << 0) /* vtpm is processing a command */
> > > > Declare this already in the fix, and just leave the rename here.
> > > You mean the fix patch does not use 'true' anymore but uses the
> > > TPM_STATUS_BUSY flag already but the name is still tpm_processing_cmd? And
> > > literally only the renaming of this field is done in the 2nd patch?
> > I can fixup these patches, and use '1', instead of true. No need to send
> > new ones.
> >
> > Acked-by: Jarkko Sakkinen <jarkko@kernel.org>
I applied the first. If you have only one flag that you even
document as "processing the command" in the inline comment,
it makes absolutely no sense to rename it, as the current
name perfectly documents what it exactly is.
/Jarkko
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox