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

From: Eric Snowberg <eric.snowberg@oracle.com>
To: Mimi Zohar <zohar@linux.ibm.com>
Cc: keyrings@vger.kernel.org,
	linux-integrity <linux-integrity@vger.kernel.org>,
	David Howells <dhowells@redhat.com>,
	David Woodhouse <dwmw2@infradead.org>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	"David S . Miller" <davem@davemloft.net>,
	Jarkko Sakkinen <jarkko@kernel.org>,
	James Morris <jmorris@namei.org>,
	"Serge E . Hallyn" <serge@hallyn.com>,
	keescook@chromium.org, gregkh@linuxfoundation.org,
	torvalds@linux-foundation.org, scott.branden@broadcom.com,
	weiyongjun1@huawei.com, nayna@linux.ibm.com, ebiggers@google.com,
	ardb@kernel.org, nramas@linux.microsoft.com, lszubowi@redhat.com,
	linux-kernel@vger.kernel.org, linux-crypto@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	James.Bottomley@HansenPartnership.com, pjones@redhat.com,
	glin@suse.com, "konrad.wilk@oracle.com" <konrad.wilk@oracle.com>
Subject: Re: [PATCH RFC v2 10/12] KEYS: link system_trusted_keys to mok_trusted_keys
Date: Thu, 5 Aug 2021 19:29:44 -0600	[thread overview]
Message-ID: <44ADB68B-4310-462B-96A8-2F69759BA2D8@oracle.com> (raw)
In-Reply-To: <6c751dadf4ce7385d0391ea26f1c7e4e910219e0.camel@linux.ibm.com>

> On Aug 5, 2021, at 7:58 AM, Mimi Zohar <zohar@linux.ibm.com> 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.