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

From: Jarkko Sakkinen
Date: Sat Jan 15 2022 - 14:16:04 EST


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

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

/Jarkko