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

[Jarkko Sakkinen]

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@xxxxxxxxxxxxx> 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