[PATCH v8 00/17] Enroll kernel keys thru MOK

[PATCH v8 00/17] 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  a link to the mokutil [5] changes I have made to support 
this new functionality.  The shim changes have now been accepted
upstream [6].

Upstream shim is located here [7], the build instructions are here [8].
TLDR:

$ git clone --recurse-submodules https://github.com/rhboot/shim
$ cd shim
$ make

After building shim, move shimx64.efi and mmx64.efi to the vendor or 
distribution specific directory on your EFI System Partition (assuming
you are building on x86). The instructions above are the minimal
steps needed to build shim to test this feature. It is assumed
Secure Boot shall not be enabled for this testing. To do testing
with Secure Boot enabled, all steps in the build instructions [8]
must be followed.

Instructions for building mokutil (including the new changes):

$ git clone -b mokvars-v3 https://github.com/esnowberg/mokutil.git
$ cd mokutil/
$ ./autogen.sh
$ make

[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/mokvars-v3
[6] https://github.com/rhboot/shim/commit/4e513405b4f1641710115780d19dcec130c5208f
[7] https://github.com/rhboot/shim
[8] https://github.com/rhboot/shim/blob/main/BUILDING


Eric Snowberg (17):
  KEYS: Create static version of public_key_verify_signature
  integrity: Fix warning about missing prototypes
  integrity: Introduce a Linux keyring called machine
  integrity: Do not allow machine keyring updates following init
  X.509: Parse Basic Constraints for CA
  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
  integrity: store reference to machine keyring
  KEYS: link machine trusted keys to secondary_trusted_keys
  efi/mokvar: move up init order
  integrity: Trust MOK keys if MokListTrustedRT found
  integrity: Only use machine keyring when uefi_check_trust_mok_keys is
    true

 certs/system_keyring.c                        | 48 +++++++++++-
 crypto/asymmetric_keys/restrict.c             | 43 +++++++++++
 crypto/asymmetric_keys/x509_cert_parser.c     |  9 +++
 drivers/firmware/efi/mokvar-table.c           |  2 +-
 include/crypto/public_key.h                   | 25 ++++++
 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          | 18 ++++-
 .../platform_certs/keyring_handler.h          |  5 ++
 security/integrity/platform_certs/load_uefi.c |  4 +-
 .../platform_certs/machine_keyring.c          | 77 +++++++++++++++++++
 14 files changed, 287 insertions(+), 11 deletions(-)
 create mode 100644 security/integrity/platform_certs/machine_keyring.c


base-commit: 136057256686de39cc3a07c2e39ef6bc43003ff6
-- 
2.18.4

[PATCH v8 01/17] KEYS: Create static version of public_key_verify_signature

[Eric Snowberg]

The kernel test robot reports undefined reference to
public_key_verify_signature when CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE is
not defined. Create a static version in this case and return -EINVAL.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
v7: Initial version
v8: Code unmodified from v7 added Mimi's Reviewed-by
---
 include/crypto/public_key.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
index f603325c0c30..a9b2e600b7cc 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -80,7 +80,16 @@ extern int create_signature(struct kernel_pkey_params *, const void *, void *);
 extern int verify_signature(const struct key *,
 			    const struct public_key_signature *);
 
+#if IS_REACHABLE(CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE)
 int public_key_verify_signature(const struct public_key *pkey,
 				const struct public_key_signature *sig);
+#else
+static inline
+int public_key_verify_signature(const struct public_key *pkey,
+				const struct public_key_signature *sig)
+{
+	return -EINVAL;
+}
+#endif
 
 #endif /* _LINUX_PUBLIC_KEY_H */
-- 
2.18.4

[PATCH v8 02/17] integrity: Fix warning about missing prototypes

[Eric Snowberg]

make W=1 generates the following warning in keyring_handler.c

security/integrity/platform_certs/keyring_handler.c:71:30: warning: no previous prototype for get_handler_for_db [-Wmissing-prototypes]
 __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
                              ^~~~~~~~~~~~~~~~~~
security/integrity/platform_certs/keyring_handler.c:82:30: warning: no previous prototype for get_handler_for_dbx [-Wmissing-prototypes]
 __init efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_type)
                              ^~~~~~~~~~~~~~~~~~~
Add the missing prototypes by including keyring_handler.h.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
v7: Initial version
v8: Code unmodified from v7 added Mimi's Reviewed-by
---
 security/integrity/platform_certs/keyring_handler.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
index 5604bd57c990..e9791be98fd9 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -9,6 +9,7 @@
 #include <keys/asymmetric-type.h>
 #include <keys/system_keyring.h>
 #include "../integrity.h"
+#include "keyring_handler.h"
 
 static efi_guid_t efi_cert_x509_guid __initdata = EFI_CERT_X509_GUID;
 static efi_guid_t efi_cert_x509_sha256_guid __initdata =
-- 
2.18.4

[PATCH v8 03/17] 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>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.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)
v7: Depend on LOAD_UEFI_KEYS instead EFI for mokvar code
v8: Code unmodified from v7 added Mimi's Reviewed-by
---
 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..12879dec251d 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 LOAD_UEFI_KEYS
+	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 v8 04/17] 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>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
v2: Initial version
v4: Unmodified from v2
v5: Rename to machine keyring
v6: Add additional comment (suggested by Jarkko)
v7: Unmodified from v6
v8: Code unmodified from v7 added Mimi's Reviewed-by
---
 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 v8 05/17] X.509: Parse Basic Constraints for CA

[Eric Snowberg]

Parse the X.509 Basic Constraints.  The basic constraints extension
identifies whether the subject of the certificate is a CA.

BasicConstraints ::= SEQUENCE {
        cA                      BOOLEAN DEFAULT FALSE,
        pathLenConstraint       INTEGER (0..MAX) OPTIONAL }

If the CA is true, store it in a new public_key field call key_is_ca.
This will be used in a follow on patch that requires knowing if the
public key is a CA.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v7: Initial version
v8: Moved key_is_ca after key_is_private, recommended by Mimi
---
 crypto/asymmetric_keys/x509_cert_parser.c | 9 +++++++++
 include/crypto/public_key.h               | 1 +
 2 files changed, 10 insertions(+)

diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
index 6d003096b5bc..f4299b8a4926 100644
--- a/crypto/asymmetric_keys/x509_cert_parser.c
+++ b/crypto/asymmetric_keys/x509_cert_parser.c
@@ -571,6 +571,15 @@ int x509_process_extension(void *context, size_t hdrlen,
 		return 0;
 	}
 
+	if (ctx->last_oid == OID_basicConstraints) {
+		if (v[0] != (ASN1_CONS_BIT | ASN1_SEQ))
+			return -EBADMSG;
+		if (v[1] != vlen - 2)
+			return -EBADMSG;
+		if (v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
+			ctx->cert->pub->key_is_ca = true;
+	}
+
 	return 0;
 }
 
diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
index a9b2e600b7cc..72dcbc06ef9c 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -26,6 +26,7 @@ struct public_key {
 	void *params;
 	u32 paramlen;
 	bool key_is_private;
+	bool key_is_ca;
 	const char *id_type;
 	const char *pkey_algo;
 };
-- 
2.18.4

[PATCH v8 06/17] 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.

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
v7: Check for CA restruction in public key
v8: Fix issue found by build bot when asym keys not defined in the config
---
 crypto/asymmetric_keys/restrict.c | 43 +++++++++++++++++++++++++++++++
 include/crypto/public_key.h       | 15 +++++++++++
 2 files changed, 58 insertions(+)

diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 84cefe3b3585..a891c598a2aa 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -108,6 +108,49 @@ 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.
+ * @trust_keyring: 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 the
+ * certificate is not a CA. -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;
+
+	if (!pkey->key_is_ca)
+		return -ENOKEY;
+
+	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 72dcbc06ef9c..06e34d3340c4 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -72,6 +72,21 @@ extern int restrict_link_by_key_or_keyring_chain(struct key *trust_keyring,
 						 const union key_payload *payload,
 						 struct key *trusted);
 
+#if IS_REACHABLE(CONFIG_ASYMMETRIC_KEY_TYPE)
+extern int restrict_link_by_ca(struct key *dest_keyring,
+			       const struct key_type *type,
+			       const union key_payload *payload,
+			       struct key *trust_keyring);
+#else
+static inline int restrict_link_by_ca(struct key *dest_keyring,
+				      const struct key_type *type,
+				      const union key_payload *payload,
+				      struct key *trust_keyring)
+{
+	return 0;
+}
+#endif
+
 extern int query_asymmetric_key(const struct kernel_pkey_params *,
 				struct kernel_pkey_query *);
 
-- 
2.18.4

[PATCH v8 07/17] 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)
v8: Unmodified from v6
---
 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 v8 08/17] 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>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.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
v7: Unmodified from v5
v8: Code unmodified from v7 added Mimi's Reviewed-by
---
 .../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 e9791be98fd9..4872850d081f 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -67,7 +67,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)
 {
@@ -76,6 +76,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 v8 09/17] KEYS: Rename get_builtin_and_secondary_restriction

[Eric Snowberg]

In preparation for returning either the existing
restrict_link_by_builtin_and_secondary_trusted or the upcoming
restriction that includes the trusted builtin, secondary and
machine keys, 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>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
v6: Initial version
v7: Unmodified from v7
v8: Code unmodified from v7, added Mimi's Reviewed-by
---
 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 v8 10/17] 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
v8: 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 v8 11/17] 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)
v7: Unmodified from v6
v8: Add missing parameter definitions (suggested by Mimi)
---
 certs/system_keyring.c        | 27 +++++++++++++++++++++++++++
 include/keys/system_keyring.h |  6 ++++++
 2 files changed, 33 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index bc7e44fc82c2..8a2fd1dc15db 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -99,6 +99,33 @@ void __init set_machine_trusted_keys(struct key *keyring)
 {
 	machine_trusted_keys = keyring;
 }
+
+/**
+ * restrict_link_by_builtin_secondary_and_machine - Restrict keyring addition.
+ * @dest_keyring: Keyring being linked to.
+ * @type: The type of key being added.
+ * @payload: The payload of the new key.
+ * @restrict_key: A ring of keys that can be used to vouch for the new cert.
+ *
+ * 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 v8 12/17] 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>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
v4: Initial version (consolidated two previous patches)
v5: Rename to machine keyring
v6: Account for restriction being renamed earlier
v7: Unmodified from v6
v8: Code unmodified from v7 added Mimi's Reviewed-by
---
 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 8a2fd1dc15db..07f410918e62 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 v8 13/17] 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
v8: 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 v8 14/17] KEYS: link machine trusted keys to secondary_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
v7: Unmodified from v5
v8: Change patch subject name, code unmodified from v7
---
 certs/system_keyring.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 07f410918e62..463f676857f0 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 v8 15/17] efi/mokvar: move up init order

[Eric Snowberg]

Move up the init order so it can be used by the new machine keyring.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v7: Initial version
v8: Unmodified from v7
---
 drivers/firmware/efi/mokvar-table.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/mokvar-table.c b/drivers/firmware/efi/mokvar-table.c
index 38722d2009e2..5ed0602c2f75 100644
--- a/drivers/firmware/efi/mokvar-table.c
+++ b/drivers/firmware/efi/mokvar-table.c
@@ -359,4 +359,4 @@ static int __init efi_mokvar_sysfs_init(void)
 	}
 	return err;
 }
-device_initcall(efi_mokvar_sysfs_init);
+fs_initcall(efi_mokvar_sysfs_init);
-- 
2.18.4

[PATCH v8 16/17] 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 themselves 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.

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
v7: Use mokvar table instead of EFI var (suggested by Peter Jones)
v8: Unmodified from v7
---
 .../platform_certs/machine_keyring.c          | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/security/integrity/platform_certs/machine_keyring.c b/security/integrity/platform_certs/machine_keyring.c
index ea2ac2f9f2b5..09fd8f20c756 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,21 @@ 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 MOK 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)
+{
+	struct efi_mokvar_table_entry *mokvar_entry;
+
+	mokvar_entry = efi_mokvar_entry_find("MokListTrustedRT");
+
+	if (mokvar_entry)
+		return true;
+
+	return false;
+}
-- 
2.18.4

[PATCH v8 17/17] 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
v7: Made trust_mok static
v8: Unmodified from v7
---
 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 4872850d081f..1db4d3b4356d 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -83,7 +83,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 09fd8f20c756..7aaed7950b6e 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"
 
+static bool trust_mok;
+
 static __init int machine_keyring_init(void)
 {
 	int rc;
@@ -59,3 +61,17 @@ static __init bool uefi_check_trust_mok_keys(void)
 
 	return false;
 }
+
+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 v8 03/17] integrity: Introduce a Linux keyring called machine

[Mimi Zohar]

Hi Eric,

On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
> +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

Shouldn't this be "ASYMMETRIC_PUBLIC_KEY_SUBTYPE=y"?   With this
change, is  "KEYS: Create static version of
public_key_verify_signature" trusted needed?

Mimi

> +       depends on SYSTEM_BLACKLIST_KEYRING
> +       depends on LOAD_UEFI_KEYS
> +       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.
> +

Re: [PATCH v8 03/17] integrity: Introduce a Linux keyring called machine

[Jarkko Sakkinen]

On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
> 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>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

/Jarkko

Re: [PATCH v8 04/17] integrity: Do not allow machine keyring updates following init

[Jarkko Sakkinen]

On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
> 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>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

For completeness (even if stating the obvious) it would be nice to
say explicitly why no additional keys are not allowed after the init.

/Jarkko

Re: [PATCH v8 05/17] X.509: Parse Basic Constraints for CA

[Jarkko Sakkinen]

On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
> Parse the X.509 Basic Constraints.  The basic constraints extension
> identifies whether the subject of the certificate is a CA.
> 
> BasicConstraints ::= SEQUENCE {
>         cA                      BOOLEAN DEFAULT FALSE,
>         pathLenConstraint       INTEGER (0..MAX) OPTIONAL }
> 
> If the CA is true, store it in a new public_key field call key_is_ca.
> This will be used in a follow on patch that requires knowing if the
> public key is a CA.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

You could extend the description to state why kernel needs X.509 Basic
Constraints support.

/Jarkko

> ---
> v7: Initial version
> v8: Moved key_is_ca after key_is_private, recommended by Mimi
> ---
>  crypto/asymmetric_keys/x509_cert_parser.c | 9 +++++++++
>  include/crypto/public_key.h               | 1 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c
> index 6d003096b5bc..f4299b8a4926 100644
> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -571,6 +571,15 @@ int x509_process_extension(void *context, size_t hdrlen,
>                 return 0;
>         }
>  
> +       if (ctx->last_oid == OID_basicConstraints) {
> +               if (v[0] != (ASN1_CONS_BIT | ASN1_SEQ))
> +                       return -EBADMSG;
> +               if (v[1] != vlen - 2)
> +                       return -EBADMSG;
> +               if (v[1] != 0 && v[2] == ASN1_BOOL && v[3] == 1)
> +                       ctx->cert->pub->key_is_ca = true;
> +       }
> +
>         return 0;
>  }
>  
> diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
> index a9b2e600b7cc..72dcbc06ef9c 100644
> --- a/include/crypto/public_key.h
> +++ b/include/crypto/public_key.h
> @@ -26,6 +26,7 @@ struct public_key {
>         void *params;
>         u32 paramlen;
>         bool key_is_private;
> +       bool key_is_ca;
>         const char *id_type;
>         const char *pkey_algo;
>  };

Re: [PATCH v8 06/17] KEYS: CA link restriction

[Jarkko Sakkinen]

On Tue, 2021-11-23 at 23:41 -0500, 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.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

Also here you should extend the story a bit...

/Jarkko

> ---
> 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
> v7: Check for CA restruction in public key
> v8: Fix issue found by build bot when asym keys not defined in the config
> ---
>  crypto/asymmetric_keys/restrict.c | 43 +++++++++++++++++++++++++++++++
>  include/crypto/public_key.h       | 15 +++++++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
> index 84cefe3b3585..a891c598a2aa 100644
> --- a/crypto/asymmetric_keys/restrict.c
> +++ b/crypto/asymmetric_keys/restrict.c
> @@ -108,6 +108,49 @@ 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.
> + * @trust_keyring: 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 the
> + * certificate is not a CA. -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;
> +
> +       if (!pkey->key_is_ca)
> +               return -ENOKEY;
> +
> +       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 72dcbc06ef9c..06e34d3340c4 100644
> --- a/include/crypto/public_key.h
> +++ b/include/crypto/public_key.h
> @@ -72,6 +72,21 @@ extern int restrict_link_by_key_or_keyring_chain(struct key *trust_keyring,
>                                                  const union key_payload *payload,
>                                                  struct key *trusted);
>  
> +#if IS_REACHABLE(CONFIG_ASYMMETRIC_KEY_TYPE)
> +extern int restrict_link_by_ca(struct key *dest_keyring,
> +                              const struct key_type *type,
> +                              const union key_payload *payload,
> +                              struct key *trust_keyring);
> +#else
> +static inline int restrict_link_by_ca(struct key *dest_keyring,
> +                                     const struct key_type *type,
> +                                     const union key_payload *payload,
> +                                     struct key *trust_keyring)
> +{
> +       return 0;
> +}
> +#endif
> +
>  extern int query_asymmetric_key(const struct kernel_pkey_params *,
>                                 struct kernel_pkey_query *);
>  

Re: [PATCH v8 08/17] integrity: add new keyring handler for mok keys

[Jarkko Sakkinen]

Nit: in the short summary mok -> MOK

Otherwise LGTM.

/Jarkko


On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
> 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>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.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
> v7: Unmodified from v5
> v8: Code unmodified from v7 added Mimi's Reviewed-by
> ---
>  .../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 e9791be98fd9..4872850d081f 100644
> --- a/security/integrity/platform_certs/keyring_handler.c
> +++ b/security/integrity/platform_certs/keyring_handler.c
> @@ -67,7 +67,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)
>  {
> @@ -76,6 +76,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);

Re: [PATCH v8 09/17] KEYS: Rename get_builtin_and_secondary_restriction

[Jarkko Sakkinen]

On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
> In preparation for returning either the existing
> restrict_link_by_builtin_and_secondary_trusted or the upcoming
> restriction that includes the trusted builtin, secondary and
> machine keys, 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>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
> v6: Initial version
> v7: Unmodified from v7
> v8: Code unmodified from v7, added Mimi's Reviewed-by
> ---
>  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");

This is wrong order.

You should first do the changes that make the old name
obsolete and only after that have a patch that does the
rename. Unfortunately, this patch cannot possibly acked
with the current order.


/Jarkko

Re: [PATCH v8 03/17] integrity: Introduce a Linux keyring called machine

[Eric Snowberg]

> On Nov 24, 2021, at 7:49 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
>> +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
> 
> Shouldn't this be "ASYMMETRIC_PUBLIC_KEY_SUBTYPE=y"?   With this
> change, is  "KEYS: Create static version of
> public_key_verify_signature" trusted needed?

I believe it is still needed. If someone were to use the same config as the build bot, 
where ASYMMETRIC_PUBLIC_KEY_SUBTYPE is not defined and 
INTEGRITY_MACHINE_KEYRING is not defined, they would still hit the problem that 
has now been fixed in  "KEYS: Create static version of public_key_verify_signature”. 

I wish the first two patches in this series would be accepted, since I’m only carrying 
them to get past the build bot.

Re: [PATCH v8 09/17] KEYS: Rename get_builtin_and_secondary_restriction

[Eric Snowberg]

> On Nov 26, 2021, at 5:49 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
>> In preparation for returning either the existing
>> restrict_link_by_builtin_and_secondary_trusted or the upcoming
>> restriction that includes the trusted builtin, secondary and
>> machine keys, 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>
>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>> ---
>> v6: Initial version
>> v7: Unmodified from v7
>> v8: Code unmodified from v7, added Mimi's Reviewed-by
>> ---
>>  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");
> 
> This is wrong order.
> 
> You should first do the changes that make the old name
> obsolete and only after that have a patch that does the
> rename. Unfortunately, this patch cannot possibly acked
> with the current order.

I can change the order, but I'm confused how this would work for a git bisect. 
If the rename happens afterwards, now two patches will always need to be 
reverted instead of the possibility of one.  Is this your expectation?

Re: [PATCH v8 09/17] KEYS: Rename get_builtin_and_secondary_restriction

[Jarkko Sakkinen]

On Tue, Nov 30, 2021 at 05:21:45PM +0000, Eric Snowberg wrote:
> 
> 
> > On Nov 26, 2021, at 5:49 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
> >> In preparation for returning either the existing
> >> restrict_link_by_builtin_and_secondary_trusted or the upcoming
> >> restriction that includes the trusted builtin, secondary and
> >> machine keys, 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>
> >> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> >> ---
> >> v6: Initial version
> >> v7: Unmodified from v7
> >> v8: Code unmodified from v7, added Mimi's Reviewed-by
> >> ---
> >>  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");
> > 
> > This is wrong order.
> > 
> > You should first do the changes that make the old name
> > obsolete and only after that have a patch that does the
> > rename. Unfortunately, this patch cannot possibly acked
> > with the current order.
> 
> I can change the order, but I'm confused how this would work for a git bisect. 
> If the rename happens afterwards, now two patches will always need to be 
> reverted instead of the possibility of one.  Is this your expectation?

I'd drop this patch altogether. Old name is a bit ugly but does it matter
all that much?

You already 16 patches without this.

/Jarkko

Re: [PATCH v8 09/17] KEYS: Rename get_builtin_and_secondary_restriction

[Mimi Zohar]

On Wed, 2021-12-01 at 12:27 +0200, Jarkko Sakkinen wrote:
> On Tue, Nov 30, 2021 at 05:21:45PM +0000, Eric Snowberg wrote:
> > 
> > 
> > > On Nov 26, 2021, at 5:49 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > 
> > > On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
> > >> In preparation for returning either the existing
> > >> restrict_link_by_builtin_and_secondary_trusted or the upcoming
> > >> restriction that includes the trusted builtin, secondary and
> > >> machine keys, 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>
> > >> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > >> ---
> > >> v6: Initial version
> > >> v7: Unmodified from v7
> > >> v8: Code unmodified from v7, added Mimi's Reviewed-by
> > >> ---
> > >>  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");
> > > 
> > > This is wrong order.
> > > 
> > > You should first do the changes that make the old name
> > > obsolete and only after that have a patch that does the
> > > rename. Unfortunately, this patch cannot possibly acked
> > > with the current order.
> >
> > I can change the order, but I'm confused how this would work for a git bisect. 
> > If the rename happens afterwards, now two patches will always need to be 
> > reverted instead of the possibility of one.  Is this your expectation?

If the keyring name change is independent of any other changes, as
Jarkko suggested, nothing would break.

> I'd drop this patch altogether. Old name is a bit ugly but does it matter
> all that much?

The name "get_builtin_and_secondary_restriction" implies trust based on
keys in the ".builtin_trusted_keys" and ".secondary_trusted_keys"
keyrings.  This patch set is extending that to include keys on the new
".machine" keyring, by linking it to the secondary keyring.  Is leaving
the name unchanged really an option?

> 
> You already 16 patches without this.

Agreed, it's a lot.  In the past, I've asked Eric to see if some of
them could be squashed.

Mimi

Re: [PATCH v8 09/17] KEYS: Rename get_builtin_and_secondary_restriction

[Jarkko Sakkinen]

On Wed, Dec 01, 2021 at 08:46:53AM -0500, Mimi Zohar wrote:
> On Wed, 2021-12-01 at 12:27 +0200, Jarkko Sakkinen wrote:
> > On Tue, Nov 30, 2021 at 05:21:45PM +0000, Eric Snowberg wrote:
> > > 
> > > 
> > > > On Nov 26, 2021, at 5:49 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > > 
> > > > On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
> > > >> In preparation for returning either the existing
> > > >> restrict_link_by_builtin_and_secondary_trusted or the upcoming
> > > >> restriction that includes the trusted builtin, secondary and
> > > >> machine keys, 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>
> > > >> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> > > >> ---
> > > >> v6: Initial version
> > > >> v7: Unmodified from v7
> > > >> v8: Code unmodified from v7, added Mimi's Reviewed-by
> > > >> ---
> > > >>  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");
> > > > 
> > > > This is wrong order.
> > > > 
> > > > You should first do the changes that make the old name
> > > > obsolete and only after that have a patch that does the
> > > > rename. Unfortunately, this patch cannot possibly acked
> > > > with the current order.
> > >
> > > I can change the order, but I'm confused how this would work for a git bisect. 
> > > If the rename happens afterwards, now two patches will always need to be 
> > > reverted instead of the possibility of one.  Is this your expectation?
> 
> If the keyring name change is independent of any other changes, as
> Jarkko suggested, nothing would break.
> 
> > I'd drop this patch altogether. Old name is a bit ugly but does it matter
> > all that much?
> 
> The name "get_builtin_and_secondary_restriction" implies trust based on
> keys in the ".builtin_trusted_keys" and ".secondary_trusted_keys"
> keyrings.  This patch set is extending that to include keys on the new
> ".machine" keyring, by linking it to the secondary keyring.  Is leaving
> the name unchanged really an option?

Yes, it is an option, as long as it is documented correctly in the
prepending kdoc the symbol name does not matter all that much..

/Jarkko

Re: [PATCH v8 09/17] KEYS: Rename get_builtin_and_secondary_restriction

[Eric Snowberg]

> On Dec 1, 2021, at 6:46 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Wed, 2021-12-01 at 12:27 +0200, Jarkko Sakkinen wrote:
>> On Tue, Nov 30, 2021 at 05:21:45PM +0000, Eric Snowberg wrote:
>>> 
>>> 
>>>> On Nov 26, 2021, at 5:49 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>>>> 
>>>> On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
>>>>> In preparation for returning either the existing
>>>>> restrict_link_by_builtin_and_secondary_trusted or the upcoming
>>>>> restriction that includes the trusted builtin, secondary and
>>>>> machine keys, 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>
>>>>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>>>>> ---
>>>>> v6: Initial version
>>>>> v7: Unmodified from v7
>>>>> v8: Code unmodified from v7, added Mimi's Reviewed-by
>>>>> ---
>>>>> 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");
>>>> 
>>>> This is wrong order.
>>>> 
>>>> You should first do the changes that make the old name
>>>> obsolete and only after that have a patch that does the
>>>> rename. Unfortunately, this patch cannot possibly acked
>>>> with the current order.
>>> 
>>> I can change the order, but I'm confused how this would work for a git bisect. 
>>> If the rename happens afterwards, now two patches will always need to be 
>>> reverted instead of the possibility of one.  Is this your expectation?
> 
> If the keyring name change is independent of any other changes, as
> Jarkko suggested, nothing would break.
> 
>> I'd drop this patch altogether. Old name is a bit ugly but does it matter
>> all that much?
> 
> The name "get_builtin_and_secondary_restriction" implies trust based on
> keys in the ".builtin_trusted_keys" and ".secondary_trusted_keys"
> keyrings.  This patch set is extending that to include keys on the new
> ".machine" keyring, by linking it to the secondary keyring.  Is leaving
> the name unchanged really an option?
> 
>> 
>> You already 16 patches without this.
> 
> Agreed, it's a lot.  In the past, I've asked Eric to see if some of
> them could be squashed.

The series grew based on requests added in each round.  How about
I drop IMA support from the next round?  This would eliminate nine patches
from the series (5, 6, 7, 9-14), leaving six patches to introduce and enable 
the new machine keyring (3, 4, 8, 15-17).  The first two patches could 
be taken today.  The only reason the first two are included is to get this series 
through the kernel test robot.

This would allow the machine keyring to be used for module signing.  Afterwards 
I could introduce the CA restriction behind another Kconfig in a different series to 
add back IMA support. Let me know if this would be a better approach.

Re: [PATCH v8 09/17] KEYS: Rename get_builtin_and_secondary_restriction

[Mimi Zohar]

On Wed, 2021-12-15 at 18:14 +0000, Eric Snowberg wrote:
> 
> > On Dec 1, 2021, at 6:46 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Wed, 2021-12-01 at 12:27 +0200, Jarkko Sakkinen wrote:
> >> On Tue, Nov 30, 2021 at 05:21:45PM +0000, Eric Snowberg wrote:
> >>> 
> >>> 
> >>>> On Nov 26, 2021, at 5:49 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >>>> 
> >>>> On Tue, 2021-11-23 at 23:41 -0500, Eric Snowberg wrote:
> >>>>> In preparation for returning either the existing
> >>>>> restrict_link_by_builtin_and_secondary_trusted or the upcoming
> >>>>> restriction that includes the trusted builtin, secondary and
> >>>>> machine keys, 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>
> >>>>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> >>>>> ---
> >>>>> v6: Initial version
> >>>>> v7: Unmodified from v7
> >>>>> v8: Code unmodified from v7, added Mimi's Reviewed-by
> >>>>> ---
> >>>>> 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");
> >>>> 
> >>>> This is wrong order.
> >>>> 
> >>>> You should first do the changes that make the old name
> >>>> obsolete and only after that have a patch that does the
> >>>> rename. Unfortunately, this patch cannot possibly acked
> >>>> with the current order.
> >>> 
> >>> I can change the order, but I'm confused how this would work for a git bisect. 
> >>> If the rename happens afterwards, now two patches will always need to be 
> >>> reverted instead of the possibility of one.  Is this your expectation?
> > 
> > If the keyring name change is independent of any other changes, as
> > Jarkko suggested, nothing would break.
> > 
> >> I'd drop this patch altogether. Old name is a bit ugly but does it matter
> >> all that much?
> > 
> > The name "get_builtin_and_secondary_restriction" implies trust based on
> > keys in the ".builtin_trusted_keys" and ".secondary_trusted_keys"
> > keyrings.  This patch set is extending that to include keys on the new
> > ".machine" keyring, by linking it to the secondary keyring.  Is leaving
> > the name unchanged really an option?
> > 
> >> 
> >> You already 16 patches without this.
> > 
> > Agreed, it's a lot.  In the past, I've asked Eric to see if some of
> > them could be squashed.
> 
> The series grew based on requests added in each round.  How about
> I drop IMA support from the next round?  This would eliminate nine patches
> from the series (5, 6, 7, 9-14), leaving six patches to introduce and enable 
> the new machine keyring (3, 4, 8, 15-17).  The first two patches could 
> be taken today.  The only reason the first two are included is to get this series 
> through the kernel test robot.
> 
> This would allow the machine keyring to be used for module signing.  Afterwards 
> I could introduce the CA restriction behind another Kconfig in a different series to 
> add back IMA support. Let me know if this would be a better approach.

Once you allow ALL the MOK keys to be loaded onto the .machine keyring,
limiting which keys should be loaded will change the existing expected
behavior.  So you indeed would need to define a new config option.

Your patch set links the ".machine" keyring to the
".secondary_trusted_keys" kerying. Based on
IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY config being
enabled, determines whether IMA trusts the secondary keyring.

From IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY:
        help
          Keys may be added to the IMA or IMA blacklist keyrings, if
the
          key is validly signed by a CA cert in the system built-in or
          secondary trusted keyrings.

          Intermediate keys between those the kernel has compiled in
and the
          IMA keys to be added may be added to the system secondary
keyring,
          provided they are validly signed by a key already resident in
the
          built-in or secondary trusted keyrings.

A dependency to prevent enabling this configuration, if the ".machine"
keyring is enabled, would need to be defined.

thanks,

Mimi

Re: [PATCH v8 10/17] KEYS: add a reference to machine keyring

[Darren Kenny]

On Tuesday, 2021-11-23 at 23:41:17 -05, Eric Snowberg wrote:
> 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>

Reviewed-by: Darren Kenny <darren.kenny@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
> v8: 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

Re: [PATCH v8 11/17] KEYS: Introduce link restriction for machine keys

[Darren Kenny]

On Tuesday, 2021-11-23 at 23:41:18 -05, Eric Snowberg wrote:
> 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>

Reviewed-by: Darren Kenny <darren.kenny@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)
> v7: Unmodified from v6
> v8: Add missing parameter definitions (suggested by Mimi)
> ---
>  certs/system_keyring.c        | 27 +++++++++++++++++++++++++++
>  include/keys/system_keyring.h |  6 ++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index bc7e44fc82c2..8a2fd1dc15db 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -99,6 +99,33 @@ void __init set_machine_trusted_keys(struct key *keyring)
>  {
>  	machine_trusted_keys = keyring;
>  }
> +
> +/**
> + * restrict_link_by_builtin_secondary_and_machine - Restrict keyring addition.
> + * @dest_keyring: Keyring being linked to.
> + * @type: The type of key being added.
> + * @payload: The payload of the new key.
> + * @restrict_key: A ring of keys that can be used to vouch for the new cert.
> + *
> + * 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

Re: [PATCH v8 13/17] integrity: store reference to machine keyring

[Darren Kenny]

On Tuesday, 2021-11-23 at 23:41:20 -05, Eric Snowberg wrote:
> 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>

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

> ---
> v2: Initial version
> v3: Unmodified from v2
> v4: Removed trust_moklist check
> v5: Rename to machine keyring
> v8: 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

Re: [PATCH v8 14/17] KEYS: link machine trusted keys to secondary_trusted_keys

[Darren Kenny]

On Tuesday, 2021-11-23 at 23:41:21 -05, Eric Snowberg wrote:
> 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>

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

> ---
> v3: Initial version
> v4: Unmodified from v3
> v5: Rename to machine keyring
> v7: Unmodified from v5
> v8: Change patch subject name, code unmodified from v7
> ---
>  certs/system_keyring.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 07f410918e62..463f676857f0 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

Re: [PATCH v8 15/17] efi/mokvar: move up init order

[Darren Kenny]

On Tuesday, 2021-11-23 at 23:41:22 -05, Eric Snowberg wrote:
> Move up the init order so it can be used by the new machine keyring.
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

> ---
> v7: Initial version
> v8: Unmodified from v7
> ---
>  drivers/firmware/efi/mokvar-table.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/mokvar-table.c b/drivers/firmware/efi/mokvar-table.c
> index 38722d2009e2..5ed0602c2f75 100644
> --- a/drivers/firmware/efi/mokvar-table.c
> +++ b/drivers/firmware/efi/mokvar-table.c
> @@ -359,4 +359,4 @@ static int __init efi_mokvar_sysfs_init(void)
>  	}
>  	return err;
>  }
> -device_initcall(efi_mokvar_sysfs_init);
> +fs_initcall(efi_mokvar_sysfs_init);
> -- 
> 2.18.4

Re: [PATCH v8 16/17] integrity: Trust MOK keys if MokListTrustedRT found

[Darren Kenny]

On Tuesday, 2021-11-23 at 23:41:23 -05, Eric Snowberg wrote:
> 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 themselves 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.
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

Reviewed-by: Darren Kenny <darren.kenny@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
> v7: Use mokvar table instead of EFI var (suggested by Peter Jones)
> v8: Unmodified from v7
> ---
>  .../platform_certs/machine_keyring.c          | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/security/integrity/platform_certs/machine_keyring.c b/security/integrity/platform_certs/machine_keyring.c
> index ea2ac2f9f2b5..09fd8f20c756 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,21 @@ 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 MOK 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)
> +{
> +	struct efi_mokvar_table_entry *mokvar_entry;
> +
> +	mokvar_entry = efi_mokvar_entry_find("MokListTrustedRT");
> +
> +	if (mokvar_entry)
> +		return true;
> +
> +	return false;
> +}
> -- 
> 2.18.4

Re: [PATCH v8 17/17] integrity: Only use machine keyring when uefi_check_trust_mok_keys is true

[Darren Kenny]

On Tuesday, 2021-11-23 at 23:41:24 -05, Eric Snowberg wrote:
> 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>

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

> ---
> v4: Initial version
> v5: Rename to machine keyring
> v6: Unmodified from v5
> v7: Made trust_mok static
> v8: Unmodified from v7
> ---
>  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 4872850d081f..1db4d3b4356d 100644
> --- a/security/integrity/platform_certs/keyring_handler.c
> +++ b/security/integrity/platform_certs/keyring_handler.c
> @@ -83,7 +83,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 09fd8f20c756..7aaed7950b6e 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"
>  
> +static bool trust_mok;
> +
>  static __init int machine_keyring_init(void)
>  {
>  	int rc;
> @@ -59,3 +61,17 @@ static __init bool uefi_check_trust_mok_keys(void)
>  
>  	return false;
>  }
> +
> +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 v8 07/17] integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca

[Darren Kenny]

On Tuesday, 2021-11-23 at 23:41:14 -05, Eric Snowberg wrote:
> 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>

Reviewed-by: Darren Kenny <darren.kenny@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)
> v8: Unmodified from v6
> ---
>  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

Re: [PATCH v8 00/17] Enroll kernel keys thru MOK

[Jarkko Sakkinen]

On Tue, Nov 23, 2021 at 11:41:07PM -0500, 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  a link to the mokutil [5] changes I have made to support 
> this new functionality.  The shim changes have now been accepted
> upstream [6].
> 
> Upstream shim is located here [7], the build instructions are here [8].
> TLDR:
> 
> $ git clone --recurse-submodules https://github.com/rhboot/shim
> $ cd shim
> $ make
> 
> After building shim, move shimx64.efi and mmx64.efi to the vendor or 
> distribution specific directory on your EFI System Partition (assuming
> you are building on x86). The instructions above are the minimal
> steps needed to build shim to test this feature. It is assumed
> Secure Boot shall not be enabled for this testing. To do testing
> with Secure Boot enabled, all steps in the build instructions [8]
> must be followed.
> 
> Instructions for building mokutil (including the new changes):
> 
> $ git clone -b mokvars-v3 https://github.com/esnowberg/mokutil.git
> $ cd mokutil/
> $ ./autogen.sh
> $ make
> 
> [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/mokvars-v3
> [6] https://github.com/rhboot/shim/commit/4e513405b4f1641710115780d19dcec130c5208f
> [7] https://github.com/rhboot/shim
> [8] https://github.com/rhboot/shim/blob/main/BUILDING
> 
> 
> Eric Snowberg (17):
>   KEYS: Create static version of public_key_verify_signature
>   integrity: Fix warning about missing prototypes
>   integrity: Introduce a Linux keyring called machine
>   integrity: Do not allow machine keyring updates following init
>   X.509: Parse Basic Constraints for CA
>   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
>   integrity: store reference to machine keyring
>   KEYS: link machine trusted keys to secondary_trusted_keys
>   efi/mokvar: move up init order
>   integrity: Trust MOK keys if MokListTrustedRT found
>   integrity: Only use machine keyring when uefi_check_trust_mok_keys is
>     true
> 
>  certs/system_keyring.c                        | 48 +++++++++++-
>  crypto/asymmetric_keys/restrict.c             | 43 +++++++++++
>  crypto/asymmetric_keys/x509_cert_parser.c     |  9 +++
>  drivers/firmware/efi/mokvar-table.c           |  2 +-
>  include/crypto/public_key.h                   | 25 ++++++
>  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          | 18 ++++-
>  .../platform_certs/keyring_handler.h          |  5 ++
>  security/integrity/platform_certs/load_uefi.c |  4 +-
>  .../platform_certs/machine_keyring.c          | 77 +++++++++++++++++++
>  14 files changed, 287 insertions(+), 11 deletions(-)
>  create mode 100644 security/integrity/platform_certs/machine_keyring.c
> 
> 
> base-commit: 136057256686de39cc3a07c2e39ef6bc43003ff6
> -- 
> 2.18.4
> 

When I try to apply this:

$ b4  am 20211124044124.998170-8-eric.snowberg@oracle.com
Looking up https://lore.kernel.org/r/20211124044124.998170-8-eric.snowberg%40oracle.com
Analyzing 40 messages in the thread
Checking attestation on all messages, may take a moment...
# ...
$ git am -3 v8_20211123_eric_snowberg_enroll_kernel_keys_thru_mok.mbx
Applying: KEYS: Create static version of public_key_verify_signature
Applying: integrity: Fix warning about missing prototypes
Applying: integrity: Introduce a Linux keyring called machine
Applying: integrity: Do not allow machine keyring updates following init
Applying: X.509: Parse Basic Constraints for CA
Applying: KEYS: CA link restriction
error: sha1 information is lacking or useless (include/crypto/public_key.h).
error: could not build fake ancestor
Patch failed at 0006 KEYS: CA link restriction
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

BR, Jarkko

Re: [PATCH v8 16/17] integrity: Trust MOK keys if MokListTrustedRT found

[Morten Linderud]

On Tue, Nov 23, 2021 at 11:41:23PM -0500, Eric Snowberg wrote:
> 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 themselves 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.

Hi Eric,

I've been milling around on this patch-set for a while and I have a few issues
with the description of the commit and what the code actually does.

efi_mokvar_entry_find doesn't simply read an UEFI variable as the commit message
suggests, it will look for the MOK variable loaded into the EFI configuration
table. This implies we need this table setup in early boot to take usage of this
patch set.

The only bootloader that does setup this table, is the `shim` as described. But
no other bootloader implements support for the MOK EFI configuration table.

This effectively means that there is still no way for Machine Owners to load
keys into the keyring, for things like module signing, without the shim present
in the bootchain. I find this a bit weird.

Is this an intentional design decision, or could other ways be supported as
well?

-- 
Morten Linderud
PGP: 9C02FF419FECBE16

Re: [PATCH v8 16/17] integrity: Trust MOK keys if MokListTrustedRT found

[Eric Snowberg]

> On Nov 9, 2022, at 5:01 PM, Morten Linderud <morten@linderud.pw> wrote:
> 
> On Tue, Nov 23, 2021 at 11:41:23PM -0500, Eric Snowberg wrote:
>> 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 themselves 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.
> 
> Hi Eric,
> 
> I've been milling around on this patch-set for a while and I have a few issues
> with the description of the commit and what the code actually does.
> 
> efi_mokvar_entry_find doesn't simply read an UEFI variable as the commit message
> suggests, it will look for the MOK variable loaded into the EFI configuration
> table. This implies we need this table setup in early boot to take usage of this
> patch set.
> 
> The only bootloader that does setup this table, is the `shim` as described. But
> no other bootloader implements support for the MOK EFI configuration table.
> 
> This effectively means that there is still no way for Machine Owners to load
> keys into the keyring, for things like module signing, without the shim present
> in the bootchain. I find this a bit weird.
> 
> Is this an intentional design decision, or could other ways be supported as
> well?

In v6 I had it as a RT variable, during the review a request was made [1] to just 
use the EFI configuration table.  If there are other boot loaders that want to use this,
I don’t see why the code in v6 couldn’t be added back.  If the configuration table isn’t
available, it could try reading the RT var next.

1. https://patchwork.kernel.org/project/linux-integrity/patch/20210914211416.34096-13-eric.snowberg@oracle.com/#24453409

Re: [PATCH v8 16/17] integrity: Trust MOK keys if MokListTrustedRT found

[Ard Biesheuvel]

On Thu, 10 Nov 2022 at 01:01, Morten Linderud <morten@linderud.pw> wrote:
>
> On Tue, Nov 23, 2021 at 11:41:23PM -0500, Eric Snowberg wrote:
> > 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 themselves 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.
>
> Hi Eric,
>
> I've been milling around on this patch-set for a while and I have a few issues
> with the description of the commit and what the code actually does.
>
> efi_mokvar_entry_find doesn't simply read an UEFI variable as the commit message
> suggests, it will look for the MOK variable loaded into the EFI configuration
> table. This implies we need this table setup in early boot to take usage of this
> patch set.
>
> The only bootloader that does setup this table, is the `shim` as described. But
> no other bootloader implements support for the MOK EFI configuration table.
>

Does any other bootloader implement support for the (volatile)
MokListTrustedRT variable?

Note that this variable is intentionally volatile, and should be
rejected by the kernel if it is not. The point of these RT variables
or the config tables is that they can only be set at boot if a signed
and therefore trusted agent created them.

Permitting non-volatile variables here defeats the purpose of secure
boot, which aims to prevent exploits from gaining persistence. It
would be bad if you could corrupt the trusted boot chain forever by
setting a variable once.

> This effectively means that there is still no way for Machine Owners to load
> keys into the keyring, for things like module signing, without the shim present
> in the bootchain. I find this a bit weird.
>
> Is this an intentional design decision, or could other ways be supported as
> well?
>

Yes.

If we are looking for a way to use EFI variables to inject additional
certificates into the keyring without the ability to authenticate
them, we should I'd strongly recommend that we disable that by default
and add a big fat warning that it is incompatible with the guarantees
secure boot aims to provide.

Re: [PATCH v8 16/17] integrity: Trust MOK keys if MokListTrustedRT found

[James Bottomley]

On Thu, 2022-11-10 at 01:01 +0100, Morten Linderud wrote:
[...]
> efi_mokvar_entry_find doesn't simply read an UEFI variable as the
> commit message suggests, it will look for the MOK variable loaded
> into the EFI configuration table. This implies we need this table
> setup in early boot to take usage of this patch set.
> 
> The only bootloader that does setup this table, is the `shim` as
> described. But no other bootloader implements support for the MOK EFI
> configuration table.

Just to be precise: shim isn't a boot loader.  It's a trust pivot
device away from the built in UEFI keys to the Machine Owner Keys. 
Shim is designed to be used with another bootloader like grub or sd-
boot.  Now you could load a kernel directly with shim, in the same way
you could load it directly from UEFI, but that doesn't make it a
bootloader.

> 
> This effectively means that there is still no way for Machine Owners
> to load keys into the keyring, for things like module signing,
> without the shim present in the bootchain. I find this a bit weird.
> 
> Is this an intentional design decision, or could other ways be
> supported as well?

Yes, rather than try to have all bootloaders conform to the MoK
protocol, it's easier to implement it in a single purpose component
that can be used with any of them.  Essentially if you want to rely on
the UEFI keys and not do an MoK pivot (as some people do) then you can
remove shim from the sequence.

In many ways that's part of the problem with this patch set: The
underlying assumption is everyone does this trust pivot.  If you don't
do this trust pivot (I don't for instance, having replaced my UEFI keys
with my own) you can't add keys to the kernel this way.  However, how
would the kernel know whether you trust the UEFI keys or not?  The
other problem is that without the shim protocol being present, grub
can't check the kernel signature, which means that even if you do own
your own UEFI keys, you need something to replace shim, like:

https://git.kernel.org/pub/scm/linux/kernel/git/jejb/efitools.git/tree/ShimReplace.c

James

Re: [PATCH v8 16/17] integrity: Trust MOK keys if MokListTrustedRT found

[Morten Linderud]

On Thu, Nov 10, 2022 at 08:42:08AM +0100, Ard Biesheuvel wrote:
> On Thu, 10 Nov 2022 at 01:01, Morten Linderud <morten@linderud.pw> wrote:
> >
> > On Tue, Nov 23, 2021 at 11:41:23PM -0500, Eric Snowberg wrote:
> > > 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 themselves 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.
> >
> > Hi Eric,
> >
> > I've been milling around on this patch-set for a while and I have a few issues
> > with the description of the commit and what the code actually does.
> >
> > efi_mokvar_entry_find doesn't simply read an UEFI variable as the commit message
> > suggests, it will look for the MOK variable loaded into the EFI configuration
> > table. This implies we need this table setup in early boot to take usage of this
> > patch set.
> >
> > The only bootloader that does setup this table, is the `shim` as described. But
> > no other bootloader implements support for the MOK EFI configuration table.
> >
> 
> Does any other bootloader implement support for the (volatile)
> MokListTrustedRT variable?
> 

No. The efforts seems to be mostly focused on supporting a setup where shim
boots grub. Anything besides this has never really been important for the people
writing it.

The main reason for the email is to try and figure out what the EFI
configuration table adds to the threat model.

> Note that this variable is intentionally volatile, and should be
> rejected by the kernel if it is not. The point of these RT variables
> or the config tables is that they can only be set at boot if a signed
> and therefore trusted agent created them.

So the only trusted agent currently is the shim? It claims to be a "trivial EFI
application" but the interplay between mokutils and shim isn't documented and
not trivial to understand.

> Permitting non-volatile variables here defeats the purpose of secure
> boot, which aims to prevent exploits from gaining persistence. It
> would be bad if you could corrupt the trusted boot chain forever by
> setting a variable once.

Would this change if the variable could be read from the EFI configuration table
or as an EFI variable? Instead of the current situation where we only support
the configuration table?

> > This effectively means that there is still no way for Machine Owners to load
> > keys into the keyring, for things like module signing, without the shim present
> > in the bootchain. I find this a bit weird.
> >
> > Is this an intentional design decision, or could other ways be supported as
> > well?
> >
> 
> Yes.

I assume it's a yes to "intentional design decision", and my followup question
to this would be to ask where it's documented?

> If we are looking for a way to use EFI variables to inject additional
> certificates into the keyring without the ability to authenticate
> them, we should I'd strongly recommend that we disable that by default
> and add a big fat warning that it is incompatible with the guarantees
> secure boot aims to provide.

The present features are all disabled by default I believe?

Most of this seems to be knowledge residing between a few people that was
present at the implementation, and as a user-space tool author comming into this
10 years later, it's really hard to figure out how a lot of these decisions came
to be.

-- 
Morten Linderud
PGP: 9C02FF419FECBE16

Re: [PATCH v8 16/17] integrity: Trust MOK keys if MokListTrustedRT found

[Morten Linderud]

On Thu, Nov 10, 2022 at 12:54:43AM +0000, Eric Snowberg wrote:
> 
> 
> > On Nov 9, 2022, at 5:01 PM, Morten Linderud <morten@linderud.pw> wrote:
> > 
> > On Tue, Nov 23, 2021 at 11:41:23PM -0500, Eric Snowberg wrote:
> >> 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 themselves 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.
> > 
> > Hi Eric,
> > 
> > I've been milling around on this patch-set for a while and I have a few issues
> > with the description of the commit and what the code actually does.
> > 
> > efi_mokvar_entry_find doesn't simply read an UEFI variable as the commit message
> > suggests, it will look for the MOK variable loaded into the EFI configuration
> > table. This implies we need this table setup in early boot to take usage of this
> > patch set.
> > 
> > The only bootloader that does setup this table, is the `shim` as described. But
> > no other bootloader implements support for the MOK EFI configuration table.
> > 
> > This effectively means that there is still no way for Machine Owners to load
> > keys into the keyring, for things like module signing, without the shim present
> > in the bootchain. I find this a bit weird.
> > 
> > Is this an intentional design decision, or could other ways be supported as
> > well?
> 
> In v6 I had it as a RT variable, during the review a request was made [1] to just 
> use the EFI configuration table.  If there are other boot loaders that want to use this,
> I don’t see why the code in v6 couldn’t be added back.  If the configuration table isn’t
> available, it could try reading the RT var next.
> 
> 1. https://patchwork.kernel.org/project/linux-integrity/patch/20210914211416.34096-13-eric.snowberg@oracle.com/#24453409
> 

If we could support both the EFI variables and the EFI configuration table setup
it would hopefully be easier for others to implement the interface? I wouldn't
mind trying to write a patch for that if others think it's a good idea.

I'm not really sure what Peter means with "much more reliable" though.

-- 
Morten Linderud
PGP: 9C02FF419FECBE16

Re: [PATCH v8 16/17] integrity: Trust MOK keys if MokListTrustedRT found

[James Bottomley]

On Thu, 2022-11-10 at 16:06 +0100, Morten Linderud wrote:
> I'm not really sure what Peter means with "much more reliable"
> though.

It's that in-head knowledge you referred to.  You can't see the true
MoK variables because they're BootServices, meaning they're not visible
in the RunTime, which is why the shadow RT variables exist (this is a
security property: BS only variables can only be altered by trusted,
signed entities).  However lots of things can create RT variables so
you have to run through a sequence of checks on the RT shadows to try
to defeat clever attackers (like verifying the variable attributes),
because the chain of custody from BS to RT is not guaranteed.  If you
use a configuration table instead, that is BS only, the kernel (which
is also a trusted entity) has to pick it out before ExitBootServices,
so if the kernel has the table, you have a reliable chain of custody
for the entries.

James

Re: [PATCH v8 16/17] integrity: Trust MOK keys if MokListTrustedRT found

[Ard Biesheuvel]

On Thu, 10 Nov 2022 at 16:27, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Thu, 2022-11-10 at 16:06 +0100, Morten Linderud wrote:
> > I'm not really sure what Peter means with "much more reliable"
> > though.
>
> It's that in-head knowledge you referred to.  You can't see the true
> MoK variables because they're BootServices, meaning they're not visible
> in the RunTime, which is why the shadow RT variables exist (this is a
> security property: BS only variables can only be altered by trusted,
> signed entities).  However lots of things can create RT variables so
> you have to run through a sequence of checks on the RT shadows to try
> to defeat clever attackers (like verifying the variable attributes),
> because the chain of custody from BS to RT is not guaranteed.  If you
> use a configuration table instead, that is BS only, the kernel (which
> is also a trusted entity) has to pick it out before ExitBootServices,
> so if the kernel has the table, you have a reliable chain of custody
> for the entries.
>

No config table are always accessible, also at runtime under the OS.

But they are volatile so they can only have been created since the
last reset of the system, so in that sense they are similar to the
volatile RT variables aliases.

The reason for preferring config tables is that you can access them
much earlier, and without mapping the EFI runtime memory regions etc
etc