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

[Eric Snowberg]

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