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

[Eric Snowberg]

> On Jan 16, 2022, at 1:10 PM, Jarkko Sakkinen <jarkko@xxxxxxxxxx> 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@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.
>> 
>> 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@xxxxxxxxxxxxx> 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.