LinuxLists.cc - [PATCH 0/9] KEYS: Blacklisting & UEFI database load

[permalink] [raw]

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

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 <[email protected]>
---

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 ([email protected])
+ *
+ * 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;

2016-11-16 18:11:06

[permalink] [raw]

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

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 <[email protected]>
---

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) {

2016-11-16 18:11:12

[permalink] [raw]

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

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 <[email protected]>
---

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);

2016-11-16 18:11:20

[permalink] [raw]

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

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 <[email protected]>
---

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 ([email protected])
+ *
+ * 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 */

2016-11-16 18:11:38

[permalink] [raw]

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

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

Signed-off-by: Josh Boyer <[email protected]>
Signed-off-by: David Howells <[email protected]>
---

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

2016-11-16 18:11:54

[permalink] [raw]

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

From: Josh Boyer <[email protected]>

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 <[email protected]>
Signed-off-by: David Howells <[email protected]>
---

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);

2016-11-16 18:12:29

[permalink] [raw]

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

From: Josh Boyer <[email protected]>

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 <[email protected]>
Signed-off-by: David Howells <[email protected]>
---

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);

2016-11-16 18:12:45

[permalink] [raw]

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

From: Dave Howells <[email protected]>

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 <[email protected]>
---

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 ([email protected])
+ *
+ * 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

2016-11-16 18:13:14

[permalink] [raw]

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

From: Dave Howells <[email protected]>

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 <[email protected]>
---

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:
*/

2016-11-16 23:44:03

by Mat Martineau

[permalink] [raw]

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

David,

On Wed, 16 Nov 2016, David Howells wrote:

> From: Dave Howells <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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

2016-11-17 07:06:18

by Petko Manolov

[permalink] [raw]

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

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 <[email protected]>
> ---
>
> 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 ([email protected])
> + *
> + * 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2016-11-17 09:44:08

[permalink] [raw]

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

Mat Martineau <[email protected]> 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

2016-11-17 09:56:07

[permalink] [raw]

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

Petko Manolov <[email protected]> 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

2016-11-17 10:22:23

by Petko Manolov

[permalink] [raw]

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

On 16-11-17 09:56:00, David Howells wrote:
> Petko Manolov <[email protected]> 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

2016-11-17 11:18:38

[permalink] [raw]

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

Petko Manolov <[email protected]> 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

2016-11-21 14:05:38

[permalink] [raw]

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

On Thu, 2016-11-17 at 09:56 +0000, David Howells wrote:
> Petko Manolov <[email protected]> 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

2016-11-21 15:17:16

[permalink] [raw]

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

Mimi Zohar <[email protected]> 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

2016-11-21 16:07:09

[permalink] [raw]

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

On 16 November 2016 at 18:11, David Howells <[email protected]> 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 <[email protected]>
> Signed-off-by: David Howells <[email protected]>

This patch

Reviewed-by: Ard Biesheuvel <[email protected]>

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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2016-11-21 16:08:10

[permalink] [raw]

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

On 17 November 2016 at 09:44, David Howells <[email protected]> wrote:
> Mat Martineau <[email protected]> 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 <[email protected]>

2016-11-21 16:16:25

[permalink] [raw]

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

On 16 November 2016 at 18:11, David Howells <[email protected]> wrote:
> From: Josh Boyer <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: David Howells <[email protected]>
> ---
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2016-11-21 16:19:02

[permalink] [raw]

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

On 16 November 2016 at 18:11, David Howells <[email protected]> wrote:
> From: Josh Boyer <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: David Howells <[email protected]>
> ---
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2016-11-21 16:24:52

[permalink] [raw]

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

On Mon, 2016-11-21 at 15:17 +0000, David Howells wrote:
> Mimi Zohar <[email protected]> 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

2016-11-21 16:25:05

by Josh Boyer

[permalink] [raw]

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

On Mon, Nov 21, 2016 at 11:16 AM, Ard Biesheuvel
<[email protected]> wrote:
> On 16 November 2016 at 18:11, David Howells <[email protected]> wrote:
>> From: Josh Boyer <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: David Howells <[email protected]>
>> ---
>>
>> 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 [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html

2016-11-21 16:26:07

by Josh Boyer

[permalink] [raw]

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

On Mon, Nov 21, 2016 at 11:18 AM, Ard Biesheuvel
<[email protected]> wrote:
> On 16 November 2016 at 18:11, David Howells <[email protected]> wrote:
>> From: Josh Boyer <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: David Howells <[email protected]>
>> ---
>>
>> 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 [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html

2016-11-21 16:42:49

[permalink] [raw]

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

On 21 November 2016 at 16:26, Josh Boyer <[email protected]> wrote:
> On Mon, Nov 21, 2016 at 11:18 AM, Ard Biesheuvel
> <[email protected]> wrote:
>> On 16 November 2016 at 18:11, David Howells <[email protected]> wrote:
>>> From: Josh Boyer <[email protected]>
>>>
>>> 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

2016-11-21 19:05:38

by Peter Jones

[permalink] [raw]

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

On Mon, Nov 21, 2016 at 04:42:45PM +0000, Ard Biesheuvel wrote:
> On 21 November 2016 at 16:26, Josh Boyer <[email protected]> wrote:
> > On Mon, Nov 21, 2016 at 11:18 AM, Ard Biesheuvel
> > <[email protected]> wrote:
> >> On 16 November 2016 at 18:11, David Howells <[email protected]> wrote:
> >>> From: Josh Boyer <[email protected]>
> >>>
> >>> 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

2016-11-21 19:06:49

[permalink] [raw]

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

On 21 November 2016 at 20:05, Peter Jones <[email protected]> wrote:
> On Mon, Nov 21, 2016 at 04:42:45PM +0000, Ard Biesheuvel wrote:
>> On 21 November 2016 at 16:26, Josh Boyer <[email protected]> wrote:
>> > On Mon, Nov 21, 2016 at 11:18 AM, Ard Biesheuvel
>> > <[email protected]> wrote:
>> >> On 16 November 2016 at 18:11, David Howells <[email protected]> wrote:
>> >>> From: Josh Boyer <[email protected]>
>> >>>
>> >>> 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.

2016-11-21 19:18:46

by Peter Jones

[permalink] [raw]

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

On Mon, Nov 21, 2016 at 08:06:44PM +0100, Ard Biesheuvel wrote:
> On 21 November 2016 at 20:05, Peter Jones <[email protected]> wrote:
> > On Mon, Nov 21, 2016 at 04:42:45PM +0000, Ard Biesheuvel wrote:
> >> On 21 November 2016 at 16:26, Josh Boyer <[email protected]> wrote:
> >> > On Mon, Nov 21, 2016 at 11:18 AM, Ard Biesheuvel
> >> > <[email protected]> wrote:
> >> >> On 16 November 2016 at 18:11, David Howells <[email protected]> wrote:
> >> >>> From: Josh Boyer <[email protected]>
> >> >>>
> >> >>> 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

2016-11-21 19:33:36

[permalink] [raw]

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

On 21 November 2016 at 20:18, Peter Jones <[email protected]> wrote:
> On Mon, Nov 21, 2016 at 08:06:44PM +0100, Ard Biesheuvel wrote:
>> On 21 November 2016 at 20:05, Peter Jones <[email protected]> wrote:
>> > On Mon, Nov 21, 2016 at 04:42:45PM +0000, Ard Biesheuvel wrote:
>> >> On 21 November 2016 at 16:26, Josh Boyer <[email protected]> wrote:
>> >> > On Mon, Nov 21, 2016 at 11:18 AM, Ard Biesheuvel
>> >> > <[email protected]> wrote:
>> >> >> On 16 November 2016 at 18:11, David Howells <[email protected]> wrote:
>> >> >>> From: Josh Boyer <[email protected]>
>> >> >>>
>> >> >>> 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.

2016-11-24 19:17:55

[permalink] [raw]

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

On Mon, 2016-11-21 at 16:16 +0000, Ard Biesheuvel wrote:
> On 16 November 2016 at 18:11, David Howells <[email protected]>
> wrote:
> > From: Josh Boyer <[email protected]>
> >
> > 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

2016-11-24 19:22:22

[permalink] [raw]

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

On Mon, 2016-11-21 at 11:25 -0500, Josh Boyer wrote:
> On Mon, Nov 21, 2016 at 11:16 AM, Ard Biesheuvel
> <[email protected]> wrote:
> > On 16 November 2016 at 18:11, David Howells <[email protected]>
> > wrote:
> > > From: Josh Boyer <[email protected]>
> > >
> > > 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

2016-12-02 19:05:02

[permalink] [raw]

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

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 <[email protected]>
> > wrote:
> > > From: Josh Boyer <[email protected]>
> > >
> > > 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

2016-12-02 20:18:56

[permalink] [raw]

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

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 <[email protected]>
> > > wrote:
> > > > From: Josh Boyer <[email protected]>
> > > >
> > > > 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?

2018-03-06 14:07:45

by Jiri Slaby

[permalink] [raw]

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

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

2018-03-07 13:19:54

[permalink] [raw]

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

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

2018-03-07 17:20:47

[permalink] [raw]

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

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

2018-03-11 03:22:44

by joeyli

[permalink] [raw]

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

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

2018-03-19 14:13:44