[PATCH v6 00/13] Enroll kernel keys thru MOK

[PATCH v6 00/13] Enroll kernel keys thru MOK

[Eric Snowberg]

Back in 2013 Linus requested a feature to allow end-users to have the 
ability "to add their own keys and sign modules they trust". This was
his *second* order outlined here [1]. There have been many attempts 
over the years to solve this problem, all have been rejected.  Many 
of the failed attempts loaded all preboot firmware keys into the kernel,
including the Secure Boot keys. Many distributions carry one of these 
rejected attempts [2], [3], [4]. This series tries to solve this problem 
with a solution that takes into account all the problems brought up in 
the previous attempts.

On UEFI based systems, this series introduces a new Linux kernel keyring 
containing the Machine Owner Keys (MOK) called machine. It also defines
a new MOK variable in shim. This variable allows the end-user to decide 
if they want to load MOK keys into the machine keyring. Mimi has suggested 
that only CA keys contained within the MOK be loaded into the machine 
keyring. All other certs will load into the platform keyring instead.

By default, nothing changes; MOK keys are not loaded into the machine
keyring.  They are only loaded after the end-user makes the decision 
themselves.  The end-user would set this through mokutil using a new 
--trust-mok option [5]. This would work similar to how the kernel uses 
MOK variables to enable/disable signature validation as well as use/ignore 
the db. Any kernel operation that uses either the builtin or secondary 
trusted keys as a trust source shall also reference the new machine 
keyring as a trust source.

Secure Boot keys will never be loaded into the machine keyring.  They
will always be loaded into the platform keyring.  If an end-user wanted 
to load one, they would need to enroll it into the MOK.

Steps required by the end user:

Sign kernel module with user created key:
$ /usr/src/kernels/$(uname -r)/scripts/sign-file sha512 \
   machine_signing_key.priv machine_signing_key.x509 my_module.ko

Import the key into the MOK
$ mokutil --import machine_signing_key.x509

Setup the kernel to load MOK keys into the .machine keyring
$ mokutil --trust-mok

Then reboot, the MokManager will load and ask if you want to trust the
MOK key and enroll the MOK into the MOKList.  Afterwards the signed kernel
module will load.

I have included links to both the mokutil [5] and shim [6] changes I
have made to support this new functionality.

[1] https://marc.info/?l=linux-kernel&m=136185386310140&w=2
[2] https://lore.kernel.org/lkml/1479737095.2487.34.camel@linux.vnet.ibm.com/
[3] https://lore.kernel.org/lkml/1556221605.24945.3.camel@HansenPartnership.com/
[4] https://lore.kernel.org/linux-integrity/1e41f22b1f11784f1e943f32bf62034d4e054cdb.camel@HansenPartnership.com/
[5] https://github.com/esnowberg/mokutil/tree/0.3.0-mokvars-v2
[6] https://github.com/esnowberg/shim/tree/mokvars-v2

Eric Snowberg (13):
  integrity: Introduce a Linux keyring called machine
  integrity: Do not allow machine keyring updates following init
  KEYS: CA link restriction
  integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca
  integrity: add new keyring handler for mok keys
  KEYS: Rename get_builtin_and_secondary_restriction
  KEYS: add a reference to machine keyring
  KEYS: Introduce link restriction for machine keys
  KEYS: integrity: change link restriction to trust the machine keyring
  KEYS: link secondary_trusted_keys to machine trusted keys
  integrity: store reference to machine keyring
  integrity: Trust MOK keys if MokListTrustedRT found
  integrity: Only use machine keyring when uefi_check_trust_mok_keys is
    true

 certs/system_keyring.c                        | 44 +++++++++-
 crypto/asymmetric_keys/restrict.c             | 40 +++++++++
 include/crypto/public_key.h                   |  5 ++
 include/keys/system_keyring.h                 | 14 +++
 security/integrity/Kconfig                    | 12 +++
 security/integrity/Makefile                   |  1 +
 security/integrity/digsig.c                   | 23 ++++-
 security/integrity/integrity.h                | 17 +++-
 .../platform_certs/keyring_handler.c          | 17 +++-
 .../platform_certs/keyring_handler.h          |  5 ++
 security/integrity/platform_certs/load_uefi.c |  4 +-
 .../platform_certs/machine_keyring.c          | 85 +++++++++++++++++++
 12 files changed, 257 insertions(+), 10 deletions(-)
 create mode 100644 security/integrity/platform_certs/machine_keyring.c


base-commit: 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f
-- 
2.18.4

[PATCH v6 01/13] integrity: Introduce a Linux keyring called machine

[Eric Snowberg]

Many UEFI Linux distributions boot using shim.  The UEFI shim provides
what is called Machine Owner Keys (MOK). Shim uses both the UEFI Secure
Boot DB and MOK keys to validate the next step in the boot chain.  The
MOK facility can be used to import user generated keys.  These keys can
be used to sign an end-users development kernel build.  When Linux
boots, both UEFI Secure Boot DB and MOK keys get loaded in the Linux
.platform keyring.

Define a new Linux keyring called machine.  This keyring shall contain just
MOK CA keys and not the remaining keys in the platform keyring. This new
machine keyring will be used in follow on patches.  Unlike keys in the
platform keyring, keys contained in the machine keyring will be trusted
within the kernel if the end-user has chosen to do so.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v2: Removed destory keyring code
v3: Unmodified from v2
v4: Add Kconfig, merged in "integrity: add add_to_mok_keyring" 
v5: Rename to machine keyring
v6: Depend on EFI in kconfig  (suggested by Mimi)
    Test to see if ".platform" keyring is configured in
      add_to_machine_keyring (suggested by Mimi)
---
 security/integrity/Kconfig                    | 12 ++++++
 security/integrity/Makefile                   |  1 +
 security/integrity/digsig.c                   |  1 +
 security/integrity/integrity.h                | 12 +++++-
 .../platform_certs/machine_keyring.c          | 42 +++++++++++++++++++
 5 files changed, 67 insertions(+), 1 deletion(-)
 create mode 100644 security/integrity/platform_certs/machine_keyring.c

diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index 71f0177e8716..5e9b3de46900 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -62,6 +62,18 @@ config INTEGRITY_PLATFORM_KEYRING
          provided by the platform for verifying the kexec'ed kerned image
          and, possibly, the initramfs signature.
 
+config INTEGRITY_MACHINE_KEYRING
+	bool "Provide a keyring to which CA Machine Owner Keys may be added"
+	depends on SECONDARY_TRUSTED_KEYRING
+	depends on INTEGRITY_ASYMMETRIC_KEYS
+	depends on SYSTEM_BLACKLIST_KEYRING
+	depends on EFI
+	help
+	 If set, provide a keyring to which CA Machine Owner Keys (MOK) may
+	 be added. This keyring shall contain just CA MOK keys.  Unlike keys
+	 in the platform keyring, keys contained in the .machine keyring will
+	 be trusted within the kernel.
+
 config LOAD_UEFI_KEYS
        depends on INTEGRITY_PLATFORM_KEYRING
        depends on EFI
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index 7ee39d66cf16..d0ffe37dc1d6 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -10,6 +10,7 @@ integrity-$(CONFIG_INTEGRITY_AUDIT) += integrity_audit.o
 integrity-$(CONFIG_INTEGRITY_SIGNATURE) += digsig.o
 integrity-$(CONFIG_INTEGRITY_ASYMMETRIC_KEYS) += digsig_asymmetric.o
 integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += platform_certs/platform_keyring.o
+integrity-$(CONFIG_INTEGRITY_MACHINE_KEYRING) += platform_certs/machine_keyring.o
 integrity-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/efi_parser.o \
 				      platform_certs/load_uefi.o \
 				      platform_certs/keyring_handler.o
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 3b06a01bd0fd..8c315be8ad99 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -30,6 +30,7 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
 	".ima",
 #endif
 	".platform",
+	".machine",
 };
 
 #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 547425c20e11..730771eececd 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -151,7 +151,8 @@ int integrity_kernel_read(struct file *file, loff_t offset,
 #define INTEGRITY_KEYRING_EVM		0
 #define INTEGRITY_KEYRING_IMA		1
 #define INTEGRITY_KEYRING_PLATFORM	2
-#define INTEGRITY_KEYRING_MAX		3
+#define INTEGRITY_KEYRING_MACHINE	3
+#define INTEGRITY_KEYRING_MAX		4
 
 extern struct dentry *integrity_dir;
 
@@ -283,3 +284,12 @@ static inline void __init add_to_platform_keyring(const char *source,
 {
 }
 #endif
+
+#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+void __init add_to_machine_keyring(const char *source, const void *data, size_t len);
+#else
+static inline void __init add_to_machine_keyring(const char *source,
+						  const void *data, size_t len)
+{
+}
+#endif
diff --git a/security/integrity/platform_certs/machine_keyring.c b/security/integrity/platform_certs/machine_keyring.c
new file mode 100644
index 000000000000..ea2ac2f9f2b5
--- /dev/null
+++ b/security/integrity/platform_certs/machine_keyring.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Machine keyring routines.
+ *
+ * Copyright (c) 2021, Oracle and/or its affiliates.
+ */
+
+#include "../integrity.h"
+
+static __init int machine_keyring_init(void)
+{
+	int rc;
+
+	rc = integrity_init_keyring(INTEGRITY_KEYRING_MACHINE);
+	if (rc)
+		return rc;
+
+	pr_notice("Machine keyring initialized\n");
+	return 0;
+}
+device_initcall(machine_keyring_init);
+
+void __init add_to_machine_keyring(const char *source, const void *data, size_t len)
+{
+	key_perm_t perm;
+	int rc;
+
+	perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW;
+	rc = integrity_load_cert(INTEGRITY_KEYRING_MACHINE, source, data, len, perm);
+
+	/*
+	 * Some MOKList keys may not pass the machine keyring restrictions.
+	 * If the restriction check does not pass and the platform keyring
+	 * is configured, try to add it into that keyring instead.
+	 */
+	if (rc && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
+		rc = integrity_load_cert(INTEGRITY_KEYRING_PLATFORM, source,
+					 data, len, perm);
+
+	if (rc)
+		pr_info("Error adding keys to machine keyring %s\n", source);
+}
-- 
2.18.4

[PATCH v6 02/13] integrity: Do not allow machine keyring updates following init

[Eric Snowberg]

The machine keyring is setup during init.  No additional keys should be
allowed to be added afterwards.  Leave the permission as read only.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v2: Initial version
v4: Unmodified from v2
v5: Rename to machine keyring
v6: Add additional comment (suggested by Jarkko)
---
 security/integrity/digsig.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 8c315be8ad99..910fe29a5037 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -140,7 +140,13 @@ int __init integrity_init_keyring(const unsigned int id)
 		return -ENOMEM;
 
 	restriction->check = restrict_link_to_ima;
-	perm |= KEY_USR_WRITE;
+
+	/*
+	 * No additional keys shall be allowed to load into the machine
+	 * keyring following init
+	 */
+	if (id != INTEGRITY_KEYRING_MACHINE)
+		perm |= KEY_USR_WRITE;
 
 out:
 	return __integrity_init_keyring(id, perm, restriction);
-- 
2.18.4

[PATCH v6 03/13] KEYS: CA link restriction

[Eric Snowberg]

Add a new link restriction.  Restrict the addition of keys in a keyring
based on the key to be added being a CA (self-signed).

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v2: Removed secondary keyring references
v3: Removed restrict_link_by_system_trusted_or_ca
    Simplify restrict_link_by_ca - only see if the key is a CA
    Did not add __init in front of restrict_link_by_ca in case
      restriction could be resued in the future
v6: Unmodified from v3
---
 crypto/asymmetric_keys/restrict.c | 40 +++++++++++++++++++++++++++++++
 include/crypto/public_key.h       |  5 ++++
 2 files changed, 45 insertions(+)

diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 84cefe3b3585..9ae43d3f862b 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -108,6 +108,46 @@ int restrict_link_by_signature(struct key *dest_keyring,
 	return ret;
 }
 
+/**
+ * restrict_link_by_ca - Restrict additions to a ring of CA keys
+ * @dest_keyring: Keyring being linked to.
+ * @type: The type of key being added.
+ * @payload: The payload of the new key.
+ * @trusted: Unused.
+ *
+ * Check if the new certificate is a CA. If it is a CA, then mark the new
+ * certificate as being ok to link.
+ *
+ * Returns 0 if the new certificate was accepted, -ENOKEY if we could not find
+ * a matching parent certificate in the trusted list.  -ENOPKG if the signature
+ * uses unsupported crypto, or some other error if there is a matching
+ * certificate  but the signature check cannot be performed.
+ */
+int restrict_link_by_ca(struct key *dest_keyring,
+			const struct key_type *type,
+			const union key_payload *payload,
+			struct key *trust_keyring)
+{
+	const struct public_key_signature *sig;
+	const struct public_key *pkey;
+
+	if (type != &key_type_asymmetric)
+		return -EOPNOTSUPP;
+
+	sig = payload->data[asym_auth];
+	if (!sig)
+		return -ENOPKG;
+
+	if (!sig->auth_ids[0] && !sig->auth_ids[1])
+		return -ENOKEY;
+
+	pkey = payload->data[asym_crypto];
+	if (!pkey)
+		return -ENOPKG;
+
+	return public_key_verify_signature(pkey, sig);
+}
+
 static bool match_either_id(const struct asymmetric_key_ids *pair,
 			    const struct asymmetric_key_id *single)
 {
diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
index f603325c0c30..3ef299e2b008 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -71,6 +71,11 @@ extern int restrict_link_by_key_or_keyring_chain(struct key *trust_keyring,
 						 const union key_payload *payload,
 						 struct key *trusted);
 
+extern int restrict_link_by_ca(struct key *dest_keyring,
+			       const struct key_type *type,
+			       const union key_payload *payload,
+			       struct key *trust_keyring);
+
 extern int query_asymmetric_key(const struct kernel_pkey_params *,
 				struct kernel_pkey_query *);
 
-- 
2.18.4

[PATCH v6 04/13] integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca

[Eric Snowberg]

Set the restriction check for INTEGRITY_KEYRING_MACHINE keys to
restrict_link_by_ca.  This will only allow CA keys into the machine
keyring.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v2: Added !IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING check so mok
    keyring gets created even when it isn't enabled
v3: Rename restrict_link_by_system_trusted_or_ca to restrict_link_by_ca
v4: removed unnecessary restriction->check set
v5: Rename to machine keyring
v6: split line over 80 char (suggested by Mimi)
---
 security/integrity/digsig.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 910fe29a5037..e7dfc55a7c55 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -132,14 +132,18 @@ int __init integrity_init_keyring(const unsigned int id)
 		goto out;
 	}
 
-	if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING))
+	if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING) &&
+	    id != INTEGRITY_KEYRING_MACHINE)
 		return 0;
 
 	restriction = kzalloc(sizeof(struct key_restriction), GFP_KERNEL);
 	if (!restriction)
 		return -ENOMEM;
 
-	restriction->check = restrict_link_to_ima;
+	if (id == INTEGRITY_KEYRING_MACHINE)
+		restriction->check = restrict_link_by_ca;
+	else
+		restriction->check = restrict_link_to_ima;
 
 	/*
 	 * No additional keys shall be allowed to load into the machine
-- 
2.18.4

[PATCH v6 05/13] integrity: add new keyring handler for mok keys

[Eric Snowberg]

Currently both Secure Boot DB and Machine Owner Keys (MOK) go through
the same keyring handler (get_handler_for_db). With the addition of the
new machine keyring, the end-user may choose to trust MOK keys.

Introduce a new keyring handler specific for MOK keys.  If MOK keys are
trusted by the end-user, use the new keyring handler instead.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v3: Only change the keyring handler if the secondary is enabled
v4: Removed trust_moklist check
v5: Rename to machine keyring
v6: Unmodified from v5
---
 .../integrity/platform_certs/keyring_handler.c  | 17 ++++++++++++++++-
 .../integrity/platform_certs/keyring_handler.h  |  5 +++++
 security/integrity/platform_certs/load_uefi.c   |  4 ++--
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
index 5604bd57c990..445d413aec74 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -66,7 +66,7 @@ static __init void uefi_revocation_list_x509(const char *source,
 
 /*
  * Return the appropriate handler for particular signature list types found in
- * the UEFI db and MokListRT tables.
+ * the UEFI db tables.
  */
 __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
 {
@@ -75,6 +75,21 @@ __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
 	return 0;
 }
 
+/*
+ * Return the appropriate handler for particular signature list types found in
+ * the MokListRT tables.
+ */
+__init efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type)
+{
+	if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0) {
+		if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING))
+			return add_to_machine_keyring;
+		else
+			return add_to_platform_keyring;
+	}
+	return 0;
+}
+
 /*
  * Return the appropriate handler for particular signature list types found in
  * the UEFI dbx and MokListXRT tables.
diff --git a/security/integrity/platform_certs/keyring_handler.h b/security/integrity/platform_certs/keyring_handler.h
index 2462bfa08fe3..284558f30411 100644
--- a/security/integrity/platform_certs/keyring_handler.h
+++ b/security/integrity/platform_certs/keyring_handler.h
@@ -24,6 +24,11 @@ void blacklist_binary(const char *source, const void *data, size_t len);
  */
 efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type);
 
+/*
+ * Return the handler for particular signature list types found in the mok.
+ */
+efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type);
+
 /*
  * Return the handler for particular signature list types found in the dbx.
  */
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index f290f78c3f30..c1bfd1cd7cc3 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -94,7 +94,7 @@ static int __init load_moklist_certs(void)
 		rc = parse_efi_signature_list("UEFI:MokListRT (MOKvar table)",
 					      mokvar_entry->data,
 					      mokvar_entry->data_size,
-					      get_handler_for_db);
+					      get_handler_for_mok);
 		/* All done if that worked. */
 		if (!rc)
 			return rc;
@@ -109,7 +109,7 @@ static int __init load_moklist_certs(void)
 	mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
 	if (mok) {
 		rc = parse_efi_signature_list("UEFI:MokListRT",
-					      mok, moksize, get_handler_for_db);
+					      mok, moksize, get_handler_for_mok);
 		kfree(mok);
 		if (rc)
 			pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
-- 
2.18.4

[PATCH v6 06/13] KEYS: Rename get_builtin_and_secondary_restriction

[Eric Snowberg]

To improve clarity, rename get_builtin_and_secondary_restriction to
get_secondary_restriction.

Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v6: Initial version
---
 certs/system_keyring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 692365dee2bd..8f1f87579819 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -77,7 +77,7 @@ int restrict_link_by_builtin_and_secondary_trusted(
  * Allocate a struct key_restriction for the "builtin and secondary trust"
  * keyring. Only for use in system_trusted_keyring_init().
  */
-static __init struct key_restriction *get_builtin_and_secondary_restriction(void)
+static __init struct key_restriction *get_secondary_restriction(void)
 {
 	struct key_restriction *restriction;
 
@@ -117,7 +117,7 @@ static __init int system_trusted_keyring_init(void)
 			       KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH |
 			       KEY_USR_WRITE),
 			      KEY_ALLOC_NOT_IN_QUOTA,
-			      get_builtin_and_secondary_restriction(),
+			      get_secondary_restriction(),
 			      NULL);
 	if (IS_ERR(secondary_trusted_keys))
 		panic("Can't allocate secondary trusted keyring\n");
-- 
2.18.4

[PATCH v6 07/13] KEYS: add a reference to machine keyring

[Eric Snowberg]

Expose the .machine keyring created in integrity code by adding
a reference.  This makes the machine keyring accessible for keyring
restrictions in the future.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v2: Initial version
v3: set_mok_trusted_keys only available when secondary is enabled
v4: Moved code under CONFIG_INTEGRITY_MOK_KEYRING
v5: Rename to machine keyring
v6: Unmodified from v5
---
 certs/system_keyring.c        | 9 +++++++++
 include/keys/system_keyring.h | 8 ++++++++
 2 files changed, 17 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 8f1f87579819..bc7e44fc82c2 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -22,6 +22,9 @@ static struct key *builtin_trusted_keys;
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
 static struct key *secondary_trusted_keys;
 #endif
+#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+static struct key *machine_trusted_keys;
+#endif
 #ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
 static struct key *platform_trusted_keys;
 #endif
@@ -91,6 +94,12 @@ static __init struct key_restriction *get_secondary_restriction(void)
 	return restriction;
 }
 #endif
+#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+void __init set_machine_trusted_keys(struct key *keyring)
+{
+	machine_trusted_keys = keyring;
+}
+#endif
 
 /*
  * Create the trusted keyrings
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 6acd3cf13a18..98c9b10cdc17 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -38,6 +38,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
 #define restrict_link_by_builtin_and_secondary_trusted restrict_link_by_builtin_trusted
 #endif
 
+#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+extern void __init set_machine_trusted_keys(struct key *keyring);
+#else
+static inline void __init set_machine_trusted_keys(struct key *keyring)
+{
+}
+#endif
+
 extern struct pkcs7_message *pkcs7;
 #ifdef CONFIG_SYSTEM_BLACKLIST_KEYRING
 extern int mark_hash_blacklisted(const char *hash);
-- 
2.18.4

[PATCH v6 08/13] KEYS: Introduce link restriction for machine keys

[Eric Snowberg]

Introduce a new link restriction that includes the trusted builtin,
secondary and machine keys. The restriction is based on the key to be
added being vouched for by a key in any of these three keyrings.

Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v3: Initial version
v4: moved code under CONFIG_INTEGRITY_MOK_KEYRING
v5: Rename to machine keyring
v6: Change subject name (suggested by Mimi)
    Rename restrict_link_by_builtin_secondary_and_ca_trusted
      to restrict_link_by_builtin_secondary_and_machine (suggested by
      Mimi)
---
 certs/system_keyring.c        | 23 +++++++++++++++++++++++
 include/keys/system_keyring.h |  6 ++++++
 2 files changed, 29 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index bc7e44fc82c2..71a00add9805 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -99,6 +99,29 @@ void __init set_machine_trusted_keys(struct key *keyring)
 {
 	machine_trusted_keys = keyring;
 }
+
+/**
+ * restrict_link_by_builtin_secondary_and_machine
+ *
+ * Restrict the addition of keys into a keyring based on the key-to-be-added
+ * being vouched for by a key in either the built-in, the secondary, or
+ * the machine keyrings.
+ */
+int restrict_link_by_builtin_secondary_and_machine(
+	struct key *dest_keyring,
+	const struct key_type *type,
+	const union key_payload *payload,
+	struct key *restrict_key)
+{
+	if (machine_trusted_keys && type == &key_type_keyring &&
+	    dest_keyring == secondary_trusted_keys &&
+	    payload == &machine_trusted_keys->payload)
+		/* Allow the machine keyring to be added to the secondary */
+		return 0;
+
+	return restrict_link_by_builtin_and_secondary_trusted(dest_keyring, type,
+							      payload, restrict_key);
+}
 #endif
 
 /*
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 98c9b10cdc17..2419a735420f 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -39,8 +39,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
 #endif
 
 #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+extern int restrict_link_by_builtin_secondary_and_machine(
+	struct key *dest_keyring,
+	const struct key_type *type,
+	const union key_payload *payload,
+	struct key *restrict_key);
 extern void __init set_machine_trusted_keys(struct key *keyring);
 #else
+#define restrict_link_by_builtin_secondary_and_machine restrict_link_by_builtin_trusted
 static inline void __init set_machine_trusted_keys(struct key *keyring)
 {
 }
-- 
2.18.4

[PATCH v6 09/13] KEYS: integrity: change link restriction to trust the machine keyring

[Eric Snowberg]

With the introduction of the machine keyring, the end-user may choose to
trust Machine Owner Keys (MOK) within the kernel. If they have chosen to
trust them, the .machine keyring will contain these keys.  If not, the
machine keyring will always be empty.  Update the restriction check to
allow the secondary trusted keyring and ima keyring to also trust
machine keys.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v4: Initial version (consolidated two previous patches)
v5: Rename to machine keyring
v6: Account for restriction being renamed earlier
---
 certs/system_keyring.c      | 5 ++++-
 security/integrity/digsig.c | 4 ++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 71a00add9805..ba732856ebd0 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -89,7 +89,10 @@ static __init struct key_restriction *get_secondary_restriction(void)
 	if (!restriction)
 		panic("Can't allocate secondary trusted keyring restriction\n");
 
-	restriction->check = restrict_link_by_builtin_and_secondary_trusted;
+	if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING))
+		restriction->check = restrict_link_by_builtin_secondary_and_machine;
+	else
+		restriction->check = restrict_link_by_builtin_and_secondary_trusted;
 
 	return restriction;
 }
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index e7dfc55a7c55..74f73f7cc4fe 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -34,7 +34,11 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
 };
 
 #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
+#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+#define restrict_link_to_ima restrict_link_by_builtin_secondary_and_machine
+#else
 #define restrict_link_to_ima restrict_link_by_builtin_and_secondary_trusted
+#endif
 #else
 #define restrict_link_to_ima restrict_link_by_builtin_trusted
 #endif
-- 
2.18.4

[PATCH v6 10/13] KEYS: link secondary_trusted_keys to machine trusted keys

[Eric Snowberg]

Allow the .machine keyring to be linked to the secondary_trusted_keys.
After the link is created, keys contained in the .machine keyring will
automatically be searched when searching secondary_trusted_keys.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v3: Initial version
v4: Unmodified from v3
v5: Rename to machine keyring
v6: Unmodified from v5
---
 certs/system_keyring.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index ba732856ebd0..2a2dc70b126c 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -101,6 +101,9 @@ static __init struct key_restriction *get_secondary_restriction(void)
 void __init set_machine_trusted_keys(struct key *keyring)
 {
 	machine_trusted_keys = keyring;
+
+	if (key_link(secondary_trusted_keys, machine_trusted_keys) < 0)
+		panic("Can't link (machine) trusted keyrings\n");
 }
 
 /**
-- 
2.18.4

[PATCH v6 11/13] integrity: store reference to machine keyring

[Eric Snowberg]

Store a reference to the machine keyring in system keyring code. The
system keyring code needs this to complete the keyring link to
to machine keyring.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v2: Initial version
v3: Unmodified from v2
v4: Removed trust_moklist check
v5: Rename to machine keyring
v6: Unmodified from v5
---
 security/integrity/digsig.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 74f73f7cc4fe..109b58840d45 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -116,6 +116,8 @@ static int __init __integrity_init_keyring(const unsigned int id,
 	} else {
 		if (id == INTEGRITY_KEYRING_PLATFORM)
 			set_platform_trusted_keys(keyring[id]);
+		if (id == INTEGRITY_KEYRING_MACHINE)
+			set_machine_trusted_keys(keyring[id]);
 		if (id == INTEGRITY_KEYRING_IMA)
 			load_module_cert(keyring[id]);
 	}
-- 
2.18.4

[PATCH v6 12/13] integrity: Trust MOK keys if MokListTrustedRT found

[Eric Snowberg]

A new Machine Owner Key (MOK) variable called MokListTrustedRT has been
introduced in shim. When this UEFI variable is set, it indicates the
end-user has made the decision themself that they wish to trust MOK keys
within the Linux trust boundary.  It is not an error if this variable
does not exist. If it does not exist, the MOK keys should not be trusted
within the kernel.

MOK variables are mirrored from Boot Services to Runtime Services.  When
shim sees the new MokTML BS variable, it will create a new variable
(before Exit Boot Services is called) called MokListTrustedRT without
EFI_VARIABLE_NON_VOLATILE set.  Following Exit Boot Services, UEFI
variables can only be set and created with SetVariable if both
EFI_VARIABLE_RUNTIME_ACCESS & EFI_VARIABLE_NON_VOLATILE are set.
Therefore, this can not be defeated by simply creating a
MokListTrustedRT variable from Linux, the existence of
EFI_VARIABLE_NON_VOLATILE will cause uefi_check_trust_machine_keys to
return false.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v2: Removed mok_keyring_trust_setup function
v4: Unmodified from v2
v5: Rename to machine keyring
v6: Unmodified from v5
---
 .../platform_certs/machine_keyring.c          | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/security/integrity/platform_certs/machine_keyring.c b/security/integrity/platform_certs/machine_keyring.c
index ea2ac2f9f2b5..68b8f2d449dc 100644
--- a/security/integrity/platform_certs/machine_keyring.c
+++ b/security/integrity/platform_certs/machine_keyring.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2021, Oracle and/or its affiliates.
  */
 
+#include <linux/efi.h>
 #include "../integrity.h"
 
 static __init int machine_keyring_init(void)
@@ -40,3 +41,29 @@ void __init add_to_machine_keyring(const char *source, const void *data, size_t
 	if (rc)
 		pr_info("Error adding keys to machine keyring %s\n", source);
 }
+
+/*
+ * Try to load the MokListTrustedRT UEFI variable to see if we should trust
+ * the mok keys within the kernel. It is not an error if this variable
+ * does not exist.  If it does not exist, mok keys should not be trusted
+ * within the machine keyring.
+ */
+static __init bool uefi_check_trust_mok_keys(void)
+{
+	efi_status_t status;
+	unsigned int mtrust = 0;
+	unsigned long size = sizeof(mtrust);
+	efi_guid_t guid = EFI_SHIM_LOCK_GUID;
+	u32 attr;
+
+	status = efi.get_variable(L"MokListTrustedRT", &guid, &attr, &size, &mtrust);
+
+	/*
+	 * The EFI_VARIABLE_NON_VOLATILE check is to verify MokListTrustedRT
+	 * was set thru shim mirrioring and not by a user from the host os.
+	 * According to the UEFI spec, once EBS is performed, SetVariable()
+	 * will succeed only when both EFI_VARIABLE_RUNTIME_ACCESS &
+	 * EFI_VARIABLE_NON_VOLATILE are set.
+	 */
+	return (status == EFI_SUCCESS && (!(attr & EFI_VARIABLE_NON_VOLATILE)));
+}
-- 
2.18.4

[PATCH v6 13/13] integrity: Only use machine keyring when uefi_check_trust_mok_keys is true

[Eric Snowberg]

With the introduction of uefi_check_trust_mok_keys, it signifies the end-
user wants to trust the machine keyring as trusted keys.  If they have
chosen to trust the machine keyring, load the qualifying keys into it
during boot, then link it to the secondary keyring .  If the user has not
chosen to trust the machine keyring, it will be empty and not linked to
the secondary keyring.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v4: Initial version
v5: Rename to machine keyring
v6: Unmodified from v5
---
 security/integrity/digsig.c                      |  2 +-
 security/integrity/integrity.h                   |  5 +++++
 .../integrity/platform_certs/keyring_handler.c   |  2 +-
 .../integrity/platform_certs/machine_keyring.c   | 16 ++++++++++++++++
 4 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 109b58840d45..1de09c7b5f93 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -116,7 +116,7 @@ static int __init __integrity_init_keyring(const unsigned int id,
 	} else {
 		if (id == INTEGRITY_KEYRING_PLATFORM)
 			set_platform_trusted_keys(keyring[id]);
-		if (id == INTEGRITY_KEYRING_MACHINE)
+		if (id == INTEGRITY_KEYRING_MACHINE && trust_moklist())
 			set_machine_trusted_keys(keyring[id]);
 		if (id == INTEGRITY_KEYRING_IMA)
 			load_module_cert(keyring[id]);
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 730771eececd..2e214c761158 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -287,9 +287,14 @@ static inline void __init add_to_platform_keyring(const char *source,
 
 #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
 void __init add_to_machine_keyring(const char *source, const void *data, size_t len);
+bool __init trust_moklist(void);
 #else
 static inline void __init add_to_machine_keyring(const char *source,
 						  const void *data, size_t len)
 {
 }
+static inline bool __init trust_moklist(void)
+{
+	return false;
+}
 #endif
diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
index 445d413aec74..d78dd66fb048 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -82,7 +82,7 @@ __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
 __init efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type)
 {
 	if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0) {
-		if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING))
+		if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING) && trust_moklist())
 			return add_to_machine_keyring;
 		else
 			return add_to_platform_keyring;
diff --git a/security/integrity/platform_certs/machine_keyring.c b/security/integrity/platform_certs/machine_keyring.c
index 68b8f2d449dc..7b413dacade8 100644
--- a/security/integrity/platform_certs/machine_keyring.c
+++ b/security/integrity/platform_certs/machine_keyring.c
@@ -8,6 +8,8 @@
 #include <linux/efi.h>
 #include "../integrity.h"
 
+bool trust_mok;
+
 static __init int machine_keyring_init(void)
 {
 	int rc;
@@ -67,3 +69,17 @@ static __init bool uefi_check_trust_mok_keys(void)
 	 */
 	return (status == EFI_SUCCESS && (!(attr & EFI_VARIABLE_NON_VOLATILE)));
 }
+
+bool __init trust_moklist(void)
+{
+	static bool initialized;
+
+	if (!initialized) {
+		initialized = true;
+
+		if (uefi_check_trust_mok_keys())
+			trust_mok = true;
+	}
+
+	return trust_mok;
+}
-- 
2.18.4

Re: [PATCH v6 00/13] Enroll kernel keys thru MOK

[Jarkko Sakkinen]

On Tue, 2021-09-14 at 17:14 -0400, Eric Snowberg wrote:
> Back in 2013 Linus requested a feature to allow end-users to have the 
> ability "to add their own keys and sign modules they trust". This was
> his *second* order outlined here [1]. There have been many attempts 
> over the years to solve this problem, all have been rejected.  Many 
> of the failed attempts loaded all preboot firmware keys into the kernel,
> including the Secure Boot keys. Many distributions carry one of these 
> rejected attempts [2], [3], [4]. This series tries to solve this problem 
> with a solution that takes into account all the problems brought up in 
> the previous attempts.
> 
> On UEFI based systems, this series introduces a new Linux kernel keyring 
> containing the Machine Owner Keys (MOK) called machine. It also defines
> a new MOK variable in shim. This variable allows the end-user to decide 
> if they want to load MOK keys into the machine keyring. Mimi has suggested 
> that only CA keys contained within the MOK be loaded into the machine 
> keyring. All other certs will load into the platform keyring instead.
> 
> By default, nothing changes; MOK keys are not loaded into the machine
> keyring.  They are only loaded after the end-user makes the decision 
> themselves.  The end-user would set this through mokutil using a new 
> --trust-mok option [5]. This would work similar to how the kernel uses 
> MOK variables to enable/disable signature validation as well as use/ignore 
> the db. Any kernel operation that uses either the builtin or secondary 
> trusted keys as a trust source shall also reference the new machine 
> keyring as a trust source.
> 
> Secure Boot keys will never be loaded into the machine keyring.  They
> will always be loaded into the platform keyring.  If an end-user wanted 
> to load one, they would need to enroll it into the MOK.
> 
> Steps required by the end user:
> 
> Sign kernel module with user created key:
> $ /usr/src/kernels/$(uname -r)/scripts/sign-file sha512 \
>    machine_signing_key.priv machine_signing_key.x509 my_module.ko
> 
> Import the key into the MOK
> $ mokutil --import machine_signing_key.x509
> 
> Setup the kernel to load MOK keys into the .machine keyring
> $ mokutil --trust-mok
> 
> Then reboot, the MokManager will load and ask if you want to trust the
> MOK key and enroll the MOK into the MOKList.  Afterwards the signed kernel
> module will load.
> 
> I have included links to both the mokutil [5] and shim [6] changes I
> have made to support this new functionality.

How hard it is to self-compile shim and boot it with QEMU (I
do not know even the GIT location of Shim)?

I'm all my SGX testing already with TianoCore and QEMU so I
thought it might not be that huge stretch to get testing env
for this.

/Jarkko

Re: [PATCH v6 00/13] Enroll kernel keys thru MOK

[Eric Snowberg]

> On Sep 15, 2021, at 11:57 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Tue, 2021-09-14 at 17:14 -0400, Eric Snowberg wrote:
>> Back in 2013 Linus requested a feature to allow end-users to have the 
>> ability "to add their own keys and sign modules they trust". This was
>> his *second* order outlined here [1]. There have been many attempts 
>> over the years to solve this problem, all have been rejected.  Many 
>> of the failed attempts loaded all preboot firmware keys into the kernel,
>> including the Secure Boot keys. Many distributions carry one of these 
>> rejected attempts [2], [3], [4]. This series tries to solve this problem 
>> with a solution that takes into account all the problems brought up in 
>> the previous attempts.
>> 
>> On UEFI based systems, this series introduces a new Linux kernel keyring 
>> containing the Machine Owner Keys (MOK) called machine. It also defines
>> a new MOK variable in shim. This variable allows the end-user to decide 
>> if they want to load MOK keys into the machine keyring. Mimi has suggested 
>> that only CA keys contained within the MOK be loaded into the machine 
>> keyring. All other certs will load into the platform keyring instead.
>> 
>> By default, nothing changes; MOK keys are not loaded into the machine
>> keyring.  They are only loaded after the end-user makes the decision 
>> themselves.  The end-user would set this through mokutil using a new 
>> --trust-mok option [5]. This would work similar to how the kernel uses 
>> MOK variables to enable/disable signature validation as well as use/ignore 
>> the db. Any kernel operation that uses either the builtin or secondary 
>> trusted keys as a trust source shall also reference the new machine 
>> keyring as a trust source.
>> 
>> Secure Boot keys will never be loaded into the machine keyring.  They
>> will always be loaded into the platform keyring.  If an end-user wanted 
>> to load one, they would need to enroll it into the MOK.
>> 
>> Steps required by the end user:
>> 
>> Sign kernel module with user created key:
>> $ /usr/src/kernels/$(uname -r)/scripts/sign-file sha512 \
>>   machine_signing_key.priv machine_signing_key.x509 my_module.ko
>> 
>> Import the key into the MOK
>> $ mokutil --import machine_signing_key.x509
>> 
>> Setup the kernel to load MOK keys into the .machine keyring
>> $ mokutil --trust-mok
>> 
>> Then reboot, the MokManager will load and ask if you want to trust the
>> MOK key and enroll the MOK into the MOKList.  Afterwards the signed kernel
>> module will load.
>> 
>> I have included links to both the mokutil [5] and shim [6] changes I
>> have made to support this new functionality.
> 
> How hard it is to self-compile shim and boot it with QEMU (I
> do not know even the GIT location of Shim)?

It is not hard, that is the setup I use for my testing.  Upstream shim 
is located here [1].  Or you can use my repo which contains the necessary
changes [2].

[1] https://github.com/rhboot/shim
[2] https://github.com/esnowberg/shim/tree/mokvars-v2

Re: [PATCH v6 05/13] integrity: add new keyring handler for mok keys

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2960 bytes --]

Hi Eric,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f]

url:    https://github.com/0day-ci/linux/commits/Eric-Snowberg/Enroll-kernel-keys-thru-MOK/20210915-051742
base:   6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/18c80634d92ba6ecc8e3a7d0b2dd793b586fc828
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Eric-Snowberg/Enroll-kernel-keys-thru-MOK/20210915-051742
        git checkout 18c80634d92ba6ecc8e3a7d0b2dd793b586fc828
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   security/integrity/platform_certs/keyring_handler.c:71:30: error: no previous prototype for 'get_handler_for_db' [-Werror=missing-prototypes]
      71 | __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
         |                              ^~~~~~~~~~~~~~~~~~
>> security/integrity/platform_certs/keyring_handler.c:82:30: error: no previous prototype for 'get_handler_for_mok' [-Werror=missing-prototypes]
      82 | __init efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type)
         |                              ^~~~~~~~~~~~~~~~~~~
   security/integrity/platform_certs/keyring_handler.c:97:30: error: no previous prototype for 'get_handler_for_dbx' [-Werror=missing-prototypes]
      97 | __init efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_type)
         |                              ^~~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors


vim +/get_handler_for_mok +82 security/integrity/platform_certs/keyring_handler.c

    66	
    67	/*
    68	 * Return the appropriate handler for particular signature list types found in
    69	 * the UEFI db tables.
    70	 */
  > 71	__init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
    72	{
    73		if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
    74			return add_to_platform_keyring;
    75		return 0;
    76	}
    77	
    78	/*
    79	 * Return the appropriate handler for particular signature list types found in
    80	 * the MokListRT tables.
    81	 */
  > 82	__init efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type)
    83	{
    84		if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0) {
    85			if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING))
    86				return add_to_machine_keyring;
    87			else
    88				return add_to_platform_keyring;
    89		}
    90		return 0;
    91	}
    92	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66454 bytes --]

Re: [PATCH v6 05/13] integrity: add new keyring handler for mok keys

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3030 bytes --]

Hi Eric,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f]

url:    https://github.com/0day-ci/linux/commits/Eric-Snowberg/Enroll-kernel-keys-thru-MOK/20210915-051742
base:   6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/18c80634d92ba6ecc8e3a7d0b2dd793b586fc828
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Eric-Snowberg/Enroll-kernel-keys-thru-MOK/20210915-051742
        git checkout 18c80634d92ba6ecc8e3a7d0b2dd793b586fc828
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   security/integrity/platform_certs/keyring_handler.c:71:30: error: no previous prototype for 'get_handler_for_db' [-Werror=missing-prototypes]
      71 | __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
         |                              ^~~~~~~~~~~~~~~~~~
>> security/integrity/platform_certs/keyring_handler.c:82:30: error: no previous prototype for 'get_handler_for_mok' [-Werror=missing-prototypes]
      82 | __init efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type)
         |                              ^~~~~~~~~~~~~~~~~~~
   security/integrity/platform_certs/keyring_handler.c:97:30: error: no previous prototype for 'get_handler_for_dbx' [-Werror=missing-prototypes]
      97 | __init efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_type)
         |                              ^~~~~~~~~~~~~~~~~~~
   cc1: all warnings being treated as errors


vim +/get_handler_for_mok +82 security/integrity/platform_certs/keyring_handler.c

    66	
    67	/*
    68	 * Return the appropriate handler for particular signature list types found in
    69	 * the UEFI db tables.
    70	 */
  > 71	__init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
    72	{
    73		if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
    74			return add_to_platform_keyring;
    75		return 0;
    76	}
    77	
    78	/*
    79	 * Return the appropriate handler for particular signature list types found in
    80	 * the MokListRT tables.
    81	 */
  > 82	__init efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type)
    83	{
    84		if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0) {
    85			if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING))
    86				return add_to_machine_keyring;
    87			else
    88				return add_to_platform_keyring;
    89		}
    90		return 0;
    91	}
    92	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 66454 bytes --]

Re: [PATCH v6 03/13] KEYS: CA link restriction

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1515 bytes --]

Hi Eric,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f]

url:    https://github.com/0day-ci/linux/commits/Eric-Snowberg/Enroll-kernel-keys-thru-MOK/20210915-051742
base:   6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f
config: arm-randconfig-c002-20210916 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/f65599b2308bdd9f29cfafd3286622f71aafa0b5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Eric-Snowberg/Enroll-kernel-keys-thru-MOK/20210915-051742
        git checkout f65599b2308bdd9f29cfafd3286622f71aafa0b5
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arm-linux-gnueabi-ld: crypto/asymmetric_keys/restrict.o: in function `restrict_link_by_ca':
>> restrict.c:(.text+0x344): undefined reference to `public_key_verify_signature'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32113 bytes --]

Re: [PATCH v6 03/13] KEYS: CA link restriction

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1549 bytes --]

Hi Eric,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f]

url:    https://github.com/0day-ci/linux/commits/Eric-Snowberg/Enroll-kernel-keys-thru-MOK/20210915-051742
base:   6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f
config: arm-randconfig-c002-20210916 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/f65599b2308bdd9f29cfafd3286622f71aafa0b5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Eric-Snowberg/Enroll-kernel-keys-thru-MOK/20210915-051742
        git checkout f65599b2308bdd9f29cfafd3286622f71aafa0b5
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arm-linux-gnueabi-ld: crypto/asymmetric_keys/restrict.o: in function `restrict_link_by_ca':
>> restrict.c:(.text+0x344): undefined reference to `public_key_verify_signature'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32113 bytes --]

Re: [PATCH v6 03/13] KEYS: CA link restriction

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2802 bytes --]

Hi Eric,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f]

url:    https://github.com/0day-ci/linux/commits/Eric-Snowberg/Enroll-kernel-keys-thru-MOK/20210915-051742
base:   6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f
config: x86_64-randconfig-c022-20210916 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/f65599b2308bdd9f29cfafd3286622f71aafa0b5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Eric-Snowberg/Enroll-kernel-keys-thru-MOK/20210915-051742
        git checkout f65599b2308bdd9f29cfafd3286622f71aafa0b5
        # save the attached .config to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: crypto/asymmetric_keys/restrict.o: in function `restrict_link_by_ca':
>> crypto/asymmetric_keys/restrict.c:148: undefined reference to `public_key_verify_signature'


vim +148 crypto/asymmetric_keys/restrict.c

   110	
   111	/**
   112	 * restrict_link_by_ca - Restrict additions to a ring of CA keys
   113	 * @dest_keyring: Keyring being linked to.
   114	 * @type: The type of key being added.
   115	 * @payload: The payload of the new key.
   116	 * @trusted: Unused.
   117	 *
   118	 * Check if the new certificate is a CA. If it is a CA, then mark the new
   119	 * certificate as being ok to link.
   120	 *
   121	 * Returns 0 if the new certificate was accepted, -ENOKEY if we could not find
   122	 * a matching parent certificate in the trusted list.  -ENOPKG if the signature
   123	 * uses unsupported crypto, or some other error if there is a matching
   124	 * certificate  but the signature check cannot be performed.
   125	 */
   126	int restrict_link_by_ca(struct key *dest_keyring,
   127				const struct key_type *type,
   128				const union key_payload *payload,
   129				struct key *trust_keyring)
   130	{
   131		const struct public_key_signature *sig;
   132		const struct public_key *pkey;
   133	
   134		if (type != &key_type_asymmetric)
   135			return -EOPNOTSUPP;
   136	
   137		sig = payload->data[asym_auth];
   138		if (!sig)
   139			return -ENOPKG;
   140	
   141		if (!sig->auth_ids[0] && !sig->auth_ids[1])
   142			return -ENOKEY;
   143	
   144		pkey = payload->data[asym_crypto];
   145		if (!pkey)
   146			return -ENOPKG;
   147	
 > 148		return public_key_verify_signature(pkey, sig);
   149	}
   150	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33580 bytes --]

Re: [PATCH v6 03/13] KEYS: CA link restriction

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2879 bytes --]

Hi Eric,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f]

url:    https://github.com/0day-ci/linux/commits/Eric-Snowberg/Enroll-kernel-keys-thru-MOK/20210915-051742
base:   6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f
config: x86_64-randconfig-c022-20210916 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/f65599b2308bdd9f29cfafd3286622f71aafa0b5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Eric-Snowberg/Enroll-kernel-keys-thru-MOK/20210915-051742
        git checkout f65599b2308bdd9f29cfafd3286622f71aafa0b5
        # save the attached .config to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: crypto/asymmetric_keys/restrict.o: in function `restrict_link_by_ca':
>> crypto/asymmetric_keys/restrict.c:148: undefined reference to `public_key_verify_signature'


vim +148 crypto/asymmetric_keys/restrict.c

   110	
   111	/**
   112	 * restrict_link_by_ca - Restrict additions to a ring of CA keys
   113	 * @dest_keyring: Keyring being linked to.
   114	 * @type: The type of key being added.
   115	 * @payload: The payload of the new key.
   116	 * @trusted: Unused.
   117	 *
   118	 * Check if the new certificate is a CA. If it is a CA, then mark the new
   119	 * certificate as being ok to link.
   120	 *
   121	 * Returns 0 if the new certificate was accepted, -ENOKEY if we could not find
   122	 * a matching parent certificate in the trusted list.  -ENOPKG if the signature
   123	 * uses unsupported crypto, or some other error if there is a matching
   124	 * certificate  but the signature check cannot be performed.
   125	 */
   126	int restrict_link_by_ca(struct key *dest_keyring,
   127				const struct key_type *type,
   128				const union key_payload *payload,
   129				struct key *trust_keyring)
   130	{
   131		const struct public_key_signature *sig;
   132		const struct public_key *pkey;
   133	
   134		if (type != &key_type_asymmetric)
   135			return -EOPNOTSUPP;
   136	
   137		sig = payload->data[asym_auth];
   138		if (!sig)
   139			return -ENOPKG;
   140	
   141		if (!sig->auth_ids[0] && !sig->auth_ids[1])
   142			return -ENOKEY;
   143	
   144		pkey = payload->data[asym_crypto];
   145		if (!pkey)
   146			return -ENOPKG;
   147	
 > 148		return public_key_verify_signature(pkey, sig);
   149	}
   150	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 33580 bytes --]

Re: [PATCH v6 00/13] Enroll kernel keys thru MOK

[Jarkko Sakkinen]

On Wed, 2021-09-15 at 15:28 -0600, Eric Snowberg wrote:
> > On Sep 15, 2021, at 11:57 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > On Tue, 2021-09-14 at 17:14 -0400, Eric Snowberg wrote:
> > > Back in 2013 Linus requested a feature to allow end-users to have the 
> > > ability "to add their own keys and sign modules they trust". This was
> > > his *second* order outlined here [1]. There have been many attempts 
> > > over the years to solve this problem, all have been rejected.  Many 
> > > of the failed attempts loaded all preboot firmware keys into the kernel,
> > > including the Secure Boot keys. Many distributions carry one of these 
> > > rejected attempts [2], [3], [4]. This series tries to solve this problem 
> > > with a solution that takes into account all the problems brought up in 
> > > the previous attempts.
> > > 
> > > On UEFI based systems, this series introduces a new Linux kernel keyring 
> > > containing the Machine Owner Keys (MOK) called machine. It also defines
> > > a new MOK variable in shim. This variable allows the end-user to decide 
> > > if they want to load MOK keys into the machine keyring. Mimi has suggested 
> > > that only CA keys contained within the MOK be loaded into the machine 
> > > keyring. All other certs will load into the platform keyring instead.
> > > 
> > > By default, nothing changes; MOK keys are not loaded into the machine
> > > keyring.  They are only loaded after the end-user makes the decision 
> > > themselves.  The end-user would set this through mokutil using a new 
> > > --trust-mok option [5]. This would work similar to how the kernel uses 
> > > MOK variables to enable/disable signature validation as well as use/ignore 
> > > the db. Any kernel operation that uses either the builtin or secondary 
> > > trusted keys as a trust source shall also reference the new machine 
> > > keyring as a trust source.
> > > 
> > > Secure Boot keys will never be loaded into the machine keyring.  They
> > > will always be loaded into the platform keyring.  If an end-user wanted 
> > > to load one, they would need to enroll it into the MOK.
> > > 
> > > Steps required by the end user:
> > > 
> > > Sign kernel module with user created key:
> > > $ /usr/src/kernels/$(uname -r)/scripts/sign-file sha512 \
> > >   machine_signing_key.priv machine_signing_key.x509 my_module.ko
> > > 
> > > Import the key into the MOK
> > > $ mokutil --import machine_signing_key.x509
> > > 
> > > Setup the kernel to load MOK keys into the .machine keyring
> > > $ mokutil --trust-mok
> > > 
> > > Then reboot, the MokManager will load and ask if you want to trust the
> > > MOK key and enroll the MOK into the MOKList.  Afterwards the signed kernel
> > > module will load.
> > > 
> > > I have included links to both the mokutil [5] and shim [6] changes I
> > > have made to support this new functionality.
> > 
> > How hard it is to self-compile shim and boot it with QEMU (I
> > do not know even the GIT location of Shim)?
> 
> It is not hard, that is the setup I use for my testing.  Upstream shim 
> is located here [1].  Or you can use my repo which contains the necessary
> changes [2].
> 
> [1] https://github.com/rhboot/shim
> [2] https://github.com/esnowberg/shim/tree/mokvars-v2
> 

So, my 2nd Q would be: which order these should be upstreamed?

Linux patch set cannot depend on "yet to be upstreamed" things.

Code changes look good enough to me.

/Jarkko

Re: [PATCH v6 00/13] Enroll kernel keys thru MOK

[Nayna]

On 9/14/21 5:14 PM, Eric Snowberg wrote:
> Back in 2013 Linus requested a feature to allow end-users to have the
> ability "to add their own keys and sign modules they trust". This was
> his *second* order outlined here [1]. There have been many attempts
> over the years to solve this problem, all have been rejected.  Many
> of the failed attempts loaded all preboot firmware keys into the kernel,
> including the Secure Boot keys. Many distributions carry one of these
> rejected attempts [2], [3], [4]. This series tries to solve this problem
> with a solution that takes into account all the problems brought up in
> the previous attempts.
>
> On UEFI based systems, this series introduces a new Linux kernel keyring
> containing the Machine Owner Keys (MOK) called machine. It also defines
> a new MOK variable in shim. This variable allows the end-user to decide
> if they want to load MOK keys into the machine keyring. Mimi has suggested
> that only CA keys contained within the MOK be loaded into the machine
> keyring. All other certs will load into the platform keyring instead.
>
> By default, nothing changes; MOK keys are not loaded into the machine
> keyring.  They are only loaded after the end-user makes the decision
> themselves.  The end-user would set this through mokutil using a new
> --trust-mok option [5]. This would work similar to how the kernel uses
> MOK variables to enable/disable signature validation as well as use/ignore
> the db. Any kernel operation that uses either the builtin or secondary
> trusted keys as a trust source shall also reference the new machine
> keyring as a trust source.
>
> Secure Boot keys will never be loaded into the machine keyring.  They
> will always be loaded into the platform keyring.  If an end-user wanted
> to load one, they would need to enroll it into the MOK.
>
> Steps required by the end user:
>
> Sign kernel module with user created key:
> $ /usr/src/kernels/$(uname -r)/scripts/sign-file sha512 \
>     machine_signing_key.priv machine_signing_key.x509 my_module.ko
>
> Import the key into the MOK
> $ mokutil --import machine_signing_key.x509
>
> Setup the kernel to load MOK keys into the .machine keyring
> $ mokutil --trust-mok
>
> Then reboot, the MokManager will load and ask if you want to trust the
> MOK key and enroll the MOK into the MOKList.  Afterwards the signed kernel
> module will load.

machine_signing_key.x509 appears to be a code-signing, self-signed key.  
It's not a CA key, but the intent of the patchset is to load only CA 
keys to .machine keyring.

Shouldn't there be two steps: one to load the CA key into MOK, and a 
second one to load the code-signing key which is signed by this CA ?

Thanks & Regards,

       - Nayna

Re: [PATCH v6 03/13] KEYS: CA link restriction

[Nayna]

On 9/14/21 5:14 PM, Eric Snowberg wrote:
> Add a new link restriction.  Restrict the addition of keys in a keyring
> based on the key to be added being a CA (self-signed).

A self-signed cert can be a root CA cert or a code-signing cert. The way 
to differentiate a CA cert is by checking BasicConstraints CA:TRUE and 
keyUsage:keyCertSign. Refer to Section Basic Constraints and Key Usage 
in the document - https://datatracker.ietf.org/doc/html/rfc5280.

Thanks & Regards,

      - Nayna

Re: [PATCH v6 00/13] Enroll kernel keys thru MOK

[Peter Jones]

On Thu, Sep 16, 2021 at 06:15:50PM +0300, Jarkko Sakkinen wrote:
> On Wed, 2021-09-15 at 15:28 -0600, Eric Snowberg wrote:
> > > On Sep 15, 2021, at 11:57 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > 
> > > On Tue, 2021-09-14 at 17:14 -0400, Eric Snowberg wrote:
> > > > Back in 2013 Linus requested a feature to allow end-users to have the 
> > > > ability "to add their own keys and sign modules they trust". This was
> > > > his *second* order outlined here [1]. There have been many attempts 
> > > > over the years to solve this problem, all have been rejected.  Many 
> > > > of the failed attempts loaded all preboot firmware keys into the kernel,
> > > > including the Secure Boot keys. Many distributions carry one of these 
> > > > rejected attempts [2], [3], [4]. This series tries to solve this problem 
> > > > with a solution that takes into account all the problems brought up in 
> > > > the previous attempts.
> > > > 
> > > > On UEFI based systems, this series introduces a new Linux kernel keyring 
> > > > containing the Machine Owner Keys (MOK) called machine. It also defines
> > > > a new MOK variable in shim. This variable allows the end-user to decide 
> > > > if they want to load MOK keys into the machine keyring. Mimi has suggested 
> > > > that only CA keys contained within the MOK be loaded into the machine 
> > > > keyring. All other certs will load into the platform keyring instead.
> > > > 
> > > > By default, nothing changes; MOK keys are not loaded into the machine
> > > > keyring.  They are only loaded after the end-user makes the decision 
> > > > themselves.  The end-user would set this through mokutil using a new 
> > > > --trust-mok option [5]. This would work similar to how the kernel uses 
> > > > MOK variables to enable/disable signature validation as well as use/ignore 
> > > > the db. Any kernel operation that uses either the builtin or secondary 
> > > > trusted keys as a trust source shall also reference the new machine 
> > > > keyring as a trust source.
> > > > 
> > > > Secure Boot keys will never be loaded into the machine keyring.  They
> > > > will always be loaded into the platform keyring.  If an end-user wanted 
> > > > to load one, they would need to enroll it into the MOK.
> > > > 
> > > > Steps required by the end user:
> > > > 
> > > > Sign kernel module with user created key:
> > > > $ /usr/src/kernels/$(uname -r)/scripts/sign-file sha512 \
> > > >   machine_signing_key.priv machine_signing_key.x509 my_module.ko
> > > > 
> > > > Import the key into the MOK
> > > > $ mokutil --import machine_signing_key.x509
> > > > 
> > > > Setup the kernel to load MOK keys into the .machine keyring
> > > > $ mokutil --trust-mok
> > > > 
> > > > Then reboot, the MokManager will load and ask if you want to trust the
> > > > MOK key and enroll the MOK into the MOKList.  Afterwards the signed kernel
> > > > module will load.
> > > > 
> > > > I have included links to both the mokutil [5] and shim [6] changes I
> > > > have made to support this new functionality.
> > > 
> > > How hard it is to self-compile shim and boot it with QEMU (I
> > > do not know even the GIT location of Shim)?
> > 
> > It is not hard, that is the setup I use for my testing.  Upstream shim 
> > is located here [1].  Or you can use my repo which contains the necessary
> > changes [2].
> > 
> > [1] https://github.com/rhboot/shim
> > [2] https://github.com/esnowberg/shim/tree/mokvars-v2
> > 
> 
> So, my 2nd Q would be: which order these should be upstreamed?
> 
> Linux patch set cannot depend on "yet to be upstreamed" things.
> 
> Code changes look good enough to me.

We can carry this support in shim before it's in kernel.  Eric's current
patch for shim and mokutil looks mostly reasonable, though I see a few
minor nits we'll have to sort out.

-- 
        Peter

Re: [PATCH v6 12/13] integrity: Trust MOK keys if MokListTrustedRT found

[Peter Jones]

On Tue, Sep 14, 2021 at 05:14:15PM -0400, Eric Snowberg wrote:
> +/*
> + * Try to load the MokListTrustedRT UEFI variable to see if we should trust
> + * the mok keys within the kernel. It is not an error if this variable
> + * does not exist.  If it does not exist, mok keys should not be trusted
> + * within the machine keyring.
> + */
> +static __init bool uefi_check_trust_mok_keys(void)
> +{
> +	efi_status_t status;
> +	unsigned int mtrust = 0;
> +	unsigned long size = sizeof(mtrust);
> +	efi_guid_t guid = EFI_SHIM_LOCK_GUID;
> +	u32 attr;
> +
> +	status = efi.get_variable(L"MokListTrustedRT", &guid, &attr, &size, &mtrust);

This should use efi_mokvar_entry_find("MokListTrustedRT") instead,
similar to how load_moklist_certs() does.  It's a *much* more reliable
mechanism.  We don't even need to fall back to checking for the
variable, as any version of shim that populates this supports the config
table method.

-- 
        Peter

Re: [PATCH v6 00/13] Enroll kernel keys thru MOK

[Eric Snowberg]

> On Sep 16, 2021, at 2:03 PM, Nayna <nayna@linux.vnet.ibm.com> wrote:
> 
> 
> On 9/14/21 5:14 PM, Eric Snowberg wrote:
>> Back in 2013 Linus requested a feature to allow end-users to have the
>> ability "to add their own keys and sign modules they trust". This was
>> his *second* order outlined here [1]. There have been many attempts
>> over the years to solve this problem, all have been rejected.  Many
>> of the failed attempts loaded all preboot firmware keys into the kernel,
>> including the Secure Boot keys. Many distributions carry one of these
>> rejected attempts [2], [3], [4]. This series tries to solve this problem
>> with a solution that takes into account all the problems brought up in
>> the previous attempts.
>> 
>> On UEFI based systems, this series introduces a new Linux kernel keyring
>> containing the Machine Owner Keys (MOK) called machine. It also defines
>> a new MOK variable in shim. This variable allows the end-user to decide
>> if they want to load MOK keys into the machine keyring. Mimi has suggested
>> that only CA keys contained within the MOK be loaded into the machine
>> keyring. All other certs will load into the platform keyring instead.
>> 
>> By default, nothing changes; MOK keys are not loaded into the machine
>> keyring.  They are only loaded after the end-user makes the decision
>> themselves.  The end-user would set this through mokutil using a new
>> --trust-mok option [5]. This would work similar to how the kernel uses
>> MOK variables to enable/disable signature validation as well as use/ignore
>> the db. Any kernel operation that uses either the builtin or secondary
>> trusted keys as a trust source shall also reference the new machine
>> keyring as a trust source.
>> 
>> Secure Boot keys will never be loaded into the machine keyring.  They
>> will always be loaded into the platform keyring.  If an end-user wanted
>> to load one, they would need to enroll it into the MOK.
>> 
>> Steps required by the end user:
>> 
>> Sign kernel module with user created key:
>> $ /usr/src/kernels/$(uname -r)/scripts/sign-file sha512 \
>>    machine_signing_key.priv machine_signing_key.x509 my_module.ko
>> 
>> Import the key into the MOK
>> $ mokutil --import machine_signing_key.x509
>> 
>> Setup the kernel to load MOK keys into the .machine keyring
>> $ mokutil --trust-mok
>> 
>> Then reboot, the MokManager will load and ask if you want to trust the
>> MOK key and enroll the MOK into the MOKList.  Afterwards the signed kernel
>> module will load.
> 
> machine_signing_key.x509 appears to be a code-signing, self-signed key.  It's not a CA key, but the intent of the patchset is to load only CA keys to .machine keyring.
> 
> Shouldn't there be two steps: one to load the CA key into MOK, and a second one to load the code-signing key which is signed by this CA ?

I think that would depend on how the end-user wants to do things. If they 
are just looking to quickly self-sign their own kernel module, I doubt they 
would go through the effort of having two keys.  This is what I tried to 
document in the example above. 

If they do want to use multiple keys, nothing in this approach prevents it.
Their CA will be loaded into the machine keyring.  Now that the machine 
keyring is a new trust source,  they could simply load their intermediate 
or leaf cert signed by their CA into the secondary trusted keyring. Afterwards
their signed kernel module would load.

Re: [PATCH v6 00/13] Enroll kernel keys thru MOK

[Eric Snowberg]

> On Sep 16, 2021, at 4:14 PM, Peter Jones <pjones@redhat.com> wrote:
> 
> On Thu, Sep 16, 2021 at 06:15:50PM +0300, Jarkko Sakkinen wrote:
>> On Wed, 2021-09-15 at 15:28 -0600, Eric Snowberg wrote:
>>>> On Sep 15, 2021, at 11:57 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>>>> 
>>>> On Tue, 2021-09-14 at 17:14 -0400, Eric Snowberg wrote:
>>>>> Back in 2013 Linus requested a feature to allow end-users to have the 
>>>>> ability "to add their own keys and sign modules they trust". This was
>>>>> his *second* order outlined here [1]. There have been many attempts 
>>>>> over the years to solve this problem, all have been rejected.  Many 
>>>>> of the failed attempts loaded all preboot firmware keys into the kernel,
>>>>> including the Secure Boot keys. Many distributions carry one of these 
>>>>> rejected attempts [2], [3], [4]. This series tries to solve this problem 
>>>>> with a solution that takes into account all the problems brought up in 
>>>>> the previous attempts.
>>>>> 
>>>>> On UEFI based systems, this series introduces a new Linux kernel keyring 
>>>>> containing the Machine Owner Keys (MOK) called machine. It also defines
>>>>> a new MOK variable in shim. This variable allows the end-user to decide 
>>>>> if they want to load MOK keys into the machine keyring. Mimi has suggested 
>>>>> that only CA keys contained within the MOK be loaded into the machine 
>>>>> keyring. All other certs will load into the platform keyring instead.
>>>>> 
>>>>> By default, nothing changes; MOK keys are not loaded into the machine
>>>>> keyring.  They are only loaded after the end-user makes the decision 
>>>>> themselves.  The end-user would set this through mokutil using a new 
>>>>> --trust-mok option [5]. This would work similar to how the kernel uses 
>>>>> MOK variables to enable/disable signature validation as well as use/ignore 
>>>>> the db. Any kernel operation that uses either the builtin or secondary 
>>>>> trusted keys as a trust source shall also reference the new machine 
>>>>> keyring as a trust source.
>>>>> 
>>>>> Secure Boot keys will never be loaded into the machine keyring.  They
>>>>> will always be loaded into the platform keyring.  If an end-user wanted 
>>>>> to load one, they would need to enroll it into the MOK.
>>>>> 
>>>>> Steps required by the end user:
>>>>> 
>>>>> Sign kernel module with user created key:
>>>>> $ /usr/src/kernels/$(uname -r)/scripts/sign-file sha512 \
>>>>>  machine_signing_key.priv machine_signing_key.x509 my_module.ko
>>>>> 
>>>>> Import the key into the MOK
>>>>> $ mokutil --import machine_signing_key.x509
>>>>> 
>>>>> Setup the kernel to load MOK keys into the .machine keyring
>>>>> $ mokutil --trust-mok
>>>>> 
>>>>> Then reboot, the MokManager will load and ask if you want to trust the
>>>>> MOK key and enroll the MOK into the MOKList.  Afterwards the signed kernel
>>>>> module will load.
>>>>> 
>>>>> I have included links to both the mokutil [5] and shim [6] changes I
>>>>> have made to support this new functionality.
>>>> 
>>>> How hard it is to self-compile shim and boot it with QEMU (I
>>>> do not know even the GIT location of Shim)?
>>> 
>>> It is not hard, that is the setup I use for my testing.  Upstream shim 
>>> is located here [1].  Or you can use my repo which contains the necessary
>>> changes [2].
>>> 
>>> [1] https://github.com/rhboot/shim
>>> [2] https://github.com/esnowberg/shim/tree/mokvars-v2
>>> 
>> 
>> So, my 2nd Q would be: which order these should be upstreamed?
>> 
>> Linux patch set cannot depend on "yet to be upstreamed" things.
>> 
>> Code changes look good enough to me.
> 
> We can carry this support in shim before it's in kernel.  Eric's current
> patch for shim and mokutil looks mostly reasonable, though I see a few
> minor nits we'll have to sort out.

Thanks Peter.  Previously I had not sent it for a shim review since I wanted 
to make sure this approach had a path forward.  I’ll work on submitting the 
shim changes for review now.

Re: [PATCH v6 12/13] integrity: Trust MOK keys if MokListTrustedRT found

[Eric Snowberg]

> On Sep 16, 2021, at 4:19 PM, Peter Jones <pjones@redhat.com> wrote:
> 
> On Tue, Sep 14, 2021 at 05:14:15PM -0400, Eric Snowberg wrote:
>> +/*
>> + * Try to load the MokListTrustedRT UEFI variable to see if we should trust
>> + * the mok keys within the kernel. It is not an error if this variable
>> + * does not exist.  If it does not exist, mok keys should not be trusted
>> + * within the machine keyring.
>> + */
>> +static __init bool uefi_check_trust_mok_keys(void)
>> +{
>> +	efi_status_t status;
>> +	unsigned int mtrust = 0;
>> +	unsigned long size = sizeof(mtrust);
>> +	efi_guid_t guid = EFI_SHIM_LOCK_GUID;
>> +	u32 attr;
>> +
>> +	status = efi.get_variable(L"MokListTrustedRT", &guid, &attr, &size, &mtrust);
> 
> This should use efi_mokvar_entry_find("MokListTrustedRT") instead,
> similar to how load_moklist_certs() does.  It's a *much* more reliable
> mechanism.  We don't even need to fall back to checking for the
> variable, as any version of shim that populates this supports the config
> table method.

I’ll change this in v7, thanks.

Re: [PATCH v6 12/13] integrity: Trust MOK keys if MokListTrustedRT found

[Peter Jones]

On Thu, Sep 16, 2021 at 08:00:54PM -0600, Eric Snowberg wrote:
> 
> > On Sep 16, 2021, at 4:19 PM, Peter Jones <pjones@redhat.com> wrote:
> > 
> > On Tue, Sep 14, 2021 at 05:14:15PM -0400, Eric Snowberg wrote:
> >> +/*
> >> + * Try to load the MokListTrustedRT UEFI variable to see if we should trust
> >> + * the mok keys within the kernel. It is not an error if this variable
> >> + * does not exist.  If it does not exist, mok keys should not be trusted
> >> + * within the machine keyring.
> >> + */
> >> +static __init bool uefi_check_trust_mok_keys(void)
> >> +{
> >> +	efi_status_t status;
> >> +	unsigned int mtrust = 0;
> >> +	unsigned long size = sizeof(mtrust);
> >> +	efi_guid_t guid = EFI_SHIM_LOCK_GUID;
> >> +	u32 attr;
> >> +
> >> +	status = efi.get_variable(L"MokListTrustedRT", &guid, &attr, &size, &mtrust);
> > 
> > This should use efi_mokvar_entry_find("MokListTrustedRT") instead,
> > similar to how load_moklist_certs() does.  It's a *much* more reliable
> > mechanism.  We don't even need to fall back to checking for the
> > variable, as any version of shim that populates this supports the config
> > table method.
> 
> I’ll change this in v7, thanks.

We do also need to figure out a path forward for something like Dimitri
Ledkov's MokListX patch[0] from May, though it doesn't necessarily need
to hold up this patch set.  It looks like your patches will change the
structure of the keyrings it needs to apply to, but I don't see a reason
it wouldn't be conditional on the same MokListTrustedRT variable.  Any
thoughts?

[0] https://lore.kernel.org/lkml/20210512153100.285169-1-dimitri.ledkov@canonical.com/

-- 
        Peter

Re: [PATCH v6 12/13] integrity: Trust MOK keys if MokListTrustedRT found

[Eric Snowberg]

> On Sep 17, 2021, at 9:03 AM, Peter Jones <pjones@redhat.com> wrote:
> 
> On Thu, Sep 16, 2021 at 08:00:54PM -0600, Eric Snowberg wrote:
>> 
>>> On Sep 16, 2021, at 4:19 PM, Peter Jones <pjones@redhat.com> wrote:
>>> 
>>> On Tue, Sep 14, 2021 at 05:14:15PM -0400, Eric Snowberg wrote:
>>>> +/*
>>>> + * Try to load the MokListTrustedRT UEFI variable to see if we should trust
>>>> + * the mok keys within the kernel. It is not an error if this variable
>>>> + * does not exist.  If it does not exist, mok keys should not be trusted
>>>> + * within the machine keyring.
>>>> + */
>>>> +static __init bool uefi_check_trust_mok_keys(void)
>>>> +{
>>>> +	efi_status_t status;
>>>> +	unsigned int mtrust = 0;
>>>> +	unsigned long size = sizeof(mtrust);
>>>> +	efi_guid_t guid = EFI_SHIM_LOCK_GUID;
>>>> +	u32 attr;
>>>> +
>>>> +	status = efi.get_variable(L"MokListTrustedRT", &guid, &attr, &size, &mtrust);
>>> 
>>> This should use efi_mokvar_entry_find("MokListTrustedRT") instead,
>>> similar to how load_moklist_certs() does.  It's a *much* more reliable
>>> mechanism.  We don't even need to fall back to checking for the
>>> variable, as any version of shim that populates this supports the config
>>> table method.
>> 
>> I’ll change this in v7, thanks.
> 
> We do also need to figure out a path forward for something like Dimitri
> Ledkov's MokListX patch[0] from May, though it doesn't necessarily need
> to hold up this patch set.  It looks like your patches will change the
> structure of the keyrings it needs to apply to, but I don't see a reason
> it wouldn't be conditional on the same MokListTrustedRT variable.  Any
> thoughts?
> 
> [0] https://lore.kernel.org/lkml/20210512153100.285169-1-dimitri.ledkov@canonical.com/
> 

I had a little different approach I was going to send for this problem, but dropped it 
after I saw Dimitri’s patch.  Yes, we will need to figure out a way to merge the two.  
But I don’t see that being too difficult or them being incompatible with one another.

Re: [PATCH v6 00/13] Enroll kernel keys thru MOK

[Mimi Zohar]

On Thu, 2021-09-16 at 19:55 -0600, Eric Snowberg wrote:
> > On Sep 16, 2021, at 2:03 PM, Nayna <nayna@linux.vnet.ibm.com> wrote:
> > 
> > 
> > On 9/14/21 5:14 PM, Eric Snowberg wrote:
> >> Back in 2013 Linus requested a feature to allow end-users to have the
> >> ability "to add their own keys and sign modules they trust". This was
> >> his *second* order outlined here [1]. There have been many attempts
> >> over the years to solve this problem, all have been rejected.  Many
> >> of the failed attempts loaded all preboot firmware keys into the kernel,
> >> including the Secure Boot keys. Many distributions carry one of these
> >> rejected attempts [2], [3], [4]. This series tries to solve this problem
> >> with a solution that takes into account all the problems brought up in
> >> the previous attempts.
> >> 
> >> On UEFI based systems, this series introduces a new Linux kernel keyring
> >> containing the Machine Owner Keys (MOK) called machine. It also defines
> >> a new MOK variable in shim. This variable allows the end-user to decide
> >> if they want to load MOK keys into the machine keyring. Mimi has suggested
> >> that only CA keys contained within the MOK be loaded into the machine
> >> keyring. All other certs will load into the platform keyring instead.
> >> 
> >> By default, nothing changes; MOK keys are not loaded into the machine
> >> keyring.  They are only loaded after the end-user makes the decision
> >> themselves.  The end-user would set this through mokutil using a new
> >> --trust-mok option [5]. This would work similar to how the kernel uses
> >> MOK variables to enable/disable signature validation as well as use/ignore
> >> the db. Any kernel operation that uses either the builtin or secondary
> >> trusted keys as a trust source shall also reference the new machine
> >> keyring as a trust source.
> >> 
> >> Secure Boot keys will never be loaded into the machine keyring.  They
> >> will always be loaded into the platform keyring.  If an end-user wanted
> >> to load one, they would need to enroll it into the MOK.
> >> 
> >> Steps required by the end user:
> >> 
> >> Sign kernel module with user created key:
> >> $ /usr/src/kernels/$(uname -r)/scripts/sign-file sha512 \
> >>    machine_signing_key.priv machine_signing_key.x509 my_module.ko
> >> 
> >> Import the key into the MOK
> >> $ mokutil --import machine_signing_key.x509
> >> 
> >> Setup the kernel to load MOK keys into the .machine keyring
> >> $ mokutil --trust-mok
> >> 
> >> Then reboot, the MokManager will load and ask if you want to trust the
> >> MOK key and enroll the MOK into the MOKList.  Afterwards the signed kernel
> >> module will load.
> > 
> > machine_signing_key.x509 appears to be a code-signing, self-signed key.  It's not a CA key, but the intent of the patchset is to load only CA keys to .machine keyring.
> > 
> > Shouldn't there be two steps: one to load the CA key into MOK, and a second one to load the code-signing key which is signed by this CA ?
> 
> I think that would depend on how the end-user wants to do things. If they 
> are just looking to quickly self-sign their own kernel module, I doubt they 
> would go through the effort of having two keys.  This is what I tried to 
> document in the example above. 

Thank you for providing the example.  We've spent quite a bit of time
discussing CA keys stored in the MOK db being the new root(s) of trust.
While other certificates signed by the new root(s) of trust would be
added to the secondary or IMA keyrings.

At this point, the patch descriptions, and probably code, are written
in terms of CA keys stored in the MOK db, which the self signed
certificate restriction does not require.  With this design, these self
signed certs may be loaded directly onto the IMA keyring.  That's a
problem.  To fix the discrepancy between the cover letter, patch
descriptions and code, all that is needed is to verify that the self-
signed cert is in fact a CA.

thanks,

Mimi

Re: [PATCH v6 00/13] Enroll kernel keys thru MOK

[Jarkko Sakkinen]

On Thu, 2021-09-16 at 18:14 -0400, Peter Jones wrote:
> On Thu, Sep 16, 2021 at 06:15:50PM +0300, Jarkko Sakkinen wrote:
> > On Wed, 2021-09-15 at 15:28 -0600, Eric Snowberg wrote:
> > > > On Sep 15, 2021, at 11:57 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > > 
> > > > On Tue, 2021-09-14 at 17:14 -0400, Eric Snowberg wrote:
> > > > > Back in 2013 Linus requested a feature to allow end-users to have the 
> > > > > ability "to add their own keys and sign modules they trust". This was
> > > > > his *second* order outlined here [1]. There have been many attempts 
> > > > > over the years to solve this problem, all have been rejected.  Many 
> > > > > of the failed attempts loaded all preboot firmware keys into the kernel,
> > > > > including the Secure Boot keys. Many distributions carry one of these 
> > > > > rejected attempts [2], [3], [4]. This series tries to solve this problem 
> > > > > with a solution that takes into account all the problems brought up in 
> > > > > the previous attempts.
> > > > > 
> > > > > On UEFI based systems, this series introduces a new Linux kernel keyring 
> > > > > containing the Machine Owner Keys (MOK) called machine. It also defines
> > > > > a new MOK variable in shim. This variable allows the end-user to decide 
> > > > > if they want to load MOK keys into the machine keyring. Mimi has suggested 
> > > > > that only CA keys contained within the MOK be loaded into the machine 
> > > > > keyring. All other certs will load into the platform keyring instead.
> > > > > 
> > > > > By default, nothing changes; MOK keys are not loaded into the machine
> > > > > keyring.  They are only loaded after the end-user makes the decision 
> > > > > themselves.  The end-user would set this through mokutil using a new 
> > > > > --trust-mok option [5]. This would work similar to how the kernel uses 
> > > > > MOK variables to enable/disable signature validation as well as use/ignore 
> > > > > the db. Any kernel operation that uses either the builtin or secondary 
> > > > > trusted keys as a trust source shall also reference the new machine 
> > > > > keyring as a trust source.
> > > > > 
> > > > > Secure Boot keys will never be loaded into the machine keyring.  They
> > > > > will always be loaded into the platform keyring.  If an end-user wanted 
> > > > > to load one, they would need to enroll it into the MOK.
> > > > > 
> > > > > Steps required by the end user:
> > > > > 
> > > > > Sign kernel module with user created key:
> > > > > $ /usr/src/kernels/$(uname -r)/scripts/sign-file sha512 \
> > > > >   machine_signing_key.priv machine_signing_key.x509 my_module.ko
> > > > > 
> > > > > Import the key into the MOK
> > > > > $ mokutil --import machine_signing_key.x509
> > > > > 
> > > > > Setup the kernel to load MOK keys into the .machine keyring
> > > > > $ mokutil --trust-mok
> > > > > 
> > > > > Then reboot, the MokManager will load and ask if you want to trust the
> > > > > MOK key and enroll the MOK into the MOKList.  Afterwards the signed kernel
> > > > > module will load.
> > > > > 
> > > > > I have included links to both the mokutil [5] and shim [6] changes I
> > > > > have made to support this new functionality.
> > > > 
> > > > How hard it is to self-compile shim and boot it with QEMU (I
> > > > do not know even the GIT location of Shim)?
> > > 
> > > It is not hard, that is the setup I use for my testing.  Upstream shim 
> > > is located here [1].  Or you can use my repo which contains the necessary
> > > changes [2].
> > > 
> > > [1] https://github.com/rhboot/shim
> > > [2] https://github.com/esnowberg/shim/tree/mokvars-v2
> > > 
> > 
> > So, my 2nd Q would be: which order these should be upstreamed?
> > 
> > Linux patch set cannot depend on "yet to be upstreamed" things.
> > 
> > Code changes look good enough to me.
> 
> We can carry this support in shim before it's in kernel.  Eric's current
> patch for shim and mokutil looks mostly reasonable, though I see a few
> minor nits we'll have to sort out.

I would revisit this patch set after there is an official shim release
out containing the new API. No  kernel patches, which depend on any
non-upstream changes, can be rightfully reviewed.

/Jarkko