Re: [PATCH RFC v2 10/12] KEYS: link system_trusted_keys to mok_trusted_keys

[Eric Snowberg]

> On Aug 5, 2021, at 7:58 AM, Mimi Zohar <zohar@xxxxxxxxxxxxx> wrote:
> 
> On Mon, 2021-07-26 at 13:13 -0400, Eric Snowberg wrote:
> 
>> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
>> index dcaf74102ab2..b27ae30eaadc 100644
>> --- a/certs/system_keyring.c
>> +++ b/certs/system_keyring.c
>> @@ -45,6 +45,15 @@ int restrict_link_by_builtin_trusted(struct key *dest_keyring,
>> 				     const union key_payload *payload,
>> 				     struct key *restriction_key)
>> {
>> +	/* If the secondary trusted keyring is not enabled, we may link
>> +	 * through to the mok keyring and the search may follow that link.
>> +	 */
> 
> Refer to section "8) Commenting" of Documentation/process/coding-
> style.rst for the format of multi line comments.

Sure, I’ll fix this in the next version.

>> +	if (mok_trusted_keys && type == &key_type_keyring &&
>> +	    dest_keyring == builtin_trusted_keys &&
>> +	    payload == &mok_trusted_keys->payload)
>> +		/* Allow the mok keyring to be added to the builtin */
>> +		return 0;
>> +
> 
> Unless you're changing the meaning of the restriction, then a new
> restriction needs to be defined.  In this case, please don't change the
> meaning of restrict_link_by_builtin_trusted().  Instead define a new
> restriction named restrict_link_by_builtin_and_ca_trusted().


Along with this

>> 	return restrict_link_by_signature(dest_keyring, type, payload,
>> 					  builtin_trusted_keys);
>> }
>> @@ -91,6 +100,15 @@ int restrict_link_by_builtin_and_secondary_trusted(
>> 		/* Allow the builtin keyring to be added to the secondary */
>> 		return 0;
>> 
>> +	/* If we have a secondary trusted keyring, it may contain a link
>> +	 * through to the mok keyring and the search may follow that link.
>> +	 */
>> +	if (mok_trusted_keys && type == &key_type_keyring &&
>> +	    dest_keyring == secondary_trusted_keys &&
>> +	    payload == &mok_trusted_keys->payload)
>> +		/* Allow the mok keyring to be added to the secondary */
>> +		return 0;
>> +
> 
> Similarly here, please define a new restriction maybe named
> restrict_link_by_builtin_secondary_and_ca_trusted().   To avoid code
> duplication, the new restriction could be a wrapper around the existing
> function.

and this too.

> 
>> 	return restrict_link_by_signature(dest_keyring, type, payload,
>> 					  secondary_trusted_keys);
>> }
>> @@ -321,5 +339,8 @@ void __init set_platform_trusted_keys(struct key *keyring)
>> void __init set_mok_trusted_keys(struct key *keyring)
>> {
>> 	mok_trusted_keys = keyring;
>> +
>> +	if (key_link(system_trusted_keys, mok_trusted_keys) < 0)
>> +		panic("Can't link (mok) trusted keyrings\n");
>> }
> 
> From the thread discussion on 00/12:
> 
> Only the builtin keys should ever be on the builtin keyring.  The
> builtin keyring would need to be linked to the mok keyring.  But in the
> secondary keyring case, the mok keyring would be linked to the
> secondary keyring, similar to how the builtin keyring is linked to the
> secondary keyring.
> 
>        if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0)
>                panic("Can't link trusted keyrings\n");


This part is confusing me though.

Here are some of the tests I’m performing with the current series:

Initial setup:
Create and enroll my own key into the MOK.
Sign a kernel, kernel module and IMA key with my new CA key.
Boot with lockdown enabled (to enforce sig validation).

Kernel built with CONFIG_SECONDARY_TRUSTED_KEYRING=y

$ keyctl show %:.secondary_trusted_keys
Keyring
 530463486 ---lswrv      0     0  keyring: .secondary_trusted_keys
 411466727 ---lswrv      0     0   \_ keyring: .builtin_trusted_keys
 979167715 ---lswrv      0     0   |   \_ asymmetric: Build time autogenerated kernel key: 07a56e29cfa1e21379aff2c522efff7d1963202a
 534573591 ---lswrv      0     0   |   \_ asymmetric: Oracle-CA: Oracle certificate signing key: aeefb4bfde095cacaabff81dd266974b1b4e23b8
 968109018 ---lswrv      0     0   \_ keyring: .mok
 857795115 ---lswrv      0     0       \_ asymmetric: Erics-CA: UEK signing key: 9bfa6860483aa46bd83f7fa1289d9fc35799e93b

With this setup I can:
* load a kernel module signed with my CA key
* run "kexec -ls" with the kernel signed with my CA key
* run "kexec -ls" with a kernel signed by a key in the platform keyring
* load another key into the secondary trusted keyring that is signed by my CA key
* load a key into the ima keyring, signed by my CA key

Kernel built without CONFIG_SECONDARY_TRUSTED_KEYRING defined

$ keyctl show %:.builtin_trusted_keys
Keyring
 812785375 ---lswrv      0     0  keyring: .builtin_trusted_keys
 455418681 ---lswrv      0     0   \_ keyring: .mok
 910809006 ---lswrv      0     0   |   \_ asymmetric: Erics-CA: UEK signing key: 9bfa6860483aa46bd83f7fa1289d9fc35799e93b
 115345009 ---lswrv      0     0   \_ asymmetric: Oracle-CA: Oracle certificate signing key: aeefb4bfde095cacaabff81dd266974b1b4e23b8
 513131506 ---lswrv      0     0   \_ asymmetric: Build time autogenerated kernel key: 22353509f203b55b84f15d0aadeddc134b646185

With this setup I can:
* load a kernel module signed with my CA key
* run "kexec -ls" with the kernel signed with my CA key
* run "kexec -ls" with a kernel signed by a key in the platform keyring
* load a key into the ima keyring, signed by my CA key

So why would the linking need to be switched?  Is there a test I’m
missing?  Thanks.