[PATCH v9 0/8] Enroll kernel keys thru MOK

[PATCH v9 0/8] 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. 

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.

Unlike previous versions of this patch set, IMA support has been removed
to simplify the series. After acceptance, a follow-on series will add IMA 
support.

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 (8):
  integrity: Fix warning about missing prototypes
  integrity: Introduce a Linux keyring called machine
  integrity: add new keyring handler for mok keys
  KEYS: store reference to machine keyring
  KEYS: Introduce link restriction for machine 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                        | 44 ++++++++++-
 drivers/firmware/efi/mokvar-table.c           |  2 +-
 include/keys/system_keyring.h                 | 14 ++++
 security/integrity/Kconfig                    | 13 ++++
 security/integrity/Makefile                   |  1 +
 security/integrity/digsig.c                   | 15 +++-
 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 +++++++++++++++++++
 11 files changed, 202 insertions(+), 8 deletions(-)
 create mode 100644 security/integrity/platform_certs/machine_keyring.c


base-commit: c9e6606c7fe92b50a02ce51dda82586ebdf99b48
-- 
2.18.4

[PATCH v9 1/8] 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
v9: Unmodified from v8
---
 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 v9 2/8] integrity: Introduce a Linux keyring called machine

[Eric Snowberg]

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

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

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v2: Removed destory keyring code
v3: Unmodified from v2
v4: Add Kconfig, merged in "integrity: add add_to_mok_keyring" 
v5: Rename to machine keyring
v6: Depend on EFI in kconfig  (suggested by Mimi)
    Test to see if ".platform" keyring is configured in
      add_to_machine_keyring (suggested by Mimi)
v7: Depend on LOAD_UEFI_KEYS instead EFI for mokvar code
v8: Code unmodified from v7 added Mimi's Reviewed-by
v9: Removed Reviewed-by. Prevent IMA from being able to
     use the machine keyring since the CA restrictions
     have been removed.
---
 security/integrity/Kconfig                    | 13 ++++++
 security/integrity/Makefile                   |  1 +
 security/integrity/digsig.c                   | 13 +++++-
 security/integrity/integrity.h                | 12 +++++-
 .../platform_certs/machine_keyring.c          | 42 +++++++++++++++++++
 5 files changed, 78 insertions(+), 3 deletions(-)
 create mode 100644 security/integrity/platform_certs/machine_keyring.c

diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index 71f0177e8716..de02964dd421 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -62,6 +62,19 @@ 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
+	depends on !IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
+	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..2b7fa85613c0 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
@@ -126,7 +127,8 @@ int __init integrity_init_keyring(const unsigned int id)
 	perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW
 		| KEY_USR_READ | KEY_USR_SEARCH;
 
-	if (id == INTEGRITY_KEYRING_PLATFORM) {
+	if (id == INTEGRITY_KEYRING_PLATFORM ||
+	    id == INTEGRITY_KEYRING_MACHINE) {
 		restriction = NULL;
 		goto out;
 	}
@@ -139,7 +141,14 @@ int __init integrity_init_keyring(const unsigned int id)
 		return -ENOMEM;
 
 	restriction->check = restrict_link_to_ima;
-	perm |= KEY_USR_WRITE;
+
+	/*
+	 * MOK keys can only be added through a read-only runtime services
+	 * UEFI variable during boot. No additional keys shall be allowed to
+	 * load into the machine keyring following init from userspace.
+	 */
+	if (id != INTEGRITY_KEYRING_MACHINE)
+		perm |= KEY_USR_WRITE;
 
 out:
 	return __integrity_init_keyring(id, perm, restriction);
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 v9 3/8] 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
v9: Unmodified from v8
---
 .../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 v9 4/8] KEYS: store reference to machine keyring

[Eric Snowberg]

Expose the .machine keyring created in integrity code by adding
a reference.  Store a reference to the machine keyring in
system keyring code. The system keyring code needs this to complete
the keyring link to the 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
v9: Combine with "add reference to machine keyring" patch
---
 certs/system_keyring.c        | 9 +++++++++
 include/keys/system_keyring.h | 8 ++++++++
 security/integrity/digsig.c   | 2 ++
 3 files changed, 19 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 692365dee2bd..08ea542c8096 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_builtin_and_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);
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 2b7fa85613c0..7b719aa76188 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -112,6 +112,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 v9 5/8] 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.

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.

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.

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)
v9: Combine with "change link restriction to trust the machine keyring"
      patch
---
 certs/system_keyring.c        | 35 ++++++++++++++++++++++++++++++++++-
 include/keys/system_keyring.h |  6 ++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 08ea542c8096..05b66ce9d1c9 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -89,7 +89,10 @@ static __init struct key_restriction *get_builtin_and_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;
 }
@@ -98,6 +101,36 @@ static __init struct key_restriction *get_builtin_and_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");
+}
+
+/**
+ * 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 v9 6/8] 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
v9: 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 v9 7/8] 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)
v9: 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 v9 8/8] 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 7b719aa76188..c8c8a4a4e7a0 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -112,7 +112,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 v9 1/8] integrity: Fix warning about missing prototypes

[Jarkko Sakkinen]

On Wed, Jan 05, 2022 at 06:50:05PM -0500, Eric Snowberg wrote:
> 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
> v9: Unmodified from v8
> ---
>  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
> 

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

BR, Jarkko

Re: [PATCH v9 3/8] integrity: add new keyring handler for mok keys

[Jarkko Sakkinen]

On Wed, Jan 05, 2022 at 06:50:07PM -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
> v9: Unmodified from v8
> ---
>  .../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
> 

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

/Jarkko

Re: [PATCH v9 4/8] KEYS: store reference to machine keyring

[Jarkko Sakkinen]

On Wed, Jan 05, 2022 at 06:50:08PM -0500, Eric Snowberg wrote:
> Expose the .machine keyring created in integrity code by adding
> a reference.  Store a reference to the machine keyring in
> system keyring code. The system keyring code needs this to complete
> the keyring link to the 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
> v9: Combine with "add reference to machine keyring" patch
> ---
>  certs/system_keyring.c        | 9 +++++++++
>  include/keys/system_keyring.h | 8 ++++++++
>  security/integrity/digsig.c   | 2 ++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 692365dee2bd..08ea542c8096 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_builtin_and_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);
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 2b7fa85613c0..7b719aa76188 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -112,6 +112,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
> 


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

BR, Jarkko

Re: [PATCH v9 5/8] KEYS: Introduce link restriction for machine keys

[Jarkko Sakkinen]

On Wed, Jan 05, 2022 at 06:50:09PM -0500, 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.
> 
> 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.
> 
> 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.
> 
> 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)
> v9: Combine with "change link restriction to trust the machine keyring"
>       patch
> ---
>  certs/system_keyring.c        | 35 ++++++++++++++++++++++++++++++++++-
>  include/keys/system_keyring.h |  6 ++++++
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 08ea542c8096..05b66ce9d1c9 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -89,7 +89,10 @@ static __init struct key_restriction *get_builtin_and_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;
>  }
> @@ -98,6 +101,36 @@ static __init struct key_restriction *get_builtin_and_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");
> +}
> +
> +/**
> + * 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
> 

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

/Jarkko

Re: [PATCH v9 6/8] efi/mokvar: move up init order

[Jarkko Sakkinen]

On Wed, Jan 05, 2022 at 06:50:10PM -0500, 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>
> ---
> v7: Initial version
> v9: 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
> 

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

/Jarkko

Re: [PATCH v9 7/8] integrity: Trust MOK keys if MokListTrustedRT found

[Jarkko Sakkinen]

On Wed, Jan 05, 2022 at 06:50:11PM -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.
> 
> 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)
> v9: 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
> 


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

BR, Jarkko

Re: [PATCH v9 8/8] integrity: Only use machine keyring when uefi_check_trust_mok_keys is true

[Jarkko Sakkinen]

On Wed, Jan 05, 2022 at 06:50:12PM -0500, 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>
> ---
> 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 7b719aa76188..c8c8a4a4e7a0 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -112,7 +112,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
> 

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

Mimi, have you tested these patches already?

/Jarkko

Re: [PATCH v9 8/8] integrity: Only use machine keyring when uefi_check_trust_mok_keys is true

[Mimi Zohar]

On Sun, 2022-01-09 at 00:30 +0200, Jarkko Sakkinen wrote:
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> Mimi, have you tested these patches already?

Sorry, not yet this version this of the patch set.  Planning to test
shortly.

Mimi

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

[Mimi Zohar]

On Wed, 2022-01-05 at 18:50 -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.

True, from an IMA perspective only the CA keys should be loaded onto
the .machine keyring, but this version (v9) of the patch set does not
enforce that.  The patch set and this paragraph are out of sync.

Jarkko, my concern is that once this version of the patch set is
upstreamed, would limiting which keys may be loaded onto the .machine
keyring be considered a regression?

thanks,

Mimi

> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
> v1: Initial version
> v2: Removed destory keyring code
> v3: Unmodified from v2
> v4: Add Kconfig, merged in "integrity: add add_to_mok_keyring" 
> v5: Rename to machine keyring
> v6: Depend on EFI in kconfig  (suggested by Mimi)
>     Test to see if ".platform" keyring is configured in
>       add_to_machine_keyring (suggested by Mimi)
> v7: Depend on LOAD_UEFI_KEYS instead EFI for mokvar code
> v8: Code unmodified from v7 added Mimi's Reviewed-by
> v9: Removed Reviewed-by. Prevent IMA from being able to
>      use the machine keyring since the CA restrictions
>      have been removed.

Re: [PATCH v9 5/8] KEYS: Introduce link restriction for machine keys

[Mimi Zohar]

On Wed, 2022-01-05 at 18:50 -0500, 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.
> 
> 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.

As suggested the Kconfig in "[PATCH v9 2/8] integrity: Introduce a
Linux keyring called machine" only loads the platform keys onto the
.machine keyring, when
IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY is not enabled.  The
last sentence needs to be updated to reflect v9.

thanks,

Mimi

Re: [PATCH v9 8/8] integrity: Only use machine keyring when uefi_check_trust_mok_keys is true

[Mimi Zohar]

On Sat, 2022-01-08 at 20:47 -0500, Mimi Zohar wrote:
> On Sun, 2022-01-09 at 00:30 +0200, Jarkko Sakkinen wrote:
> > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > Mimi, have you tested these patches already?
> 
> Sorry, not yet this version this of the patch set.  Planning to test
> shortly.

I've only tested v9 1/8 - 6/8 patches on top of Takashi Iwai and Joey
Lee's patches, which are queued to be upstreamed.

92ad19559ea9 integrity: Do not load MOK and MOKx when secure boot be
disabled
54bf7fa3efd0 ima: Fix undefined arch_ima_get_secureboot() and co

With secure boot enabled and
IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY disabled,  all of
the MOK keys are loaded onto the .machine keyring.  With secure boot
disabled, none of the MOK keys are loaded onto either the .platform or
.machine keyrings, irrespective of
IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY.

This patch set is working as expected, but the v9 2/8 & 5/8 patch
descriptions haven't been updated, nor have the related comments been
updated.  Please note that the subsequent patch set will limit the MOK
keys being loaded onto the .machine keyring to only the MOK CA keys.

thanks,

Mimi

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

[Eric Snowberg]

> On Jan 9, 2022, at 2:57 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Wed, 2022-01-05 at 18:50 -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.
> 
> True, from an IMA perspective only the CA keys should be loaded onto
> the .machine keyring, but this version (v9) of the patch set does not
> enforce that.  The patch set and this paragraph are out of sync.

I missed that when I dropped IMA support.  I will strike that sentence
in the next round.  Or if no code changes are identified, feel free to
remove that sentence.  

> Jarkko, my concern is that once this version of the patch set is
> upstreamed, would limiting which keys may be loaded onto the .machine
> keyring be considered a regression?


Currently certificates built into the kernel do not have a CA restriction on them.  
IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
be advisable for a kernel to be built with a CA, nothing currently enforces it. 

My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
This Kconfig would do the CA enforcement on the machine keyring.  However if the 
Kconfig option was not set for enforcement, it would work as it does in this series, 
plus it would allow IMA to work with non-CA keys.  This would be done by removing 
the restriction placed in this patch. Let me know your thoughts on whether this would 
be an appropriate solution.  I believe this would get around what you are identifying as 
a possible regression.

Re: [PATCH v9 5/8] KEYS: Introduce link restriction for machine keys

[Eric Snowberg]

> On Jan 9, 2022, at 3:42 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Wed, 2022-01-05 at 18:50 -0500, 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.
>> 
>> 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.
> 
> As suggested the Kconfig in "[PATCH v9 2/8] integrity: Introduce a
> Linux keyring called machine" only loads the platform keys onto the
> .machine keyring, when
> IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY is not enabled.  The
> last sentence needs to be updated to reflect v9.

I missed that when I dropped IMA support.  I will remove the ima reference in the next
round.  Thanks.

Re: [PATCH v9 8/8] integrity: Only use machine keyring when uefi_check_trust_mok_keys is true

[Jarkko Sakkinen]

On Sat, Jan 08, 2022 at 08:47:21PM -0500, Mimi Zohar wrote:
> On Sun, 2022-01-09 at 00:30 +0200, Jarkko Sakkinen wrote:
> > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > Mimi, have you tested these patches already?
> 
> Sorry, not yet this version this of the patch set.  Planning to test
> shortly.
> 
> Mimi
> 

Yeah, it is otherwise good IMHO. It's too late for v5.17 tho, so there is
no rush. We will get this to v5.18.

/Jarkko

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

[Mimi Zohar]

On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> > Jarkko, my concern is that once this version of the patch set is
> > upstreamed, would limiting which keys may be loaded onto the .machine
> > keyring be considered a regression?
> 
> 
> Currently certificates built into the kernel do not have a CA restriction on them.  
> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> 
> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> Kconfig option was not set for enforcement, it would work as it does in this series, 
> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> the restriction placed in this patch. Let me know your thoughts on whether this would 
> be an appropriate solution.  I believe this would get around what you are identifying as 
> a possible regression.

True the problem currently exists with the builtin keys, but there's a
major difference between trusting the builtin keys and those being
loading via MOK.  This is an integrity gap that needs to be closed and
shouldn't be expanded to keys on the .machine keyring.

"plus it would allow IMA to work with non-CA keys" is unacceptable.

Mimi

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

[Eric Snowberg]

> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
>>> Jarkko, my concern is that once this version of the patch set is
>>> upstreamed, would limiting which keys may be loaded onto the .machine
>>> keyring be considered a regression?
>> 
>> 
>> Currently certificates built into the kernel do not have a CA restriction on them.  
>> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
>> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
>> 
>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
>> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
>> Kconfig option was not set for enforcement, it would work as it does in this series, 
>> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
>> the restriction placed in this patch. Let me know your thoughts on whether this would 
>> be an appropriate solution.  I believe this would get around what you are identifying as 
>> a possible regression.
> 
> True the problem currently exists with the builtin keys, but there's a
> major difference between trusting the builtin keys and those being
> loading via MOK.  This is an integrity gap that needs to be closed and
> shouldn't be expanded to keys on the .machine keyring.
> 
> "plus it would allow IMA to work with non-CA keys" is unacceptable.

Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
cover letter, which adds IMA support, on why it is unacceptable for the end-user to
make this decision?

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

[Mimi Zohar]

On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
> 
> > On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> >>> Jarkko, my concern is that once this version of the patch set is
> >>> upstreamed, would limiting which keys may be loaded onto the .machine
> >>> keyring be considered a regression?
> >> 
> >> 
> >> Currently certificates built into the kernel do not have a CA restriction on them.  
> >> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> >> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> >> 
> >> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> >> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> >> Kconfig option was not set for enforcement, it would work as it does in this series, 
> >> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> >> the restriction placed in this patch. Let me know your thoughts on whether this would 
> >> be an appropriate solution.  I believe this would get around what you are identifying as 
> >> a possible regression.
> > 
> > True the problem currently exists with the builtin keys, but there's a
> > major difference between trusting the builtin keys and those being
> > loading via MOK.  This is an integrity gap that needs to be closed and
> > shouldn't be expanded to keys on the .machine keyring.
> > 
> > "plus it would allow IMA to work with non-CA keys" is unacceptable.
> 
> Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
> cover letter, which adds IMA support, on why it is unacceptable for the end-user to
> make this decision?

The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
"help" is very clear:

        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.

Mimi

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

[Mimi Zohar]

On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
> > 
> > > On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > 
> > > On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> > >>> Jarkko, my concern is that once this version of the patch set is
> > >>> upstreamed, would limiting which keys may be loaded onto the .machine
> > >>> keyring be considered a regression?
> > >> 
> > >> 
> > >> Currently certificates built into the kernel do not have a CA restriction on them.  
> > >> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> > >> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> > >> 
> > >> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> > >> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> > >> Kconfig option was not set for enforcement, it would work as it does in this series, 
> > >> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> > >> the restriction placed in this patch. Let me know your thoughts on whether this would 
> > >> be an appropriate solution.  I believe this would get around what you are identifying as 
> > >> a possible regression.
> > > 
> > > True the problem currently exists with the builtin keys, but there's a
> > > major difference between trusting the builtin keys and those being
> > > loading via MOK.  This is an integrity gap that needs to be closed and
> > > shouldn't be expanded to keys on the .machine keyring.
> > > 
> > > "plus it would allow IMA to work with non-CA keys" is unacceptable.
> > 
> > Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
> > cover letter, which adds IMA support, on why it is unacceptable for the end-user to
> > make this decision?
> 
> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> "help" is very clear:

[Reposting the text due to email formatting issues.]

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

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


The first paragraph requires "validly signed by a CA cert in the system
built-in or secondary trusted keyrings" for keys to be loaded onto the
IMA keyring.  This Kconfig is limited to just the builtin and secondary
keyrings.  Changing this silently to include the ".machine" keyring
introduces integrity risks that previously did not exist.  A new IMA
Kconfig needs to be defined to allow all three keyrings - builtin,
machine, and secondary.

The second paragraph implies that only CA and intermediate CA keys are
on secondary keyring, or as in our case the ".machine" keyring linked
to the secondary keyring.

Mimi

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

[Mimi Zohar]

On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
> > 
> > > On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > 
> > > On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> > >>> Jarkko, my concern is that once this version of the patch set is
> > >>> upstreamed, would limiting which keys may be loaded onto the .machine
> > >>> keyring be considered a regression?
> > >> 
> > >> 
> > >> Currently certificates built into the kernel do not have a CA restriction on them.  
> > >> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> > >> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> > >> 
> > >> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> > >> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> > >> Kconfig option was not set for enforcement, it would work as it does in this series, 
> > >> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> > >> the restriction placed in this patch. Let me know your thoughts on whether this would 
> > >> be an appropriate solution.  I believe this would get around what you are identifying as 
> > >> a possible regression.
> > > 
> > > True the problem currently exists with the builtin keys, but there's a
> > > major difference between trusting the builtin keys and those being
> > > loading via MOK.  This is an integrity gap that needs to be closed and
> > > shouldn't be expanded to keys on the .machine keyring.
> > > 
> > > "plus it would allow IMA to work with non-CA keys" is unacceptable.
> > 
> > Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
> > cover letter, which adds IMA support, on why it is unacceptable for the end-user to
> > make this decision?
> 
> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> "help" is very clear:

[Reposting the text due to email formatting issues.]

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

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


The first paragraph requires "validly signed by a CA cert in the system
built-in or secondary trusted keyrings" for keys to be loaded onto the
IMA keyring.  This Kconfig is limited to just the builtin and secondary
keyrings.  Changing this silently to include the ".machine" keyring
introduces integrity risks that previously did not exist.  A new IMA
Kconfig needs to be defined to allow all three keyrings - builtin,
machine, and secondary.

The second paragraph implies that only CA and intermediate CA keys are
on secondary keyring, or as in our case the ".machine" keyring linked
to the secondary keyring.

Mimi

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

[Eric Snowberg]

> On Jan 12, 2022, at 12:41 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
>> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
>>> 
>>>> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>> 
>>>> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
>>>>>> Jarkko, my concern is that once this version of the patch set is
>>>>>> upstreamed, would limiting which keys may be loaded onto the .machine
>>>>>> keyring be considered a regression?
>>>>> 
>>>>> 
>>>>> Currently certificates built into the kernel do not have a CA restriction on them.  
>>>>> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
>>>>> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
>>>>> 
>>>>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
>>>>> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
>>>>> Kconfig option was not set for enforcement, it would work as it does in this series, 
>>>>> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
>>>>> the restriction placed in this patch. Let me know your thoughts on whether this would 
>>>>> be an appropriate solution.  I believe this would get around what you are identifying as 
>>>>> a possible regression.
>>>> 
>>>> True the problem currently exists with the builtin keys, but there's a
>>>> major difference between trusting the builtin keys and those being
>>>> loading via MOK.  This is an integrity gap that needs to be closed and
>>>> shouldn't be expanded to keys on the .machine keyring.
>>>> 
>>>> "plus it would allow IMA to work with non-CA keys" is unacceptable.
>>> 
>>> Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
>>> cover letter, which adds IMA support, on why it is unacceptable for the end-user to
>>> make this decision?
>> 
>> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
>> "help" is very clear:
> 
> [Reposting the text due to email formatting issues.]
> 
> help
>  Keys may be added to the IMA or IMA blacklist keyrings, if the
>  key is validly signed by a CA cert in the system built-in or
>  secondary trusted keyrings.
> 
>  Intermediate keys between those the kernel has compiled in and the 
>  IMA keys to be added may be added to the system secondary keyring,
>  provided they are validly signed by a key already resident in the
>  built-in or secondary trusted keyrings.
> 
> 
> The first paragraph requires "validly signed by a CA cert in the system
> built-in or secondary trusted keyrings" for keys to be loaded onto the
> IMA keyring.  This Kconfig is limited to just the builtin and secondary
> keyrings.  Changing this silently to include the ".machine" keyring
> introduces integrity risks that previously did not exist.  A new IMA
> Kconfig needs to be defined to allow all three keyrings - builtin,
> machine, and secondary.
> 
> The second paragraph implies that only CA and intermediate CA keys are
> on secondary keyring, or as in our case the ".machine" keyring linked
> to the secondary keyring.

Got it, thanks.  I’ll use this in the cover letter that introduces the CA restrictions 
to enable IMA.

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

[Jarkko Sakkinen]

On Wed, Jan 12, 2022 at 02:41:47PM -0500, Mimi Zohar wrote:
> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
> > On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
> > > 
> > > > On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > > 
> > > > On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> > > >>> Jarkko, my concern is that once this version of the patch set is
> > > >>> upstreamed, would limiting which keys may be loaded onto the .machine
> > > >>> keyring be considered a regression?
> > > >> 
> > > >> 
> > > >> Currently certificates built into the kernel do not have a CA restriction on them.  
> > > >> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> > > >> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> > > >> 
> > > >> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> > > >> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> > > >> Kconfig option was not set for enforcement, it would work as it does in this series, 
> > > >> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> > > >> the restriction placed in this patch. Let me know your thoughts on whether this would 
> > > >> be an appropriate solution.  I believe this would get around what you are identifying as 
> > > >> a possible regression.
> > > > 
> > > > True the problem currently exists with the builtin keys, but there's a
> > > > major difference between trusting the builtin keys and those being
> > > > loading via MOK.  This is an integrity gap that needs to be closed and
> > > > shouldn't be expanded to keys on the .machine keyring.
> > > > 
> > > > "plus it would allow IMA to work with non-CA keys" is unacceptable.
> > > 
> > > Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
> > > cover letter, which adds IMA support, on why it is unacceptable for the end-user to
> > > make this decision?
> > 
> > The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> > "help" is very clear:
> 
> [Reposting the text due to email formatting issues.]
> 
> help
>   Keys may be added to the IMA or IMA blacklist keyrings, if the
>   key is validly signed by a CA cert in the system built-in or
>   secondary trusted keyrings.
> 
>   Intermediate keys between those the kernel has compiled in and the 
>   IMA keys to be added may be added to the system secondary keyring,
>   provided they are validly signed by a key already resident in the
>   built-in or secondary trusted keyrings.
> 
> 
> The first paragraph requires "validly signed by a CA cert in the system
> built-in or secondary trusted keyrings" for keys to be loaded onto the
> IMA keyring.  This Kconfig is limited to just the builtin and secondary
> keyrings.  Changing this silently to include the ".machine" keyring
> introduces integrity risks that previously did not exist.  A new IMA
> Kconfig needs to be defined to allow all three keyrings - builtin,
> machine, and secondary.
> 
> The second paragraph implies that only CA and intermediate CA keys are
> on secondary keyring, or as in our case the ".machine" keyring linked
> to the secondary keyring.
> 
> Mimi
> 
I have also now test environment for this patch set but if there are
any possible changes, I'm waiting for a new version, as it is anyway
for 5.18 cycle earliest.

/Jarkko

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

[Eric Snowberg]

> On Jan 15, 2022, at 10:11 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Wed, Jan 12, 2022 at 02:41:47PM -0500, Mimi Zohar wrote:
>> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
>>> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
>>>> 
>>>>> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>> 
>>>>> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
>>>>>>> Jarkko, my concern is that once this version of the patch set is
>>>>>>> upstreamed, would limiting which keys may be loaded onto the .machine
>>>>>>> keyring be considered a regression?
>>>>>> 
>>>>>> 
>>>>>> Currently certificates built into the kernel do not have a CA restriction on them.  
>>>>>> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
>>>>>> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
>>>>>> 
>>>>>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
>>>>>> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
>>>>>> Kconfig option was not set for enforcement, it would work as it does in this series, 
>>>>>> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
>>>>>> the restriction placed in this patch. Let me know your thoughts on whether this would 
>>>>>> be an appropriate solution.  I believe this would get around what you are identifying as 
>>>>>> a possible regression.
>>>>> 
>>>>> True the problem currently exists with the builtin keys, but there's a
>>>>> major difference between trusting the builtin keys and those being
>>>>> loading via MOK.  This is an integrity gap that needs to be closed and
>>>>> shouldn't be expanded to keys on the .machine keyring.
>>>>> 
>>>>> "plus it would allow IMA to work with non-CA keys" is unacceptable.
>>>> 
>>>> Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
>>>> cover letter, which adds IMA support, on why it is unacceptable for the end-user to
>>>> make this decision?
>>> 
>>> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
>>> "help" is very clear:
>> 
>> [Reposting the text due to email formatting issues.]
>> 
>> help
>>  Keys may be added to the IMA or IMA blacklist keyrings, if the
>>  key is validly signed by a CA cert in the system built-in or
>>  secondary trusted keyrings.
>> 
>>  Intermediate keys between those the kernel has compiled in and the 
>>  IMA keys to be added may be added to the system secondary keyring,
>>  provided they are validly signed by a key already resident in the
>>  built-in or secondary trusted keyrings.
>> 
>> 
>> The first paragraph requires "validly signed by a CA cert in the system
>> built-in or secondary trusted keyrings" for keys to be loaded onto the
>> IMA keyring.  This Kconfig is limited to just the builtin and secondary
>> keyrings.  Changing this silently to include the ".machine" keyring
>> introduces integrity risks that previously did not exist.  A new IMA
>> Kconfig needs to be defined to allow all three keyrings - builtin,
>> machine, and secondary.
>> 
>> The second paragraph implies that only CA and intermediate CA keys are
>> on secondary keyring, or as in our case the ".machine" keyring linked
>> to the secondary keyring.
>> 
>> Mimi
>> 
> I have also now test environment for this patch set but if there are
> any possible changes, I'm waiting for a new version, as it is anyway
> for 5.18 cycle earliest.

Other than the two sentence changes, I have not seen anything identified 
code wise requiring a change.  If you’d like me to respin a v10 with the sentence 
changes let me know.  Or if you want to remove the ima reference, that works 
too.  Just let me know how you want to handle this.  Thanks.

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

[Jarkko Sakkinen]

On Sat, Jan 15, 2022 at 07:12:35PM +0000, Eric Snowberg wrote:
> 
> 
> > On Jan 15, 2022, at 10:11 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > On Wed, Jan 12, 2022 at 02:41:47PM -0500, Mimi Zohar wrote:
> >> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
> >>> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
> >>>> 
> >>>>> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>>>> 
> >>>>> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> >>>>>>> Jarkko, my concern is that once this version of the patch set is
> >>>>>>> upstreamed, would limiting which keys may be loaded onto the .machine
> >>>>>>> keyring be considered a regression?
> >>>>>> 
> >>>>>> 
> >>>>>> Currently certificates built into the kernel do not have a CA restriction on them.  
> >>>>>> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> >>>>>> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> >>>>>> 
> >>>>>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> >>>>>> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> >>>>>> Kconfig option was not set for enforcement, it would work as it does in this series, 
> >>>>>> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> >>>>>> the restriction placed in this patch. Let me know your thoughts on whether this would 
> >>>>>> be an appropriate solution.  I believe this would get around what you are identifying as 
> >>>>>> a possible regression.
> >>>>> 
> >>>>> True the problem currently exists with the builtin keys, but there's a
> >>>>> major difference between trusting the builtin keys and those being
> >>>>> loading via MOK.  This is an integrity gap that needs to be closed and
> >>>>> shouldn't be expanded to keys on the .machine keyring.
> >>>>> 
> >>>>> "plus it would allow IMA to work with non-CA keys" is unacceptable.
> >>>> 
> >>>> Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
> >>>> cover letter, which adds IMA support, on why it is unacceptable for the end-user to
> >>>> make this decision?
> >>> 
> >>> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> >>> "help" is very clear:
> >> 
> >> [Reposting the text due to email formatting issues.]
> >> 
> >> help
> >>  Keys may be added to the IMA or IMA blacklist keyrings, if the
> >>  key is validly signed by a CA cert in the system built-in or
> >>  secondary trusted keyrings.
> >> 
> >>  Intermediate keys between those the kernel has compiled in and the 
> >>  IMA keys to be added may be added to the system secondary keyring,
> >>  provided they are validly signed by a key already resident in the
> >>  built-in or secondary trusted keyrings.
> >> 
> >> 
> >> The first paragraph requires "validly signed by a CA cert in the system
> >> built-in or secondary trusted keyrings" for keys to be loaded onto the
> >> IMA keyring.  This Kconfig is limited to just the builtin and secondary
> >> keyrings.  Changing this silently to include the ".machine" keyring
> >> introduces integrity risks that previously did not exist.  A new IMA
> >> Kconfig needs to be defined to allow all three keyrings - builtin,
> >> machine, and secondary.
> >> 
> >> The second paragraph implies that only CA and intermediate CA keys are
> >> on secondary keyring, or as in our case the ".machine" keyring linked
> >> to the secondary keyring.
> >> 
> >> Mimi
> >> 
> > I have also now test environment for this patch set but if there are
> > any possible changes, I'm waiting for a new version, as it is anyway
> > for 5.18 cycle earliest.
> 
> Other than the two sentence changes, I have not seen anything identified 
> code wise requiring a change.  If you’d like me to respin a v10 with the sentence 
> changes let me know.  Or if you want to remove the ima reference, that works 
> too.  Just let me know how you want to handle this.  Thanks.

I'm basically waiting also Mimi to test this as I do not have IMA test
environment.

From my side:

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

/Jarkko

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

[Jarkko Sakkinen]

On Sat, Jan 15, 2022 at 09:14:45PM +0200, Jarkko Sakkinen wrote:
> On Sat, Jan 15, 2022 at 07:12:35PM +0000, Eric Snowberg wrote:
> > 
> > 
> > > On Jan 15, 2022, at 10:11 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > 
> > > On Wed, Jan 12, 2022 at 02:41:47PM -0500, Mimi Zohar wrote:
> > >> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
> > >>> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
> > >>>> 
> > >>>>> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > >>>>> 
> > >>>>> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> > >>>>>>> Jarkko, my concern is that once this version of the patch set is
> > >>>>>>> upstreamed, would limiting which keys may be loaded onto the .machine
> > >>>>>>> keyring be considered a regression?
> > >>>>>> 
> > >>>>>> 
> > >>>>>> Currently certificates built into the kernel do not have a CA restriction on them.  
> > >>>>>> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> > >>>>>> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> > >>>>>> 
> > >>>>>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> > >>>>>> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> > >>>>>> Kconfig option was not set for enforcement, it would work as it does in this series, 
> > >>>>>> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> > >>>>>> the restriction placed in this patch. Let me know your thoughts on whether this would 
> > >>>>>> be an appropriate solution.  I believe this would get around what you are identifying as 
> > >>>>>> a possible regression.
> > >>>>> 
> > >>>>> True the problem currently exists with the builtin keys, but there's a
> > >>>>> major difference between trusting the builtin keys and those being
> > >>>>> loading via MOK.  This is an integrity gap that needs to be closed and
> > >>>>> shouldn't be expanded to keys on the .machine keyring.
> > >>>>> 
> > >>>>> "plus it would allow IMA to work with non-CA keys" is unacceptable.
> > >>>> 
> > >>>> Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
> > >>>> cover letter, which adds IMA support, on why it is unacceptable for the end-user to
> > >>>> make this decision?
> > >>> 
> > >>> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> > >>> "help" is very clear:
> > >> 
> > >> [Reposting the text due to email formatting issues.]
> > >> 
> > >> help
> > >>  Keys may be added to the IMA or IMA blacklist keyrings, if the
> > >>  key is validly signed by a CA cert in the system built-in or
> > >>  secondary trusted keyrings.
> > >> 
> > >>  Intermediate keys between those the kernel has compiled in and the 
> > >>  IMA keys to be added may be added to the system secondary keyring,
> > >>  provided they are validly signed by a key already resident in the
> > >>  built-in or secondary trusted keyrings.
> > >> 
> > >> 
> > >> The first paragraph requires "validly signed by a CA cert in the system
> > >> built-in or secondary trusted keyrings" for keys to be loaded onto the
> > >> IMA keyring.  This Kconfig is limited to just the builtin and secondary
> > >> keyrings.  Changing this silently to include the ".machine" keyring
> > >> introduces integrity risks that previously did not exist.  A new IMA
> > >> Kconfig needs to be defined to allow all three keyrings - builtin,
> > >> machine, and secondary.
> > >> 
> > >> The second paragraph implies that only CA and intermediate CA keys are
> > >> on secondary keyring, or as in our case the ".machine" keyring linked
> > >> to the secondary keyring.
> > >> 
> > >> Mimi
> > >> 
> > > I have also now test environment for this patch set but if there are
> > > any possible changes, I'm waiting for a new version, as it is anyway
> > > for 5.18 cycle earliest.
> > 
> > Other than the two sentence changes, I have not seen anything identified 
> > code wise requiring a change.  If you’d like me to respin a v10 with the sentence 
> > changes let me know.  Or if you want to remove the ima reference, that works 
> > too.  Just let me know how you want to handle this.  Thanks.
> 
> I'm basically waiting also Mimi to test this as I do not have IMA test
> environment.
> 
> From my side:
> 
> Tested-by: Jarkko Sakkinen <jarkko@kernel.org>

I can pick the whole thing at the time when I get green light.

/Jarkko

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

[Mimi Zohar]

On Sat, 2022-01-15 at 21:15 +0200, Jarkko Sakkinen wrote:
> On Sat, Jan 15, 2022 at 09:14:45PM +0200, Jarkko Sakkinen wrote:
> > On Sat, Jan 15, 2022 at 07:12:35PM +0000, Eric Snowberg wrote:
> > > 
> > > 
> > > > On Jan 15, 2022, at 10:11 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > > 
> > > > On Wed, Jan 12, 2022 at 02:41:47PM -0500, Mimi Zohar wrote:
> > > >> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
> > > >>> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
> > > >>>> 
> > > >>>>> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > >>>>> 
> > > >>>>> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> > > >>>>>>> Jarkko, my concern is that once this version of the patch set is
> > > >>>>>>> upstreamed, would limiting which keys may be loaded onto the .machine
> > > >>>>>>> keyring be considered a regression?
> > > >>>>>> 
> > > >>>>>> 
> > > >>>>>> Currently certificates built into the kernel do not have a CA restriction on them.  
> > > >>>>>> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> > > >>>>>> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> > > >>>>>> 
> > > >>>>>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> > > >>>>>> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> > > >>>>>> Kconfig option was not set for enforcement, it would work as it does in this series, 
> > > >>>>>> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> > > >>>>>> the restriction placed in this patch. Let me know your thoughts on whether this would 
> > > >>>>>> be an appropriate solution.  I believe this would get around what you are identifying as 
> > > >>>>>> a possible regression.
> > > >>>>> 
> > > >>>>> True the problem currently exists with the builtin keys, but there's a
> > > >>>>> major difference between trusting the builtin keys and those being
> > > >>>>> loading via MOK.  This is an integrity gap that needs to be closed and
> > > >>>>> shouldn't be expanded to keys on the .machine keyring.
> > > >>>>> 
> > > >>>>> "plus it would allow IMA to work with non-CA keys" is unacceptable.
> > > >>>> 
> > > >>>> Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
> > > >>>> cover letter, which adds IMA support, on why it is unacceptable for the end-user to
> > > >>>> make this decision?
> > > >>> 
> > > >>> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> > > >>> "help" is very clear:
> > > >> 
> > > >> [Reposting the text due to email formatting issues.]
> > > >> 
> > > >> help
> > > >>  Keys may be added to the IMA or IMA blacklist keyrings, if the
> > > >>  key is validly signed by a CA cert in the system built-in or
> > > >>  secondary trusted keyrings.
> > > >> 
> > > >>  Intermediate keys between those the kernel has compiled in and the 
> > > >>  IMA keys to be added may be added to the system secondary keyring,
> > > >>  provided they are validly signed by a key already resident in the
> > > >>  built-in or secondary trusted keyrings.
> > > >> 
> > > >> 
> > > >> The first paragraph requires "validly signed by a CA cert in the system
> > > >> built-in or secondary trusted keyrings" for keys to be loaded onto the
> > > >> IMA keyring.  This Kconfig is limited to just the builtin and secondary
> > > >> keyrings.  Changing this silently to include the ".machine" keyring
> > > >> introduces integrity risks that previously did not exist.  A new IMA
> > > >> Kconfig needs to be defined to allow all three keyrings - builtin,
> > > >> machine, and secondary.
> > > >> 
> > > >> The second paragraph implies that only CA and intermediate CA keys are
> > > >> on secondary keyring, or as in our case the ".machine" keyring linked
> > > >> to the secondary keyring.
> > > >> 
> > > >> Mimi
> > > >> 
> > > > I have also now test environment for this patch set but if there are
> > > > any possible changes, I'm waiting for a new version, as it is anyway
> > > > for 5.18 cycle earliest.
> > > 
> > > Other than the two sentence changes, I have not seen anything identified 
> > > code wise requiring a change.  If you’d like me to respin a v10 with the sentence 
> > > changes let me know.  Or if you want to remove the ima reference, that works 
> > > too.  Just let me know how you want to handle this.  Thanks.
> > 
> > I'm basically waiting also Mimi to test this as I do not have IMA test
> > environment.
> > 
> > From my side:
> > 
> > Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> I can pick the whole thing at the time when I get green light.

The MOK keys are not loaded onto the .machine keyring if
CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY is enabled. 
From an IMA perspective nothing has changed.

After the IMA references in the patch descriptions are removed, feel
free to add Tested-by: Mimi Zohar <zohar@linux.ibm.com> on patches 1 -
5.

thanks,

Mimi

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

[Jarkko Sakkinen]

On Sat, Jan 15, 2022 at 09:55:47PM -0500, Mimi Zohar wrote:
> On Sat, 2022-01-15 at 21:15 +0200, Jarkko Sakkinen wrote:
> > On Sat, Jan 15, 2022 at 09:14:45PM +0200, Jarkko Sakkinen wrote:
> > > On Sat, Jan 15, 2022 at 07:12:35PM +0000, Eric Snowberg wrote:
> > > > 
> > > > 
> > > > > On Jan 15, 2022, at 10:11 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > > > > 
> > > > > On Wed, Jan 12, 2022 at 02:41:47PM -0500, Mimi Zohar wrote:
> > > > >> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
> > > > >>> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
> > > > >>>> 
> > > > >>>>> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > > >>>>> 
> > > > >>>>> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
> > > > >>>>>>> Jarkko, my concern is that once this version of the patch set is
> > > > >>>>>>> upstreamed, would limiting which keys may be loaded onto the .machine
> > > > >>>>>>> keyring be considered a regression?
> > > > >>>>>> 
> > > > >>>>>> 
> > > > >>>>>> Currently certificates built into the kernel do not have a CA restriction on them.  
> > > > >>>>>> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
> > > > >>>>>> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
> > > > >>>>>> 
> > > > >>>>>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
> > > > >>>>>> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
> > > > >>>>>> Kconfig option was not set for enforcement, it would work as it does in this series, 
> > > > >>>>>> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
> > > > >>>>>> the restriction placed in this patch. Let me know your thoughts on whether this would 
> > > > >>>>>> be an appropriate solution.  I believe this would get around what you are identifying as 
> > > > >>>>>> a possible regression.
> > > > >>>>> 
> > > > >>>>> True the problem currently exists with the builtin keys, but there's a
> > > > >>>>> major difference between trusting the builtin keys and those being
> > > > >>>>> loading via MOK.  This is an integrity gap that needs to be closed and
> > > > >>>>> shouldn't be expanded to keys on the .machine keyring.
> > > > >>>>> 
> > > > >>>>> "plus it would allow IMA to work with non-CA keys" is unacceptable.
> > > > >>>> 
> > > > >>>> Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
> > > > >>>> cover letter, which adds IMA support, on why it is unacceptable for the end-user to
> > > > >>>> make this decision?
> > > > >>> 
> > > > >>> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> > > > >>> "help" is very clear:
> > > > >> 
> > > > >> [Reposting the text due to email formatting issues.]
> > > > >> 
> > > > >> help
> > > > >>  Keys may be added to the IMA or IMA blacklist keyrings, if the
> > > > >>  key is validly signed by a CA cert in the system built-in or
> > > > >>  secondary trusted keyrings.
> > > > >> 
> > > > >>  Intermediate keys between those the kernel has compiled in and the 
> > > > >>  IMA keys to be added may be added to the system secondary keyring,
> > > > >>  provided they are validly signed by a key already resident in the
> > > > >>  built-in or secondary trusted keyrings.
> > > > >> 
> > > > >> 
> > > > >> The first paragraph requires "validly signed by a CA cert in the system
> > > > >> built-in or secondary trusted keyrings" for keys to be loaded onto the
> > > > >> IMA keyring.  This Kconfig is limited to just the builtin and secondary
> > > > >> keyrings.  Changing this silently to include the ".machine" keyring
> > > > >> introduces integrity risks that previously did not exist.  A new IMA
> > > > >> Kconfig needs to be defined to allow all three keyrings - builtin,
> > > > >> machine, and secondary.
> > > > >> 
> > > > >> The second paragraph implies that only CA and intermediate CA keys are
> > > > >> on secondary keyring, or as in our case the ".machine" keyring linked
> > > > >> to the secondary keyring.
> > > > >> 
> > > > >> Mimi
> > > > >> 
> > > > > I have also now test environment for this patch set but if there are
> > > > > any possible changes, I'm waiting for a new version, as it is anyway
> > > > > for 5.18 cycle earliest.
> > > > 
> > > > Other than the two sentence changes, I have not seen anything identified 
> > > > code wise requiring a change.  If you’d like me to respin a v10 with the sentence 
> > > > changes let me know.  Or if you want to remove the ima reference, that works 
> > > > too.  Just let me know how you want to handle this.  Thanks.
> > > 
> > > I'm basically waiting also Mimi to test this as I do not have IMA test
> > > environment.
> > > 
> > > From my side:
> > > 
> > > Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
> > 
> > I can pick the whole thing at the time when I get green light.
> 
> The MOK keys are not loaded onto the .machine keyring if
> CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY is enabled. 
> From an IMA perspective nothing has changed.
> 
> After the IMA references in the patch descriptions are removed, feel
> free to add Tested-by: Mimi Zohar <zohar@linux.ibm.com> on patches 1 -
> 5.
> 
> thanks,
> 
> Mimi

Eric, for me it would be at least a convenience, and overally it would
make sure that I pick the right thing if you would fix the typos (and
you can add all the tested-by tags of course as no functional changes).

There's been times when I've manually "just fixed typos", and failed in a
way or another because of human error. Just want to make sure that we
have exactly the right content applied, I hope you understand my point
of view. And we are early for the 5.18 release cycle anyway.

BR, Jarkko

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

[Eric Snowberg]

> On Jan 16, 2022, at 1:10 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Sat, Jan 15, 2022 at 09:55:47PM -0500, Mimi Zohar wrote:
>> On Sat, 2022-01-15 at 21:15 +0200, Jarkko Sakkinen wrote:
>>> On Sat, Jan 15, 2022 at 09:14:45PM +0200, Jarkko Sakkinen wrote:
>>>> On Sat, Jan 15, 2022 at 07:12:35PM +0000, Eric Snowberg wrote:
>>>>> 
>>>>> 
>>>>>> On Jan 15, 2022, at 10:11 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>>>>>> 
>>>>>> On Wed, Jan 12, 2022 at 02:41:47PM -0500, Mimi Zohar wrote:
>>>>>>> On Tue, 2022-01-11 at 20:14 -0500, Mimi Zohar wrote:
>>>>>>>> On Tue, 2022-01-11 at 21:26 +0000, Eric Snowberg wrote:
>>>>>>>>> 
>>>>>>>>>> On Jan 11, 2022, at 11:16 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>>>>> 
>>>>>>>>>> On Mon, 2022-01-10 at 23:25 +0000, Eric Snowberg wrote:
>>>>>>>>>>>> Jarkko, my concern is that once this version of the patch set is
>>>>>>>>>>>> upstreamed, would limiting which keys may be loaded onto the .machine
>>>>>>>>>>>> keyring be considered a regression?
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> Currently certificates built into the kernel do not have a CA restriction on them.  
>>>>>>>>>>> IMA will trust anything in this keyring even if the CA bit is not set.  While it would 
>>>>>>>>>>> be advisable for a kernel to be built with a CA, nothing currently enforces it. 
>>>>>>>>>>> 
>>>>>>>>>>> My thinking for the dropped CA restriction patches was to introduce a new Kconfig.  
>>>>>>>>>>> This Kconfig would do the CA enforcement on the machine keyring.  However if the 
>>>>>>>>>>> Kconfig option was not set for enforcement, it would work as it does in this series, 
>>>>>>>>>>> plus it would allow IMA to work with non-CA keys.  This would be done by removing 
>>>>>>>>>>> the restriction placed in this patch. Let me know your thoughts on whether this would 
>>>>>>>>>>> be an appropriate solution.  I believe this would get around what you are identifying as 
>>>>>>>>>>> a possible regression.
>>>>>>>>>> 
>>>>>>>>>> True the problem currently exists with the builtin keys, but there's a
>>>>>>>>>> major difference between trusting the builtin keys and those being
>>>>>>>>>> loading via MOK.  This is an integrity gap that needs to be closed and
>>>>>>>>>> shouldn't be expanded to keys on the .machine keyring.
>>>>>>>>>> 
>>>>>>>>>> "plus it would allow IMA to work with non-CA keys" is unacceptable.
>>>>>>>>> 
>>>>>>>>> Ok, I’ll leave that part out.  Could you clarify the wording I should include in the future 
>>>>>>>>> cover letter, which adds IMA support, on why it is unacceptable for the end-user to
>>>>>>>>> make this decision?
>>>>>>>> 
>>>>>>>> The Kconfig IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
>>>>>>>> "help" is very clear:
>>>>>>> 
>>>>>>> [Reposting the text due to email formatting issues.]
>>>>>>> 
>>>>>>> help
>>>>>>> Keys may be added to the IMA or IMA blacklist keyrings, if the
>>>>>>> key is validly signed by a CA cert in the system built-in or
>>>>>>> secondary trusted keyrings.
>>>>>>> 
>>>>>>> Intermediate keys between those the kernel has compiled in and the 
>>>>>>> IMA keys to be added may be added to the system secondary keyring,
>>>>>>> provided they are validly signed by a key already resident in the
>>>>>>> built-in or secondary trusted keyrings.
>>>>>>> 
>>>>>>> 
>>>>>>> The first paragraph requires "validly signed by a CA cert in the system
>>>>>>> built-in or secondary trusted keyrings" for keys to be loaded onto the
>>>>>>> IMA keyring.  This Kconfig is limited to just the builtin and secondary
>>>>>>> keyrings.  Changing this silently to include the ".machine" keyring
>>>>>>> introduces integrity risks that previously did not exist.  A new IMA
>>>>>>> Kconfig needs to be defined to allow all three keyrings - builtin,
>>>>>>> machine, and secondary.
>>>>>>> 
>>>>>>> The second paragraph implies that only CA and intermediate CA keys are
>>>>>>> on secondary keyring, or as in our case the ".machine" keyring linked
>>>>>>> to the secondary keyring.
>>>>>>> 
>>>>>>> Mimi
>>>>>>> 
>>>>>> I have also now test environment for this patch set but if there are
>>>>>> any possible changes, I'm waiting for a new version, as it is anyway
>>>>>> for 5.18 cycle earliest.
>>>>> 
>>>>> Other than the two sentence changes, I have not seen anything identified 
>>>>> code wise requiring a change.  If you’d like me to respin a v10 with the sentence 
>>>>> changes let me know.  Or if you want to remove the ima reference, that works 
>>>>> too.  Just let me know how you want to handle this.  Thanks.
>>>> 
>>>> I'm basically waiting also Mimi to test this as I do not have IMA test
>>>> environment.
>>>> 
>>>> From my side:
>>>> 
>>>> Tested-by: Jarkko Sakkinen <jarkko@kernel.org>
>>> 
>>> I can pick the whole thing at the time when I get green light.
>> 
>> The MOK keys are not loaded onto the .machine keyring if
>> CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY is enabled. 
>> From an IMA perspective nothing has changed.
>> 
>> After the IMA references in the patch descriptions are removed, feel
>> free to add Tested-by: Mimi Zohar <zohar@linux.ibm.com> on patches 1 -
>> 5.
>> 
>> thanks,
>> 
>> Mimi
> 
> Eric, for me it would be at least a convenience, and overally it would
> make sure that I pick the right thing if you would fix the typos (and
> you can add all the tested-by tags of course as no functional changes).
> 
> There's been times when I've manually "just fixed typos", and failed in a
> way or another because of human error. Just want to make sure that we
> have exactly the right content applied, I hope you understand my point
> of view. And we are early for the 5.18 release cycle anyway.

No problem, I’ll put together a v10 with the changes.  Thanks for your review.