[PATCH 0/4] keys: Add EFI_CERT_X509_GUID support for dbx/mokx entries

[PATCH 0/4] keys: Add EFI_CERT_X509_GUID support for dbx/mokx entries

[David Howells]

Here's my take on v5 of Eric Snowberg's patches[1]:

This series of patches adds support for EFI_CERT_X509_GUID entries [2].  It has
been expanded to not only include dbx entries but also entries in the mokx.
Additionally Eric included his patches to preload these certificate [3].

The patches can be found on the following branch:

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-cve-2020-26541-branch

Changes:
 - I've modified the first patch in the series to fix a configuration
   problem[4][5], to move the added functions to a more logical place within the
   file and to add kerneldoc comments.

Link: https://lore.kernel.org/r/20210122181054.32635-1-eric.snowberg@oracle.com [1]
Link: https://patchwork.kernel.org/project/linux-security-module/patch/20200916004927.64276-1-eric.snowberg@oracle.com/ [2]
Link: https://lore.kernel.org/patchwork/cover/1315485/ [3]
Link: https://lore.kernel.org/r/bc2c24e3-ed68-2521-0bf4-a1f6be4a895d@infradead.org/ [4]
Link: https://lore.kernel.org/r/20210225125638.1841436-1-arnd@kernel.org/ [5]

David
---
Eric Snowberg (4):
      certs: Add EFI_CERT_X509_GUID support for dbx entries
      certs: Move load_system_certificate_list to a common function
      certs: Add ability to preload revocation certs
      integrity: Load mokx variables into the blacklist keyring


 certs/Kconfig                                 |  8 +++
 certs/Makefile                                | 20 ++++++-
 certs/blacklist.c                             | 17 ++++++
 certs/common.c                                | 56 +++++++++++++++++++
 certs/common.h                                |  9 +++
 certs/revocation_certificates.S               | 21 +++++++
 certs/system_keyring.c                        | 49 +---------------
 scripts/Makefile                              |  1 +
 security/integrity/platform_certs/load_uefi.c | 20 ++++++-
 9 files changed, 150 insertions(+), 51 deletions(-)
 create mode 100644 certs/common.c
 create mode 100644 certs/common.h
 create mode 100644 certs/revocation_certificates.S

[PATCH 1/4] certs: Add EFI_CERT_X509_GUID support for dbx entries

[David Howells]

From: Eric Snowberg <eric.snowberg@oracle.com>

This fixes CVE-2020-26541.

The Secure Boot Forbidden Signature Database, dbx, contains a list of now
revoked signatures and keys previously approved to boot with UEFI Secure
Boot enabled.  The dbx is capable of containing any number of
EFI_CERT_X509_SHA256_GUID, EFI_CERT_SHA256_GUID, and EFI_CERT_X509_GUID
entries.

Currently when EFI_CERT_X509_GUID are contained in the dbx, the entries are
skipped.

Add support for EFI_CERT_X509_GUID dbx entries. When a EFI_CERT_X509_GUID
is found, it is added as an asymmetrical key to the .blacklist keyring.
Anytime the .platform keyring is used, the keys in the .blacklist keyring
are referenced, if a matching key is found, the key will be rejected.

[DH: Made the following changes:
 - Added to have a config option to enable the facility.  This allows a
   Kconfig solution to make sure that pkcs7_validate_trust() is enabled.
 - Moved the functions out from the middle of the blacklist functions.
 - Added kerneldoc comments.]

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jarkko Sakkinen <jarkko@kernel.org>
cc: Randy Dunlap <rdunlap@infradead.org>
cc: Mickaël Salaün <mic@digikod.net>
cc: Arnd Bergmann <arnd@kernel.org>
cc: keyrings@vger.kernel.org
Link: https://lore.kernel.org/r/20200901165143.10295-1-eric.snowberg@oracle.com/
Link: https://lore.kernel.org/r/20200909172736.73003-1-eric.snowberg@oracle.com/ # v2
Link: https://lore.kernel.org/r/20200911182230.62266-1-eric.snowberg@oracle.com/ # v3
Link: https://lore.kernel.org/r/20200916004927.64276-1-eric.snowberg@oracle.com/ # v4
Link: https://lore.kernel.org/r/2660556.1610545213@warthog.procyon.org.uk/
Link: https://lore.kernel.org/r/20210122181054.32635-2-eric.snowberg@oracle.com/ # v5
Link: https://lore.kernel.org/r/bc2c24e3-ed68-2521-0bf4-a1f6be4a895d@infradead.org/
Link: https://lore.kernel.org/r/20210225125638.1841436-1-arnd@kernel.org/
---

 certs/Kconfig                                      |    9 ++++
 certs/blacklist.c                                  |   43 ++++++++++++++++++++
 certs/blacklist.h                                  |    2 +
 certs/system_keyring.c                             |    6 +++
 include/keys/system_keyring.h                      |   15 +++++++
 .../integrity/platform_certs/keyring_handler.c     |   11 +++++
 6 files changed, 86 insertions(+)

diff --git a/certs/Kconfig b/certs/Kconfig
index c94e93d8bccf..76e469b56a77 100644
--- a/certs/Kconfig
+++ b/certs/Kconfig
@@ -83,4 +83,13 @@ config SYSTEM_BLACKLIST_HASH_LIST
 	  wrapper to incorporate the list into the kernel.  Each <hash> should
 	  be a string of hex digits.
 
+config SYSTEM_REVOCATION_LIST
+	bool "Provide system-wide ring of revocation certificates"
+	depends on SYSTEM_BLACKLIST_KEYRING
+	depends on PKCS7_MESSAGE_PARSER=y
+	help
+	  If set, this allows revocation certificates to be stored in the
+	  blacklist keyring and implements a hook whereby a PKCS#7 message can
+	  be checked to see if it matches such a certificate.
+
 endmenu
diff --git a/certs/blacklist.c b/certs/blacklist.c
index bffe4c6f4a9e..2b8644123d5f 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -145,6 +145,49 @@ int is_binary_blacklisted(const u8 *hash, size_t hash_len)
 }
 EXPORT_SYMBOL_GPL(is_binary_blacklisted);
 
+#ifdef CONFIG_SYSTEM_REVOCATION_LIST
+/**
+ * add_key_to_revocation_list - Add a revocation certificate to the blacklist
+ * @data: The data blob containing the certificate
+ * @size: The size of data blob
+ */
+int add_key_to_revocation_list(const char *data, size_t size)
+{
+	key_ref_t key;
+
+	key = key_create_or_update(make_key_ref(blacklist_keyring, true),
+				   "asymmetric",
+				   NULL,
+				   data,
+				   size,
+				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
+				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
+
+	if (IS_ERR(key)) {
+		pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
+		return PTR_ERR(key);
+	}
+
+	return 0;
+}
+
+/**
+ * is_key_on_revocation_list - Determine if the key for a PKCS#7 message is revoked
+ * @pkcs7: The PKCS#7 message to check
+ */
+int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
+{
+	int ret;
+
+	ret = pkcs7_validate_trust(pkcs7, blacklist_keyring);
+
+	if (ret == 0)
+		return -EKEYREJECTED;
+
+	return -ENOKEY;
+}
+#endif
+
 /*
  * Initialise the blacklist
  */
diff --git a/certs/blacklist.h b/certs/blacklist.h
index 1efd6fa0dc60..51b320cf8574 100644
--- a/certs/blacklist.h
+++ b/certs/blacklist.h
@@ -1,3 +1,5 @@
 #include <linux/kernel.h>
+#include <linux/errno.h>
+#include <crypto/pkcs7.h>
 
 extern const char __initconst *const blacklist_hashes[];
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 4b693da488f1..ed98754d5795 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -242,6 +242,12 @@ int verify_pkcs7_message_sig(const void *data, size_t len,
 			pr_devel("PKCS#7 platform keyring is not available\n");
 			goto error;
 		}
+
+		ret = is_key_on_revocation_list(pkcs7);
+		if (ret != -ENOKEY) {
+			pr_devel("PKCS#7 platform key is on revocation list\n");
+			goto error;
+		}
 	}
 	ret = pkcs7_validate_trust(pkcs7, trusted_keys);
 	if (ret < 0) {
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index fb8b07daa9d1..875e002a4180 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -31,6 +31,7 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
 #define restrict_link_by_builtin_and_secondary_trusted restrict_link_by_builtin_trusted
 #endif
 
+extern struct pkcs7_message *pkcs7;
 #ifdef CONFIG_SYSTEM_BLACKLIST_KEYRING
 extern int mark_hash_blacklisted(const char *hash);
 extern int is_hash_blacklisted(const u8 *hash, size_t hash_len,
@@ -49,6 +50,20 @@ static inline int is_binary_blacklisted(const u8 *hash, size_t hash_len)
 }
 #endif
 
+#ifdef CONFIG_SYSTEM_REVOCATION_LIST
+extern int add_key_to_revocation_list(const char *data, size_t size);
+extern int is_key_on_revocation_list(struct pkcs7_message *pkcs7);
+#else
+static inline int add_key_to_revocation_list(const char *data, size_t size)
+{
+	return 0;
+}
+static inline int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
+{
+	return -ENOKEY;
+}
+#endif
+
 #ifdef CONFIG_IMA_BLACKLIST_KEYRING
 extern struct key *ima_blacklist_keyring;
 
diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
index c5ba695c10e3..5604bd57c990 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -55,6 +55,15 @@ static __init void uefi_blacklist_binary(const char *source,
 	uefi_blacklist_hash(source, data, len, "bin:", 4);
 }
 
+/*
+ * Add an X509 cert to the revocation list.
+ */
+static __init void uefi_revocation_list_x509(const char *source,
+					     const void *data, size_t len)
+{
+	add_key_to_revocation_list(data, len);
+}
+
 /*
  * Return the appropriate handler for particular signature list types found in
  * the UEFI db and MokListRT tables.
@@ -76,5 +85,7 @@ __init efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_type)
 		return uefi_blacklist_x509_tbs;
 	if (efi_guidcmp(*sig_type, efi_cert_sha256_guid) == 0)
 		return uefi_blacklist_binary;
+	if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
+		return uefi_revocation_list_x509;
 	return 0;
 }

[PATCH 2/4] certs: Move load_system_certificate_list to a common function

[David Howells]

From: Eric Snowberg <eric.snowberg@oracle.com>

Move functionality within load_system_certificate_list to a common
function, so it can be reused in the future.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Link: https://lore.kernel.org/r/20200930201508.35113-2-eric.snowberg@oracle.com/
Link: https://lore.kernel.org/r/20210122181054.32635-3-eric.snowberg@oracle.com/ # v5
---

 certs/Makefile         |    2 +-
 certs/common.c         |   56 ++++++++++++++++++++++++++++++++++++++++++++++++
 certs/common.h         |    9 ++++++++
 certs/system_keyring.c |   49 +++---------------------------------------
 4 files changed, 69 insertions(+), 47 deletions(-)
 create mode 100644 certs/common.c
 create mode 100644 certs/common.h

diff --git a/certs/Makefile b/certs/Makefile
index f4c25b67aad9..f4b90bad8690 100644
--- a/certs/Makefile
+++ b/certs/Makefile
@@ -3,7 +3,7 @@
 # Makefile for the linux kernel signature checking certificates.
 #
 
-obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o
+obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o common.o
 obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist.o
 ifneq ($(CONFIG_SYSTEM_BLACKLIST_HASH_LIST),"")
 obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist_hashes.o
diff --git a/certs/common.c b/certs/common.c
new file mode 100644
index 000000000000..83800f51a1a1
--- /dev/null
+++ b/certs/common.c
@@ -0,0 +1,56 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <linux/kernel.h>
+#include <linux/key.h>
+
+int load_certificate_list(const u8 cert_list[],
+			  const unsigned long list_size,
+			  const struct key *keyring)
+{
+	key_ref_t key;
+	const u8 *p, *end;
+	size_t plen;
+
+	p = cert_list;
+	end = p + list_size;
+	while (p < end) {
+		/* Each cert begins with an ASN.1 SEQUENCE tag and must be more
+		 * than 256 bytes in size.
+		 */
+		if (end - p < 4)
+			goto dodgy_cert;
+		if (p[0] != 0x30 &&
+		    p[1] != 0x82)
+			goto dodgy_cert;
+		plen = (p[2] << 8) | p[3];
+		plen += 4;
+		if (plen > end - p)
+			goto dodgy_cert;
+
+		key = key_create_or_update(make_key_ref(keyring, 1),
+					   "asymmetric",
+					   NULL,
+					   p,
+					   plen,
+					   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
+					   KEY_USR_VIEW | KEY_USR_READ),
+					   KEY_ALLOC_NOT_IN_QUOTA |
+					   KEY_ALLOC_BUILT_IN |
+					   KEY_ALLOC_BYPASS_RESTRICTION);
+		if (IS_ERR(key)) {
+			pr_err("Problem loading in-kernel X.509 certificate (%ld)\n",
+			       PTR_ERR(key));
+		} else {
+			pr_notice("Loaded X.509 cert '%s'\n",
+				  key_ref_to_ptr(key)->description);
+			key_ref_put(key);
+		}
+		p += plen;
+	}
+
+	return 0;
+
+dodgy_cert:
+	pr_err("Problem parsing in-kernel X.509 certificate list\n");
+	return 0;
+}
diff --git a/certs/common.h b/certs/common.h
new file mode 100644
index 000000000000..abdb5795936b
--- /dev/null
+++ b/certs/common.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef _CERT_COMMON_H
+#define _CERT_COMMON_H
+
+int load_certificate_list(const u8 cert_list[], const unsigned long list_size,
+			  const struct key *keyring);
+
+#endif
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index ed98754d5795..0c9a4795e847 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -16,6 +16,7 @@
 #include <keys/asymmetric-type.h>
 #include <keys/system_keyring.h>
 #include <crypto/pkcs7.h>
+#include "common.h"
 
 static struct key *builtin_trusted_keys;
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
@@ -137,54 +138,10 @@ device_initcall(system_trusted_keyring_init);
  */
 static __init int load_system_certificate_list(void)
 {
-	key_ref_t key;
-	const u8 *p, *end;
-	size_t plen;
-
 	pr_notice("Loading compiled-in X.509 certificates\n");
 
-	p = system_certificate_list;
-	end = p + system_certificate_list_size;
-	while (p < end) {
-		/* Each cert begins with an ASN.1 SEQUENCE tag and must be more
-		 * than 256 bytes in size.
-		 */
-		if (end - p < 4)
-			goto dodgy_cert;
-		if (p[0] != 0x30 &&
-		    p[1] != 0x82)
-			goto dodgy_cert;
-		plen = (p[2] << 8) | p[3];
-		plen += 4;
-		if (plen > end - p)
-			goto dodgy_cert;
-
-		key = key_create_or_update(make_key_ref(builtin_trusted_keys, 1),
-					   "asymmetric",
-					   NULL,
-					   p,
-					   plen,
-					   ((KEY_POS_ALL & ~KEY_POS_SETATTR) |
-					   KEY_USR_VIEW | KEY_USR_READ),
-					   KEY_ALLOC_NOT_IN_QUOTA |
-					   KEY_ALLOC_BUILT_IN |
-					   KEY_ALLOC_BYPASS_RESTRICTION);
-		if (IS_ERR(key)) {
-			pr_err("Problem loading in-kernel X.509 certificate (%ld)\n",
-			       PTR_ERR(key));
-		} else {
-			pr_notice("Loaded X.509 cert '%s'\n",
-				  key_ref_to_ptr(key)->description);
-			key_ref_put(key);
-		}
-		p += plen;
-	}
-
-	return 0;
-
-dodgy_cert:
-	pr_err("Problem parsing in-kernel X.509 certificate list\n");
-	return 0;
+	return load_certificate_list(system_certificate_list, system_certificate_list_size,
+				     builtin_trusted_keys);
 }
 late_initcall(load_system_certificate_list);
 

[PATCH 3/4] certs: Add ability to preload revocation certs

[David Howells]

From: Eric Snowberg <eric.snowberg@oracle.com>

Add a new Kconfig option called SYSTEM_REVOCATION_KEYS. If set,
this option should be the filename of a PEM-formated file containing
X.509 certificates to be included in the default blacklist keyring.

[DH: Changed this to make the new Kconfig option depend on the option to
enable the facility.]

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Link: https://lore.kernel.org/r/20200930201508.35113-3-eric.snowberg@oracle.com/
Link: https://lore.kernel.org/r/20210122181054.32635-4-eric.snowberg@oracle.com/ # v5
---

 certs/Kconfig                   |    8 ++++++++
 certs/Makefile                  |   18 ++++++++++++++++--
 certs/blacklist.c               |   17 +++++++++++++++++
 certs/revocation_certificates.S |   21 +++++++++++++++++++++
 scripts/Makefile                |    1 +
 5 files changed, 63 insertions(+), 2 deletions(-)
 create mode 100644 certs/revocation_certificates.S

diff --git a/certs/Kconfig b/certs/Kconfig
index 76e469b56a77..ab88d2a7f3c7 100644
--- a/certs/Kconfig
+++ b/certs/Kconfig
@@ -92,4 +92,12 @@ config SYSTEM_REVOCATION_LIST
 	  blacklist keyring and implements a hook whereby a PKCS#7 message can
 	  be checked to see if it matches such a certificate.
 
+config SYSTEM_REVOCATION_KEYS
+	string "X.509 certificates to be preloaded into the system blacklist keyring"
+	depends on SYSTEM_REVOCATION_LIST
+	help
+	  If set, this option should be the filename of a PEM-formatted file
+	  containing X.509 certificates to be included in the default blacklist
+	  keyring.
+
 endmenu
diff --git a/certs/Makefile b/certs/Makefile
index f4b90bad8690..e3f4926fd21e 100644
--- a/certs/Makefile
+++ b/certs/Makefile
@@ -4,7 +4,7 @@
 #
 
 obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o common.o
-obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist.o
+obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist.o revocation_certificates.o common.o
 ifneq ($(CONFIG_SYSTEM_BLACKLIST_HASH_LIST),"")
 obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist_hashes.o
 else
@@ -29,7 +29,7 @@ $(obj)/x509_certificate_list: scripts/extract-cert $(SYSTEM_TRUSTED_KEYS_SRCPREF
 	$(call if_changed,extract_certs,$(SYSTEM_TRUSTED_KEYS_SRCPREFIX)$(CONFIG_SYSTEM_TRUSTED_KEYS))
 endif # CONFIG_SYSTEM_TRUSTED_KEYRING
 
-clean-files := x509_certificate_list .x509.list
+clean-files := x509_certificate_list .x509.list x509_revocation_list
 
 ifeq ($(CONFIG_MODULE_SIG),y)
 ###############################################################################
@@ -104,3 +104,17 @@ targets += signing_key.x509
 $(obj)/signing_key.x509: scripts/extract-cert $(X509_DEP) FORCE
 	$(call if_changed,extract_certs,$(MODULE_SIG_KEY_SRCPREFIX)$(CONFIG_MODULE_SIG_KEY))
 endif # CONFIG_MODULE_SIG
+
+ifeq ($(CONFIG_SYSTEM_BLACKLIST_KEYRING),y)
+
+$(eval $(call config_filename,SYSTEM_REVOCATION_KEYS))
+
+$(obj)/revocation_certificates.o: $(obj)/x509_revocation_list
+
+quiet_cmd_extract_certs  = EXTRACT_CERTS   $(patsubst "%",%,$(2))
+      cmd_extract_certs  = scripts/extract-cert $(2) $@
+
+targets += x509_revocation_list
+$(obj)/x509_revocation_list: scripts/extract-cert $(SYSTEM_REVOCATION_KEYS_SRCPREFIX)$(SYSTEM_REVOCATION_KEYS_FILENAME) FORCE
+	$(call if_changed,extract_certs,$(SYSTEM_REVOCATION_KEYS_SRCPREFIX)$(CONFIG_SYSTEM_REVOCATION_KEYS))
+endif
diff --git a/certs/blacklist.c b/certs/blacklist.c
index 2b8644123d5f..723b19c96256 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -17,9 +17,13 @@
 #include <linux/uidgid.h>
 #include <keys/system_keyring.h>
 #include "blacklist.h"
+#include "common.h"
 
 static struct key *blacklist_keyring;
 
+extern __initconst const u8 revocation_certificate_list[];
+extern __initconst const unsigned long revocation_certificate_list_size;
+
 /*
  * The description must be a type prefix, a colon and then an even number of
  * hex digits.  The hash is kept in the description.
@@ -220,3 +224,16 @@ static int __init blacklist_init(void)
  * Must be initialised before we try and load the keys into the keyring.
  */
 device_initcall(blacklist_init);
+
+/*
+ * Load the compiled-in list of revocation X.509 certificates.
+ */
+static __init int load_revocation_certificate_list(void)
+{
+	if (revocation_certificate_list_size)
+		pr_notice("Loading compiled-in revocation X.509 certificates\n");
+
+	return load_certificate_list(revocation_certificate_list, revocation_certificate_list_size,
+				     blacklist_keyring);
+}
+late_initcall(load_revocation_certificate_list);
diff --git a/certs/revocation_certificates.S b/certs/revocation_certificates.S
new file mode 100644
index 000000000000..f21aae8a8f0e
--- /dev/null
+++ b/certs/revocation_certificates.S
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#include <linux/export.h>
+#include <linux/init.h>
+
+	__INITRODATA
+
+	.align 8
+	.globl revocation_certificate_list
+revocation_certificate_list:
+__revocation_list_start:
+	.incbin "certs/x509_revocation_list"
+__revocation_list_end:
+
+	.align 8
+	.globl revocation_certificate_list_size
+revocation_certificate_list_size:
+#ifdef CONFIG_64BIT
+	.quad __revocation_list_end - __revocation_list_start
+#else
+	.long __revocation_list_end - __revocation_list_start
+#endif
diff --git a/scripts/Makefile b/scripts/Makefile
index b5418ec587fb..983b785f13cb 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -11,6 +11,7 @@ hostprogs-always-$(CONFIG_ASN1)				+= asn1_compiler
 hostprogs-always-$(CONFIG_MODULE_SIG_FORMAT)		+= sign-file
 hostprogs-always-$(CONFIG_SYSTEM_TRUSTED_KEYRING)	+= extract-cert
 hostprogs-always-$(CONFIG_SYSTEM_EXTRA_CERTIFICATE)	+= insert-sys-cert
+ hostprogs-always-$(CONFIG_SYSTEM_BLACKLIST_KEYRING)	+= extract-cert
 
 HOSTCFLAGS_sorttable.o = -I$(srctree)/tools/include
 HOSTCFLAGS_asn1_compiler.o = -I$(srctree)/include

[PATCH 4/4] integrity: Load mokx variables into the blacklist keyring

[David Howells]

From: Eric Snowberg <eric.snowberg@oracle.com>

During boot the Secure Boot Forbidden Signature Database, dbx,
is loaded into the blacklist keyring.  Systems booted with shim
have an equivalent Forbidden Signature Database called mokx.
Currently mokx is only used by shim and grub, the contents are
ignored by the kernel.

Add the ability to load mokx into the blacklist keyring during boot.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jarkko Sakkinen <jarkko@kernel.org>
Link: https://lore.kernel.org/r/20210122181054.32635-5-eric.snowberg@oracle.com/ # v5
Link: https://lore.kernel.org/r/c33c8e3839a41e9654f41cc92c7231104931b1d7.camel@HansenPartnership.com/
---

 security/integrity/platform_certs/load_uefi.c |   20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index ee4b4c666854..f290f78c3f30 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -132,8 +132,9 @@ static int __init load_moklist_certs(void)
 static int __init load_uefi_certs(void)
 {
 	efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID;
-	void *db = NULL, *dbx = NULL;
-	unsigned long dbsize = 0, dbxsize = 0;
+	efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
+	void *db = NULL, *dbx = NULL, *mokx = NULL;
+	unsigned long dbsize = 0, dbxsize = 0, mokxsize = 0;
 	efi_status_t status;
 	int rc = 0;
 
@@ -175,6 +176,21 @@ static int __init load_uefi_certs(void)
 		kfree(dbx);
 	}
 
+	mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize, &status);
+	if (!mokx) {
+		if (status == EFI_NOT_FOUND)
+			pr_debug("mokx variable wasn't found\n");
+		else
+			pr_info("Couldn't get mokx list\n");
+	} else {
+		rc = parse_efi_signature_list("UEFI:MokListXRT",
+					      mokx, mokxsize,
+					      get_handler_for_dbx);
+		if (rc)
+			pr_err("Couldn't parse mokx signatures %d\n", rc);
+		kfree(mokx);
+	}
+
 	/* Load the MokListRT certs */
 	rc = load_moklist_certs();
 

Re: [PATCH 0/4] keys: Add EFI_CERT_X509_GUID support for dbx/mokx entries

[Eric Snowberg]

> On Feb 25, 2021, at 1:58 PM, David Howells <dhowells@redhat.com> wrote:
> 
> 
> Here's my take on v5 of Eric Snowberg's patches[1]:
> 
> This series of patches adds support for EFI_CERT_X509_GUID entries [2].  It has
> been expanded to not only include dbx entries but also entries in the mokx.
> Additionally Eric included his patches to preload these certificate [3].
> 
> The patches can be found on the following branch:
> 
> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-cve-2020-26541-branch
> 
> Changes:
> - I've modified the first patch in the series to fix a configuration
>   problem[4][5], to move the added functions to a more logical place within the
>   file and to add kerneldoc comments.

Thanks for making these changes.  I reviewed and tested the series again.  The only 
thing I found is we may want to fold in this patch [1] too.  Other than that, 
everything LGTM.

[1] https://patchwork.kernel.org/project/keyrings/patch/20210204033208.1389901-1-eric.snowberg@oracle.com/


> 
> Link: https://lore.kernel.org/r/20210122181054.32635-1-eric.snowberg@oracle.com [1]
> Link: https://patchwork.kernel.org/project/linux-security-module/patch/20200916004927.64276-1-eric.snowberg@oracle.com/ [2]
> Link: https://lore.kernel.org/patchwork/cover/1315485/ [3]
> Link: https://lore.kernel.org/r/bc2c24e3-ed68-2521-0bf4-a1f6be4a895d@infradead.org/ [4]
> Link: https://lore.kernel.org/r/20210225125638.1841436-1-arnd@kernel.org/ [5]
> 
> David
> ---
> Eric Snowberg (4):
>      certs: Add EFI_CERT_X509_GUID support for dbx entries
>      certs: Move load_system_certificate_list to a common function
>      certs: Add ability to preload revocation certs
>      integrity: Load mokx variables into the blacklist keyring
> 
> 
> certs/Kconfig                                 |  8 +++
> certs/Makefile                                | 20 ++++++-
> certs/blacklist.c                             | 17 ++++++
> certs/common.c                                | 56 +++++++++++++++++++
> certs/common.h                                |  9 +++
> certs/revocation_certificates.S               | 21 +++++++
> certs/system_keyring.c                        | 49 +---------------
> scripts/Makefile                              |  1 +
> security/integrity/platform_certs/load_uefi.c | 20 ++++++-
> 9 files changed, 150 insertions(+), 51 deletions(-)
> create mode 100644 certs/common.c
> create mode 100644 certs/common.h
> create mode 100644 certs/revocation_certificates.S
> 
> 

Re: [PATCH 3/4] certs: Add ability to preload revocation certs

[Nathan Chancellor]

On Thu, Feb 25, 2021 at 08:58:55PM +0000, David Howells wrote:
> From: Eric Snowberg <eric.snowberg@oracle.com>
> 
> Add a new Kconfig option called SYSTEM_REVOCATION_KEYS. If set,
> this option should be the filename of a PEM-formated file containing
> X.509 certificates to be included in the default blacklist keyring.
> 
> [DH: Changed this to make the new Kconfig option depend on the option to
> enable the facility.]
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Link: https://lore.kernel.org/r/20200930201508.35113-3-eric.snowberg@oracle.com/
> Link: https://lore.kernel.org/r/20210122181054.32635-4-eric.snowberg@oracle.com/ # v5
> ---
> 
>  certs/Kconfig                   |    8 ++++++++
>  certs/Makefile                  |   18 ++++++++++++++++--
>  certs/blacklist.c               |   17 +++++++++++++++++
>  certs/revocation_certificates.S |   21 +++++++++++++++++++++
>  scripts/Makefile                |    1 +
>  5 files changed, 63 insertions(+), 2 deletions(-)
>  create mode 100644 certs/revocation_certificates.S
> 
> diff --git a/certs/Kconfig b/certs/Kconfig
> index 76e469b56a77..ab88d2a7f3c7 100644
> --- a/certs/Kconfig
> +++ b/certs/Kconfig
> @@ -92,4 +92,12 @@ config SYSTEM_REVOCATION_LIST
>  	  blacklist keyring and implements a hook whereby a PKCS#7 message can
>  	  be checked to see if it matches such a certificate.
>  
> +config SYSTEM_REVOCATION_KEYS
> +	string "X.509 certificates to be preloaded into the system blacklist keyring"
> +	depends on SYSTEM_REVOCATION_LIST
> +	help
> +	  If set, this option should be the filename of a PEM-formatted file
> +	  containing X.509 certificates to be included in the default blacklist
> +	  keyring.
> +
>  endmenu
> diff --git a/certs/Makefile b/certs/Makefile
> index f4b90bad8690..e3f4926fd21e 100644
> --- a/certs/Makefile
> +++ b/certs/Makefile
> @@ -4,7 +4,7 @@
>  #
>  
>  obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o common.o
> -obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist.o
> +obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist.o revocation_certificates.o common.o
>  ifneq ($(CONFIG_SYSTEM_BLACKLIST_HASH_LIST),"")
>  obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist_hashes.o
>  else
> @@ -29,7 +29,7 @@ $(obj)/x509_certificate_list: scripts/extract-cert $(SYSTEM_TRUSTED_KEYS_SRCPREF
>  	$(call if_changed,extract_certs,$(SYSTEM_TRUSTED_KEYS_SRCPREFIX)$(CONFIG_SYSTEM_TRUSTED_KEYS))
>  endif # CONFIG_SYSTEM_TRUSTED_KEYRING
>  
> -clean-files := x509_certificate_list .x509.list
> +clean-files := x509_certificate_list .x509.list x509_revocation_list
>  
>  ifeq ($(CONFIG_MODULE_SIG),y)
>  ###############################################################################
> @@ -104,3 +104,17 @@ targets += signing_key.x509
>  $(obj)/signing_key.x509: scripts/extract-cert $(X509_DEP) FORCE
>  	$(call if_changed,extract_certs,$(MODULE_SIG_KEY_SRCPREFIX)$(CONFIG_MODULE_SIG_KEY))
>  endif # CONFIG_MODULE_SIG
> +
> +ifeq ($(CONFIG_SYSTEM_BLACKLIST_KEYRING),y)
> +
> +$(eval $(call config_filename,SYSTEM_REVOCATION_KEYS))
> +
> +$(obj)/revocation_certificates.o: $(obj)/x509_revocation_list
> +
> +quiet_cmd_extract_certs  = EXTRACT_CERTS   $(patsubst "%",%,$(2))
> +      cmd_extract_certs  = scripts/extract-cert $(2) $@
> +
> +targets += x509_revocation_list
> +$(obj)/x509_revocation_list: scripts/extract-cert $(SYSTEM_REVOCATION_KEYS_SRCPREFIX)$(SYSTEM_REVOCATION_KEYS_FILENAME) FORCE
> +	$(call if_changed,extract_certs,$(SYSTEM_REVOCATION_KEYS_SRCPREFIX)$(CONFIG_SYSTEM_REVOCATION_KEYS))
> +endif
> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index 2b8644123d5f..723b19c96256 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c
> @@ -17,9 +17,13 @@
>  #include <linux/uidgid.h>
>  #include <keys/system_keyring.h>
>  #include "blacklist.h"
> +#include "common.h"
>  
>  static struct key *blacklist_keyring;
>  
> +extern __initconst const u8 revocation_certificate_list[];
> +extern __initconst const unsigned long revocation_certificate_list_size;
> +
>  /*
>   * The description must be a type prefix, a colon and then an even number of
>   * hex digits.  The hash is kept in the description.
> @@ -220,3 +224,16 @@ static int __init blacklist_init(void)
>   * Must be initialised before we try and load the keys into the keyring.
>   */
>  device_initcall(blacklist_init);
> +
> +/*
> + * Load the compiled-in list of revocation X.509 certificates.
> + */
> +static __init int load_revocation_certificate_list(void)
> +{
> +	if (revocation_certificate_list_size)
> +		pr_notice("Loading compiled-in revocation X.509 certificates\n");
> +
> +	return load_certificate_list(revocation_certificate_list, revocation_certificate_list_size,
> +				     blacklist_keyring);
> +}
> +late_initcall(load_revocation_certificate_list);
> diff --git a/certs/revocation_certificates.S b/certs/revocation_certificates.S
> new file mode 100644
> index 000000000000..f21aae8a8f0e
> --- /dev/null
> +++ b/certs/revocation_certificates.S
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#include <linux/export.h>
> +#include <linux/init.h>
> +
> +	__INITRODATA
> +
> +	.align 8
> +	.globl revocation_certificate_list
> +revocation_certificate_list:
> +__revocation_list_start:
> +	.incbin "certs/x509_revocation_list"
> +__revocation_list_end:
> +
> +	.align 8
> +	.globl revocation_certificate_list_size
> +revocation_certificate_list_size:
> +#ifdef CONFIG_64BIT
> +	.quad __revocation_list_end - __revocation_list_start
> +#else
> +	.long __revocation_list_end - __revocation_list_start
> +#endif
> diff --git a/scripts/Makefile b/scripts/Makefile
> index b5418ec587fb..983b785f13cb 100644
> --- a/scripts/Makefile
> +++ b/scripts/Makefile
> @@ -11,6 +11,7 @@ hostprogs-always-$(CONFIG_ASN1)				+= asn1_compiler
>  hostprogs-always-$(CONFIG_MODULE_SIG_FORMAT)		+= sign-file
>  hostprogs-always-$(CONFIG_SYSTEM_TRUSTED_KEYRING)	+= extract-cert
>  hostprogs-always-$(CONFIG_SYSTEM_EXTRA_CERTIFICATE)	+= insert-sys-cert
> + hostprogs-always-$(CONFIG_SYSTEM_BLACKLIST_KEYRING)	+= extract-cert
>  
>  HOSTCFLAGS_sorttable.o = -I$(srctree)/tools/include
>  HOSTCFLAGS_asn1_compiler.o = -I$(srctree)/include
> 
> 

This patch is broken when CONFIG_SYSTEM_BLACKLIST_KEYRING is set but
CONFIG_SYSTEM_REVOCATION_LIST is unset, meaning that
CONFIG_SYSTEM_REVOCATION_KEYS is not defined at all. This is very easy
to reproduce with x86_64_defconfig.

$ make -skj"$(nproc)" O=build/x86_64 defconfig

$ scripts/config --file build/x86_64/.config -e SYSTEM_BLACKLIST_KEYRING

$ make -skj"$(nproc)" O=build/x86_64 olddefconfig certs/
At main.c:154:
- SSL error:0909006C:PEM routines:get_name:no start line: crypto/pem/pem_lib.c:745
extract-cert: /home/nathan/cbl/src/linux-next/: Is a directory
make[3]: *** [/home/nathan/cbl/src/linux-next/certs/Makefile:119: certs/x509_revocation_list] Error 1
...

This happens with every single distribution configuration that I test.

Cheers,
Nathan

Re: [PATCH 4/4] integrity: Load mokx variables into the blacklist keyring

[Dimitri John Ledkov]

On 25/02/2021 20:59, David Howells wrote:
> From: Eric Snowberg <eric.snowberg@oracle.com>
> 
> During boot the Secure Boot Forbidden Signature Database, dbx,
> is loaded into the blacklist keyring.  Systems booted with shim
> have an equivalent Forbidden Signature Database called mokx.
> Currently mokx is only used by shim and grub, the contents are
> ignored by the kernel.
> 
> Add the ability to load mokx into the blacklist keyring during boot.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Jarkko Sakkinen <jarkko@kernel.org>
> Link: https://lore.kernel.org/r/20210122181054.32635-5-eric.snowberg@oracle.com/ # v5
> Link: https://lore.kernel.org/r/c33c8e3839a41e9654f41cc92c7231104931b1d7.camel@HansenPartnership.com/
> ---
> 
>  security/integrity/platform_certs/load_uefi.c |   20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> index ee4b4c666854..f290f78c3f30 100644
> --- a/security/integrity/platform_certs/load_uefi.c
> +++ b/security/integrity/platform_certs/load_uefi.c
> @@ -132,8 +132,9 @@ static int __init load_moklist_certs(void)
>  static int __init load_uefi_certs(void)
>  {
>  	efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID;
> -	void *db = NULL, *dbx = NULL;
> -	unsigned long dbsize = 0, dbxsize = 0;
> +	efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
> +	void *db = NULL, *dbx = NULL, *mokx = NULL;
> +	unsigned long dbsize = 0, dbxsize = 0, mokxsize = 0;
>  	efi_status_t status;
>  	int rc = 0;
>  
> @@ -175,6 +176,21 @@ static int __init load_uefi_certs(void)
>  		kfree(dbx);
>  	}
>  
> +	mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize, &status);
> +	if (!mokx) {
> +		if (status == EFI_NOT_FOUND)
> +			pr_debug("mokx variable wasn't found\n");
> +		else
> +			pr_info("Couldn't get mokx list\n");
> +	} else {
> +		rc = parse_efi_signature_list("UEFI:MokListXRT",
> +					      mokx, mokxsize,
> +					      get_handler_for_dbx);
> +		if (rc)
> +			pr_err("Couldn't parse mokx signatures %d\n", rc);
> +		kfree(mokx);
> +	}
> +


My preference would be if the above hunk was moved into the
load_moklist_certs() function which is called just below. Such that
loading of MokListRT & MOkListXRT are done next to each other.

And also implement loading the same way it is done for MokListRT -
specifically via the EFI MOKvar config table & then via a variable.

See 726bd8965a5f112d9601f7ce68effa1e46e02bf2 otherwise large MokListXRT
will fail to parse.

>  	/* Load the MokListRT certs */
>  	rc = load_moklist_certs();
>  
> 
> 
> 

Re: [PATCH 4/4] integrity: Load mokx variables into the blacklist keyring

[Eric Snowberg]

> On Mar 12, 2021, at 11:39 AM, Dimitri John Ledkov <dimitri.ledkov@canonical.com> wrote:
> 
> On 25/02/2021 20:59, David Howells wrote:
>> From: Eric Snowberg <eric.snowberg@oracle.com>
>> 
>> During boot the Secure Boot Forbidden Signature Database, dbx,
>> is loaded into the blacklist keyring.  Systems booted with shim
>> have an equivalent Forbidden Signature Database called mokx.
>> Currently mokx is only used by shim and grub, the contents are
>> ignored by the kernel.
>> 
>> Add the ability to load mokx into the blacklist keyring during boot.
>> 
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>> Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>> Signed-off-by: David Howells <dhowells@redhat.com>
>> cc: Jarkko Sakkinen <jarkko@kernel.org>
>> Link: https://lore.kernel.org/r/20210122181054.32635-5-eric.snowberg@oracle.com/ # v5
>> Link: https://lore.kernel.org/r/c33c8e3839a41e9654f41cc92c7231104931b1d7.camel@HansenPartnership.com/
>> ---
>> 
>> security/integrity/platform_certs/load_uefi.c |   20 ++++++++++++++++++--
>> 1 file changed, 18 insertions(+), 2 deletions(-)
>> 
>> diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
>> index ee4b4c666854..f290f78c3f30 100644
>> --- a/security/integrity/platform_certs/load_uefi.c
>> +++ b/security/integrity/platform_certs/load_uefi.c
>> @@ -132,8 +132,9 @@ static int __init load_moklist_certs(void)
>> static int __init load_uefi_certs(void)
>> {
>> 	efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID;
>> -	void *db = NULL, *dbx = NULL;
>> -	unsigned long dbsize = 0, dbxsize = 0;
>> +	efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
>> +	void *db = NULL, *dbx = NULL, *mokx = NULL;
>> +	unsigned long dbsize = 0, dbxsize = 0, mokxsize = 0;
>> 	efi_status_t status;
>> 	int rc = 0;
>> 
>> @@ -175,6 +176,21 @@ static int __init load_uefi_certs(void)
>> 		kfree(dbx);
>> 	}
>> 
>> +	mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize, &status);
>> +	if (!mokx) {
>> +		if (status == EFI_NOT_FOUND)
>> +			pr_debug("mokx variable wasn't found\n");
>> +		else
>> +			pr_info("Couldn't get mokx list\n");
>> +	} else {
>> +		rc = parse_efi_signature_list("UEFI:MokListXRT",
>> +					      mokx, mokxsize,
>> +					      get_handler_for_dbx);
>> +		if (rc)
>> +			pr_err("Couldn't parse mokx signatures %d\n", rc);
>> +		kfree(mokx);
>> +	}
>> +
> 
> 
> My preference would be if the above hunk was moved into the
> load_moklist_certs() function which is called just below. Such that
> loading of MokListRT & MOkListXRT are done next to each other.
> 
> And also implement loading the same way it is done for MokListRT -
> specifically via the EFI MOKvar config table & then via a variable.
> 
> See 726bd8965a5f112d9601f7ce68effa1e46e02bf2 otherwise large MokListXRT
> will fail to parse.

Is this support available from shim now?  Previously I thought only
MOK could be loaded from the config table, not MOKx.

Re: [PATCH 4/4] integrity: Load mokx variables into the blacklist keyring

[Dimitri John Ledkov]

[-- Attachment #1.1: Type: text/plain, Size: 4019 bytes --]

On 12/03/2021 21:49, Eric Snowberg wrote:
> 
>> On Mar 12, 2021, at 11:39 AM, Dimitri John Ledkov <dimitri.ledkov@canonical.com> wrote:
>>
>> On 25/02/2021 20:59, David Howells wrote:
>>> From: Eric Snowberg <eric.snowberg@oracle.com>
>>>
>>> During boot the Secure Boot Forbidden Signature Database, dbx,
>>> is loaded into the blacklist keyring.  Systems booted with shim
>>> have an equivalent Forbidden Signature Database called mokx.
>>> Currently mokx is only used by shim and grub, the contents are
>>> ignored by the kernel.
>>>
>>> Add the ability to load mokx into the blacklist keyring during boot.
>>>
>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>>> Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>>> Signed-off-by: David Howells <dhowells@redhat.com>
>>> cc: Jarkko Sakkinen <jarkko@kernel.org>
>>> Link: https://lore.kernel.org/r/20210122181054.32635-5-eric.snowberg@oracle.com/ # v5
>>> Link: https://lore.kernel.org/r/c33c8e3839a41e9654f41cc92c7231104931b1d7.camel@HansenPartnership.com/
>>> ---
>>>
>>> security/integrity/platform_certs/load_uefi.c |   20 ++++++++++++++++++--
>>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
>>> index ee4b4c666854..f290f78c3f30 100644
>>> --- a/security/integrity/platform_certs/load_uefi.c
>>> +++ b/security/integrity/platform_certs/load_uefi.c
>>> @@ -132,8 +132,9 @@ static int __init load_moklist_certs(void)
>>> static int __init load_uefi_certs(void)
>>> {
>>> 	efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID;
>>> -	void *db = NULL, *dbx = NULL;
>>> -	unsigned long dbsize = 0, dbxsize = 0;
>>> +	efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
>>> +	void *db = NULL, *dbx = NULL, *mokx = NULL;
>>> +	unsigned long dbsize = 0, dbxsize = 0, mokxsize = 0;
>>> 	efi_status_t status;
>>> 	int rc = 0;
>>>
>>> @@ -175,6 +176,21 @@ static int __init load_uefi_certs(void)
>>> 		kfree(dbx);
>>> 	}
>>>
>>> +	mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize, &status);
>>> +	if (!mokx) {
>>> +		if (status == EFI_NOT_FOUND)
>>> +			pr_debug("mokx variable wasn't found\n");
>>> +		else
>>> +			pr_info("Couldn't get mokx list\n");
>>> +	} else {
>>> +		rc = parse_efi_signature_list("UEFI:MokListXRT",
>>> +					      mokx, mokxsize,
>>> +					      get_handler_for_dbx);
>>> +		if (rc)
>>> +			pr_err("Couldn't parse mokx signatures %d\n", rc);
>>> +		kfree(mokx);
>>> +	}
>>> +
>>
>>
>> My preference would be if the above hunk was moved into the
>> load_moklist_certs() function which is called just below. Such that
>> loading of MokListRT & MOkListXRT are done next to each other.
>>
>> And also implement loading the same way it is done for MokListRT -
>> specifically via the EFI MOKvar config table & then via a variable.
>>
>> See 726bd8965a5f112d9601f7ce68effa1e46e02bf2 otherwise large MokListXRT
>> will fail to parse.
> 
> Is this support available from shim now?  Previously I thought only
> MOK could be loaded from the config table, not MOKx.
> 

It is about to become available across all distributions with the next
shim as everyone is about to ship SBAT capable shims.

From my system with the next shim & 5.10 kernel I have:

$ ls /sys/firmware/efi/mok-variables/
MokIgnoreDB  MokListRT  MokListXRT  MokSBStateRT  SbatRT

It's not just a single Mok variable, but _all_ mok variables are
available from the mok-table that are used to determine mok state.
Including whether or not db should be ignored, whether or not signature
verification is turned off, and what are the SBAT generation revocations
are, in addition to MokListRT & MokListXRT.

For example, kernel could gain further functionality to honor the user
choices and disable loading db controlled by MokIgnoreDB especially
since shim chooses to not consider db certificates & hashes as trust-worthy.

Regards,

Dimitri.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] integrity: Load mokx variables into the blacklist keyring

[Eric Snowberg]

> On Mar 12, 2021, at 4:53 PM, Dimitri John Ledkov <dimitri.ledkov@canonical.com> wrote:
> 
> On 12/03/2021 21:49, Eric Snowberg wrote:
>> 
>>> On Mar 12, 2021, at 11:39 AM, Dimitri John Ledkov <dimitri.ledkov@canonical.com> wrote:
>>> 
>>> On 25/02/2021 20:59, David Howells wrote:
>>>> From: Eric Snowberg <eric.snowberg@oracle.com>
>>>> 
>>>> During boot the Secure Boot Forbidden Signature Database, dbx,
>>>> is loaded into the blacklist keyring.  Systems booted with shim
>>>> have an equivalent Forbidden Signature Database called mokx.
>>>> Currently mokx is only used by shim and grub, the contents are
>>>> ignored by the kernel.
>>>> 
>>>> Add the ability to load mokx into the blacklist keyring during boot.
>>>> 
>>>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>>>> Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>>>> Signed-off-by: David Howells <dhowells@redhat.com>
>>>> cc: Jarkko Sakkinen <jarkko@kernel.org>
>>>> Link: https://lore.kernel.org/r/20210122181054.32635-5-eric.snowberg@oracle.com/ # v5
>>>> Link: https://lore.kernel.org/r/c33c8e3839a41e9654f41cc92c7231104931b1d7.camel@HansenPartnership.com/
>>>> ---
>>>> 
>>>> security/integrity/platform_certs/load_uefi.c |   20 ++++++++++++++++++--
>>>> 1 file changed, 18 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
>>>> index ee4b4c666854..f290f78c3f30 100644
>>>> --- a/security/integrity/platform_certs/load_uefi.c
>>>> +++ b/security/integrity/platform_certs/load_uefi.c
>>>> @@ -132,8 +132,9 @@ static int __init load_moklist_certs(void)
>>>> static int __init load_uefi_certs(void)
>>>> {
>>>> 	efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID;
>>>> -	void *db = NULL, *dbx = NULL;
>>>> -	unsigned long dbsize = 0, dbxsize = 0;
>>>> +	efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
>>>> +	void *db = NULL, *dbx = NULL, *mokx = NULL;
>>>> +	unsigned long dbsize = 0, dbxsize = 0, mokxsize = 0;
>>>> 	efi_status_t status;
>>>> 	int rc = 0;
>>>> 
>>>> @@ -175,6 +176,21 @@ static int __init load_uefi_certs(void)
>>>> 		kfree(dbx);
>>>> 	}
>>>> 
>>>> +	mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize, &status);
>>>> +	if (!mokx) {
>>>> +		if (status == EFI_NOT_FOUND)
>>>> +			pr_debug("mokx variable wasn't found\n");
>>>> +		else
>>>> +			pr_info("Couldn't get mokx list\n");
>>>> +	} else {
>>>> +		rc = parse_efi_signature_list("UEFI:MokListXRT",
>>>> +					      mokx, mokxsize,
>>>> +					      get_handler_for_dbx);
>>>> +		if (rc)
>>>> +			pr_err("Couldn't parse mokx signatures %d\n", rc);
>>>> +		kfree(mokx);
>>>> +	}
>>>> +
>>> 
>>> 
>>> My preference would be if the above hunk was moved into the
>>> load_moklist_certs() function which is called just below. Such that
>>> loading of MokListRT & MOkListXRT are done next to each other.
>>> 
>>> And also implement loading the same way it is done for MokListRT -
>>> specifically via the EFI MOKvar config table & then via a variable.
>>> 
>>> See 726bd8965a5f112d9601f7ce68effa1e46e02bf2 otherwise large MokListXRT
>>> will fail to parse.
>> 
>> Is this support available from shim now?  Previously I thought only
>> MOK could be loaded from the config table, not MOKx.
>> 
> 
> It is about to become available across all distributions with the next
> shim as everyone is about to ship SBAT capable shims.

When I tested this change, I thought it was around 25+ certs and
hundreds of hashes before shim started having problems. Someone
needing the config list must really have a large list. It would
be nice of the MOKx in shim would support a TBS certificate hash,
it would really save space.

If MOKx will be available thru a config table in the next shim, 
I’ll prepare a follow on patch to add this support. 

> From my system with the next shim & 5.10 kernel I have:
> 
> $ ls /sys/firmware/efi/mok-variables/
> MokIgnoreDB  MokListRT  MokListXRT  MokSBStateRT  SbatRT
> 
> It's not just a single Mok variable, but _all_ mok variables are
> available from the mok-table that are used to determine mok state.
> Including whether or not db should be ignored, whether or not signature
> verification is turned off, and what are the SBAT generation revocations
> are, in addition to MokListRT & MokListXRT.
> 
> For example, kernel could gain further functionality to honor the user
> choices and disable loading db controlled by MokIgnoreDB especially
> since shim chooses to not consider db certificates & hashes as trust-worthy.

Isn’t this already handled by uefi_check_ignore_db()?

Re: [PATCH 4/4] integrity: Load mokx variables into the blacklist keyring

[David Howells]

Eric Snowberg <eric.snowberg@oracle.com> wrote:

> If MOKx will be available thru a config table in the next shim, 
> I'll prepare a follow on patch to add this support. 

Can this go separately, or would it be better rolled into the existing
patchset?

David

Re: [PATCH 4/4] integrity: Load mokx variables into the blacklist keyring

[Eric Snowberg]

> On Mar 13, 2021, at 1:13 AM, David Howells <dhowells@redhat.com> wrote:
> 
> Eric Snowberg <eric.snowberg@oracle.com> wrote:
> 
>> If MOKx will be available thru a config table in the next shim, 
>> I'll prepare a follow on patch to add this support. 
> 
> Can this go separately, or would it be better rolled into the existing
> patchset?

IMHO, since you have already sent a pull request and this is not available
yet in shim, it seems save to have it go separately.  I should have time 
to send something out next week to address this change.

Re: [PATCH 4/4] integrity: Load mokx variables into the blacklist keyring

[David Howells]

Eric Snowberg <eric.snowberg@oracle.com> wrote:

> > Can this go separately, or would it be better rolled into the existing
> > patchset?
> 
> IMHO, since you have already sent a pull request and this is not available
> yet in shim, it seems save to have it go separately.  I should have time 
> to send something out next week to address this change.

Ok, thanks.

David

Re: [PATCH 4/4] integrity: Load mokx variables into the blacklist keyring

[Dimitri John Ledkov]

On Sat, Mar 13, 2021 at 2:40 PM Eric Snowberg <eric.snowberg@oracle.com> wrote:
>
>
> > On Mar 13, 2021, at 1:13 AM, David Howells <dhowells@redhat.com> wrote:
> >
> > Eric Snowberg <eric.snowberg@oracle.com> wrote:
> >
> >> If MOKx will be available thru a config table in the next shim,
> >> I'll prepare a follow on patch to add this support.
> >
> > Can this go separately, or would it be better rolled into the existing
> > patchset?
>
> IMHO, since you have already sent a pull request and this is not available
> yet in shim, it seems save to have it go separately.  I should have time
> to send something out next week to address this change.
>

I don't think that was correct to call it "not available yet in shim".
Shim has always exported all the variables, it's just at the time most
shims from most distros did not have any contents in MokX. And as
usual with all EFI variables, empty ones do not exist.

The shim from Ubuntu which is now public contains 1 cert & 378 on
x86_64 and 1 cert & 170 hashes. Thus it is likely to fail to be read
by kernel correctly unless it uses efi_mokvar_entry_find(). These
patches are tagged with CVE number which it does not address
completely in some configurations. Are you working on a patch to add
efi_mokvar_entry_find() or do you want me to write it / test it /
submit it for review?


-- 
Regards,

Dimitri.