[PATCH 0/9] KEYS: Blacklisting & UEFI database load

[PATCH 0/9] KEYS: Blacklisting & UEFI database load

[David Howells]

Here are two sets of patches.  Firstly, the first three patches provide a
blacklist, making the following changes:

 (1) Add a new key type, blacklist, that is a slimline key that just
     contains a blacklisted hash and a note saying what it applies to.

 (2) Add a system keyring, .blacklist, that holds blacklisted hashes.

 (3) Add a pair of functions, one that blacklists a hash and the other that
     queries as to whether a hash is blacklisted.

 (4) Check the TBS hash of an X.509 cert against the blacklist.

 (5) Check the PKCS#7 content hash against the blacklist.

 (6) Allow a file of blacklisted hashes to be included in the build.

Secondly, the remaining patches allow the UEFI database to be used to load
the system keyrings:

 (1) Kernel initialisation is permitted to add keys to the
     .secondary_trusted_keys keyring.

 (2) A parser is added to parse the contents of the UEFI variables that
     contain keys and hashes.

 (3) The UEFI db and MokListRT variables are parsed for keys which are
     loaded into the secondary keyring.

 (4) The UEFI dbx variable is parsed for hashes to be blacklisted.

 (5) Use of the UEFI db variable can be suppressed by another UEFI variable.

The patches can be found here also:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-blacklist

and:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-uefi

David
---
Dave Howells (2):
      efi: Add EFI signature data types
      efi: Add an EFI signature blob parser

David Howells (5):
      KEYS: Add a system blacklist keyring
      X.509: Allow X.509 certs to be blacklisted
      PKCS#7: Handle blacklisted certificates
      KEYS: Allow unrestricted boot-time addition of keys to secondary keyring
      efi: Add SHIM and image security database GUID definitions

Josh Boyer (2):
      MODSIGN: Import certificates from UEFI Secure Boot
      MODSIGN: Allow the "db" UEFI variable to be suppressed


 certs/Kconfig                            |   42 +++++++
 certs/Makefile                           |   11 ++
 certs/blacklist.c                        |  174 +++++++++++++++++++++++++++
 certs/blacklist.h                        |    3 
 certs/blacklist_hashes.c                 |    6 +
 certs/blacklist_nohashes.c               |    5 +
 certs/efi_parser.c                       |  112 ++++++++++++++++++
 certs/internal.h                         |   18 +++
 certs/load_uefi.c                        |  192 ++++++++++++++++++++++++++++++
 certs/system_keyring.c                   |   33 +++++
 crypto/asymmetric_keys/pkcs7_parser.h    |    1 
 crypto/asymmetric_keys/pkcs7_verify.c    |   32 ++++-
 crypto/asymmetric_keys/x509_parser.h     |    1 
 crypto/asymmetric_keys/x509_public_key.c |   15 ++
 include/keys/system_keyring.h            |   12 ++
 include/linux/efi.h                      |   36 ++++++
 16 files changed, 685 insertions(+), 8 deletions(-)
 create mode 100644 certs/blacklist.c
 create mode 100644 certs/blacklist.h
 create mode 100644 certs/blacklist_hashes.c
 create mode 100644 certs/blacklist_nohashes.c
 create mode 100644 certs/efi_parser.c
 create mode 100644 certs/internal.h
 create mode 100644 certs/load_uefi.c

[PATCH 1/9] KEYS: Add a system blacklist keyring

[David Howells]

Add the following:

 (1) A new system keyring that is used to store information about
     blacklisted certificates and signatures.

 (2) A new key type (called 'blacklist') that is used to store a
     blacklisted hash in its description as a hex string.  The key accepts
     no payload.

 (3) The ability to configure a list of blacklisted hashes into the kernel
     at build time.  This is done by setting
     CONFIG_SYSTEM_BLACKLIST_HASH_LIST to the filename of a list of hashes
     that are in the form:

	"<hash>", "<hash>", ..., "<hash>"

     where each <hash> is a hex string representation of the hash and must
     include all necessary leading zeros to pad the hash to the right size.

The above are enabled with CONFIG_SYSTEM_BLACKLIST_KEYRING.

Once the kernel is booted, the blacklist keyring can be listed:

	root@andromeda ~]# keyctl show %:.blacklist
	Keyring
	 723359729 ---lswrv      0     0  keyring: .blacklist
	 676257228 ---lswrv      0     0   \_ blacklist: 123412341234c55c1dcc601ab8e172917706aa32fb5eaf826813547fdf02dd46

The blacklist cannot currently be modified by userspace, but it will be
possible to load it, for example, from the UEFI blacklist database.

A later commit will make it possible to load blacklisted asymmetric keys in
here too.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 certs/Kconfig                 |   18 ++++
 certs/Makefile                |    6 +
 certs/blacklist.c             |  174 +++++++++++++++++++++++++++++++++++++++++
 certs/blacklist.h             |    3 +
 certs/blacklist_hashes.c      |    6 +
 certs/blacklist_nohashes.c    |    5 +
 include/keys/system_keyring.h |   12 +++
 7 files changed, 224 insertions(+)
 create mode 100644 certs/blacklist.c
 create mode 100644 certs/blacklist.h
 create mode 100644 certs/blacklist_hashes.c
 create mode 100644 certs/blacklist_nohashes.c

diff --git a/certs/Kconfig b/certs/Kconfig
index fc5955f5fc8a..6ce51ede9e9b 100644
--- a/certs/Kconfig
+++ b/certs/Kconfig
@@ -64,4 +64,22 @@ config SECONDARY_TRUSTED_KEYRING
 	  those keys are not blacklisted and are vouched for by a key built
 	  into the kernel or already in the secondary trusted keyring.
 
+config SYSTEM_BLACKLIST_KEYRING
+	bool "Provide system-wide ring of blacklisted keys"
+	depends on KEYS
+	help
+	  Provide a system keyring to which blacklisted keys can be added.
+	  Keys in the keyring are considered entirely untrusted.  Keys in this
+	  keyring are used by the module signature checking to reject loading
+	  of modules signed with a blacklisted key.
+
+config SYSTEM_BLACKLIST_HASH_LIST
+	string "Hashes to be preloaded into the system blacklist keyring"
+	depends on SYSTEM_BLACKLIST_KEYRING
+	help
+	  If set, this option should be the filename of a list of hashes in the
+	  form "<hash>", "<hash>", ... .  This will be included into a C
+	  wrapper to incorporate the list into the kernel.  Each <hash> should
+	  be a string of hex digits.
+
 endmenu
diff --git a/certs/Makefile b/certs/Makefile
index 2773c4afa24c..4119bb376ea1 100644
--- a/certs/Makefile
+++ b/certs/Makefile
@@ -3,6 +3,12 @@
 #
 
 obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o
+obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist.o
+ifneq ($(CONFIG_SYSTEM_BLACKLIST_HASH_LIST),"")
+obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist_hashes.o
+else
+obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist_nohashes.o
+endif
 
 ifeq ($(CONFIG_SYSTEM_TRUSTED_KEYRING),y)
 
diff --git a/certs/blacklist.c b/certs/blacklist.c
new file mode 100644
index 000000000000..3eddce0e307a
--- /dev/null
+++ b/certs/blacklist.c
@@ -0,0 +1,174 @@
+/* System hash blacklist.
+ *
+ * Copyright (C) 2016 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+#define pr_fmt(fmt) "blacklist: "fmt
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/key.h>
+#include <linux/key-type.h>
+#include <linux/sched.h>
+#include <linux/ctype.h>
+#include <linux/err.h>
+#include <linux/seq_file.h>
+#include <keys/system_keyring.h>
+#include "blacklist.h"
+
+static struct key *blacklist_keyring;
+
+/*
+ * The description must be a type prefix, a colon and then an even number of
+ * hex digits.  The hash is kept in the description.
+ */
+static int blacklist_vet_description(const char *desc)
+{
+	int n = 0;
+
+	if (*desc == ':')
+		return -EINVAL;
+	for (; *desc; desc++)
+		if (*desc == ':')
+			goto found_colon;
+	return -EINVAL;
+
+found_colon:
+	desc++;
+	for (; *desc; desc++) {
+		if (!isxdigit(*desc))
+			return -EINVAL;
+		n++;
+	}
+
+	if (n == 0 || n & 1)
+		return -EINVAL;
+	return 0;
+}
+
+/*
+ * The hash to be blacklisted is expected to be in the description.  There will
+ * be no payload.
+ */
+static int blacklist_preparse(struct key_preparsed_payload *prep)
+{
+	if (prep->datalen > 0)
+		return -EINVAL;
+	return 0;
+}
+
+static void blacklist_free_preparse(struct key_preparsed_payload *prep)
+{
+}
+
+static void blacklist_describe(const struct key *key, struct seq_file *m)
+{
+	seq_puts(m, key->description);
+}
+
+static struct key_type key_type_blacklist = {
+	.name			= "blacklist",
+	.vet_description	= blacklist_vet_description,
+	.preparse		= blacklist_preparse,
+	.free_preparse		= blacklist_free_preparse,
+	.instantiate		= generic_key_instantiate,
+	.describe		= blacklist_describe,
+};
+
+/**
+ * mark_hash_blacklisted - Add a hash to the system blacklist
+ * @hash - The hash as a hex string with a type prefix (eg. "tbs:23aa429783")
+ */
+int mark_hash_blacklisted(const char *hash)
+{
+	key_ref_t key;
+
+	key = key_create_or_update(make_key_ref(blacklist_keyring, true),
+				   "blacklist",
+				   hash,
+				   NULL,
+				   0,
+				   ((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 blacklisting hash (%ld)\n", PTR_ERR(key));
+		return PTR_ERR(key);
+	}
+	return 0;
+}
+
+/**
+ * is_hash_blacklisted - Determine if a hash is blacklisted
+ * @hash: The hash to be checked as a binary blob
+ * @hash_len: The length of the binary hash
+ * @type: Type of hash
+ */
+int is_hash_blacklisted(const u8 *hash, size_t hash_len, const char *type)
+{
+	key_ref_t kref;
+	size_t type_len = strlen(type);
+	char *buffer, *p;
+	int ret = 0;
+
+	buffer = kmalloc(type_len + 1 + hash_len * 2 + 1, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+	p = memcpy(buffer, type, type_len);
+	p += type_len;
+	*p++ = ':';
+	bin2hex(p, hash, hash_len);
+	p += hash_len * 2;
+	*p = 0;
+
+	kref = keyring_search(make_key_ref(blacklist_keyring, true),
+			      &key_type_blacklist, buffer);
+	if (!IS_ERR(kref)) {
+		key_ref_put(kref);
+		ret = -EKEYREJECTED;
+	}
+
+	kfree(buffer);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(is_hash_blacklisted);
+
+/*
+ * Intialise the blacklist
+ */
+static int __init blacklist_init(void)
+{
+	const char *const *bl;
+
+	if (register_key_type(&key_type_blacklist) < 0)
+		panic("Can't allocate system blacklist key type\n");
+
+	blacklist_keyring =
+		keyring_alloc(".blacklist",
+			      KUIDT_INIT(0), KGIDT_INIT(0),
+			      current_cred(),
+			      (KEY_POS_ALL & ~KEY_POS_SETATTR) |
+			      KEY_USR_VIEW | KEY_USR_READ |
+			      KEY_USR_SEARCH,
+			      KEY_ALLOC_NOT_IN_QUOTA |
+			      KEY_FLAG_KEEP,
+			      NULL, NULL);
+	if (IS_ERR(blacklist_keyring))
+		panic("Can't allocate system blacklist keyring\n");
+
+	for (bl = blacklist_hashes; *bl; bl++)
+		if (mark_hash_blacklisted(*bl) < 0)
+			pr_err("- blacklisting failed\n");
+	return 0;
+}
+
+/*
+ * Must be initialised before we try and load the keys into the keyring.
+ */
+device_initcall(blacklist_init);
diff --git a/certs/blacklist.h b/certs/blacklist.h
new file mode 100644
index 000000000000..150d82da8e99
--- /dev/null
+++ b/certs/blacklist.h
@@ -0,0 +1,3 @@
+#include <linux/kernel.h>
+
+extern const char __initdata *const blacklist_hashes[];
diff --git a/certs/blacklist_hashes.c b/certs/blacklist_hashes.c
new file mode 100644
index 000000000000..5bd449f7db17
--- /dev/null
+++ b/certs/blacklist_hashes.c
@@ -0,0 +1,6 @@
+#include "blacklist.h"
+
+const char __initdata *const blacklist_hashes[] = {
+#include CONFIG_SYSTEM_BLACKLIST_HASH_LIST
+	, NULL
+};
diff --git a/certs/blacklist_nohashes.c b/certs/blacklist_nohashes.c
new file mode 100644
index 000000000000..851de10706a5
--- /dev/null
+++ b/certs/blacklist_nohashes.c
@@ -0,0 +1,5 @@
+#include "blacklist.h"
+
+const char __initdata *const blacklist_hashes[] = {
+	NULL
+};
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index fbd4647767e9..0d8762622ab9 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -33,6 +33,18 @@ 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_SYSTEM_BLACKLIST_KEYRING
+extern int mark_hash_blacklisted(const char *hash);
+extern int is_hash_blacklisted(const u8 *hash, size_t hash_len,
+			       const char *type);
+#else
+static inline int is_hash_blacklisted(const u8 *hash, size_t hash_len,
+				      const char *type)
+{
+	return 0;
+}
+#endif
+
 #ifdef CONFIG_IMA_BLACKLIST_KEYRING
 extern struct key *ima_blacklist_keyring;
 

[PATCH 2/9] X.509: Allow X.509 certs to be blacklisted

[David Howells]

Allow X.509 certs to be blacklisted based on their TBSCertificate hash.
This is convenient since we have to determine this anyway to be able to
check the signature on an X.509 certificate.  This is also what UEFI uses
in its blacklist.

If a certificate built into the kernel is blacklisted, something like the
following might then be seen during boot:

	X.509: Cert 123412341234c55c1dcc601ab8e172917706aa32fb5eaf826813547fdf02dd46 is blacklisted
	Problem loading in-kernel X.509 certificate (-129)

where the hex string shown is the blacklisted hash.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 crypto/asymmetric_keys/x509_parser.h     |    1 +
 crypto/asymmetric_keys/x509_public_key.c |   15 +++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
index 05eef1c68881..e373e7483812 100644
--- a/crypto/asymmetric_keys/x509_parser.h
+++ b/crypto/asymmetric_keys/x509_parser.h
@@ -42,6 +42,7 @@ struct x509_certificate {
 	bool		self_signed;		/* T if self-signed (check unsupported_sig too) */
 	bool		unsupported_key;	/* T if key uses unsupported crypto */
 	bool		unsupported_sig;	/* T if signature uses unsupported crypto */
+	bool		blacklisted;
 };
 
 /*
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index fb732296cd36..eea71dc9686c 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -84,6 +84,16 @@ int x509_get_sig_params(struct x509_certificate *cert)
 		goto error_2;
 	might_sleep();
 	ret = crypto_shash_finup(desc, cert->tbs, cert->tbs_size, sig->digest);
+	if (ret < 0)
+		goto error_2;
+
+	ret = is_hash_blacklisted(sig->digest, sig->digest_size, "tbs");
+	if (ret == -EKEYREJECTED) {
+		pr_err("Cert %*phN is blacklisted\n",
+		       sig->digest_size, sig->digest);
+		cert->blacklisted = true;
+		ret = 0;
+	}
 
 error_2:
 	kfree(desc);
@@ -186,6 +196,11 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
 			 cert->sig->pkey_algo, cert->sig->hash_algo);
 	}
 
+	/* Don't permit addition of blacklisted keys */
+	ret = -EKEYREJECTED;
+	if (cert->blacklisted)
+		goto error_free_cert;
+
 	/* Propose a description */
 	sulen = strlen(cert->subject);
 	if (cert->raw_skid) {

[PATCH 3/9] PKCS#7: Handle blacklisted certificates

[David Howells]

PKCS#7: Handle certificates that are blacklisted when verifying the chain
of trust on the signatures on a PKCS#7 message.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 crypto/asymmetric_keys/pkcs7_parser.h |    1 +
 crypto/asymmetric_keys/pkcs7_verify.c |   32 ++++++++++++++++++++++++--------
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/crypto/asymmetric_keys/pkcs7_parser.h b/crypto/asymmetric_keys/pkcs7_parser.h
index f4e81074f5e0..ac341e19e530 100644
--- a/crypto/asymmetric_keys/pkcs7_parser.h
+++ b/crypto/asymmetric_keys/pkcs7_parser.h
@@ -23,6 +23,7 @@ struct pkcs7_signed_info {
 	struct x509_certificate *signer; /* Signing certificate (in msg->certs) */
 	unsigned	index;
 	bool		unsupported_crypto;	/* T if not usable due to missing crypto */
+	bool		blacklisted;
 
 	/* Message digest - the digest of the Content Data (or NULL) */
 	const void	*msgdigest;
diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 2ffd69769466..2d93d9eccb4d 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -190,6 +190,18 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
 			 x509->subject,
 			 x509->raw_serial_size, x509->raw_serial);
 		x509->seen = true;
+
+		if (x509->blacklisted) {
+			/* If this cert is blacklisted, then mark everything
+			 * that depends on this as blacklisted too.
+			 */
+			sinfo->blacklisted = true;
+			for (p = sinfo->signer; p != x509; p = p->signer)
+				p->blacklisted = true;
+			pr_debug("- blacklisted\n");
+			return 0;
+		}
+
 		if (x509->unsupported_key)
 			goto unsupported_crypto_in_x509;
 
@@ -357,17 +369,19 @@ static int pkcs7_verify_one(struct pkcs7_message *pkcs7,
  *
  *  (*) -EBADMSG if some part of the message was invalid, or:
  *
- *  (*) -ENOPKG if none of the signature chains are verifiable because suitable
- *	crypto modules couldn't be found, or:
+ *  (*) 0 if no signature chains were found to be blacklisted or to contain
+ *	unsupported crypto, or:
  *
- *  (*) 0 if all the signature chains that don't incur -ENOPKG can be verified
- *	(note that a signature chain may be of zero length), or:
+ *  (*) -EKEYREJECTED if a blacklisted key was encountered, or:
+ *
+ *  (*) -ENOPKG if none of the signature chains are verifiable because suitable
+ *	crypto modules couldn't be found.
  */
 int pkcs7_verify(struct pkcs7_message *pkcs7,
 		 enum key_being_used_for usage)
 {
 	struct pkcs7_signed_info *sinfo;
-	int enopkg = -ENOPKG;
+	int actual_ret = -ENOPKG;
 	int ret;
 
 	kenter("");
@@ -412,6 +426,8 @@ int pkcs7_verify(struct pkcs7_message *pkcs7,
 
 	for (sinfo = pkcs7->signed_infos; sinfo; sinfo = sinfo->next) {
 		ret = pkcs7_verify_one(pkcs7, sinfo);
+		if (sinfo->blacklisted && actual_ret == -ENOPKG)
+			actual_ret = -EKEYREJECTED;
 		if (ret < 0) {
 			if (ret == -ENOPKG) {
 				sinfo->unsupported_crypto = true;
@@ -420,11 +436,11 @@ int pkcs7_verify(struct pkcs7_message *pkcs7,
 			kleave(" = %d", ret);
 			return ret;
 		}
-		enopkg = 0;
+		actual_ret = 0;
 	}
 
-	kleave(" = %d", enopkg);
-	return enopkg;
+	kleave(" = %d", actual_ret);
+	return actual_ret;
 }
 EXPORT_SYMBOL_GPL(pkcs7_verify);
 

[PATCH 4/9] KEYS: Allow unrestricted boot-time addition of keys to secondary keyring

[David Howells]

Allow keys to be added to the system secondary certificates keyring during
kernel initialisation in an unrestricted fashion.  Such keys are implicitly
trusted and don't have their trust chains checked on link.

This allows keys in the UEFI database to be added in secure boot mode for
the purposes of module signing.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 certs/internal.h       |   18 ++++++++++++++++++
 certs/system_keyring.c |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)
 create mode 100644 certs/internal.h

diff --git a/certs/internal.h b/certs/internal.h
new file mode 100644
index 000000000000..5dcbefb0c23a
--- /dev/null
+++ b/certs/internal.h
@@ -0,0 +1,18 @@
+/* Internal definitions
+ *
+ * Copyright (C) 2016 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+/*
+ * system_keyring.c
+ */
+#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
+extern void __init add_trusted_secondary_key(const char *source,
+					     const void *data, size_t len);
+#endif
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 50979d6dcecd..dfddcf6e6c88 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -17,6 +17,7 @@
 #include <keys/asymmetric-type.h>
 #include <keys/system_keyring.h>
 #include <crypto/pkcs7.h>
+#include "internal.h"
 
 static struct key *builtin_trusted_keys;
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
@@ -242,3 +243,35 @@ int verify_pkcs7_signature(const void *data, size_t len,
 EXPORT_SYMBOL_GPL(verify_pkcs7_signature);
 
 #endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
+
+#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
+/**
+ * add_trusted_secondary_key - Add to secondary keyring with no validation
+ * @source: Source of key
+ * @data: The blob holding the key
+ * @len: The length of the data blob
+ *
+ * Add a key to the secondary keyring without checking its trust chain.  This
+ * is available only during kernel initialisation.
+ */
+void __init add_trusted_secondary_key(const char *source,
+				      const void *data, size_t len)
+{
+	key_ref_t key;
+
+	key = key_create_or_update(make_key_ref(secondary_trusted_keys, 1),
+				   "asymmetric",
+				   NULL, data, len,
+				   (KEY_POS_ALL & ~KEY_POS_SETATTR) |
+				   KEY_USR_VIEW,
+				   KEY_ALLOC_NOT_IN_QUOTA |
+				   KEY_ALLOC_BYPASS_RESTRICTION);
+
+	if (IS_ERR(key))
+		pr_err("Problem loading %s X.509 certificate (%ld)\n",
+		       source, PTR_ERR(key));
+	else
+		pr_notice("Loaded %s cert '%s' linked to secondary sys keyring\n",
+			  source, key_ref_to_ptr(key)->description);
+}
+#endif /* CONFIG_SECONDARY_TRUSTED_KEYRING */

Re: [PATCH 4/9] KEYS: Allow unrestricted boot-time addition of keys to secondary keyring

[Petko Manolov]

On 16-11-16 18:11:13, David Howells wrote:
> Allow keys to be added to the system secondary certificates keyring during 
> kernel initialisation in an unrestricted fashion.  Such keys are implicitly 
> trusted and don't have their trust chains checked on link.

Well, I for one do not explicitly trust these keys.  I may even want to 
completely remove or replace them.

> This allows keys in the UEFI database to be added in secure boot mode for the 
> purposes of module signing.

The key import should not be automatic, it should be optional.  Same applies to 
the validation process.


		Petko


> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  certs/internal.h       |   18 ++++++++++++++++++
>  certs/system_keyring.c |   33 +++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
>  create mode 100644 certs/internal.h
> 
> diff --git a/certs/internal.h b/certs/internal.h
> new file mode 100644
> index 000000000000..5dcbefb0c23a
> --- /dev/null
> +++ b/certs/internal.h
> @@ -0,0 +1,18 @@
> +/* Internal definitions
> + *
> + * Copyright (C) 2016 Red Hat, Inc. All Rights Reserved.
> + * Written by David Howells (dhowells@redhat.com)
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public Licence
> + * as published by the Free Software Foundation; either version
> + * 2 of the Licence, or (at your option) any later version.
> + */
> +
> +/*
> + * system_keyring.c
> + */
> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> +extern void __init add_trusted_secondary_key(const char *source,
> +					     const void *data, size_t len);
> +#endif
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 50979d6dcecd..dfddcf6e6c88 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -17,6 +17,7 @@
>  #include <keys/asymmetric-type.h>
>  #include <keys/system_keyring.h>
>  #include <crypto/pkcs7.h>
> +#include "internal.h"
>  
>  static struct key *builtin_trusted_keys;
>  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> @@ -242,3 +243,35 @@ int verify_pkcs7_signature(const void *data, size_t len,
>  EXPORT_SYMBOL_GPL(verify_pkcs7_signature);
>  
>  #endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
> +
> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> +/**
> + * add_trusted_secondary_key - Add to secondary keyring with no validation
> + * @source: Source of key
> + * @data: The blob holding the key
> + * @len: The length of the data blob
> + *
> + * Add a key to the secondary keyring without checking its trust chain.  This
> + * is available only during kernel initialisation.
> + */
> +void __init add_trusted_secondary_key(const char *source,
> +				      const void *data, size_t len)
> +{
> +	key_ref_t key;
> +
> +	key = key_create_or_update(make_key_ref(secondary_trusted_keys, 1),
> +				   "asymmetric",
> +				   NULL, data, len,
> +				   (KEY_POS_ALL & ~KEY_POS_SETATTR) |
> +				   KEY_USR_VIEW,
> +				   KEY_ALLOC_NOT_IN_QUOTA |
> +				   KEY_ALLOC_BYPASS_RESTRICTION);
> +
> +	if (IS_ERR(key))
> +		pr_err("Problem loading %s X.509 certificate (%ld)\n",
> +		       source, PTR_ERR(key));
> +	else
> +		pr_notice("Loaded %s cert '%s' linked to secondary sys keyring\n",
> +			  source, key_ref_to_ptr(key)->description);
> +}
> +#endif /* CONFIG_SECONDARY_TRUSTED_KEYRING */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 4/9] KEYS: Allow unrestricted boot-time addition of keys to secondary keyring

[David Howells]

Petko Manolov <petkan@mip-labs.com> wrote:

> On 16-11-16 18:11:13, David Howells wrote:
> > Allow keys to be added to the system secondary certificates keyring during 
> > kernel initialisation in an unrestricted fashion.  Such keys are implicitly 
> > trusted and don't have their trust chains checked on link.
> 
> Well, I for one do not explicitly trust these keys.  I may even want to 
> completely remove or replace them.

Fine be me.  However, if you remove them all I would guess that you cannot
perform a secure boot.

Note that it's to be expected that the keys being loaded from the UEFI
database cannot have their signatures checked - which is why they would have
to be implicitly trusted.  For the same reason, the kernel does not check the
signatures on the keys compiled into the kernel image.

> > This allows keys in the UEFI database to be added in secure boot mode for
> > the purposes of module signing.
> 
> The key import should not be automatic, it should be optional.

You can argue this either way.  There's a config option to allow you to turn
this on or off.  Arguably, this should be split in two: one for the whitelist
(db, MokListRT) and one for the blacklist (dbx).

Further, possibly I should add an option that allows this to be restricted to
secure boot mode only.

> Same applies to the validation process.

Depends what you mean by "the validation process"?  The use of secure boot at
all?  The checking of signatures on keys?  Module signing?

David

Re: [PATCH 4/9] KEYS: Allow unrestricted boot-time addition of keys to secondary keyring

[Petko Manolov]

On 16-11-17 09:56:00, David Howells wrote:
> Petko Manolov <petkan@mip-labs.com> wrote:
> 
> > On 16-11-16 18:11:13, David Howells wrote:
> > > Allow keys to be added to the system secondary certificates keyring during 
> > > kernel initialisation in an unrestricted fashion.  Such keys are 
> > > implicitly trusted and don't have their trust chains checked on link.
> > 
> > Well, I for one do not explicitly trust these keys.  I may even want to 
> > completely remove or replace them.
> 
> Fine be me.  However, if you remove them all I would guess that you cannot 
> perform a secure boot.

Maybe not on PC, but there's plenty of other architectures out there.  What if i 
replace all UEFI keys with my own?

> Note that it's to be expected that the keys being loaded from the UEFI 
> database cannot have their signatures checked - which is why they would have 
> to be implicitly trusted.  For the same reason, the kernel does not check the 
> signatures on the keys compiled into the kernel image.

I build all kernels that matter to me and i _do_ trust myself.  Unfortunately i 
can't say the same for any corporation out there.

Trusting a key because your vendor shipped the HW with it so that you have no 
way to verify the signature is pretty weak argument IMHO.

However, I am also well aware that most people just don't care. :)

> > > This allows keys in the UEFI database to be added in secure boot mode for 
> > > the purposes of module signing.
> > 
> > The key import should not be automatic, it should be optional.
> 
> You can argue this either way.  There's a config option to allow you to turn 
> this on or off.  Arguably, this should be split in two: one for the whitelist 
> (db, MokListRT) and one for the blacklist (dbx).

I did not see the config option.  There is one?

Right now i can't decide whether whitelist should go along with blacklist or 
there should be separate options.  I guess for whoever goes down this path it 
would make sense to use either both or none of them.

> Further, possibly I should add an option that allows this to be restricted to
> secure boot mode only.

Please do.  It doesn't make much sense otherwise.

> > Same applies to the validation process.
> 
> Depends what you mean by "the validation process"?  The use of secure boot at 
> all?  The checking of signatures on keys?  Module signing?

Nevermind.  The keys signature can't be verified in the classic UEFI case.


		Petko

Re: [PATCH 4/9] KEYS: Allow unrestricted boot-time addition of keys to secondary keyring

[David Howells]

Petko Manolov <petkan@mip-labs.com> wrote:

> > > Well, I for one do not explicitly trust these keys.  I may even want to 
> > > completely remove or replace them.
> > 
> > Fine be me.  However, if you remove them all I would guess that you cannot 
> > perform a secure boot.
> 
> Maybe not on PC, but there's plenty of other architectures out there.  What
> if i replace all UEFI keys with my own?

Then I would imagine that you can do a secure boot, but that you have to sign
your own shim, grub, kernel, etc..

> > Note that it's to be expected that the keys being loaded from the UEFI
> > database cannot have their signatures checked - which is why they would
> > have to be implicitly trusted.  For the same reason, the kernel does not
> > check the signatures on the keys compiled into the kernel image.
> 
> I build all kernels that matter to me and i _do_ trust myself.
> Unfortunately i can't say the same for any corporation out there.
> 
> Trusting a key because your vendor shipped the HW with it so that you have no 
> way to verify the signature is pretty weak argument IMHO.

I'm not making an argument there.  There is a reason I think that I can't
check them.  Well, possibly I could *if* those keys are actually signed *and*
I have certs built into the kernel by which I can verify all those keys in
UEFI variables.  I don't know whether this is actually practical.

> > You can argue this either way.  There's a config option to allow you to
> > turn this on or off.  Arguably, this should be split in two: one for the
> > whitelist (db, MokListRT) and one for the blacklist (dbx).
> 
> I did not see the config option.  There is one?

See patch 8 where these variables are actually parsed.  CONFIG_LOAD_UEFI_KEYS
is available there.

David

Re: [PATCH 4/9] KEYS: Allow unrestricted boot-time addition of keys to secondary keyring

[Mimi Zohar]

On Thu, 2016-11-17 at 09:56 +0000, David Howells wrote:
> Petko Manolov <petkan@mip-labs.com> wrote:
> 
> > On 16-11-16 18:11:13, David Howells wrote:
> > > Allow keys to be added to the system secondary certificates keyring during 
> > > kernel initialisation in an unrestricted fashion.  Such keys are implicitly 
> > > trusted and don't have their trust chains checked on link.
> > 
> > Well, I for one do not explicitly trust these keys.  I may even want to 
> > completely remove or replace them.
> 
> Fine be me.  However, if you remove them all I would guess that you cannot
> perform a secure boot.
> 
> Note that it's to be expected that the keys being loaded from the UEFI
> database cannot have their signatures checked - which is why they would have
> to be implicitly trusted.  For the same reason, the kernel does not check the
> signatures on the keys compiled into the kernel image.

Sigh, we've been here before, discussed this before.  Different keys
should be trusted at different levels.  Nothing has changed.  Just
because I trust a key in UEFI for UEFI, doesn't mean that I trust that
same key once the kernel has booted.

This time not only are you bringing the keys from UEFI up to the kernel,
but by adding these keys to the secondary trusted keyring, they are
allowed to add other keys they've signed to the secondary trusted
keyring.

If the UEFI keys are just for verifying kernel modules, why not define a
separate UEFI keyring, which can be used, if enabled, just for verifying
kernel modules, instead of affecting all signature verification?

IMA's root of trust goes back to UEFI, but transitions to the builtin
kernel keyring and, if enabled,  the secondary keyring on boot.

> > > This allows keys in the UEFI database to be added in secure boot mode for
> > > the purposes of module signing.
> > 
> > The key import should not be automatic, it should be optional.
> 
> You can argue this either way.  There's a config option to allow you to turn
> this on or off.  Arguably, this should be split in two: one for the whitelist
> (db, MokListRT) and one for the blacklist (dbx).

By "config", you're not referring to a Kconfig option, but a UEFI db
option, making it hidden/unknown to someone building a kernel.  If you
really want to add this support, make it clear and easily seen by
defining a "restrict_link_by_builtin_or_uefi" function.

Mimi

Re: [PATCH 4/9] KEYS: Allow unrestricted boot-time addition of keys to secondary keyring

[David Howells]

Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:

> > > > This allows keys in the UEFI database to be added in secure boot mode
> > > > for the purposes of module signing.
> > > 
> > > The key import should not be automatic, it should be optional.
> > 
> > You can argue this either way.  There's a config option to allow you to
> > turn this on or off.  Arguably, this should be split in two: one for the
> > whitelist (db, MokListRT) and one for the blacklist (dbx).
> 
> By "config", you're not referring to a Kconfig option, but a UEFI db
> option, making it hidden/unknown to someone building a kernel.  If you
> really want to add this support, make it clear and easily seen by
> defining a "restrict_link_by_builtin_or_uefi" function.

No: by "config" I *am* referring to Kconfig.

David

Re: [PATCH 4/9] KEYS: Allow unrestricted boot-time addition of keys to secondary keyring

[Mimi Zohar]

On Mon, 2016-11-21 at 15:17 +0000, David Howells wrote:
> Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> 
> > > > > This allows keys in the UEFI database to be added in secure boot mode
> > > > > for the purposes of module signing.
> > > > 
> > > > The key import should not be automatic, it should be optional.
> > > 
> > > You can argue this either way.  There's a config option to allow you to
> > > turn this on or off.  Arguably, this should be split in two: one for the
> > > whitelist (db, MokListRT) and one for the blacklist (dbx).
> > 
> > By "config", you're not referring to a Kconfig option, but a UEFI db
> > option, making it hidden/unknown to someone building a kernel.  If you
> > really want to add this support, make it clear and easily seen by
> > defining a "restrict_link_by_builtin_or_uefi" function.
> 
> No: by "config" I *am* referring to Kconfig.

Good,  I found the Kconfig LOAD_UEFI_KEYS option for loading the keys on
the keyring.  Lets say that someone does want to use those keys for
kernel modules, but only for kernel modules, not for any other types of
files (eg. kexec kernel image/initramfs)?

Mimi

[PATCH 5/9] efi: Add SHIM and image security database GUID definitions

[David Howells]

Add the definitions for shim and image security database, both of which
are used widely in various Linux distros.

Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 include/linux/efi.h |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 2d089487d2da..acfdea8381de 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -592,6 +592,9 @@ void efi_native_runtime_setup(void);
 #define EFI_MEMORY_ATTRIBUTES_TABLE_GUID	EFI_GUID(0xdcfa911d, 0x26eb, 0x469f,  0xa2, 0x20, 0x38, 0xb7, 0xdc, 0x46, 0x12, 0x20)
 #define EFI_CONSOLE_OUT_DEVICE_GUID		EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4,  0x9a, 0x46, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
 
+#define EFI_IMAGE_SECURITY_DATABASE_GUID	EFI_GUID(0xd719b2cb, 0x3d3a, 0x4596, 0xa3, 0xbc, 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
+#define EFI_SHIM_LOCK_GUID			EFI_GUID(0x605dab50, 0xe046, 0x4300, 0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
+
 /*
  * This GUID is used to pass to the kernel proper the struct screen_info
  * structure that was populated by the stub based on the GOP protocol instance

Re: [PATCH 5/9] efi: Add SHIM and image security database GUID definitions

[Ard Biesheuvel]

On 16 November 2016 at 18:11, David Howells <dhowells@redhat.com> wrote:
> Add the definitions for shim and image security database, both of which
> are used widely in various Linux distros.
>
> Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
> Signed-off-by: David Howells <dhowells@redhat.com>

This patch

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

but I do have a concern that I will explain in reply to the patch that
uses these definitions.

> ---
>
>  include/linux/efi.h |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 2d089487d2da..acfdea8381de 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -592,6 +592,9 @@ void efi_native_runtime_setup(void);
>  #define EFI_MEMORY_ATTRIBUTES_TABLE_GUID       EFI_GUID(0xdcfa911d, 0x26eb, 0x469f,  0xa2, 0x20, 0x38, 0xb7, 0xdc, 0x46, 0x12, 0x20)
>  #define EFI_CONSOLE_OUT_DEVICE_GUID            EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4,  0x9a, 0x46, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
>
> +#define EFI_IMAGE_SECURITY_DATABASE_GUID       EFI_GUID(0xd719b2cb, 0x3d3a, 0x4596, 0xa3, 0xbc, 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
> +#define EFI_SHIM_LOCK_GUID                     EFI_GUID(0x605dab50, 0xe046, 0x4300, 0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
> +
>  /*
>   * This GUID is used to pass to the kernel proper the struct screen_info
>   * structure that was populated by the stub based on the GOP protocol instance
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/9] efi: Add EFI signature data types

[David Howells]

From: Dave Howells <dhowells@redhat.com>

Add the data types that are used for containing hashes, keys and
certificates for cryptographic verification along with their corresponding
type GUIDs.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 include/linux/efi.h |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index acfdea8381de..6fb50bafedc7 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -594,6 +594,9 @@ void efi_native_runtime_setup(void);
 
 #define EFI_IMAGE_SECURITY_DATABASE_GUID	EFI_GUID(0xd719b2cb, 0x3d3a, 0x4596, 0xa3, 0xbc, 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
 #define EFI_SHIM_LOCK_GUID			EFI_GUID(0x605dab50, 0xe046, 0x4300, 0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
+#define EFI_CERT_SHA256_GUID			EFI_GUID(0xc1c41626, 0x504c, 0x4092, 0xac, 0xa9, 0x41, 0xf9, 0x36, 0x93, 0x43, 0x28 )
+#define EFI_CERT_X509_GUID			EFI_GUID(0xa5c059a1, 0x94e4, 0x4aa7, 0x87, 0xb5, 0xab, 0x15, 0x5c, 0x2b, 0xf0, 0x72 )
+#define EFI_CERT_X509_SHA256_GUID		EFI_GUID(0x3bd2a492, 0x96c0, 0x4079, 0xb4, 0x20, 0xfc, 0xf9, 0x8e, 0xf1, 0x03, 0xed );
 
 /*
  * This GUID is used to pass to the kernel proper the struct screen_info
@@ -853,6 +856,27 @@ typedef struct {
 	efi_memory_desc_t entry[0];
 } efi_memory_attributes_table_t;
 
+typedef struct  {
+	efi_guid_t signature_owner;
+	u8 signature_data[];
+} efi_signature_data_t;
+
+typedef struct {
+	efi_guid_t signature_type;
+	u32 signature_list_size;
+	u32 signature_header_size;
+	u32 signature_size;
+	u8 signature_header[];
+	/* efi_signature_data_t signatures[][] */
+} efi_signature_list_t;
+
+typedef u8 efi_sha256_hash_t[32];
+
+typedef struct {
+	efi_sha256_hash_t to_be_signed_hash;
+	efi_time_t time_of_revocation;
+} efi_cert_x509_sha256_t;
+
 /*
  * All runtime access to EFI goes through this structure:
  */

Re: [PATCH 6/9] efi: Add EFI signature data types

[Mat Martineau]

David,

On Wed, 16 Nov 2016, David Howells wrote:

> From: Dave Howells <dhowells@redhat.com>
>
> Add the data types that are used for containing hashes, keys and
> certificates for cryptographic verification along with their corresponding
> type GUIDs.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
> include/linux/efi.h |   24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index acfdea8381de..6fb50bafedc7 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -594,6 +594,9 @@ void efi_native_runtime_setup(void);
>
> #define EFI_IMAGE_SECURITY_DATABASE_GUID	EFI_GUID(0xd719b2cb, 0x3d3a, 0x4596, 0xa3, 0xbc, 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
> #define EFI_SHIM_LOCK_GUID			EFI_GUID(0x605dab50, 0xe046, 0x4300, 0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
> +#define EFI_CERT_SHA256_GUID			EFI_GUID(0xc1c41626, 0x504c, 0x4092, 0xac, 0xa9, 0x41, 0xf9, 0x36, 0x93, 0x43, 0x28 )
> +#define EFI_CERT_X509_GUID			EFI_GUID(0xa5c059a1, 0x94e4, 0x4aa7, 0x87, 0xb5, 0xab, 0x15, 0x5c, 0x2b, 0xf0, 0x72 )
> +#define EFI_CERT_X509_SHA256_GUID		EFI_GUID(0x3bd2a492, 0x96c0, 0x4079, 0xb4, 0x20, 0xfc, 0xf9, 0x8e, 0xf1, 0x03, 0xed );

The trailing ';' on EFI_CERT_X509_SHA256_GUID could trip someone up later. 
As long as you're fixing that, the spaces before the closing parens are 
inconsistent with the other GUID definitions in this file.

> /*
>  * This GUID is used to pass to the kernel proper the struct screen_info
> @@ -853,6 +856,27 @@ typedef struct {
> 	efi_memory_desc_t entry[0];
> } efi_memory_attributes_table_t;
>
> +typedef struct  {
> +	efi_guid_t signature_owner;
> +	u8 signature_data[];
> +} efi_signature_data_t;
> +
> +typedef struct {
> +	efi_guid_t signature_type;
> +	u32 signature_list_size;
> +	u32 signature_header_size;
> +	u32 signature_size;
> +	u8 signature_header[];
> +	/* efi_signature_data_t signatures[][] */
> +} efi_signature_list_t;
> +
> +typedef u8 efi_sha256_hash_t[32];
> +
> +typedef struct {
> +	efi_sha256_hash_t to_be_signed_hash;
> +	efi_time_t time_of_revocation;
> +} efi_cert_x509_sha256_t;
> +
> /*
>  * All runtime access to EFI goes through this structure:
>  */

--
Mat Martineau
Intel OTC

Re: [PATCH 6/9] efi: Add EFI signature data types

[David Howells]

Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:

> The trailing ';' on EFI_CERT_X509_SHA256_GUID could trip someone up later. As
> long as you're fixing that, the spaces before the closing parens are
> inconsistent with the other GUID definitions in this file.

Fixed, thanks.

David

Re: [PATCH 6/9] efi: Add EFI signature data types

[Ard Biesheuvel]

On 17 November 2016 at 09:44, David Howells <dhowells@redhat.com> wrote:
> Mat Martineau <mathew.j.martineau@linux.intel.com> wrote:
>
>> The trailing ';' on EFI_CERT_X509_SHA256_GUID could trip someone up later. As
>> long as you're fixing that, the spaces before the closing parens are
>> inconsistent with the other GUID definitions in this file.
>
> Fixed, thanks.
>

With the fix

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

[PATCH 7/9] efi: Add an EFI signature blob parser

[David Howells]

From: Dave Howells <dhowells@redhat.com>

Add a function to parse an EFI signature blob looking for elements of
interest.  A list is made up of a series of sublists, where all the
elements in a sublist are of the same type, but sublists can be of
different types.

For each sublist encountered, the function pointed to by the
get_handler_for_guid argument is called with the type specifier GUID and
returns either a pointer to a function to handle elements of that type or
NULL if the type is not of interest.

If the sublist is of interest, each element is passed to the handler
function in turn.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 certs/Kconfig       |    8 ++++
 certs/Makefile      |    1 
 certs/efi_parser.c  |  112 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/efi.h |    9 ++++
 4 files changed, 130 insertions(+)
 create mode 100644 certs/efi_parser.c

diff --git a/certs/Kconfig b/certs/Kconfig
index 6ce51ede9e9b..630ae09bbea2 100644
--- a/certs/Kconfig
+++ b/certs/Kconfig
@@ -82,4 +82,12 @@ config SYSTEM_BLACKLIST_HASH_LIST
 	  wrapper to incorporate the list into the kernel.  Each <hash> should
 	  be a string of hex digits.
 
+config EFI_SIGNATURE_LIST_PARSER
+	bool "EFI signature list parser"
+	depends on EFI
+	select X509_CERTIFICATE_PARSER
+	help
+	  This option provides support for parsing EFI signature lists for
+	  X.509 certificates and turning them into keys.
+
 endmenu
diff --git a/certs/Makefile b/certs/Makefile
index 4119bb376ea1..738151ac76af 100644
--- a/certs/Makefile
+++ b/certs/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist_hashes.o
 else
 obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist_nohashes.o
 endif
+obj-$(CONFIG_EFI_SIGNATURE_LIST_PARSER) += efi_parser.o
 
 ifeq ($(CONFIG_SYSTEM_TRUSTED_KEYRING),y)
 
diff --git a/certs/efi_parser.c b/certs/efi_parser.c
new file mode 100644
index 000000000000..4e396f98f5c7
--- /dev/null
+++ b/certs/efi_parser.c
@@ -0,0 +1,112 @@
+/* EFI signature/key/certificate list parser
+ *
+ * Copyright (C) 2012, 2016 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowells@redhat.com)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public Licence
+ * as published by the Free Software Foundation; either version
+ * 2 of the Licence, or (at your option) any later version.
+ */
+
+#define pr_fmt(fmt) "EFI: "fmt
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/err.h>
+#include <linux/efi.h>
+
+/**
+ * parse_efi_signature_list - Parse an EFI signature list for certificates
+ * @source: The source of the key
+ * @data: The data blob to parse
+ * @size: The size of the data blob
+ * @get_handler_for_guid: Get the handler func for the sig type (or NULL)
+ *
+ * Parse an EFI signature list looking for elements of interest.  A list is
+ * made up of a series of sublists, where all the elements in a sublist are of
+ * the same type, but sublists can be of different types.
+ *
+ * For each sublist encountered, the @get_handler_for_guid function is called
+ * with the type specifier GUID and returns either a pointer to a function to
+ * handle elements of that type or NULL if the type is not of interest.
+ *
+ * If the sublist is of interest, each element is passed to the handler
+ * function in turn.
+ *
+ * Error EBADMSG is returned if the list doesn't parse correctly and 0 is
+ * returned if the list was parsed correctly.  No error can be returned from
+ * the @get_handler_for_guid function or the element handler function it
+ * returns.
+ */
+int __init parse_efi_signature_list(
+	const char *source,
+	const void *data, size_t size,
+	efi_element_handler_t (*get_handler_for_guid)(const efi_guid_t *))
+{
+	efi_element_handler_t handler;
+	unsigned offs = 0;
+
+	pr_devel("-->%s(,%zu)\n", __func__, size);
+
+	while (size > 0) {
+		const efi_signature_data_t *elem;
+		efi_signature_list_t list;
+		size_t lsize, esize, hsize, elsize;
+
+		if (size < sizeof(list))
+			return -EBADMSG;
+
+		memcpy(&list, data, sizeof(list));
+		pr_devel("LIST[%04x] guid=%pUl ls=%x hs=%x ss=%x\n",
+			 offs,
+			 list.signature_type.b, list.signature_list_size,
+			 list.signature_header_size, list.signature_size);
+
+		lsize = list.signature_list_size;
+		hsize = list.signature_header_size;
+		esize = list.signature_size;
+		elsize = lsize - sizeof(list) - hsize;
+
+		if (lsize > size) {
+			pr_devel("<--%s() = -EBADMSG [overrun @%x]\n",
+				 __func__, offs);
+			return -EBADMSG;
+		}
+
+		if (lsize < sizeof(list) ||
+		    lsize - sizeof(list) < hsize ||
+		    esize < sizeof(*elem) ||
+		    elsize < esize ||
+		    elsize % esize != 0) {
+			pr_devel("- bad size combo @%x\n", offs);
+			return -EBADMSG;
+		}
+
+		handler = get_handler_for_guid(&list.signature_type);
+		if (!handler) {
+			data += lsize;
+			size -= lsize;
+			offs += lsize;
+			continue;
+		}
+
+		data += sizeof(list) + hsize;
+		size -= sizeof(list) + hsize;
+		offs += sizeof(list) + hsize;
+
+		for (; elsize > 0; elsize -= esize) {
+			elem = data;
+
+			pr_devel("ELEM[%04x]\n", offs);
+			handler(source,
+				&elem->signature_data,
+				esize - sizeof(*elem));
+
+			data += esize;
+			size -= esize;
+			offs += esize;
+		}
+	}
+
+	return 0;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 6fb50bafedc7..11372fb8784c 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1032,6 +1032,15 @@ extern int efi_memattr_apply_permissions(struct mm_struct *mm,
 char * __init efi_md_typeattr_format(char *buf, size_t size,
 				     const efi_memory_desc_t *md);
 
+
+typedef void (*efi_element_handler_t)(const char *source,
+				      const void *element_data,
+				      size_t element_size);
+extern int __init parse_efi_signature_list(
+	const char *source,
+	const void *data, size_t size,
+	efi_element_handler_t (*get_handler_for_guid)(const efi_guid_t *));
+
 /**
  * efi_range_is_wc - check the WC bit on an address range
  * @start: starting kvirt address

[PATCH 8/9] MODSIGN: Import certificates from UEFI Secure Boot

[David Howells]

From: Josh Boyer <jwboyer@fedoraproject.org>

Secure Boot stores a list of allowed certificates in the 'db' variable.
This imports those certificates into the system trusted keyring.  This
allows for a third party signing certificate to be used in conjunction
with signed modules.  By importing the public certificate into the 'db'
variable, a user can allow a module signed with that certificate to
load.  The shim UEFI bootloader has a similar certificate list stored
in the 'MokListRT' variable.  We import those as well.

Secure Boot also maintains a list of disallowed certificates in the 'dbx'
variable.  We load those certificates into the newly introduced system
blacklist keyring and forbid any module signed with those from loading and
forbid the use within the kernel of any key with a matching hash.

This facility is enabled by setting CONFIG_LOAD_UEFI_KEYS.

Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 certs/Kconfig     |   16 +++++
 certs/Makefile    |    4 +
 certs/load_uefi.c |  168 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 188 insertions(+)
 create mode 100644 certs/load_uefi.c

diff --git a/certs/Kconfig b/certs/Kconfig
index 630ae09bbea2..3b09c5edc1a2 100644
--- a/certs/Kconfig
+++ b/certs/Kconfig
@@ -90,4 +90,20 @@ config EFI_SIGNATURE_LIST_PARSER
 	  This option provides support for parsing EFI signature lists for
 	  X.509 certificates and turning them into keys.
 
+config LOAD_UEFI_KEYS
+	bool "Load certs and blacklist from UEFI db for module checking"
+	depends on SYSTEM_BLACKLIST_KEYRING
+	depends on SECONDARY_TRUSTED_KEYRING
+	depends on EFI
+	select EFI_SIGNATURE_LIST_PARSER
+	help
+	  If the kernel is booted in secure boot mode, this option will cause
+	  the kernel to load the certificates from the UEFI db and MokListRT
+	  into the secondary trusted keyring.  It will also load any X.509
+	  SHA256 hashes in the dbx list into the blacklist.
+
+	  The effect of this is that, if the kernel is booted in secure boot
+	  mode, modules signed with UEFI-stored keys will be permitted to be
+	  loaded and keys that match the blacklist will be rejected.
+
 endmenu
diff --git a/certs/Makefile b/certs/Makefile
index 738151ac76af..a5e057af98a3 100644
--- a/certs/Makefile
+++ b/certs/Makefile
@@ -11,6 +11,10 @@ obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist_nohashes.o
 endif
 obj-$(CONFIG_EFI_SIGNATURE_LIST_PARSER) += efi_parser.o
 
+obj-$(CONFIG_LOAD_UEFI_KEYS) += load_uefi.o
+$(obj)/load_uefi.o: KBUILD_CFLAGS += -fshort-wchar
+
+
 ifeq ($(CONFIG_SYSTEM_TRUSTED_KEYRING),y)
 
 $(eval $(call config_filename,SYSTEM_TRUSTED_KEYS))
diff --git a/certs/load_uefi.c b/certs/load_uefi.c
new file mode 100644
index 000000000000..b44e464c3ff4
--- /dev/null
+++ b/certs/load_uefi.c
@@ -0,0 +1,168 @@
+#include <linux/kernel.h>
+#include <linux/sched.h>
+#include <linux/cred.h>
+#include <linux/err.h>
+#include <linux/efi.h>
+#include <linux/slab.h>
+#include <keys/asymmetric-type.h>
+#include <keys/system_keyring.h>
+#include "internal.h"
+
+static __initdata efi_guid_t efi_cert_x509_guid = EFI_CERT_X509_GUID;
+static __initdata efi_guid_t efi_cert_x509_sha256_guid = EFI_CERT_X509_SHA256_GUID;
+static __initdata efi_guid_t efi_cert_sha256_guid = EFI_CERT_SHA256_GUID;
+
+/*
+ * Get a certificate list blob from the named EFI variable.
+ */
+static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
+				  unsigned long *size)
+{
+	efi_status_t status;
+	unsigned long lsize = 4;
+	unsigned long tmpdb[4];
+	void *db;
+
+	status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
+	if (status != EFI_BUFFER_TOO_SMALL) {
+		pr_err("Couldn't get size: 0x%lx\n", status);
+		return NULL;
+	}
+
+	db = kmalloc(lsize, GFP_KERNEL);
+	if (!db) {
+		pr_err("Couldn't allocate memory for uefi cert list\n");
+		return NULL;
+	}
+
+	status = efi.get_variable(name, guid, NULL, &lsize, db);
+	if (status != EFI_SUCCESS) {
+		kfree(db);
+		pr_err("Error reading db var: 0x%lx\n", status);
+		return NULL;
+	}
+
+	*size = lsize;
+	return db;
+}
+
+/*
+ * Blacklist an X509 TBS hash.
+ */
+static __init void uefi_blacklist_x509_tbs(const char *source,
+					   const void *data, size_t len)
+{
+	char *hash, *p;
+
+	hash = kmalloc(4 + len * 2 + 1, GFP_KERNEL);
+	if (!hash)
+		return;
+	p = memcpy(hash, "tbs:", 4);
+	p += 4;
+	bin2hex(p, data, len);
+	p += len * 2;
+	*p = 0;
+
+	mark_hash_blacklisted(hash);
+	kfree(hash);
+}
+
+/*
+ * Blacklist the hash of an executable.
+ */
+static __init void uefi_blacklist_binary(const char *source,
+					 const void *data, size_t len)
+{
+	char *hash, *p;
+
+	hash = kmalloc(4 + len * 2 + 1, GFP_KERNEL);
+	if (!hash)
+		return;
+	p = memcpy(hash, "bin:", 4);
+	p += 4;
+	bin2hex(p, data, len);
+	p += len * 2;
+	*p = 0;
+
+	mark_hash_blacklisted(hash);
+	kfree(hash);
+}
+
+/*
+ * Return the appropriate handler for particular signature list types found in
+ * the UEFI db and MokListRT tables.
+ */
+static __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
+{
+	if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
+		return add_trusted_secondary_key;
+	return 0;
+}
+
+/*
+ * Return the appropriate handler for particular signature list types found in
+ * the UEFI dbx and MokListXRT tables.
+ */
+static __init efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_type)
+{
+	if (efi_guidcmp(*sig_type, efi_cert_x509_sha256_guid) == 0)
+		return uefi_blacklist_x509_tbs;
+	if (efi_guidcmp(*sig_type, efi_cert_sha256_guid) == 0)
+		return uefi_blacklist_binary;
+	return 0;
+}
+
+/*
+ * Load the certs contained in the UEFI databases
+ */
+static int __init load_uefi_certs(void)
+{
+	efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID;
+	efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
+	void *db = NULL, *dbx = NULL, *mok = NULL;
+	unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
+	int rc = 0;
+
+	if (!efi.get_variable)
+		return false;
+
+	/* Get db, MokListRT, and dbx.  They might not exist, so it isn't
+	 * an error if we can't get them.
+	 */
+	db = get_cert_list(L"db", &secure_var, &dbsize);
+	if (!db) {
+		pr_err("MODSIGN: Couldn't get UEFI db list\n");
+	} else {
+		rc = parse_efi_signature_list("UEFI:db",
+					      db, dbsize, get_handler_for_db);
+		if (rc)
+			pr_err("Couldn't parse db signatures: %d\n", rc);
+		kfree(db);
+	}
+
+	mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
+	if (!mok) {
+		pr_info("MODSIGN: Couldn't get UEFI MokListRT\n");
+	} else {
+		rc = parse_efi_signature_list("UEFI:MokListRT",
+					      mok, moksize, get_handler_for_db);
+		if (rc)
+			pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
+		kfree(mok);
+	}
+
+	dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
+	if (!dbx) {
+		pr_info("MODSIGN: Couldn't get UEFI dbx list\n");
+	} else {
+		rc = parse_efi_signature_list("UEFI:dbx",
+					      dbx, dbxsize,
+					      get_handler_for_dbx);
+		if (rc)
+			pr_err("Couldn't parse dbx signatures: %d\n", rc);
+		kfree(dbx);
+	}
+
+	return rc;
+}
+late_initcall(load_uefi_certs);

Re: [PATCH 8/9] MODSIGN: Import certificates from UEFI Secure Boot

[Ard Biesheuvel]

On 16 November 2016 at 18:11, David Howells <dhowells@redhat.com> wrote:
> From: Josh Boyer <jwboyer@fedoraproject.org>
>
> Secure Boot stores a list of allowed certificates in the 'db' variable.
> This imports those certificates into the system trusted keyring.  This
> allows for a third party signing certificate to be used in conjunction
> with signed modules.  By importing the public certificate into the 'db'
> variable, a user can allow a module signed with that certificate to
> load.  The shim UEFI bootloader has a similar certificate list stored
> in the 'MokListRT' variable.  We import those as well.
>

This sounds like a bad idea to me. For the standard databases like db
and dbx, we can rely on the firmware to ensure that they are what you
expect. For MokListRt, not so much: anyone with sufficient
capabilities can generate such a variable from userland, and not every
arch/distro combo will be using shim and/or mokmanager. (The debates
are still ongoing, but my position is that there is no need for shim
at all on ARM given that the M$ problem only exists on x86)

> Secure Boot also maintains a list of disallowed certificates in the 'dbx'
> variable.  We load those certificates into the newly introduced system
> blacklist keyring and forbid any module signed with those from loading and
> forbid the use within the kernel of any key with a matching hash.
>
> This facility is enabled by setting CONFIG_LOAD_UEFI_KEYS.
>
> Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
>  certs/Kconfig     |   16 +++++
>  certs/Makefile    |    4 +
>  certs/load_uefi.c |  168 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 188 insertions(+)
>  create mode 100644 certs/load_uefi.c
>
> diff --git a/certs/Kconfig b/certs/Kconfig
> index 630ae09bbea2..3b09c5edc1a2 100644
> --- a/certs/Kconfig
> +++ b/certs/Kconfig
> @@ -90,4 +90,20 @@ config EFI_SIGNATURE_LIST_PARSER
>           This option provides support for parsing EFI signature lists for
>           X.509 certificates and turning them into keys.
>
> +config LOAD_UEFI_KEYS
> +       bool "Load certs and blacklist from UEFI db for module checking"
> +       depends on SYSTEM_BLACKLIST_KEYRING
> +       depends on SECONDARY_TRUSTED_KEYRING
> +       depends on EFI
> +       select EFI_SIGNATURE_LIST_PARSER
> +       help
> +         If the kernel is booted in secure boot mode, this option will cause
> +         the kernel to load the certificates from the UEFI db and MokListRT
> +         into the secondary trusted keyring.  It will also load any X.509
> +         SHA256 hashes in the dbx list into the blacklist.
> +
> +         The effect of this is that, if the kernel is booted in secure boot
> +         mode, modules signed with UEFI-stored keys will be permitted to be
> +         loaded and keys that match the blacklist will be rejected.
> +
>  endmenu
> diff --git a/certs/Makefile b/certs/Makefile
> index 738151ac76af..a5e057af98a3 100644
> --- a/certs/Makefile
> +++ b/certs/Makefile
> @@ -11,6 +11,10 @@ obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist_nohashes.o
>  endif
>  obj-$(CONFIG_EFI_SIGNATURE_LIST_PARSER) += efi_parser.o
>
> +obj-$(CONFIG_LOAD_UEFI_KEYS) += load_uefi.o
> +$(obj)/load_uefi.o: KBUILD_CFLAGS += -fshort-wchar
> +
> +
>  ifeq ($(CONFIG_SYSTEM_TRUSTED_KEYRING),y)
>
>  $(eval $(call config_filename,SYSTEM_TRUSTED_KEYS))
> diff --git a/certs/load_uefi.c b/certs/load_uefi.c
> new file mode 100644
> index 000000000000..b44e464c3ff4
> --- /dev/null
> +++ b/certs/load_uefi.c
> @@ -0,0 +1,168 @@
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/cred.h>
> +#include <linux/err.h>
> +#include <linux/efi.h>
> +#include <linux/slab.h>
> +#include <keys/asymmetric-type.h>
> +#include <keys/system_keyring.h>
> +#include "internal.h"
> +
> +static __initdata efi_guid_t efi_cert_x509_guid = EFI_CERT_X509_GUID;
> +static __initdata efi_guid_t efi_cert_x509_sha256_guid = EFI_CERT_X509_SHA256_GUID;
> +static __initdata efi_guid_t efi_cert_sha256_guid = EFI_CERT_SHA256_GUID;
> +
> +/*
> + * Get a certificate list blob from the named EFI variable.
> + */
> +static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> +                                 unsigned long *size)
> +{
> +       efi_status_t status;
> +       unsigned long lsize = 4;
> +       unsigned long tmpdb[4];
> +       void *db;
> +
> +       status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
> +       if (status != EFI_BUFFER_TOO_SMALL) {
> +               pr_err("Couldn't get size: 0x%lx\n", status);
> +               return NULL;
> +       }
> +
> +       db = kmalloc(lsize, GFP_KERNEL);
> +       if (!db) {
> +               pr_err("Couldn't allocate memory for uefi cert list\n");
> +               return NULL;
> +       }
> +
> +       status = efi.get_variable(name, guid, NULL, &lsize, db);
> +       if (status != EFI_SUCCESS) {
> +               kfree(db);
> +               pr_err("Error reading db var: 0x%lx\n", status);
> +               return NULL;
> +       }
> +
> +       *size = lsize;
> +       return db;
> +}
> +
> +/*
> + * Blacklist an X509 TBS hash.
> + */
> +static __init void uefi_blacklist_x509_tbs(const char *source,
> +                                          const void *data, size_t len)
> +{
> +       char *hash, *p;
> +
> +       hash = kmalloc(4 + len * 2 + 1, GFP_KERNEL);
> +       if (!hash)
> +               return;
> +       p = memcpy(hash, "tbs:", 4);
> +       p += 4;
> +       bin2hex(p, data, len);
> +       p += len * 2;
> +       *p = 0;
> +
> +       mark_hash_blacklisted(hash);
> +       kfree(hash);
> +}
> +
> +/*
> + * Blacklist the hash of an executable.
> + */
> +static __init void uefi_blacklist_binary(const char *source,
> +                                        const void *data, size_t len)
> +{
> +       char *hash, *p;
> +
> +       hash = kmalloc(4 + len * 2 + 1, GFP_KERNEL);
> +       if (!hash)
> +               return;
> +       p = memcpy(hash, "bin:", 4);
> +       p += 4;
> +       bin2hex(p, data, len);
> +       p += len * 2;
> +       *p = 0;
> +
> +       mark_hash_blacklisted(hash);
> +       kfree(hash);
> +}
> +
> +/*
> + * Return the appropriate handler for particular signature list types found in
> + * the UEFI db and MokListRT tables.
> + */
> +static __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
> +{
> +       if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
> +               return add_trusted_secondary_key;
> +       return 0;
> +}
> +
> +/*
> + * Return the appropriate handler for particular signature list types found in
> + * the UEFI dbx and MokListXRT tables.
> + */
> +static __init efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_type)
> +{
> +       if (efi_guidcmp(*sig_type, efi_cert_x509_sha256_guid) == 0)
> +               return uefi_blacklist_x509_tbs;
> +       if (efi_guidcmp(*sig_type, efi_cert_sha256_guid) == 0)
> +               return uefi_blacklist_binary;
> +       return 0;
> +}
> +
> +/*
> + * Load the certs contained in the UEFI databases
> + */
> +static int __init load_uefi_certs(void)
> +{
> +       efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID;
> +       efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
> +       void *db = NULL, *dbx = NULL, *mok = NULL;
> +       unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
> +       int rc = 0;
> +
> +       if (!efi.get_variable)
> +               return false;
> +
> +       /* Get db, MokListRT, and dbx.  They might not exist, so it isn't
> +        * an error if we can't get them.
> +        */
> +       db = get_cert_list(L"db", &secure_var, &dbsize);
> +       if (!db) {
> +               pr_err("MODSIGN: Couldn't get UEFI db list\n");
> +       } else {
> +               rc = parse_efi_signature_list("UEFI:db",
> +                                             db, dbsize, get_handler_for_db);
> +               if (rc)
> +                       pr_err("Couldn't parse db signatures: %d\n", rc);
> +               kfree(db);
> +       }
> +
> +       mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
> +       if (!mok) {
> +               pr_info("MODSIGN: Couldn't get UEFI MokListRT\n");
> +       } else {
> +               rc = parse_efi_signature_list("UEFI:MokListRT",
> +                                             mok, moksize, get_handler_for_db);
> +               if (rc)
> +                       pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
> +               kfree(mok);
> +       }
> +
> +       dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
> +       if (!dbx) {
> +               pr_info("MODSIGN: Couldn't get UEFI dbx list\n");
> +       } else {
> +               rc = parse_efi_signature_list("UEFI:dbx",
> +                                             dbx, dbxsize,
> +                                             get_handler_for_dbx);
> +               if (rc)
> +                       pr_err("Couldn't parse dbx signatures: %d\n", rc);
> +               kfree(dbx);
> +       }
> +
> +       return rc;
> +}
> +late_initcall(load_uefi_certs);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9] MODSIGN: Import certificates from UEFI Secure Boot

[Josh Boyer]

On Mon, Nov 21, 2016 at 11:16 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 16 November 2016 at 18:11, David Howells <dhowells@redhat.com> wrote:
>> From: Josh Boyer <jwboyer@fedoraproject.org>
>>
>> Secure Boot stores a list of allowed certificates in the 'db' variable.
>> This imports those certificates into the system trusted keyring.  This
>> allows for a third party signing certificate to be used in conjunction
>> with signed modules.  By importing the public certificate into the 'db'
>> variable, a user can allow a module signed with that certificate to
>> load.  The shim UEFI bootloader has a similar certificate list stored
>> in the 'MokListRT' variable.  We import those as well.
>>
>
> This sounds like a bad idea to me. For the standard databases like db
> and dbx, we can rely on the firmware to ensure that they are what you
> expect. For MokListRt, not so much: anyone with sufficient
> capabilities can generate such a variable from userland, and not every
> arch/distro combo will be using shim and/or mokmanager. (The debates
> are still ongoing, but my position is that there is no need for shim
> at all on ARM given that the M$ problem only exists on x86)

In order for MokListRT to be modified, the user has to have physical
access and boot into Mok and complete the enrollment.  That is no
different than simply enrolling in db or dbx.  I don't see a
difference in security under the thread model that Secure Boot is
attempting to protect against.

josh

>
>> Secure Boot also maintains a list of disallowed certificates in the 'dbx'
>> variable.  We load those certificates into the newly introduced system
>> blacklist keyring and forbid any module signed with those from loading and
>> forbid the use within the kernel of any key with a matching hash.
>>
>> This facility is enabled by setting CONFIG_LOAD_UEFI_KEYS.
>>
>> Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
>> Signed-off-by: David Howells <dhowells@redhat.com>
>> ---
>>
>>  certs/Kconfig     |   16 +++++
>>  certs/Makefile    |    4 +
>>  certs/load_uefi.c |  168 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 188 insertions(+)
>>  create mode 100644 certs/load_uefi.c
>>
>> diff --git a/certs/Kconfig b/certs/Kconfig
>> index 630ae09bbea2..3b09c5edc1a2 100644
>> --- a/certs/Kconfig
>> +++ b/certs/Kconfig
>> @@ -90,4 +90,20 @@ config EFI_SIGNATURE_LIST_PARSER
>>           This option provides support for parsing EFI signature lists for
>>           X.509 certificates and turning them into keys.
>>
>> +config LOAD_UEFI_KEYS
>> +       bool "Load certs and blacklist from UEFI db for module checking"
>> +       depends on SYSTEM_BLACKLIST_KEYRING
>> +       depends on SECONDARY_TRUSTED_KEYRING
>> +       depends on EFI
>> +       select EFI_SIGNATURE_LIST_PARSER
>> +       help
>> +         If the kernel is booted in secure boot mode, this option will cause
>> +         the kernel to load the certificates from the UEFI db and MokListRT
>> +         into the secondary trusted keyring.  It will also load any X.509
>> +         SHA256 hashes in the dbx list into the blacklist.
>> +
>> +         The effect of this is that, if the kernel is booted in secure boot
>> +         mode, modules signed with UEFI-stored keys will be permitted to be
>> +         loaded and keys that match the blacklist will be rejected.
>> +
>>  endmenu
>> diff --git a/certs/Makefile b/certs/Makefile
>> index 738151ac76af..a5e057af98a3 100644
>> --- a/certs/Makefile
>> +++ b/certs/Makefile
>> @@ -11,6 +11,10 @@ obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist_nohashes.o
>>  endif
>>  obj-$(CONFIG_EFI_SIGNATURE_LIST_PARSER) += efi_parser.o
>>
>> +obj-$(CONFIG_LOAD_UEFI_KEYS) += load_uefi.o
>> +$(obj)/load_uefi.o: KBUILD_CFLAGS += -fshort-wchar
>> +
>> +
>>  ifeq ($(CONFIG_SYSTEM_TRUSTED_KEYRING),y)
>>
>>  $(eval $(call config_filename,SYSTEM_TRUSTED_KEYS))
>> diff --git a/certs/load_uefi.c b/certs/load_uefi.c
>> new file mode 100644
>> index 000000000000..b44e464c3ff4
>> --- /dev/null
>> +++ b/certs/load_uefi.c
>> @@ -0,0 +1,168 @@
>> +#include <linux/kernel.h>
>> +#include <linux/sched.h>
>> +#include <linux/cred.h>
>> +#include <linux/err.h>
>> +#include <linux/efi.h>
>> +#include <linux/slab.h>
>> +#include <keys/asymmetric-type.h>
>> +#include <keys/system_keyring.h>
>> +#include "internal.h"
>> +
>> +static __initdata efi_guid_t efi_cert_x509_guid = EFI_CERT_X509_GUID;
>> +static __initdata efi_guid_t efi_cert_x509_sha256_guid = EFI_CERT_X509_SHA256_GUID;
>> +static __initdata efi_guid_t efi_cert_sha256_guid = EFI_CERT_SHA256_GUID;
>> +
>> +/*
>> + * Get a certificate list blob from the named EFI variable.
>> + */
>> +static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
>> +                                 unsigned long *size)
>> +{
>> +       efi_status_t status;
>> +       unsigned long lsize = 4;
>> +       unsigned long tmpdb[4];
>> +       void *db;
>> +
>> +       status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
>> +       if (status != EFI_BUFFER_TOO_SMALL) {
>> +               pr_err("Couldn't get size: 0x%lx\n", status);
>> +               return NULL;
>> +       }
>> +
>> +       db = kmalloc(lsize, GFP_KERNEL);
>> +       if (!db) {
>> +               pr_err("Couldn't allocate memory for uefi cert list\n");
>> +               return NULL;
>> +       }
>> +
>> +       status = efi.get_variable(name, guid, NULL, &lsize, db);
>> +       if (status != EFI_SUCCESS) {
>> +               kfree(db);
>> +               pr_err("Error reading db var: 0x%lx\n", status);
>> +               return NULL;
>> +       }
>> +
>> +       *size = lsize;
>> +       return db;
>> +}
>> +
>> +/*
>> + * Blacklist an X509 TBS hash.
>> + */
>> +static __init void uefi_blacklist_x509_tbs(const char *source,
>> +                                          const void *data, size_t len)
>> +{
>> +       char *hash, *p;
>> +
>> +       hash = kmalloc(4 + len * 2 + 1, GFP_KERNEL);
>> +       if (!hash)
>> +               return;
>> +       p = memcpy(hash, "tbs:", 4);
>> +       p += 4;
>> +       bin2hex(p, data, len);
>> +       p += len * 2;
>> +       *p = 0;
>> +
>> +       mark_hash_blacklisted(hash);
>> +       kfree(hash);
>> +}
>> +
>> +/*
>> + * Blacklist the hash of an executable.
>> + */
>> +static __init void uefi_blacklist_binary(const char *source,
>> +                                        const void *data, size_t len)
>> +{
>> +       char *hash, *p;
>> +
>> +       hash = kmalloc(4 + len * 2 + 1, GFP_KERNEL);
>> +       if (!hash)
>> +               return;
>> +       p = memcpy(hash, "bin:", 4);
>> +       p += 4;
>> +       bin2hex(p, data, len);
>> +       p += len * 2;
>> +       *p = 0;
>> +
>> +       mark_hash_blacklisted(hash);
>> +       kfree(hash);
>> +}
>> +
>> +/*
>> + * Return the appropriate handler for particular signature list types found in
>> + * the UEFI db and MokListRT tables.
>> + */
>> +static __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
>> +{
>> +       if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
>> +               return add_trusted_secondary_key;
>> +       return 0;
>> +}
>> +
>> +/*
>> + * Return the appropriate handler for particular signature list types found in
>> + * the UEFI dbx and MokListXRT tables.
>> + */
>> +static __init efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_type)
>> +{
>> +       if (efi_guidcmp(*sig_type, efi_cert_x509_sha256_guid) == 0)
>> +               return uefi_blacklist_x509_tbs;
>> +       if (efi_guidcmp(*sig_type, efi_cert_sha256_guid) == 0)
>> +               return uefi_blacklist_binary;
>> +       return 0;
>> +}
>> +
>> +/*
>> + * Load the certs contained in the UEFI databases
>> + */
>> +static int __init load_uefi_certs(void)
>> +{
>> +       efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID;
>> +       efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
>> +       void *db = NULL, *dbx = NULL, *mok = NULL;
>> +       unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
>> +       int rc = 0;
>> +
>> +       if (!efi.get_variable)
>> +               return false;
>> +
>> +       /* Get db, MokListRT, and dbx.  They might not exist, so it isn't
>> +        * an error if we can't get them.
>> +        */
>> +       db = get_cert_list(L"db", &secure_var, &dbsize);
>> +       if (!db) {
>> +               pr_err("MODSIGN: Couldn't get UEFI db list\n");
>> +       } else {
>> +               rc = parse_efi_signature_list("UEFI:db",
>> +                                             db, dbsize, get_handler_for_db);
>> +               if (rc)
>> +                       pr_err("Couldn't parse db signatures: %d\n", rc);
>> +               kfree(db);
>> +       }
>> +
>> +       mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
>> +       if (!mok) {
>> +               pr_info("MODSIGN: Couldn't get UEFI MokListRT\n");
>> +       } else {
>> +               rc = parse_efi_signature_list("UEFI:MokListRT",
>> +                                             mok, moksize, get_handler_for_db);
>> +               if (rc)
>> +                       pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
>> +               kfree(mok);
>> +       }
>> +
>> +       dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
>> +       if (!dbx) {
>> +               pr_info("MODSIGN: Couldn't get UEFI dbx list\n");
>> +       } else {
>> +               rc = parse_efi_signature_list("UEFI:dbx",
>> +                                             dbx, dbxsize,
>> +                                             get_handler_for_dbx);
>> +               if (rc)
>> +                       pr_err("Couldn't parse dbx signatures: %d\n", rc);
>> +               kfree(dbx);
>> +       }
>> +
>> +       return rc;
>> +}
>> +late_initcall(load_uefi_certs);
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 8/9] MODSIGN: Import certificates from UEFI Secure Boot

[James Bottomley]

On Mon, 2016-11-21 at 11:25 -0500, Josh Boyer wrote:
> On Mon, Nov 21, 2016 at 11:16 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> > On 16 November 2016 at 18:11, David Howells <dhowells@redhat.com>
> > wrote:
> > > From: Josh Boyer <jwboyer@fedoraproject.org>
> > > 
> > > Secure Boot stores a list of allowed certificates in the 'db' 
> > > variable. This imports those certificates into the system trusted 
> > > keyring.   This allows for a third party signing certificate to 
> > > be used in conjunction with signed modules.  By importing the 
> > > public certificate into the 'db' variable, a user can allow a 
> > > module signed with that certificate to load.  The shim UEFI 
> > > bootloader has a similar certificate list stored in the
> > > 'MokListRT' variable.  We import those as well.
> > > 
> > 
> > This sounds like a bad idea to me. For the standard databases like 
> > db and dbx, we can rely on the firmware to ensure that they are 
> > what you expect. For MokListRt, not so much: anyone with sufficient
> > capabilities can generate such a variable from userland, and not 
> > every arch/distro combo will be using shim and/or mokmanager. (The
> > debates are still ongoing, but my position is that there is no need 
> > for shim at all on ARM given that the M$ problem only exists on
> > x86)
> 
> In order for MokListRT to be modified, the user has to have physical
> access and boot into Mok and complete the enrollment.  That is no
> different than simply enrolling in db or dbx.  I don't see a
> difference in security under the thread model that Secure Boot is
> attempting to protect against.

Isn't a potential attack to write to MokListRT and then force a kexec? 
 That means shim doesn't validate the variable yet you treat it as
though it has been validated.

James

Re: [PATCH 8/9] MODSIGN: Import certificates from UEFI Secure Boot

[James Bottomley]

On Mon, 2016-11-21 at 16:16 +0000, Ard Biesheuvel wrote:
> On 16 November 2016 at 18:11, David Howells <dhowells@redhat.com>
> wrote:
> > From: Josh Boyer <jwboyer@fedoraproject.org>
> > 
> > Secure Boot stores a list of allowed certificates in the 'db' 
> > variable. This imports those certificates into the system trusted 
> > keyring.   This allows for a third party signing certificate to be 
> > used in conjunction with signed modules.  By importing the public 
> > certificate into the 'db' variable, a user can allow a module 
> > signed with that certificate to load.  The shim UEFI bootloader has 
> > a similar certificate list stored in the 'MokListRT' variable.  We
> > import those as well.
> > 
> 
> This sounds like a bad idea to me. For the standard databases like db
> and dbx, we can rely on the firmware to ensure that they are what you
> expect.

Actually, I think it's a bad idea for the opposite reason: Shim
explicitly pivots the root of trust away from the db keys to its own
Moklist keys.  We have no choice and are forced to trust db for the
secure boot part, but once we're in the kernel proper, I'd argue that
we would only want to trust the pivoted root, i.e. Moklist.

Trusting both could generate unwanted consequences, like pressure on
Microsoft to sign modules or worse, pressure on OEMs to include module
keys or hashes ... or worst of all OEMs signing external modules.

>  For MokListRt, not so much: anyone with sufficient
> capabilities can generate such a variable from userland, and not 
> every arch/distro combo will be using shim and/or mokmanager. (The 
> debates are still ongoing, but my position is that there is no need 
> for shim at all on ARM given that the M$ problem only exists on x86)

OK, so on this point, I'm already not using Shim on my x86 box. 
 However, what you find if you're using grub is that because grub
doesn't do signature verification, you still have to use the shim
protocol callout, so you need something between UEFI and grub to load
at least this protocol.  I suppose this would go away once we can
persuade grub to verify signatures.

James

Re: [PATCH 8/9] MODSIGN: Import certificates from UEFI Secure Boot

[James Bottomley]

On Thu, 2016-11-24 at 11:17 -0800, James Bottomley wrote:
> On Mon, 2016-11-21 at 16:16 +0000, Ard Biesheuvel wrote:
> > On 16 November 2016 at 18:11, David Howells <dhowells@redhat.com>
> > wrote:
> > > From: Josh Boyer <jwboyer@fedoraproject.org>
> > > 
> > > Secure Boot stores a list of allowed certificates in the 'db' 
> > > variable. This imports those certificates into the system trusted
> > > keyring.   This allows for a third party signing certificate to 
> > > be used in conjunction with signed modules.  By importing the 
> > > public certificate into the 'db' variable, a user can allow a 
> > > module signed with that certificate to load.  The shim UEFI 
> > > bootloader has a similar certificate list stored in the 
> > > 'MokListRT' variable.   We import those as well.
> > > 
> > 
> > This sounds like a bad idea to me. For the standard databases like 
> > db and dbx, we can rely on the firmware to ensure that they are 
> > what you expect.
> 
> Actually, I think it's a bad idea for the opposite reason: Shim
> explicitly pivots the root of trust away from the db keys to its own
> Moklist keys.  We have no choice and are forced to trust db for the
> secure boot part, but once we're in the kernel proper, I'd argue that
> we would only want to trust the pivoted root, i.e. Moklist.
> 
> Trusting both could generate unwanted consequences, like pressure on
> Microsoft to sign modules or worse, pressure on OEMs to include 
> module keys or hashes ... or worst of all OEMs signing external
> modules.
> 
> >  For MokListRt, not so much: anyone with sufficient
> > capabilities can generate such a variable from userland, and not 
> > every arch/distro combo will be using shim and/or mokmanager. (The 
> > debates are still ongoing, but my position is that there is no need
> > for shim at all on ARM given that the M$ problem only exists on
> > x86)
> 
> OK, so on this point, I'm already not using Shim on my x86 box. 
>  However, what you find if you're using grub is that because grub
> doesn't do signature verification, you still have to use the shim
> protocol callout, so you need something between UEFI and grub to load
> at least this protocol.  I suppose this would go away once we can
> persuade grub to verify signatures.

Hm, that got crickets.

Let me propose an alternative mechanism then.

My problem is that although I am forced to trust the secure boot keys
for the UEFI security boundary, I don't necessarily want to trust them
for signing things for my kernel, so I want to pivot (or at
leastselectively weed out) keys.  Shim already has this concept
partially with MokIgnoreDB.

For the purposes of the kernel, I think we simply need a variable, lets
call it MokKernelCerts, that gives the list of certificates to import
into the kernel keyring.  I think this variable should be BS NV only
(not RT) meaning we have to collect it before ExitBootServices().  The
reason for this is to ensure it's populated by a trusted entity within
the UEFI secure boot boundary.  This will cause a kexec problem, so we
might have to relax this and use a RT shadow as we already do for
MokList.  The idea is that we populate the kernel certificates only
from this variable, so policy can be decided by the bootloader (or
something else which runs within the secure boot environment).

You can stop reading here if you're not interested in *how* this policy
would work.

To make it work, Shim or one of the other intermediates would set up
the variable.  we could communicate policy to it with the usual Foo,
FooUpdate mechanism we already use for MokList.  The default policy (if
the variable doesn't exist on firstboot) can be whatever the distro
wants, so if Fedora wants all the secure boot certs, it can do that and
other distros can follow their own stricter or less strict policies. 
 The user would be able to overwrite this using the Update process,
which could be password verified like MokList already is.

Does this sound acceptable to everyone?

James

Re: [PATCH 8/9] MODSIGN: Import certificates from UEFI Secure Boot

[Mimi Zohar]

Since this discussion affects which keys can be added to trusted
keyrings, cc'ing linux-ima-devel.

On Fri, 2016-12-02 at 10:57 -0800, James Bottomley wrote:
> On Thu, 2016-11-24 at 11:17 -0800, James Bottomley wrote:
> > On Mon, 2016-11-21 at 16:16 +0000, Ard Biesheuvel wrote:
> > > On 16 November 2016 at 18:11, David Howells <dhowells@redhat.com>
> > > wrote:
> > > > From: Josh Boyer <jwboyer@fedoraproject.org>
> > > > 
> > > > Secure Boot stores a list of allowed certificates in the 'db' 
> > > > variable. This imports those certificates into the system trusted
> > > > keyring.   This allows for a third party signing certificate to 
> > > > be used in conjunction with signed modules.  By importing the 
> > > > public certificate into the 'db' variable, a user can allow a 
> > > > module signed with that certificate to load.  The shim UEFI 
> > > > bootloader has a similar certificate list stored in the 
> > > > 'MokListRT' variable.   We import those as well.
> > > > 
> > > 
> > > This sounds like a bad idea to me. For the standard databases like 
> > > db and dbx, we can rely on the firmware to ensure that they are 
> > > what you expect.
> > 
> > Actually, I think it's a bad idea for the opposite reason: Shim
> > explicitly pivots the root of trust away from the db keys to its own
> > Moklist keys.  We have no choice and are forced to trust db for the
> > secure boot part, but once we're in the kernel proper, I'd argue that
> > we would only want to trust the pivoted root, i.e. Moklist.
> > 
> > Trusting both could generate unwanted consequences, like pressure on
> > Microsoft to sign modules or worse, pressure on OEMs to include 
> > module keys or hashes ... or worst of all OEMs signing external
> > modules.
> > 
> > >  For MokListRt, not so much: anyone with sufficient
> > > capabilities can generate such a variable from userland, and not 
> > > every arch/distro combo will be using shim and/or mokmanager. (The 
> > > debates are still ongoing, but my position is that there is no need
> > > for shim at all on ARM given that the M$ problem only exists on
> > > x86)
> > 
> > OK, so on this point, I'm already not using Shim on my x86 box. 
> >  However, what you find if you're using grub is that because grub
> > doesn't do signature verification, you still have to use the shim
> > protocol callout, so you need something between UEFI and grub to load
> > at least this protocol.  I suppose this would go away once we can
> > persuade grub to verify signatures.
> 
> Hm, that got crickets.
> 
> Let me propose an alternative mechanism then.
> 
> My problem is that although I am forced to trust the secure boot keys
> for the UEFI security boundary, I don't necessarily want to trust them
> for signing things for my kernel, so I want to pivot (or at
> leastselectively weed out) keys.  Shim already has this concept
> partially with MokIgnoreDB.
> 
> For the purposes of the kernel, I think we simply need a variable, lets
> call it MokKernelCerts, that gives the list of certificates to import
> into the kernel keyring.  I think this variable should be BS NV only
> (not RT) meaning we have to collect it before ExitBootServices().  The
> reason for this is to ensure it's populated by a trusted entity within
> the UEFI secure boot boundary.  This will cause a kexec problem, so we
> might have to relax this and use a RT shadow as we already do for
> MokList.  The idea is that we populate the kernel certificates only
> from this variable, so policy can be decided by the bootloader (or
> something else which runs within the secure boot environment).
> 
> You can stop reading here if you're not interested in *how* this policy
> would work.
> 
> To make it work, Shim or one of the other intermediates would set up
> the variable.  we could communicate policy to it with the usual Foo,
> FooUpdate mechanism we already use for MokList.  The default policy (if
> the variable doesn't exist on firstboot) can be whatever the distro
> wants, so if Fedora wants all the secure boot certs, it can do that and
> other distros can follow their own stricter or less strict policies. 
>  The user would be able to overwrite this using the Update process,
> which could be password verified like MokList already is.
> 
> Does this sound acceptable to everyone?

[PATCH 9/9] MODSIGN: Allow the "db" UEFI variable to be suppressed

[David Howells]

From: Josh Boyer <jwboyer@fedoraproject.org>

If a user tells shim to not use the certs/hashes in the UEFI db variable
for verification purposes, shim will set a UEFI variable called
MokIgnoreDB.  Have the uefi import code look for this and ignore the db
variable if it is found.

Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 certs/load_uefi.c |   44 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 10 deletions(-)

diff --git a/certs/load_uefi.c b/certs/load_uefi.c
index b44e464c3ff4..3d8845986019 100644
--- a/certs/load_uefi.c
+++ b/certs/load_uefi.c
@@ -13,6 +13,26 @@ static __initdata efi_guid_t efi_cert_x509_sha256_guid = EFI_CERT_X509_SHA256_GU
 static __initdata efi_guid_t efi_cert_sha256_guid = EFI_CERT_SHA256_GUID;
 
 /*
+ * Look to see if a UEFI variable called MokIgnoreDB exists and return true if
+ * it does.
+ *
+ * This UEFI variable is set by the shim if a user tells the shim to not use
+ * the certs/hashes in the UEFI db variable for verification purposes.  If it
+ * is set, we should ignore the db variable also and the true return indicates
+ * this.
+ */
+static __init bool uefi_check_ignore_db(void)
+{
+	efi_status_t status;
+	unsigned int db = 0;
+	unsigned long size = sizeof(db);
+	efi_guid_t guid = EFI_SHIM_LOCK_GUID;
+
+	status = efi.get_variable(L"MokIgnoreDB", &guid, NULL, &size, &db);
+	return status == EFI_SUCCESS;
+}
+
+/*
  * Get a certificate list blob from the named EFI variable.
  */
 static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
@@ -113,7 +133,9 @@ static __init efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_ty
 }
 
 /*
- * Load the certs contained in the UEFI databases
+ * Load the certs contained in the UEFI databases into the secondary trusted
+ * keyring and the UEFI blacklisted X.509 cert SHA256 hashes into the blacklist
+ * keyring.
  */
 static int __init load_uefi_certs(void)
 {
@@ -129,15 +151,17 @@ static int __init load_uefi_certs(void)
 	/* Get db, MokListRT, and dbx.  They might not exist, so it isn't
 	 * an error if we can't get them.
 	 */
-	db = get_cert_list(L"db", &secure_var, &dbsize);
-	if (!db) {
-		pr_err("MODSIGN: Couldn't get UEFI db list\n");
-	} else {
-		rc = parse_efi_signature_list("UEFI:db",
-					      db, dbsize, get_handler_for_db);
-		if (rc)
-			pr_err("Couldn't parse db signatures: %d\n", rc);
-		kfree(db);
+	if (!uefi_check_ignore_db()) {
+		db = get_cert_list(L"db", &secure_var, &dbsize);
+		if (!db) {
+			pr_err("MODSIGN: Couldn't get UEFI db list\n");
+		} else {
+			rc = parse_efi_signature_list("UEFI:db",
+						      db, dbsize, get_handler_for_db);
+			if (rc)
+				pr_err("Couldn't parse db signatures: %d\n", rc);
+			kfree(db);
+		}
 	}
 
 	mok = get_cert_list(L"MokListRT", &mok_var, &moksize);

Re: [PATCH 9/9] MODSIGN: Allow the "db" UEFI variable to be suppressed

[Ard Biesheuvel]

On 16 November 2016 at 18:11, David Howells <dhowells@redhat.com> wrote:
> From: Josh Boyer <jwboyer@fedoraproject.org>
>
> If a user tells shim to not use the certs/hashes in the UEFI db variable
> for verification purposes, shim will set a UEFI variable called
> MokIgnoreDB.  Have the uefi import code look for this and ignore the db
> variable if it is found.
>

Similar concern as in the previous patch: it appears to me that you
can DoS a machine by setting MokIgnoreDB if, e.g., its modules are
signed against a cert that resides in db, and shim/mokmanager are not
being used.


> Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
>  certs/load_uefi.c |   44 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/certs/load_uefi.c b/certs/load_uefi.c
> index b44e464c3ff4..3d8845986019 100644
> --- a/certs/load_uefi.c
> +++ b/certs/load_uefi.c
> @@ -13,6 +13,26 @@ static __initdata efi_guid_t efi_cert_x509_sha256_guid = EFI_CERT_X509_SHA256_GU
>  static __initdata efi_guid_t efi_cert_sha256_guid = EFI_CERT_SHA256_GUID;
>
>  /*
> + * Look to see if a UEFI variable called MokIgnoreDB exists and return true if
> + * it does.
> + *
> + * This UEFI variable is set by the shim if a user tells the shim to not use
> + * the certs/hashes in the UEFI db variable for verification purposes.  If it
> + * is set, we should ignore the db variable also and the true return indicates
> + * this.
> + */
> +static __init bool uefi_check_ignore_db(void)
> +{
> +       efi_status_t status;
> +       unsigned int db = 0;
> +       unsigned long size = sizeof(db);
> +       efi_guid_t guid = EFI_SHIM_LOCK_GUID;
> +
> +       status = efi.get_variable(L"MokIgnoreDB", &guid, NULL, &size, &db);
> +       return status == EFI_SUCCESS;
> +}
> +
> +/*
>   * Get a certificate list blob from the named EFI variable.
>   */
>  static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> @@ -113,7 +133,9 @@ static __init efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_ty
>  }
>
>  /*
> - * Load the certs contained in the UEFI databases
> + * Load the certs contained in the UEFI databases into the secondary trusted
> + * keyring and the UEFI blacklisted X.509 cert SHA256 hashes into the blacklist
> + * keyring.
>   */
>  static int __init load_uefi_certs(void)
>  {
> @@ -129,15 +151,17 @@ static int __init load_uefi_certs(void)
>         /* Get db, MokListRT, and dbx.  They might not exist, so it isn't
>          * an error if we can't get them.
>          */
> -       db = get_cert_list(L"db", &secure_var, &dbsize);
> -       if (!db) {
> -               pr_err("MODSIGN: Couldn't get UEFI db list\n");
> -       } else {
> -               rc = parse_efi_signature_list("UEFI:db",
> -                                             db, dbsize, get_handler_for_db);
> -               if (rc)
> -                       pr_err("Couldn't parse db signatures: %d\n", rc);
> -               kfree(db);
> +       if (!uefi_check_ignore_db()) {
> +               db = get_cert_list(L"db", &secure_var, &dbsize);
> +               if (!db) {
> +                       pr_err("MODSIGN: Couldn't get UEFI db list\n");
> +               } else {
> +                       rc = parse_efi_signature_list("UEFI:db",
> +                                                     db, dbsize, get_handler_for_db);
> +                       if (rc)
> +                               pr_err("Couldn't parse db signatures: %d\n", rc);
> +                       kfree(db);
> +               }
>         }
>
>         mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 9/9] MODSIGN: Allow the "db" UEFI variable to be suppressed

[Josh Boyer]

On Mon, Nov 21, 2016 at 11:18 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 16 November 2016 at 18:11, David Howells <dhowells@redhat.com> wrote:
>> From: Josh Boyer <jwboyer@fedoraproject.org>
>>
>> If a user tells shim to not use the certs/hashes in the UEFI db variable
>> for verification purposes, shim will set a UEFI variable called
>> MokIgnoreDB.  Have the uefi import code look for this and ignore the db
>> variable if it is found.
>>
>
> Similar concern as in the previous patch: it appears to me that you
> can DoS a machine by setting MokIgnoreDB if, e.g., its modules are
> signed against a cert that resides in db, and shim/mokmanager are not
> being used.

If shim/mokmanager aren't used, then you can't actually modify
MokIgnoreDB.  Again, it requires physical access and a reboot into
mokmanager to actually take effect.

josh


>> Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
>> Signed-off-by: David Howells <dhowells@redhat.com>
>> ---
>>
>>  certs/load_uefi.c |   44 ++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/certs/load_uefi.c b/certs/load_uefi.c
>> index b44e464c3ff4..3d8845986019 100644
>> --- a/certs/load_uefi.c
>> +++ b/certs/load_uefi.c
>> @@ -13,6 +13,26 @@ static __initdata efi_guid_t efi_cert_x509_sha256_guid = EFI_CERT_X509_SHA256_GU
>>  static __initdata efi_guid_t efi_cert_sha256_guid = EFI_CERT_SHA256_GUID;
>>
>>  /*
>> + * Look to see if a UEFI variable called MokIgnoreDB exists and return true if
>> + * it does.
>> + *
>> + * This UEFI variable is set by the shim if a user tells the shim to not use
>> + * the certs/hashes in the UEFI db variable for verification purposes.  If it
>> + * is set, we should ignore the db variable also and the true return indicates
>> + * this.
>> + */
>> +static __init bool uefi_check_ignore_db(void)
>> +{
>> +       efi_status_t status;
>> +       unsigned int db = 0;
>> +       unsigned long size = sizeof(db);
>> +       efi_guid_t guid = EFI_SHIM_LOCK_GUID;
>> +
>> +       status = efi.get_variable(L"MokIgnoreDB", &guid, NULL, &size, &db);
>> +       return status == EFI_SUCCESS;
>> +}
>> +
>> +/*
>>   * Get a certificate list blob from the named EFI variable.
>>   */
>>  static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
>> @@ -113,7 +133,9 @@ static __init efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_ty
>>  }
>>
>>  /*
>> - * Load the certs contained in the UEFI databases
>> + * Load the certs contained in the UEFI databases into the secondary trusted
>> + * keyring and the UEFI blacklisted X.509 cert SHA256 hashes into the blacklist
>> + * keyring.
>>   */
>>  static int __init load_uefi_certs(void)
>>  {
>> @@ -129,15 +151,17 @@ static int __init load_uefi_certs(void)
>>         /* Get db, MokListRT, and dbx.  They might not exist, so it isn't
>>          * an error if we can't get them.
>>          */
>> -       db = get_cert_list(L"db", &secure_var, &dbsize);
>> -       if (!db) {
>> -               pr_err("MODSIGN: Couldn't get UEFI db list\n");
>> -       } else {
>> -               rc = parse_efi_signature_list("UEFI:db",
>> -                                             db, dbsize, get_handler_for_db);
>> -               if (rc)
>> -                       pr_err("Couldn't parse db signatures: %d\n", rc);
>> -               kfree(db);
>> +       if (!uefi_check_ignore_db()) {
>> +               db = get_cert_list(L"db", &secure_var, &dbsize);
>> +               if (!db) {
>> +                       pr_err("MODSIGN: Couldn't get UEFI db list\n");
>> +               } else {
>> +                       rc = parse_efi_signature_list("UEFI:db",
>> +                                                     db, dbsize, get_handler_for_db);
>> +                       if (rc)
>> +                               pr_err("Couldn't parse db signatures: %d\n", rc);
>> +                       kfree(db);
>> +               }
>>         }
>>
>>         mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 9/9] MODSIGN: Allow the "db" UEFI variable to be suppressed

[Ard Biesheuvel]

On 21 November 2016 at 16:26, Josh Boyer <jwboyer@fedoraproject.org> wrote:
> On Mon, Nov 21, 2016 at 11:18 AM, Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
>> On 16 November 2016 at 18:11, David Howells <dhowells@redhat.com> wrote:
>>> From: Josh Boyer <jwboyer@fedoraproject.org>
>>>
>>> If a user tells shim to not use the certs/hashes in the UEFI db variable
>>> for verification purposes, shim will set a UEFI variable called
>>> MokIgnoreDB.  Have the uefi import code look for this and ignore the db
>>> variable if it is found.
>>>
>>
>> Similar concern as in the previous patch: it appears to me that you
>> can DoS a machine by setting MokIgnoreDB if, e.g., its modules are
>> signed against a cert that resides in db, and shim/mokmanager are not
>> being used.
>
> If shim/mokmanager aren't used, then you can't actually modify
> MokIgnoreDB.  Again, it requires physical access and a reboot into
> mokmanager to actually take effect.
>

This does the trick as well

printf "\x07\x00\x00\x00\x01" >
/sys/firmware/efi/efivars/MokIgnoreDB-605dab50-e046-4300-abb6-3dd810dd8b23

Re: [PATCH 9/9] MODSIGN: Allow the "db" UEFI variable to be suppressed

[Peter Jones]

On Mon, Nov 21, 2016 at 04:42:45PM +0000, Ard Biesheuvel wrote:
> On 21 November 2016 at 16:26, Josh Boyer <jwboyer@fedoraproject.org> wrote:
> > On Mon, Nov 21, 2016 at 11:18 AM, Ard Biesheuvel
> > <ard.biesheuvel@linaro.org> wrote:
> >> On 16 November 2016 at 18:11, David Howells <dhowells@redhat.com> wrote:
> >>> From: Josh Boyer <jwboyer@fedoraproject.org>
> >>>
> >>> If a user tells shim to not use the certs/hashes in the UEFI db variable
> >>> for verification purposes, shim will set a UEFI variable called
> >>> MokIgnoreDB.  Have the uefi import code look for this and ignore the db
> >>> variable if it is found.
> >>>
> >>
> >> Similar concern as in the previous patch: it appears to me that you
> >> can DoS a machine by setting MokIgnoreDB if, e.g., its modules are
> >> signed against a cert that resides in db, and shim/mokmanager are not
> >> being used.
> >
> > If shim/mokmanager aren't used, then you can't actually modify
> > MokIgnoreDB.  Again, it requires physical access and a reboot into
> > mokmanager to actually take effect.
> >
> 
> This does the trick as well
> 
> printf "\x07\x00\x00\x00\x01" >
> /sys/firmware/efi/efivars/MokIgnoreDB-605dab50-e046-4300-abb6-3dd810dd8b23

So that really means two things.  First, kernel should only honor any of
the Mok* variables if they're Boot Services-only variables.  Second, to
avoid the DoS, shim should create them all as Boot Services-only the
first time it boots.  That'll prevent them from being created post-boot.

-- 
        Peter

Re: [PATCH 9/9] MODSIGN: Allow the "db" UEFI variable to be suppressed

[Ard Biesheuvel]

On 21 November 2016 at 20:05, Peter Jones <pjones@redhat.com> wrote:
> On Mon, Nov 21, 2016 at 04:42:45PM +0000, Ard Biesheuvel wrote:
>> On 21 November 2016 at 16:26, Josh Boyer <jwboyer@fedoraproject.org> wrote:
>> > On Mon, Nov 21, 2016 at 11:18 AM, Ard Biesheuvel
>> > <ard.biesheuvel@linaro.org> wrote:
>> >> On 16 November 2016 at 18:11, David Howells <dhowells@redhat.com> wrote:
>> >>> From: Josh Boyer <jwboyer@fedoraproject.org>
>> >>>
>> >>> If a user tells shim to not use the certs/hashes in the UEFI db variable
>> >>> for verification purposes, shim will set a UEFI variable called
>> >>> MokIgnoreDB.  Have the uefi import code look for this and ignore the db
>> >>> variable if it is found.
>> >>>
>> >>
>> >> Similar concern as in the previous patch: it appears to me that you
>> >> can DoS a machine by setting MokIgnoreDB if, e.g., its modules are
>> >> signed against a cert that resides in db, and shim/mokmanager are not
>> >> being used.
>> >
>> > If shim/mokmanager aren't used, then you can't actually modify
>> > MokIgnoreDB.  Again, it requires physical access and a reboot into
>> > mokmanager to actually take effect.
>> >
>>
>> This does the trick as well
>>
>> printf "\x07\x00\x00\x00\x01" >
>> /sys/firmware/efi/efivars/MokIgnoreDB-605dab50-e046-4300-abb6-3dd810dd8b23
>
> So that really means two things.  First, kernel should only honor any of
> the Mok* variables if they're Boot Services-only variables.  Second, to
> avoid the DoS, shim should create them all as Boot Services-only the
> first time it boots.  That'll prevent them from being created post-boot.
>

All of that assumes you are using shim and mokmanager in the first place.

Re: [PATCH 9/9] MODSIGN: Allow the "db" UEFI variable to be suppressed

[Peter Jones]

On Mon, Nov 21, 2016 at 08:06:44PM +0100, Ard Biesheuvel wrote:
> On 21 November 2016 at 20:05, Peter Jones <pjones@redhat.com> wrote:
> > On Mon, Nov 21, 2016 at 04:42:45PM +0000, Ard Biesheuvel wrote:
> >> On 21 November 2016 at 16:26, Josh Boyer <jwboyer@fedoraproject.org> wrote:
> >> > On Mon, Nov 21, 2016 at 11:18 AM, Ard Biesheuvel
> >> > <ard.biesheuvel@linaro.org> wrote:
> >> >> On 16 November 2016 at 18:11, David Howells <dhowells@redhat.com> wrote:
> >> >>> From: Josh Boyer <jwboyer@fedoraproject.org>
> >> >>>
> >> >>> If a user tells shim to not use the certs/hashes in the UEFI db variable
> >> >>> for verification purposes, shim will set a UEFI variable called
> >> >>> MokIgnoreDB.  Have the uefi import code look for this and ignore the db
> >> >>> variable if it is found.
> >> >>>
> >> >>
> >> >> Similar concern as in the previous patch: it appears to me that you
> >> >> can DoS a machine by setting MokIgnoreDB if, e.g., its modules are
> >> >> signed against a cert that resides in db, and shim/mokmanager are not
> >> >> being used.
> >> >
> >> > If shim/mokmanager aren't used, then you can't actually modify
> >> > MokIgnoreDB.  Again, it requires physical access and a reboot into
> >> > mokmanager to actually take effect.
> >> >
> >>
> >> This does the trick as well
> >>
> >> printf "\x07\x00\x00\x00\x01" >
> >> /sys/firmware/efi/efivars/MokIgnoreDB-605dab50-e046-4300-abb6-3dd810dd8b23
> >
> > So that really means two things.  First, kernel should only honor any of
> > the Mok* variables if they're Boot Services-only variables.  Second, to
> > avoid the DoS, shim should create them all as Boot Services-only the
> > first time it boots.  That'll prevent them from being created post-boot.
> >
> 
> All of that assumes you are using shim and mokmanager in the first place.

No, it doesn't.  If you're not using shim, there's no DoS problem,
because what would you be DoSing?  And likewise, if you're not using
Secure Boot at all, you have no guarantee of anything about your boot
environment, starting with the idea that the boot loader isn't hostile.
If you're not using Secure Boot, a hostile pre-boot driver could easily
add DB entries just as easily as MokList entries, or any other
variables.

The fact that keys can be injected is true with or without this patch,
though it does make it easier.  But making a boot loader that injects
keys into the kernel's built-in keyring isn't actually very difficult.

If you're not using firmware enforced SB and shim, you do not have
security against this.

-- 
        Peter

Re: [PATCH 9/9] MODSIGN: Allow the "db" UEFI variable to be suppressed

[Ard Biesheuvel]

On 21 November 2016 at 20:18, Peter Jones <pjones@redhat.com> wrote:
> On Mon, Nov 21, 2016 at 08:06:44PM +0100, Ard Biesheuvel wrote:
>> On 21 November 2016 at 20:05, Peter Jones <pjones@redhat.com> wrote:
>> > On Mon, Nov 21, 2016 at 04:42:45PM +0000, Ard Biesheuvel wrote:
>> >> On 21 November 2016 at 16:26, Josh Boyer <jwboyer@fedoraproject.org> wrote:
>> >> > On Mon, Nov 21, 2016 at 11:18 AM, Ard Biesheuvel
>> >> > <ard.biesheuvel@linaro.org> wrote:
>> >> >> On 16 November 2016 at 18:11, David Howells <dhowells@redhat.com> wrote:
>> >> >>> From: Josh Boyer <jwboyer@fedoraproject.org>
>> >> >>>
>> >> >>> If a user tells shim to not use the certs/hashes in the UEFI db variable
>> >> >>> for verification purposes, shim will set a UEFI variable called
>> >> >>> MokIgnoreDB.  Have the uefi import code look for this and ignore the db
>> >> >>> variable if it is found.
>> >> >>>
>> >> >>
>> >> >> Similar concern as in the previous patch: it appears to me that you
>> >> >> can DoS a machine by setting MokIgnoreDB if, e.g., its modules are
>> >> >> signed against a cert that resides in db, and shim/mokmanager are not
>> >> >> being used.
>> >> >
>> >> > If shim/mokmanager aren't used, then you can't actually modify
>> >> > MokIgnoreDB.  Again, it requires physical access and a reboot into
>> >> > mokmanager to actually take effect.
>> >> >
>> >>
>> >> This does the trick as well
>> >>
>> >> printf "\x07\x00\x00\x00\x01" >
>> >> /sys/firmware/efi/efivars/MokIgnoreDB-605dab50-e046-4300-abb6-3dd810dd8b23
>> >
>> > So that really means two things.  First, kernel should only honor any of
>> > the Mok* variables if they're Boot Services-only variables.  Second, to
>> > avoid the DoS, shim should create them all as Boot Services-only the
>> > first time it boots.  That'll prevent them from being created post-boot.
>> >
>>
>> All of that assumes you are using shim and mokmanager in the first place.
>
> No, it doesn't.  If you're not using shim, there's no DoS problem,
> because what would you be DoSing?

Well, if I lose the ability to load modules that have been signed with
a key that resides in db, simply because someone managed to set a
variable that the kernel treats as 'special' even though I am not
using shim/mok in the first place, I would call that an effective DoS
if that means I cannot find my root partition anymore. I suppose
checking for the runtime attribute on the MokIgnoreDB variable
mitigates this somewhat, but it still makes me feel uneasy that the
kernel hardwires variable names that are specific to shim/mokmanager
rather than defined by the UEFI spec.

>  And likewise, if you're not using
> Secure Boot at all, you have no guarantee of anything about your boot
> environment, starting with the idea that the boot loader isn't hostile.
> If you're not using Secure Boot, a hostile pre-boot driver could easily
> add DB entries just as easily as MokList entries, or any other
> variables.
>

I am talking about Secure Boot with shim or MokManager, which are
unlikely to be necessary in many cases on ARM/arm64

> The fact that keys can be injected is true with or without this patch,
> though it does make it easier.  But making a boot loader that injects
> keys into the kernel's built-in keyring isn't actually very difficult.
>
> If you're not using firmware enforced SB and shim, you do not have
> security against this.
>

My objection is against 'magic' variables like MokIgnoreDB and
MokListRT, both of which leave gaping security holes if used in the
proposed way on systems that use Secure Boot but are not using shim or
MokManager.

Adding the contents of MokListRT to the set of trusted keys is a *bad*
idea unless I can be 100% sure that shim/mokmanager were involved in
my boot chain.

Re: [PATCH 0/9] KEYS: Blacklisting & UEFI database load

[Jiri Slaby]

On 11/16/2016, 07:10 PM, David Howells wrote:
> Here are two sets of patches.  Firstly, the first three patches provide a
> blacklist, making the following changes:
...
> Secondly, the remaining patches allow the UEFI database to be used to load
> the system keyrings:
...
> Dave Howells (2):
>       efi: Add EFI signature data types
>       efi: Add an EFI signature blob parser
> 
> David Howells (5):
>       KEYS: Add a system blacklist keyring
>       X.509: Allow X.509 certs to be blacklisted
>       PKCS#7: Handle blacklisted certificates
>       KEYS: Allow unrestricted boot-time addition of keys to secondary keyring
>       efi: Add SHIM and image security database GUID definitions
> 
> Josh Boyer (2):
>       MODSIGN: Import certificates from UEFI Secure Boot
>       MODSIGN: Allow the "db" UEFI variable to be suppressed

Hi,

what's the status of this please? Distributors (I checked SUSE, RedHat
and Ubuntu) have to carry these patches and every of them have to
forward-port the patches to new kernels. So are you going to resend the
PR to have this merged?

thanks,
-- 
js
suse labs

Re: [PATCH 0/9] KEYS: Blacklisting & UEFI database load

[Mimi Zohar]

On Tue, 2018-03-06 at 15:05 +0100, Jiri Slaby wrote:
> On 11/16/2016, 07:10 PM, David Howells wrote:
> > Here are two sets of patches.  Firstly, the first three patches provide a
> > blacklist, making the following changes:
> ...
> > Secondly, the remaining patches allow the UEFI database to be used to load
> > the system keyrings:
> ...
> > Dave Howells (2):
> >       efi: Add EFI signature data types
> >       efi: Add an EFI signature blob parser
> > 
> > David Howells (5):
> >       KEYS: Add a system blacklist keyring
> >       X.509: Allow X.509 certs to be blacklisted
> >       PKCS#7: Handle blacklisted certificates
> >       KEYS: Allow unrestricted boot-time addition of keys to secondary keyring
> >       efi: Add SHIM and image security database GUID definitions
> > 
> > Josh Boyer (2):
> >       MODSIGN: Import certificates from UEFI Secure Boot
> >       MODSIGN: Allow the "db" UEFI variable to be suppressed
> 
> Hi,
> 
> what's the status of this please? Distributors (I checked SUSE, RedHat
> and Ubuntu) have to carry these patches and every of them have to
> forward-port the patches to new kernels. So are you going to resend the
> PR to have this merged?

With secure boot enabled, we establish a signature chain of trust,
rooted in HW, up to the kernel and then transition from those keys to
a new set of keys builtin the kernel and loaded onto the
builtin_trusted_keys (builtin).

Enabling the secondary_builtin_keys (secondary) allows keys signed by
a key on the builtin keyring to be added to the secondary keyring.
 Any key, signed by a key on either the builtin or secondary keyring,
can be added to the IMA trusted keyring.

The "KEYS: Allow unrestricted boot-time addition of keys to secondary
keyring" patch loads the platform keys directly onto the secondary
keyring, without requiring them to be signed by a key on the builtin
or secondary keyring.  With this change, any key signed by a platfrom
key on the secondary, can be loaded onto the .ima trusted keyring.

Just because I trust the platform keys prior to booting the kernel,
doesn't mean that I *want* to trust those keys once booted.  There
are, however, places where we need access to those keys to verify a
signature (eg. kexec kernel image).

Nayna Jain's "certs: define a trusted platform keyring" patch set
introduces a new, separate keyring for these platform keys.

Mimi

Re: [PATCH 0/9] KEYS: Blacklisting & UEFI database load

[James Bottomley]

On Wed, 2018-03-07 at 08:18 -0500, Mimi Zohar wrote:
> On Tue, 2018-03-06 at 15:05 +0100, Jiri Slaby wrote:
> > what's the status of this please? Distributors (I checked SUSE,
> > RedHat and Ubuntu) have to carry these patches and every of them
> > have to forward-port the patches to new kernels. So are you going
> > to resend the PR to have this merged?
[...]
> Just because I trust the platform keys prior to booting the kernel,
> doesn't mean that I *want* to trust those keys once booted.  There
> are, however, places where we need access to those keys to verify a
> signature (eg. kexec kernel image).

Which is essentially the reason I always give when these patches come
back

> Nayna Jain's "certs: define a trusted platform keyring" patch set
> introduces a new, separate keyring for these platform keys.

Perhaps, to break the deadlock, we should ask Jiří what the reason is
the distros want these keys to be trusted.  Apart from the Microsoft
key, it will also give you an OEM key in your trusted keyring.  Is it
something to do with OEM supplied modules?

James

Re: [PATCH 0/9] KEYS: Blacklisting & UEFI database load

[joeyli]

On Wed, Mar 07, 2018 at 07:28:37AM -0800, James Bottomley wrote:
> On Wed, 2018-03-07 at 08:18 -0500, Mimi Zohar wrote:
> > On Tue, 2018-03-06 at 15:05 +0100, Jiri Slaby wrote:
> > > what's the status of this please? Distributors (I checked SUSE,
> > > RedHat and Ubuntu) have to carry these patches and every of them
> > > have to forward-port the patches to new kernels. So are you going
> > > to resend the PR to have this merged?
> [...]
> > Just because I trust the platform keys prior to booting the kernel,
> > doesn't mean that I *want* to trust those keys once booted.  There
> > are, however, places where we need access to those keys to verify a
> > signature (eg. kexec kernel image).
> 
> Which is essentially the reason I always give when these patches come
> back
>

Josh Boyer's "MODSIGN: Allow the "db" UEFI variable to be suppressed"
patch checks MokIgnoreDB variable to ignore db:

https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-uefi&id=7c395b30a33a617c5cc2cdd419300af71277b79a

I think that we can consider to use MokAllowDB. Which means that kernel
ignores DB by default.

> > Nayna Jain's "certs: define a trusted platform keyring" patch set
> > introduces a new, separate keyring for these platform keys.
> 
> Perhaps, to break the deadlock, we should ask Jiří what the reason is
> the distros want these keys to be trusted.  Apart from the Microsoft
> key, it will also give you an OEM key in your trusted keyring.  Is it
> something to do with OEM supplied modules?
>

As I remember that some manufacturers uses certificate in db to
sign their kernel module. We need to discuss with them for switching
to mok. Currently I do not know all use cases for using db.

There have some benefits for using db:

 - User does not need to deal with shim-mokmanager to enroll mok.
   Target machine doesn't need to reboot and user doesn't need to
   face to mokmanager UI.  

 - The db is a authenticated variable, it's still secure when secure
   boot is disabled.
   The db is a authenticated variable that it can only be modified
   by manufacturer's key. Kernel can trust it when secure boot
   is disabled. It's useful for we do not need to taint kernel
   for loading a manufacturer's kernel module even secure boot is
   disabled.

 - Do not need to worry about the space of NVRAM and the EFI firmware
   implementation for writing a boot time variable.
  
But I also agree that we should not trust all keys (like Microsoft key)
in db by default.

Thanks a lot!
Joey Lee

Re: [PATCH 0/9] KEYS: Blacklisting & UEFI database load

[Mimi Zohar]

On Sun, 2018-03-11 at 11:20 +0800, joeyli wrote:
> On Wed, Mar 07, 2018 at 07:28:37AM -0800, James Bottomley wrote:
> > On Wed, 2018-03-07 at 08:18 -0500, Mimi Zohar wrote:
> > > On Tue, 2018-03-06 at 15:05 +0100, Jiri Slaby wrote:
> > > > what's the status of this please? Distributors (I checked SUSE,
> > > > RedHat and Ubuntu) have to carry these patches and every of them
> > > > have to forward-port the patches to new kernels. So are you going
> > > > to resend the PR to have this merged?
> > [...]
> > > Just because I trust the platform keys prior to booting the kernel,
> > > doesn't mean that I *want* to trust those keys once booted.  There
> > > are, however, places where we need access to those keys to verify a
> > > signature (eg. kexec kernel image).
> > 
> > Which is essentially the reason I always give when these patches come
> > back
> >
> 
> Josh Boyer's "MODSIGN: Allow the "db" UEFI variable to be suppressed"
> patch checks MokIgnoreDB variable to ignore db:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-uefi&id=7c395b30a33a617c5cc2cdd419300af71277b79a
> 
> I think that we can consider to use MokAllowDB. Which means that kernel
> ignores DB by default.

Not all systems have a shim layer.  This design is really x86
specific.  Allowing shim keys, but ignoring DB, does not address those
systems.

> > > Nayna Jain's "certs: define a trusted platform keyring" patch set
> > > introduces a new, separate keyring for these platform keys.
> > 
> > Perhaps, to break the deadlock, we should ask Jiří what the reason is
> > the distros want these keys to be trusted.  Apart from the Microsoft
> > key, it will also give you an OEM key in your trusted keyring.  Is it
> > something to do with OEM supplied modules?
> >
> 
> As I remember that some manufacturers uses certificate in db to
> sign their kernel module. We need to discuss with them for switching
> to mok. Currently I do not know all use cases for using db.
> 
> There have some benefits for using db:
> 
>  - User does not need to deal with shim-mokmanager to enroll mok.
>    Target machine doesn't need to reboot and user doesn't need to
>    face to mokmanager UI.  

The reason for trusting enrolled shim keys is because it requires
physical presence.  (I kind of remember hearing that this changed.
 There is some method of accepting enrolled keys that does not require
physical presence.)

>  - The db is a authenticated variable, it's still secure when secure
>    boot is disabled.
>    The db is a authenticated variable that it can only be modified
>    by manufacturer's key. Kernel can trust it when secure boot
>    is disabled. It's useful for we do not need to taint kernel
>    for loading a manufacturer's kernel module even secure boot is
>    disabled.
> 
>  - Do not need to worry about the space of NVRAM and the EFI firmware
>    implementation for writing a boot time variable.
>   
> But I also agree that we should not trust all keys (like Microsoft key)
> in db by default.

Between requiring a shim layer and relying on physical presence, I'm
not convinced this is the best solution.  Do we really want to support
different methods for different architectures?

Mimi

Re: [PATCH 0/9] KEYS: Blacklisting & UEFI database load

[joeyli]

Hi Mimi,

On Mon, Mar 19, 2018 at 10:12:03AM -0400, Mimi Zohar wrote:
> On Sun, 2018-03-11 at 11:20 +0800, joeyli wrote:
> > On Wed, Mar 07, 2018 at 07:28:37AM -0800, James Bottomley wrote:
> > > On Wed, 2018-03-07 at 08:18 -0500, Mimi Zohar wrote:
> > > > On Tue, 2018-03-06 at 15:05 +0100, Jiri Slaby wrote:
> > > > > what's the status of this please? Distributors (I checked SUSE,
> > > > > RedHat and Ubuntu) have to carry these patches and every of them
> > > > > have to forward-port the patches to new kernels. So are you going
> > > > > to resend the PR to have this merged?
> > > [...]
> > > > Just because I trust the platform keys prior to booting the kernel,
> > > > doesn't mean that I *want* to trust those keys once booted.  There
> > > > are, however, places where we need access to those keys to verify a
> > > > signature (eg. kexec kernel image).
> > > 
> > > Which is essentially the reason I always give when these patches come
> > > back
> > >
> > 
> > Josh Boyer's "MODSIGN: Allow the "db" UEFI variable to be suppressed"
> > patch checks MokIgnoreDB variable to ignore db:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-uefi&id=7c395b30a33a617c5cc2cdd419300af71277b79a
> > 
> > I think that we can consider to use MokAllowDB. Which means that kernel
> > ignores DB by default.
> 
> Not all systems have a shim layer.  This design is really x86
> specific.  Allowing shim keys, but ignoring DB, does not address those
> systems.
>

Actually shim is EFI specific but not x86 specific. I agree with you that
not all system has shim. But at least shim provides a way to interact with
user to detect the physical accessing.

For the system doesn't have shim, kernel can provide a boot option for user
to trust the keys in DB. But it also means that the boot option can be
enabled without physical accessing.
 
> > > > Nayna Jain's "certs: define a trusted platform keyring" patch set
> > > > introduces a new, separate keyring for these platform keys.
> > > 
> > > Perhaps, to break the deadlock, we should ask Jiří what the reason is
> > > the distros want these keys to be trusted.  Apart from the Microsoft
> > > key, it will also give you an OEM key in your trusted keyring.  Is it
> > > something to do with OEM supplied modules?
> > >
> > 
> > As I remember that some manufacturers uses certificate in db to
> > sign their kernel module. We need to discuss with them for switching
> > to mok. Currently I do not know all use cases for using db.
> > 
> > There have some benefits for using db:
> > 
> >  - User does not need to deal with shim-mokmanager to enroll mok.
> >    Target machine doesn't need to reboot and user doesn't need to
> >    face to mokmanager UI.  
> 
> The reason for trusting enrolled shim keys is because it requires
> physical presence.  (I kind of remember hearing that this changed.
>  There is some method of accepting enrolled keys that does not require
> physical presence.)
>

Could you please provide more detail for those methods? Thanks!
 
> >  - The db is a authenticated variable, it's still secure when secure
> >    boot is disabled.
> >    The db is a authenticated variable that it can only be modified
> >    by manufacturer's key. Kernel can trust it when secure boot
> >    is disabled. It's useful for we do not need to taint kernel
> >    for loading a manufacturer's kernel module even secure boot is
> >    disabled.
> > 
> >  - Do not need to worry about the space of NVRAM and the EFI firmware
> >    implementation for writing a boot time variable.
> >   
> > But I also agree that we should not trust all keys (like Microsoft key)
> > in db by default.
> 
> Between requiring a shim layer and relying on physical presence, I'm
> not convinced this is the best solution.  Do we really want to support
> different methods for different architectures?
>

It's not the best solution because it relies on other layers. But it's
currently the only solution for general EFI firmware. Or you have other
solution can be used for all architectures?

Thanks a lot!
Joey Lee