diff mbox series

[v9,2/8] integrity: Introduce a Linux keyring called machine

Message ID 20220105235012.2497118-3-eric.snowberg@oracle.com
State Superseded
Headers show
Series [v9,1/8] integrity: Fix warning about missing prototypes | expand

Commit Message

Eric Snowberg Jan. 5, 2022, 11:50 p.m. UTC
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.

Define a new Linux keyring called machine.  This keyring shall contain just
MOK CA keys and not the remaining keys in the platform keyring. This new
machine keyring will be used in follow on patches.  Unlike keys in the
platform keyring, keys contained in the machine 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
v4: Add Kconfig, merged in "integrity: add add_to_mok_keyring" 
v5: Rename to machine keyring
v6: Depend on EFI in kconfig  (suggested by Mimi)
    Test to see if ".platform" keyring is configured in
      add_to_machine_keyring (suggested by Mimi)
v7: Depend on LOAD_UEFI_KEYS instead EFI for mokvar code
v8: Code unmodified from v7 added Mimi's Reviewed-by
v9: Removed Reviewed-by. Prevent IMA from being able to
     use the machine keyring since the CA restrictions
     have been removed.
---
 security/integrity/Kconfig                    | 13 ++++++
 security/integrity/Makefile                   |  1 +
 security/integrity/digsig.c                   | 13 +++++-
 security/integrity/integrity.h                | 12 +++++-
 .../platform_certs/machine_keyring.c          | 42 +++++++++++++++++++
 5 files changed, 78 insertions(+), 3 deletions(-)
 create mode 100644 security/integrity/platform_certs/machine_keyring.c

Comments

Mimi Zohar Jan. 9, 2022, 9:57 p.m. UTC | #1
On Wed, 2022-01-05 at 18:50 -0500, 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.
> 
> Define a new Linux keyring called machine.  This keyring shall contain just
> MOK CA keys and not the remaining keys in the platform keyring. This new
> machine keyring will be used in follow on patches.  Unlike keys in the
> platform keyring, keys contained in the machine keyring will be trusted
> within the kernel if the end-user has chosen to do so.

True, from an IMA perspective only the CA keys should be loaded onto
the .machine keyring, but this version (v9) of the patch set does not
enforce that.  The patch set and this paragraph are out of sync.

Jarkko, my concern is that once this version of the patch set is
upstreamed, would limiting which keys may be loaded onto the .machine
keyring be considered a regression?

thanks,

Mimi

> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
> v1: Initial version
> v2: Removed destory keyring code
> v3: Unmodified from v2
> v4: Add Kconfig, merged in "integrity: add add_to_mok_keyring" 
> v5: Rename to machine keyring
> v6: Depend on EFI in kconfig  (suggested by Mimi)
>     Test to see if ".platform" keyring is configured in
>       add_to_machine_keyring (suggested by Mimi)
> v7: Depend on LOAD_UEFI_KEYS instead EFI for mokvar code
> v8: Code unmodified from v7 added Mimi's Reviewed-by
> v9: Removed Reviewed-by. Prevent IMA from being able to
>      use the machine keyring since the CA restrictions
>      have been removed.
Eric Snowberg Jan. 10, 2022, 11:25 p.m. UTC | #2
> On Jan 9, 2022, at 2:57 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Wed, 2022-01-05 at 18:50 -0500, 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.
>> 
>> Define a new Linux keyring called machine.  This keyring shall contain just
>> MOK CA keys and not the remaining keys in the platform keyring. This new
>> machine keyring will be used in follow on patches.  Unlike keys in the
>> platform keyring, keys contained in the machine keyring will be trusted
>> within the kernel if the end-user has chosen to do so.
> 
> True, from an IMA perspective only the CA keys should be loaded onto
> the .machine keyring, but this version (v9) of the patch set does not
> enforce that.  The patch set and this paragraph are out of sync.

I missed that when I dropped IMA support.  I will strike that sentence
in the next round.  Or if no code changes are identified, feel free to
remove that sentence.  

> Jarkko, my concern is that once this version of the patch set is
> upstreamed, would limiting which keys may be loaded onto the .machine
> keyring be considered a regression?


Currently certificates built into the kernel do not have a CA restriction on them.  
IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
be advisable for a kernel to be built with a CA, nothing currently enforces it. 

My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
This Kconfig would do the CA enforcement on the machine keyring.  However if the 
Kconfig option was not set for enforcement, it would work as it does in this series, 
plus it would allow IMA to work with non-CA keys.  This would be done by removing 
the restriction placed in this patch. Let me know your thoughts on whether this would 
be an appropriate solution.  I believe this would get around what you are identifying as 
a possible regression.
Mimi Zohar Jan. 11, 2022, 6:16 p.m. UTC | #3
On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> > Jarkko, my concern is that once this version of the patch set is
> > upstreamed, would limiting which keys may be loaded onto the .machine
> > keyring be considered a regression?
> 
> 
> Currently certificates built into the kernel do not have a CA restriction on them.  
> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> 
> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> Kconfig option was not set for enforcement, it would work as it does in this series, 
> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> the restriction placed in this patch. Let me know your thoughts on whether this would 
> be an appropriate solution.  I believe this would get around what you are identifying as 
> a possible regression.

True the problem currently exists with the builtin keys, but there's a
major difference between trusting the builtin keys and those being
loading via MOK.  This is an integrity gap that needs to be closed and
shouldn't be expanded to keys on the .machine keyring.

"plus it would allow IMA to work with non-CA keys" is unacceptable.

Mimi
Eric Snowberg Jan. 11, 2022, 9:26 p.m. UTC | #4
> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
>>> Jarkko, my concern is that once this version of the patch set is
>>> upstreamed, would limiting which keys may be loaded onto the .machine
>>> keyring be considered a regression?
>> 
>> 
>> Currently certificates built into the kernel do not have a CA restriction on them.  
>> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
>> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
>> 
>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
>> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
>> Kconfig option was not set for enforcement, it would work as it does in this series, 
>> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
>> the restriction placed in this patch. Let me know your thoughts on whether this would 
>> be an appropriate solution.  I believe this would get around what you are identifying as 
>> a possible regression.
> 
> True the problem currently exists with the builtin keys, but there's a
> major difference between trusting the builtin keys and those being
> loading via MOK.  This is an integrity gap that needs to be closed and
> shouldn't be expanded to keys on the .machine keyring.
> 
> "plus it would allow IMA to work with non-CA keys" is unacceptable.

Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
cover letter, which adds IMA support, on why it is unacceptable for the end-user to
make this decision?
Mimi Zohar Jan. 12, 2022, 1:14 a.m. UTC | #5
On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
> 
> > On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> >>> Jarkko, my concern is that once this version of the patch set is
> >>> upstreamed, would limiting which keys may be loaded onto the .machine
> >>> keyring be considered a regression?
> >> 
> >> 
> >> Currently certificates built into the kernel do not have a CA restriction on them.  
> >> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> >> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> >> 
> >> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> >> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> >> Kconfig option was not set for enforcement, it would work as it does in this series, 
> >> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> >> the restriction placed in this patch. Let me know your thoughts on whether this would 
> >> be an appropriate solution.  I believe this would get around what you are identifying as 
> >> a possible regression.
> > 
> > True the problem currently exists with the builtin keys, but there's a
> > major difference between trusting the builtin keys and those being
> > loading via MOK.  This is an integrity gap that needs to be closed and
> > shouldn't be expanded to keys on the .machine keyring.
> > 
> > "plus it would allow IMA to work with non-CA keys" is unacceptable.
> 
> Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
> cover letter, which adds IMA support, on why it is unacceptable for the end-user to
> make this decision?

The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
"help" is very clear:

        help
          Keys may be added to the IMA or IMA blacklist keyrings, if
the
          key is validly signed by a CA cert in the system built-in or
          secondary trusted keyrings.

          Intermediate keys between those the kernel has compiled in
and the
          IMA keys to be added may be added to the system secondary
keyring,
          provided they are validly signed by a key already resident in
the
          built-in or secondary trusted keyrings.

Mimi
Mimi Zohar Jan. 12, 2022, 7:41 p.m. UTC | #6
On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
> > 
> > > On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > 
> > > On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> > >>> Jarkko, my concern is that once this version of the patch set is
> > >>> upstreamed, would limiting which keys may be loaded onto the .machine
> > >>> keyring be considered a regression?
> > >> 
> > >> 
> > >> Currently certificates built into the kernel do not have a CA restriction on them.  
> > >> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> > >> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> > >> 
> > >> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> > >> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> > >> Kconfig option was not set for enforcement, it would work as it does in this series, 
> > >> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> > >> the restriction placed in this patch. Let me know your thoughts on whether this would 
> > >> be an appropriate solution.  I believe this would get around what you are identifying as 
> > >> a possible regression.
> > > 
> > > True the problem currently exists with the builtin keys, but there's a
> > > major difference between trusting the builtin keys and those being
> > > loading via MOK.  This is an integrity gap that needs to be closed and
> > > shouldn't be expanded to keys on the .machine keyring.
> > > 
> > > "plus it would allow IMA to work with non-CA keys" is unacceptable.
> > 
> > Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
> > cover letter, which adds IMA support, on why it is unacceptable for the end-user to
> > make this decision?
> 
> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> "help" is very clear:

[Reposting the text due to email formatting issues.]

help
  Keys may be added to the IMA or IMA blacklist keyrings, if the
  key is validly signed by a CA cert in the system built-in or
  secondary trusted keyrings.

  Intermediate keys between those the kernel has compiled in and the 
  IMA keys to be added may be added to the system secondary keyring,
  provided they are validly signed by a key already resident in the
  built-in or secondary trusted keyrings.


The first paragraph requires "validly signed by a CA cert in the system
built-in or secondary trusted keyrings" for keys to be loaded onto the
IMA keyring.  This Kconfig is limited to just the builtin and secondary
keyrings.  Changing this silently to include the ".machine" keyring
introduces integrity risks that previously did not exist.  A new IMA
Kconfig needs to be defined to allow all three keyrings - builtin,
machine, and secondary.

The second paragraph implies that only CA and intermediate CA keys are
on secondary keyring, or as in our case the ".machine" keyring linked
to the secondary keyring.

Mimi
Mimi Zohar Jan. 12, 2022, 7:41 p.m. UTC | #7
On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
> > 
> > > On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > 
> > > On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> > >>> Jarkko, my concern is that once this version of the patch set is
> > >>> upstreamed, would limiting which keys may be loaded onto the .machine
> > >>> keyring be considered a regression?
> > >> 
> > >> 
> > >> Currently certificates built into the kernel do not have a CA restriction on them.  
> > >> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> > >> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> > >> 
> > >> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> > >> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> > >> Kconfig option was not set for enforcement, it would work as it does in this series, 
> > >> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> > >> the restriction placed in this patch. Let me know your thoughts on whether this would 
> > >> be an appropriate solution.  I believe this would get around what you are identifying as 
> > >> a possible regression.
> > > 
> > > True the problem currently exists with the builtin keys, but there's a
> > > major difference between trusting the builtin keys and those being
> > > loading via MOK.  This is an integrity gap that needs to be closed and
> > > shouldn't be expanded to keys on the .machine keyring.
> > > 
> > > "plus it would allow IMA to work with non-CA keys" is unacceptable.
> > 
> > Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
> > cover letter, which adds IMA support, on why it is unacceptable for the end-user to
> > make this decision?
> 
> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> "help" is very clear:

[Reposting the text due to email formatting issues.]

help
  Keys may be added to the IMA or IMA blacklist keyrings, if the
  key is validly signed by a CA cert in the system built-in or
  secondary trusted keyrings.

  Intermediate keys between those the kernel has compiled in and the 
  IMA keys to be added may be added to the system secondary keyring,
  provided they are validly signed by a key already resident in the
  built-in or secondary trusted keyrings.


The first paragraph requires "validly signed by a CA cert in the system
built-in or secondary trusted keyrings" for keys to be loaded onto the
IMA keyring.  This Kconfig is limited to just the builtin and secondary
keyrings.  Changing this silently to include the ".machine" keyring
introduces integrity risks that previously did not exist.  A new IMA
Kconfig needs to be defined to allow all three keyrings - builtin,
machine, and secondary.

The second paragraph implies that only CA and intermediate CA keys are
on secondary keyring, or as in our case the ".machine" keyring linked
to the secondary keyring.

Mimi
Eric Snowberg Jan. 12, 2022, 11 p.m. UTC | #8
> On Jan 12, 2022, at 12:41 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
>> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
>>> 
>>>> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>> 
>>>> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
>>>>>> Jarkko, my concern is that once this version of the patch set is
>>>>>> upstreamed, would limiting which keys may be loaded onto the .machine
>>>>>> keyring be considered a regression?
>>>>> 
>>>>> 
>>>>> Currently certificates built into the kernel do not have a CA restriction on them.  
>>>>> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
>>>>> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
>>>>> 
>>>>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
>>>>> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
>>>>> Kconfig option was not set for enforcement, it would work as it does in this series, 
>>>>> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
>>>>> the restriction placed in this patch. Let me know your thoughts on whether this would 
>>>>> be an appropriate solution.  I believe this would get around what you are identifying as 
>>>>> a possible regression.
>>>> 
>>>> True the problem currently exists with the builtin keys, but there's a
>>>> major difference between trusting the builtin keys and those being
>>>> loading via MOK.  This is an integrity gap that needs to be closed and
>>>> shouldn't be expanded to keys on the .machine keyring.
>>>> 
>>>> "plus it would allow IMA to work with non-CA keys" is unacceptable.
>>> 
>>> Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
>>> cover letter, which adds IMA support, on why it is unacceptable for the end-user to
>>> make this decision?
>> 
>> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
>> "help" is very clear:
> 
> [Reposting the text due to email formatting issues.]
> 
> help
>  Keys may be added to the IMA or IMA blacklist keyrings, if the
>  key is validly signed by a CA cert in the system built-in or
>  secondary trusted keyrings.
> 
>  Intermediate keys between those the kernel has compiled in and the 
>  IMA keys to be added may be added to the system secondary keyring,
>  provided they are validly signed by a key already resident in the
>  built-in or secondary trusted keyrings.
> 
> 
> The first paragraph requires "validly signed by a CA cert in the system
> built-in or secondary trusted keyrings" for keys to be loaded onto the
> IMA keyring.  This Kconfig is limited to just the builtin and secondary
> keyrings.  Changing this silently to include the ".machine" keyring
> introduces integrity risks that previously did not exist.  A new IMA
> Kconfig needs to be defined to allow all three keyrings - builtin,
> machine, and secondary.
> 
> The second paragraph implies that only CA and intermediate CA keys are
> on secondary keyring, or as in our case the ".machine" keyring linked
> to the secondary keyring.

Got it, thanks.  I’ll use this in the cover letter that introduces the CA restrictions 
to enable IMA.
Jarkko Sakkinen Jan. 15, 2022, 5:11 p.m. UTC | #9
On Wed, Jan 12, 2022 at 02:41:47PM -0500, Mimi Zohar wrote:
> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
> > On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
> > > 
> > > > On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > > 
> > > > On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> > > >>> Jarkko, my concern is that once this version of the patch set is
> > > >>> upstreamed, would limiting which keys may be loaded onto the .machine
> > > >>> keyring be considered a regression?
> > > >> 
> > > >> 
> > > >> Currently certificates built into the kernel do not have a CA restriction on them.  
> > > >> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> > > >> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> > > >> 
> > > >> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> > > >> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> > > >> Kconfig option was not set for enforcement, it would work as it does in this series, 
> > > >> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> > > >> the restriction placed in this patch. Let me know your thoughts on whether this would 
> > > >> be an appropriate solution.  I believe this would get around what you are identifying as 
> > > >> a possible regression.
> > > > 
> > > > True the problem currently exists with the builtin keys, but there's a
> > > > major difference between trusting the builtin keys and those being
> > > > loading via MOK.  This is an integrity gap that needs to be closed and
> > > > shouldn't be expanded to keys on the .machine keyring.
> > > > 
> > > > "plus it would allow IMA to work with non-CA keys" is unacceptable.
> > > 
> > > Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
> > > cover letter, which adds IMA support, on why it is unacceptable for the end-user to
> > > make this decision?
> > 
> > The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> > "help" is very clear:
> 
> [Reposting the text due to email formatting issues.]
> 
> help
>   Keys may be added to the IMA or IMA blacklist keyrings, if the
>   key is validly signed by a CA cert in the system built-in or
>   secondary trusted keyrings.
> 
>   Intermediate keys between those the kernel has compiled in and the 
>   IMA keys to be added may be added to the system secondary keyring,
>   provided they are validly signed by a key already resident in the
>   built-in or secondary trusted keyrings.
> 
> 
> The first paragraph requires "validly signed by a CA cert in the system
> built-in or secondary trusted keyrings" for keys to be loaded onto the
> IMA keyring.  This Kconfig is limited to just the builtin and secondary
> keyrings.  Changing this silently to include the ".machine" keyring
> introduces integrity risks that previously did not exist.  A new IMA
> Kconfig needs to be defined to allow all three keyrings - builtin,
> machine, and secondary.
> 
> The second paragraph implies that only CA and intermediate CA keys are
> on secondary keyring, or as in our case the ".machine" keyring linked
> to the secondary keyring.
> 
> Mimi
> 
I have also now test environment for this patch set but if there are
any possible changes, I'm waiting for a new version, as it is anyway
for 5.18 cycle earliest.

/Jarkko
Eric Snowberg Jan. 15, 2022, 7:12 p.m. UTC | #10
> On Jan 15, 2022, at 10:11 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Wed, Jan 12, 2022 at 02:41:47PM -0500, Mimi Zohar wrote:
>> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
>>> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
>>>> 
>>>>> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>> 
>>>>> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
>>>>>>> Jarkko, my concern is that once this version of the patch set is
>>>>>>> upstreamed, would limiting which keys may be loaded onto the .machine
>>>>>>> keyring be considered a regression?
>>>>>> 
>>>>>> 
>>>>>> Currently certificates built into the kernel do not have a CA restriction on them.  
>>>>>> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
>>>>>> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
>>>>>> 
>>>>>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
>>>>>> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
>>>>>> Kconfig option was not set for enforcement, it would work as it does in this series, 
>>>>>> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
>>>>>> the restriction placed in this patch. Let me know your thoughts on whether this would 
>>>>>> be an appropriate solution.  I believe this would get around what you are identifying as 
>>>>>> a possible regression.
>>>>> 
>>>>> True the problem currently exists with the builtin keys, but there's a
>>>>> major difference between trusting the builtin keys and those being
>>>>> loading via MOK.  This is an integrity gap that needs to be closed and
>>>>> shouldn't be expanded to keys on the .machine keyring.
>>>>> 
>>>>> "plus it would allow IMA to work with non-CA keys" is unacceptable.
>>>> 
>>>> Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
>>>> cover letter, which adds IMA support, on why it is unacceptable for the end-user to
>>>> make this decision?
>>> 
>>> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
>>> "help" is very clear:
>> 
>> [Reposting the text due to email formatting issues.]
>> 
>> help
>>  Keys may be added to the IMA or IMA blacklist keyrings, if the
>>  key is validly signed by a CA cert in the system built-in or
>>  secondary trusted keyrings.
>> 
>>  Intermediate keys between those the kernel has compiled in and the 
>>  IMA keys to be added may be added to the system secondary keyring,
>>  provided they are validly signed by a key already resident in the
>>  built-in or secondary trusted keyrings.
>> 
>> 
>> The first paragraph requires "validly signed by a CA cert in the system
>> built-in or secondary trusted keyrings" for keys to be loaded onto the
>> IMA keyring.  This Kconfig is limited to just the builtin and secondary
>> keyrings.  Changing this silently to include the ".machine" keyring
>> introduces integrity risks that previously did not exist.  A new IMA
>> Kconfig needs to be defined to allow all three keyrings - builtin,
>> machine, and secondary.
>> 
>> The second paragraph implies that only CA and intermediate CA keys are
>> on secondary keyring, or as in our case the ".machine" keyring linked
>> to the secondary keyring.
>> 
>> Mimi
>> 
> I have also now test environment for this patch set but if there are
> any possible changes, I'm waiting for a new version, as it is anyway
> for 5.18 cycle earliest.

Other than the two sentence changes, I have not seen anything identified 
code wise requiring a change.  If you’d like me to respin a v10 with the sentence 
changes let me know.  Or if you want to remove the ima reference, that works 
too.  Just let me know how you want to handle this.  Thanks.
Jarkko Sakkinen Jan. 15, 2022, 7:14 p.m. UTC | #11
On Sat, Jan 15, 2022 at 07:12:35PM +0000, Eric Snowberg wrote:
> 
> 
> > On Jan 15, 2022, at 10:11 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > On Wed, Jan 12, 2022 at 02:41:47PM -0500, Mimi Zohar wrote:
> >> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
> >>> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
> >>>> 
> >>>>> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>>>> 
> >>>>> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> >>>>>>> Jarkko, my concern is that once this version of the patch set is
> >>>>>>> upstreamed, would limiting which keys may be loaded onto the .machine
> >>>>>>> keyring be considered a regression?
> >>>>>> 
> >>>>>> 
> >>>>>> Currently certificates built into the kernel do not have a CA restriction on them.  
> >>>>>> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> >>>>>> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> >>>>>> 
> >>>>>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> >>>>>> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> >>>>>> Kconfig option was not set for enforcement, it would work as it does in this series, 
> >>>>>> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> >>>>>> the restriction placed in this patch. Let me know your thoughts on whether this would 
> >>>>>> be an appropriate solution.  I believe this would get around what you are identifying as 
> >>>>>> a possible regression.
> >>>>> 
> >>>>> True the problem currently exists with the builtin keys, but there's a
> >>>>> major difference between trusting the builtin keys and those being
> >>>>> loading via MOK.  This is an integrity gap that needs to be closed and
> >>>>> shouldn't be expanded to keys on the .machine keyring.
> >>>>> 
> >>>>> "plus it would allow IMA to work with non-CA keys" is unacceptable.
> >>>> 
> >>>> Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
> >>>> cover letter, which adds IMA support, on why it is unacceptable for the end-user to
> >>>> make this decision?
> >>> 
> >>> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> >>> "help" is very clear:
> >> 
> >> [Reposting the text due to email formatting issues.]
> >> 
> >> help
> >>  Keys may be added to the IMA or IMA blacklist keyrings, if the
> >>  key is validly signed by a CA cert in the system built-in or
> >>  secondary trusted keyrings.
> >> 
> >>  Intermediate keys between those the kernel has compiled in and the 
> >>  IMA keys to be added may be added to the system secondary keyring,
> >>  provided they are validly signed by a key already resident in the
> >>  built-in or secondary trusted keyrings.
> >> 
> >> 
> >> The first paragraph requires "validly signed by a CA cert in the system
> >> built-in or secondary trusted keyrings" for keys to be loaded onto the
> >> IMA keyring.  This Kconfig is limited to just the builtin and secondary
> >> keyrings.  Changing this silently to include the ".machine" keyring
> >> introduces integrity risks that previously did not exist.  A new IMA
> >> Kconfig needs to be defined to allow all three keyrings - builtin,
> >> machine, and secondary.
> >> 
> >> The second paragraph implies that only CA and intermediate CA keys are
> >> on secondary keyring, or as in our case the ".machine" keyring linked
> >> to the secondary keyring.
> >> 
> >> Mimi
> >> 
> > I have also now test environment for this patch set but if there are
> > any possible changes, I'm waiting for a new version, as it is anyway
> > for 5.18 cycle earliest.
> 
> Other than the two sentence changes, I have not seen anything identified 
> code wise requiring a change.  If you’d like me to respin a v10 with the sentence 
> changes let me know.  Or if you want to remove the ima reference, that works 
> too.  Just let me know how you want to handle this.  Thanks.

I'm basically waiting also Mimi to test this as I do not have IMA test
environment.
Jarkko Sakkinen Jan. 15, 2022, 7:15 p.m. UTC | #12
On Sat, Jan 15, 2022 at 09:14:45PM +0200, Jarkko Sakkinen wrote:
> On Sat, Jan 15, 2022 at 07:12:35PM +0000, Eric Snowberg wrote:
> > 
> > 
> > > On Jan 15, 2022, at 10:11 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > 
> > > On Wed, Jan 12, 2022 at 02:41:47PM -0500, Mimi Zohar wrote:
> > >> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
> > >>> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
> > >>>> 
> > >>>>> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > >>>>> 
> > >>>>> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> > >>>>>>> Jarkko, my concern is that once this version of the patch set is
> > >>>>>>> upstreamed, would limiting which keys may be loaded onto the .machine
> > >>>>>>> keyring be considered a regression?
> > >>>>>> 
> > >>>>>> 
> > >>>>>> Currently certificates built into the kernel do not have a CA restriction on them.  
> > >>>>>> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> > >>>>>> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> > >>>>>> 
> > >>>>>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> > >>>>>> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> > >>>>>> Kconfig option was not set for enforcement, it would work as it does in this series, 
> > >>>>>> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> > >>>>>> the restriction placed in this patch. Let me know your thoughts on whether this would 
> > >>>>>> be an appropriate solution.  I believe this would get around what you are identifying as 
> > >>>>>> a possible regression.
> > >>>>> 
> > >>>>> True the problem currently exists with the builtin keys, but there's a
> > >>>>> major difference between trusting the builtin keys and those being
> > >>>>> loading via MOK.  This is an integrity gap that needs to be closed and
> > >>>>> shouldn't be expanded to keys on the .machine keyring.
> > >>>>> 
> > >>>>> "plus it would allow IMA to work with non-CA keys" is unacceptable.
> > >>>> 
> > >>>> Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
> > >>>> cover letter, which adds IMA support, on why it is unacceptable for the end-user to
> > >>>> make this decision?
> > >>> 
> > >>> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> > >>> "help" is very clear:
> > >> 
> > >> [Reposting the text due to email formatting issues.]
> > >> 
> > >> help
> > >>  Keys may be added to the IMA or IMA blacklist keyrings, if the
> > >>  key is validly signed by a CA cert in the system built-in or
> > >>  secondary trusted keyrings.
> > >> 
> > >>  Intermediate keys between those the kernel has compiled in and the 
> > >>  IMA keys to be added may be added to the system secondary keyring,
> > >>  provided they are validly signed by a key already resident in the
> > >>  built-in or secondary trusted keyrings.
> > >> 
> > >> 
> > >> The first paragraph requires "validly signed by a CA cert in the system
> > >> built-in or secondary trusted keyrings" for keys to be loaded onto the
> > >> IMA keyring.  This Kconfig is limited to just the builtin and secondary
> > >> keyrings.  Changing this silently to include the ".machine" keyring
> > >> introduces integrity risks that previously did not exist.  A new IMA
> > >> Kconfig needs to be defined to allow all three keyrings - builtin,
> > >> machine, and secondary.
> > >> 
> > >> The second paragraph implies that only CA and intermediate CA keys are
> > >> on secondary keyring, or as in our case the ".machine" keyring linked
> > >> to the secondary keyring.
> > >> 
> > >> Mimi
> > >> 
> > > I have also now test environment for this patch set but if there are
> > > any possible changes, I'm waiting for a new version, as it is anyway
> > > for 5.18 cycle earliest.
> > 
> > Other than the two sentence changes, I have not seen anything identified 
> > code wise requiring a change.  If you’d like me to respin a v10 with the sentence 
> > changes let me know.  Or if you want to remove the ima reference, that works 
> > too.  Just let me know how you want to handle this.  Thanks.
> 
> I'm basically waiting also Mimi to test this as I do not have IMA test
> environment.
> 
> From my side:
> 
> Tested-by: Jarkko Sakkinen <jarkko@kernel.org>

I can pick the whole thing at the time when I get green light.

/Jarkko
Mimi Zohar Jan. 16, 2022, 2:55 a.m. UTC | #13
On Sat, 2022-01-15 at 21:15 +0200, Jarkko Sakkinen wrote:
> On Sat, Jan 15, 2022 at 09:14:45PM +0200, Jarkko Sakkinen wrote:
> > On Sat, Jan 15, 2022 at 07:12:35PM +0000, Eric Snowberg wrote:
> > > 
> > > 
> > > > On Jan 15, 2022, at 10:11 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > > 
> > > > On Wed, Jan 12, 2022 at 02:41:47PM -0500, Mimi Zohar wrote:
> > > >> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
> > > >>> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
> > > >>>> 
> > > >>>>> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > >>>>> 
> > > >>>>> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> > > >>>>>>> Jarkko, my concern is that once this version of the patch set is
> > > >>>>>>> upstreamed, would limiting which keys may be loaded onto the .machine
> > > >>>>>>> keyring be considered a regression?
> > > >>>>>> 
> > > >>>>>> 
> > > >>>>>> Currently certificates built into the kernel do not have a CA restriction on them.  
> > > >>>>>> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> > > >>>>>> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> > > >>>>>> 
> > > >>>>>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> > > >>>>>> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> > > >>>>>> Kconfig option was not set for enforcement, it would work as it does in this series, 
> > > >>>>>> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> > > >>>>>> the restriction placed in this patch. Let me know your thoughts on whether this would 
> > > >>>>>> be an appropriate solution.  I believe this would get around what you are identifying as 
> > > >>>>>> a possible regression.
> > > >>>>> 
> > > >>>>> True the problem currently exists with the builtin keys, but there's a
> > > >>>>> major difference between trusting the builtin keys and those being
> > > >>>>> loading via MOK.  This is an integrity gap that needs to be closed and
> > > >>>>> shouldn't be expanded to keys on the .machine keyring.
> > > >>>>> 
> > > >>>>> "plus it would allow IMA to work with non-CA keys" is unacceptable.
> > > >>>> 
> > > >>>> Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
> > > >>>> cover letter, which adds IMA support, on why it is unacceptable for the end-user to
> > > >>>> make this decision?
> > > >>> 
> > > >>> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> > > >>> "help" is very clear:
> > > >> 
> > > >> [Reposting the text due to email formatting issues.]
> > > >> 
> > > >> help
> > > >>  Keys may be added to the IMA or IMA blacklist keyrings, if the
> > > >>  key is validly signed by a CA cert in the system built-in or
> > > >>  secondary trusted keyrings.
> > > >> 
> > > >>  Intermediate keys between those the kernel has compiled in and the 
> > > >>  IMA keys to be added may be added to the system secondary keyring,
> > > >>  provided they are validly signed by a key already resident in the
> > > >>  built-in or secondary trusted keyrings.
> > > >> 
> > > >> 
> > > >> The first paragraph requires "validly signed by a CA cert in the system
> > > >> built-in or secondary trusted keyrings" for keys to be loaded onto the
> > > >> IMA keyring.  This Kconfig is limited to just the builtin and secondary
> > > >> keyrings.  Changing this silently to include the ".machine" keyring
> > > >> introduces integrity risks that previously did not exist.  A new IMA
> > > >> Kconfig needs to be defined to allow all three keyrings - builtin,
> > > >> machine, and secondary.
> > > >> 
> > > >> The second paragraph implies that only CA and intermediate CA keys are
> > > >> on secondary keyring, or as in our case the ".machine" keyring linked
> > > >> to the secondary keyring.
> > > >> 
> > > >> Mimi
> > > >> 
> > > > I have also now test environment for this patch set but if there are
> > > > any possible changes, I'm waiting for a new version, as it is anyway
> > > > for 5.18 cycle earliest.
> > > 
> > > Other than the two sentence changes, I have not seen anything identified 
> > > code wise requiring a change.  If you’d like me to respin a v10 with the sentence 
> > > changes let me know.  Or if you want to remove the ima reference, that works 
> > > too.  Just let me know how you want to handle this.  Thanks.
> > 
> > I'm basically waiting also Mimi to test this as I do not have IMA test
> > environment.
> > 
> > From my side:
> > 
> > Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> I can pick the whole thing at the time when I get green light.

The MOK keys are not loaded onto the .machine keyring if
CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY is enabled.
Jarkko Sakkinen Jan. 16, 2022, 8:10 p.m. UTC | #14
On Sat, Jan 15, 2022 at 09:55:47PM -0500, Mimi Zohar wrote:
> On Sat, 2022-01-15 at 21:15 +0200, Jarkko Sakkinen wrote:
> > On Sat, Jan 15, 2022 at 09:14:45PM +0200, Jarkko Sakkinen wrote:
> > > On Sat, Jan 15, 2022 at 07:12:35PM +0000, Eric Snowberg wrote:
> > > > 
> > > > 
> > > > > On Jan 15, 2022, at 10:11 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > > > 
> > > > > On Wed, Jan 12, 2022 at 02:41:47PM -0500, Mimi Zohar wrote:
> > > > >> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
> > > > >>> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
> > > > >>>> 
> > > > >>>>> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > > >>>>> 
> > > > >>>>> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> > > > >>>>>>> Jarkko, my concern is that once this version of the patch set is
> > > > >>>>>>> upstreamed, would limiting which keys may be loaded onto the .machine
> > > > >>>>>>> keyring be considered a regression?
> > > > >>>>>> 
> > > > >>>>>> 
> > > > >>>>>> Currently certificates built into the kernel do not have a CA restriction on them.  
> > > > >>>>>> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> > > > >>>>>> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> > > > >>>>>> 
> > > > >>>>>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> > > > >>>>>> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> > > > >>>>>> Kconfig option was not set for enforcement, it would work as it does in this series, 
> > > > >>>>>> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> > > > >>>>>> the restriction placed in this patch. Let me know your thoughts on whether this would 
> > > > >>>>>> be an appropriate solution.  I believe this would get around what you are identifying as 
> > > > >>>>>> a possible regression.
> > > > >>>>> 
> > > > >>>>> True the problem currently exists with the builtin keys, but there's a
> > > > >>>>> major difference between trusting the builtin keys and those being
> > > > >>>>> loading via MOK.  This is an integrity gap that needs to be closed and
> > > > >>>>> shouldn't be expanded to keys on the .machine keyring.
> > > > >>>>> 
> > > > >>>>> "plus it would allow IMA to work with non-CA keys" is unacceptable.
> > > > >>>> 
> > > > >>>> Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
> > > > >>>> cover letter, which adds IMA support, on why it is unacceptable for the end-user to
> > > > >>>> make this decision?
> > > > >>> 
> > > > >>> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> > > > >>> "help" is very clear:
> > > > >> 
> > > > >> [Reposting the text due to email formatting issues.]
> > > > >> 
> > > > >> help
> > > > >>  Keys may be added to the IMA or IMA blacklist keyrings, if the
> > > > >>  key is validly signed by a CA cert in the system built-in or
> > > > >>  secondary trusted keyrings.
> > > > >> 
> > > > >>  Intermediate keys between those the kernel has compiled in and the 
> > > > >>  IMA keys to be added may be added to the system secondary keyring,
> > > > >>  provided they are validly signed by a key already resident in the
> > > > >>  built-in or secondary trusted keyrings.
> > > > >> 
> > > > >> 
> > > > >> The first paragraph requires "validly signed by a CA cert in the system
> > > > >> built-in or secondary trusted keyrings" for keys to be loaded onto the
> > > > >> IMA keyring.  This Kconfig is limited to just the builtin and secondary
> > > > >> keyrings.  Changing this silently to include the ".machine" keyring
> > > > >> introduces integrity risks that previously did not exist.  A new IMA
> > > > >> Kconfig needs to be defined to allow all three keyrings - builtin,
> > > > >> machine, and secondary.
> > > > >> 
> > > > >> The second paragraph implies that only CA and intermediate CA keys are
> > > > >> on secondary keyring, or as in our case the ".machine" keyring linked
> > > > >> to the secondary keyring.
> > > > >> 
> > > > >> Mimi
> > > > >> 
> > > > > I have also now test environment for this patch set but if there are
> > > > > any possible changes, I'm waiting for a new version, as it is anyway
> > > > > for 5.18 cycle earliest.
> > > > 
> > > > Other than the two sentence changes, I have not seen anything identified 
> > > > code wise requiring a change.  If you’d like me to respin a v10 with the sentence 
> > > > changes let me know.  Or if you want to remove the ima reference, that works 
> > > > too.  Just let me know how you want to handle this.  Thanks.
> > > 
> > > I'm basically waiting also Mimi to test this as I do not have IMA test
> > > environment.
> > > 
> > > From my side:
> > > 
> > > Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > I can pick the whole thing at the time when I get green light.
> 
> The MOK keys are not loaded onto the .machine keyring if
> CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY is enabled. 
> From an IMA perspective nothing has changed.
> 
> After the IMA references in the patch descriptions are removed, feel
> free to add Tested-by: Mimi Zohar <zohar@linux.ibm.com> on patches 1 -
> 5.
> 
> thanks,
> 
> Mimi

Eric, for me it would be at least a convenience, and overally it would
make sure that I pick the right thing if you would fix the typos (and
you can add all the tested-by tags of course as no functional changes).

There's been times when I've manually "just fixed typos", and failed in a
way or another because of human error. Just want to make sure that we
have exactly the right content applied, I hope you understand my point
of view. And we are early for the 5.18 release cycle anyway.

BR, Jarkko
Eric Snowberg Jan. 18, 2022, 4:32 p.m. UTC | #15
> On Jan 16, 2022, at 1:10 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Sat, Jan 15, 2022 at 09:55:47PM -0500, Mimi Zohar wrote:
>> On Sat, 2022-01-15 at 21:15 +0200, Jarkko Sakkinen wrote:
>>> On Sat, Jan 15, 2022 at 09:14:45PM +0200, Jarkko Sakkinen wrote:
>>>> On Sat, Jan 15, 2022 at 07:12:35PM +0000, Eric Snowberg wrote:
>>>>> 
>>>>> 
>>>>>> On Jan 15, 2022, at 10:11 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>>>>>> 
>>>>>> On Wed, Jan 12, 2022 at 02:41:47PM -0500, Mimi Zohar wrote:
>>>>>>> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
>>>>>>>> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
>>>>>>>>> 
>>>>>>>>>> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>>>>> 
>>>>>>>>>> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
>>>>>>>>>>>> Jarkko, my concern is that once this version of the patch set is
>>>>>>>>>>>> upstreamed, would limiting which keys may be loaded onto the .machine
>>>>>>>>>>>> keyring be considered a regression?
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> Currently certificates built into the kernel do not have a CA restriction on them.  
>>>>>>>>>>> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
>>>>>>>>>>> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
>>>>>>>>>>> 
>>>>>>>>>>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
>>>>>>>>>>> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
>>>>>>>>>>> Kconfig option was not set for enforcement, it would work as it does in this series, 
>>>>>>>>>>> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
>>>>>>>>>>> the restriction placed in this patch. Let me know your thoughts on whether this would 
>>>>>>>>>>> be an appropriate solution.  I believe this would get around what you are identifying as 
>>>>>>>>>>> a possible regression.
>>>>>>>>>> 
>>>>>>>>>> True the problem currently exists with the builtin keys, but there's a
>>>>>>>>>> major difference between trusting the builtin keys and those being
>>>>>>>>>> loading via MOK.  This is an integrity gap that needs to be closed and
>>>>>>>>>> shouldn't be expanded to keys on the .machine keyring.
>>>>>>>>>> 
>>>>>>>>>> "plus it would allow IMA to work with non-CA keys" is unacceptable.
>>>>>>>>> 
>>>>>>>>> Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
>>>>>>>>> cover letter, which adds IMA support, on why it is unacceptable for the end-user to
>>>>>>>>> make this decision?
>>>>>>>> 
>>>>>>>> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
>>>>>>>> "help" is very clear:
>>>>>>> 
>>>>>>> [Reposting the text due to email formatting issues.]
>>>>>>> 
>>>>>>> help
>>>>>>> Keys may be added to the IMA or IMA blacklist keyrings, if the
>>>>>>> key is validly signed by a CA cert in the system built-in or
>>>>>>> secondary trusted keyrings.
>>>>>>> 
>>>>>>> Intermediate keys between those the kernel has compiled in and the 
>>>>>>> IMA keys to be added may be added to the system secondary keyring,
>>>>>>> provided they are validly signed by a key already resident in the
>>>>>>> built-in or secondary trusted keyrings.
>>>>>>> 
>>>>>>> 
>>>>>>> The first paragraph requires "validly signed by a CA cert in the system
>>>>>>> built-in or secondary trusted keyrings" for keys to be loaded onto the
>>>>>>> IMA keyring.  This Kconfig is limited to just the builtin and secondary
>>>>>>> keyrings.  Changing this silently to include the ".machine" keyring
>>>>>>> introduces integrity risks that previously did not exist.  A new IMA
>>>>>>> Kconfig needs to be defined to allow all three keyrings - builtin,
>>>>>>> machine, and secondary.
>>>>>>> 
>>>>>>> The second paragraph implies that only CA and intermediate CA keys are
>>>>>>> on secondary keyring, or as in our case the ".machine" keyring linked
>>>>>>> to the secondary keyring.
>>>>>>> 
>>>>>>> Mimi
>>>>>>> 
>>>>>> I have also now test environment for this patch set but if there are
>>>>>> any possible changes, I'm waiting for a new version, as it is anyway
>>>>>> for 5.18 cycle earliest.
>>>>> 
>>>>> Other than the two sentence changes, I have not seen anything identified 
>>>>> code wise requiring a change.  If you’d like me to respin a v10 with the sentence 
>>>>> changes let me know.  Or if you want to remove the ima reference, that works 
>>>>> too.  Just let me know how you want to handle this.  Thanks.
>>>> 
>>>> I'm basically waiting also Mimi to test this as I do not have IMA test
>>>> environment.
>>>> 
>>>> From my side:
>>>> 
>>>> Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
>>> 
>>> I can pick the whole thing at the time when I get green light.
>> 
>> The MOK keys are not loaded onto the .machine keyring if
>> CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY is enabled. 
>> From an IMA perspective nothing has changed.
>> 
>> After the IMA references in the patch descriptions are removed, feel
>> free to add Tested-by: Mimi Zohar <zohar@linux.ibm.com> on patches 1 -
>> 5.
>> 
>> thanks,
>> 
>> Mimi
> 
> Eric, for me it would be at least a convenience, and overally it would
> make sure that I pick the right thing if you would fix the typos (and
> you can add all the tested-by tags of course as no functional changes).
> 
> There's been times when I've manually "just fixed typos", and failed in a
> way or another because of human error. Just want to make sure that we
> have exactly the right content applied, I hope you understand my point
> of view. And we are early for the 5.18 release cycle anyway.

No problem, I’ll put together a v10 with the changes.  Thanks for your review.
diff mbox series

Patch

diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index 71f0177e8716..de02964dd421 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -62,6 +62,19 @@  config INTEGRITY_PLATFORM_KEYRING
          provided by the platform for verifying the kexec'ed kerned image
          and, possibly, the initramfs signature.
 
+config INTEGRITY_MACHINE_KEYRING
+	bool "Provide a keyring to which CA Machine Owner Keys may be added"
+	depends on SECONDARY_TRUSTED_KEYRING
+	depends on INTEGRITY_ASYMMETRIC_KEYS
+	depends on SYSTEM_BLACKLIST_KEYRING
+	depends on LOAD_UEFI_KEYS
+	depends on !IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
+	help
+	 If set, provide a keyring to which CA Machine Owner Keys (MOK) may
+	 be added. This keyring shall contain just CA MOK keys.  Unlike keys
+	 in the platform keyring, keys contained in the .machine keyring will
+	 be trusted within the kernel.
+
 config LOAD_UEFI_KEYS
        depends on INTEGRITY_PLATFORM_KEYRING
        depends on EFI
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index 7ee39d66cf16..d0ffe37dc1d6 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -10,6 +10,7 @@  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_MACHINE_KEYRING) += platform_certs/machine_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..2b7fa85613c0 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",
+	".machine",
 };
 
 #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
@@ -126,7 +127,8 @@  int __init integrity_init_keyring(const unsigned int id)
 	perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW
 		| KEY_USR_READ | KEY_USR_SEARCH;
 
-	if (id == INTEGRITY_KEYRING_PLATFORM) {
+	if (id == INTEGRITY_KEYRING_PLATFORM ||
+	    id == INTEGRITY_KEYRING_MACHINE) {
 		restriction = NULL;
 		goto out;
 	}
@@ -139,7 +141,14 @@  int __init integrity_init_keyring(const unsigned int id)
 		return -ENOMEM;
 
 	restriction->check = restrict_link_to_ima;
-	perm |= KEY_USR_WRITE;
+
+	/*
+	 * MOK keys can only be added through a read-only runtime services
+	 * UEFI variable during boot. No additional keys shall be allowed to
+	 * load into the machine keyring following init from userspace.
+	 */
+	if (id != INTEGRITY_KEYRING_MACHINE)
+		perm |= KEY_USR_WRITE;
 
 out:
 	return __integrity_init_keyring(id, perm, restriction);
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 547425c20e11..730771eececd 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_MACHINE	3
+#define INTEGRITY_KEYRING_MAX		4
 
 extern struct dentry *integrity_dir;
 
@@ -283,3 +284,12 @@  static inline void __init add_to_platform_keyring(const char *source,
 {
 }
 #endif
+
+#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+void __init add_to_machine_keyring(const char *source, const void *data, size_t len);
+#else
+static inline void __init add_to_machine_keyring(const char *source,
+						  const void *data, size_t len)
+{
+}
+#endif
diff --git a/security/integrity/platform_certs/machine_keyring.c b/security/integrity/platform_certs/machine_keyring.c
new file mode 100644
index 000000000000..ea2ac2f9f2b5
--- /dev/null
+++ b/security/integrity/platform_certs/machine_keyring.c
@@ -0,0 +1,42 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Machine keyring routines.
+ *
+ * Copyright (c) 2021, Oracle and/or its affiliates.
+ */
+
+#include "../integrity.h"
+
+static __init int machine_keyring_init(void)
+{
+	int rc;
+
+	rc = integrity_init_keyring(INTEGRITY_KEYRING_MACHINE);
+	if (rc)
+		return rc;
+
+	pr_notice("Machine keyring initialized\n");
+	return 0;
+}
+device_initcall(machine_keyring_init);
+
+void __init add_to_machine_keyring(const char *source, const void *data, size_t len)
+{
+	key_perm_t perm;
+	int rc;
+
+	perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW;
+	rc = integrity_load_cert(INTEGRITY_KEYRING_MACHINE, source, data, len, perm);
+
+	/*
+	 * Some MOKList keys may not pass the machine keyring restrictions.
+	 * If the restriction check does not pass and the platform keyring
+	 * is configured, try to add it into that keyring instead.
+	 */
+	if (rc && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
+		rc = integrity_load_cert(INTEGRITY_KEYRING_PLATFORM, source,
+					 data, len, perm);
+
+	if (rc)
+		pr_info("Error adding keys to machine keyring %s\n", source);
+}