linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
@ 2020-09-16  0:49 Eric Snowberg
  2020-09-16 18:09 ` Jarkko Sakkinen
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: Eric Snowberg @ 2020-09-16  0:49 UTC (permalink / raw)
  To: dhowells, dwmw2, jarkko.sakkinen
  Cc: herbert, davem, jmorris, serge, nayna, zohar, eric.snowberg,
	erichte, mpe, keyrings, linux-kernel, linux-crypto,
	linux-security-module

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

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

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

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v4:
Remove unneeded symbol export found by Jarkko Sakkinen

v3:
Fixed an issue when CONFIG_PKCS7_MESSAGE_PARSER is not builtin and defined
as a module instead, pointed out by Randy Dunlap

v2: 
Fixed build issue reported by kernel test robot <lkp@intel.com>
Commit message update (suggested by Jarkko Sakkinen)
---
 certs/blacklist.c                             | 32 +++++++++++++++++++
 certs/blacklist.h                             | 12 +++++++
 certs/system_keyring.c                        |  6 ++++
 include/keys/system_keyring.h                 | 11 +++++++
 .../platform_certs/keyring_handler.c          | 11 +++++++
 5 files changed, 72 insertions(+)

diff --git a/certs/blacklist.c b/certs/blacklist.c
index 6514f9ebc943..4adac7f8fd94 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -100,6 +100,38 @@ int mark_hash_blacklisted(const char *hash)
 	return 0;
 }
 
+int mark_key_revocationlisted(const char *data, size_t size)
+{
+	key_ref_t key;
+
+	key = key_create_or_update(make_key_ref(blacklist_keyring, true),
+				   "asymmetric",
+				   NULL,
+				   data,
+				   size,
+				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
+				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
+
+	if (IS_ERR(key)) {
+		pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
+		return PTR_ERR(key);
+	}
+
+	return 0;
+}
+
+int is_key_revocationlisted(struct pkcs7_message *pkcs7)
+{
+	int ret;
+
+	ret = validate_trust(pkcs7, blacklist_keyring);
+
+	if (ret == 0)
+		return -EKEYREJECTED;
+
+	return -ENOKEY;
+}
+
 /**
  * is_hash_blacklisted - Determine if a hash is blacklisted
  * @hash: The hash to be checked as a binary blob
diff --git a/certs/blacklist.h b/certs/blacklist.h
index 1efd6fa0dc60..420bb7c86e07 100644
--- a/certs/blacklist.h
+++ b/certs/blacklist.h
@@ -1,3 +1,15 @@
 #include <linux/kernel.h>
+#include <linux/errno.h>
+#include <crypto/pkcs7.h>
 
 extern const char __initconst *const blacklist_hashes[];
+
+#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
+#define validate_trust pkcs7_validate_trust
+#else
+static inline int validate_trust(struct pkcs7_message *pkcs7,
+				 struct key *trust_keyring)
+{
+	return -ENOKEY;
+}
+#endif
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 798291177186..f8ea96219155 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -241,6 +241,12 @@ int verify_pkcs7_message_sig(const void *data, size_t len,
 			pr_devel("PKCS#7 platform keyring is not available\n");
 			goto error;
 		}
+
+		ret = is_key_revocationlisted(pkcs7);
+		if (ret != -ENOKEY) {
+			pr_devel("PKCS#7 platform key revocationlisted\n");
+			goto error;
+		}
 	}
 	ret = pkcs7_validate_trust(pkcs7, trusted_keys);
 	if (ret < 0) {
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index fb8b07daa9d1..b6991cfe1b6d 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -31,11 +31,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
 #define restrict_link_by_builtin_and_secondary_trusted restrict_link_by_builtin_trusted
 #endif
 
+extern struct pkcs7_message *pkcs7;
 #ifdef CONFIG_SYSTEM_BLACKLIST_KEYRING
 extern int mark_hash_blacklisted(const char *hash);
+extern int mark_key_revocationlisted(const char *data, size_t size);
 extern int is_hash_blacklisted(const u8 *hash, size_t hash_len,
 			       const char *type);
 extern int is_binary_blacklisted(const u8 *hash, size_t hash_len);
+extern int is_key_revocationlisted(struct pkcs7_message *pkcs7);
 #else
 static inline int is_hash_blacklisted(const u8 *hash, size_t hash_len,
 				      const char *type)
@@ -47,6 +50,14 @@ static inline int is_binary_blacklisted(const u8 *hash, size_t hash_len)
 {
 	return 0;
 }
+static inline int mark_key_revocationlisted(const char *data, size_t size)
+{
+	return 0;
+}
+static inline int is_key_revocationlisted(struct pkcs7_message *pkcs7)
+{
+	return -ENOKEY;
+}
 #endif
 
 #ifdef CONFIG_IMA_BLACKLIST_KEYRING
diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
index c5ba695c10e3..cc5a43804bc4 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -55,6 +55,15 @@ static __init void uefi_blacklist_binary(const char *source,
 	uefi_blacklist_hash(source, data, len, "bin:", 4);
 }
 
+/*
+ * Revocationlist the X509 cert
+ */
+static __init void uefi_revocationlist_x509(const char *source,
+					    const void *data, size_t len)
+{
+	mark_key_revocationlisted(data, len);
+}
+
 /*
  * Return the appropriate handler for particular signature list types found in
  * the UEFI db and MokListRT tables.
@@ -76,5 +85,7 @@ __init efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_type)
 		return uefi_blacklist_x509_tbs;
 	if (efi_guidcmp(*sig_type, efi_cert_sha256_guid) == 0)
 		return uefi_blacklist_binary;
+	if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
+		return uefi_revocationlist_x509;
 	return 0;
 }
-- 
2.18.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2020-09-16  0:49 [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries Eric Snowberg
@ 2020-09-16 18:09 ` Jarkko Sakkinen
  2020-12-10  9:49 ` David Howells
  2021-01-15 17:21 ` James Bottomley
  2 siblings, 0 replies; 22+ messages in thread
From: Jarkko Sakkinen @ 2020-09-16 18:09 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: dhowells, dwmw2, herbert, davem, jmorris, serge, nayna, zohar,
	erichte, mpe, keyrings, linux-kernel, linux-crypto,
	linux-security-module

On Tue, Sep 15, 2020 at 08:49:27PM -0400, Eric Snowberg wrote:
> The Secure Boot Forbidden Signature Database, dbx, contains a list of now
> revoked signatures and keys previously approved to boot with UEFI Secure
> Boot enabled.  The dbx is capable of containing any number of
> EFI_CERT_X509_SHA256_GUID, EFI_CERT_SHA256_GUID, and EFI_CERT_X509_GUID
> entries.
> 
> Currently when EFI_CERT_X509_GUID are contained in the dbx, the entries are
> skipped.
> 
> Add support for EFI_CERT_X509_GUID dbx entries. When a EFI_CERT_X509_GUID
> is found, it is added as an asymmetrical key to the .blacklist keyring.
> Anytime the .platform keyring is used, the keys in the .blacklist keyring
> are referenced, if a matching key is found, the key will be rejected.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

Looks good to me.

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2020-09-16  0:49 [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries Eric Snowberg
  2020-09-16 18:09 ` Jarkko Sakkinen
@ 2020-12-10  9:49 ` David Howells
  2020-12-10 18:56   ` Eric Snowberg
                     ` (2 more replies)
  2021-01-15 17:21 ` James Bottomley
  2 siblings, 3 replies; 22+ messages in thread
From: David Howells @ 2020-12-10  9:49 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: dhowells, dwmw2, jarkko.sakkinen, herbert, davem, jmorris, serge,
	nayna, zohar, erichte, mpe, keyrings, linux-kernel, linux-crypto,
	linux-security-module

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

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

Ummm...  Why this way and not as a blacklist key which takes up less space?
I'm guessing that you're using the key chain matching logic.  We really only
need to blacklist the key IDs.

Also, what should happen if a revocation cert rejected by the blacklist?

> +int mark_key_revocationlisted(const char *data, size_t size)

Hmmm...  The name looks wrong, but I can see the potential issue that kernel
keys can actually be marked revoked as a separate concept.  How about
add_key_to_revocation_list() and is_key_on_revocation_list().

David


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2020-12-10  9:49 ` David Howells
@ 2020-12-10 18:56   ` Eric Snowberg
  2021-01-12 14:57   ` David Howells
  2021-01-12 17:10   ` David Howells
  2 siblings, 0 replies; 22+ messages in thread
From: Eric Snowberg @ 2020-12-10 18:56 UTC (permalink / raw)
  To: David Howells
  Cc: dwmw2, Jarkko Sakkinen, herbert, davem, jmorris, serge, nayna,
	Mimi Zohar, erichte, mpe, keyrings, linux-kernel, linux-crypto,
	linux-security-module, Eric Snowberg


> On Dec 10, 2020, at 2:49 AM, David Howells <dhowells@redhat.com> wrote:
> 
> Eric Snowberg <eric.snowberg@oracle.com> wrote:
> 
>> Add support for EFI_CERT_X509_GUID dbx entries. When a EFI_CERT_X509_GUID
>> is found, it is added as an asymmetrical key to the .blacklist keyring.
>> Anytime the .platform keyring is used, the keys in the .blacklist keyring
>> are referenced, if a matching key is found, the key will be rejected.
> 
> Ummm...  Why this way and not as a blacklist key which takes up less space?
> I'm guessing that you're using the key chain matching logic.  We really only
> need to blacklist the key IDs.

I implemented it this way so that certs in the dbx would only impact 
the .platform keyring. I was under the impression we didn’t want to have 
Secure Boot UEFI db/dbx certs dictate keyring functionality within the kernel
itself. Meaning if we have a matching dbx cert in any other keyring (builtin,
secondary, ima, etc.), it would be allowed. If that is not how you’d like to 
see it done, let me know and I’ll make the change.

> Also, what should happen if a revocation cert rejected by the blacklist?

I’m not sure I understand the question. How would it be rejected?

>> +int mark_key_revocationlisted(const char *data, size_t size)
> 
> Hmmm...  The name looks wrong, but I can see the potential issue that kernel
> keys can actually be marked revoked as a separate concept.  How about
> add_key_to_revocation_list() and is_key_on_revocation_list().

I'll update the names in the next version.

Thanks.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2020-12-10  9:49 ` David Howells
  2020-12-10 18:56   ` Eric Snowberg
@ 2021-01-12 14:57   ` David Howells
  2021-01-13 20:41     ` Jarkko Sakkinen
  2021-01-12 17:10   ` David Howells
  2 siblings, 1 reply; 22+ messages in thread
From: David Howells @ 2021-01-12 14:57 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: dhowells, dwmw2, Jarkko Sakkinen, herbert, davem, jmorris, serge,
	nayna, Mimi Zohar, erichte, mpe, keyrings, linux-kernel,
	linux-crypto, linux-security-module

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

> > On Dec 10, 2020, at 2:49 AM, David Howells <dhowells@redhat.com> wrote:
> > 
> > Eric Snowberg <eric.snowberg@oracle.com> wrote:
> > 
> >> Add support for EFI_CERT_X509_GUID dbx entries. When a EFI_CERT_X509_GUID
> >> is found, it is added as an asymmetrical key to the .blacklist keyring.
> >> Anytime the .platform keyring is used, the keys in the .blacklist keyring
> >> are referenced, if a matching key is found, the key will be rejected.
> > 
> > Ummm...  Why this way and not as a blacklist key which takes up less space?
> > I'm guessing that you're using the key chain matching logic.  We really only
> > need to blacklist the key IDs.
> 
> I implemented it this way so that certs in the dbx would only impact 
> the .platform keyring. I was under the impression we didn’t want to have 
> Secure Boot UEFI db/dbx certs dictate keyring functionality within the kernel
> itself. Meaning if we have a matching dbx cert in any other keyring (builtin,
> secondary, ima, etc.), it would be allowed. If that is not how you’d like to 
> see it done, let me know and I’ll make the change.

I wonder if that is that the right thing to do.  I guess this is a policy
decision and may depend on the particular user.

> > Also, what should happen if a revocation cert rejected by the blacklist?
> 
> I’m not sure I understand the question. How would it be rejected?

The SHA256 of a revocation cert being loaded could match an
already-blacklisted SHA256 sum, either compiled in or already loaded from
UEFI.

David


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2020-12-10  9:49 ` David Howells
  2020-12-10 18:56   ` Eric Snowberg
  2021-01-12 14:57   ` David Howells
@ 2021-01-12 17:10   ` David Howells
  2021-01-12 19:13     ` Eric Snowberg
  2 siblings, 1 reply; 22+ messages in thread
From: David Howells @ 2021-01-12 17:10 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: dhowells, dwmw2, Jarkko Sakkinen, herbert, davem, jmorris, serge,
	nayna, Mimi Zohar, erichte, mpe, keyrings, linux-kernel,
	linux-crypto, linux-security-module

How about the attached?  I've changed the function names to something that I
think reads better, but otherwise it's the same.

David
---
commit 8913866babb96fcfe452aac6042ca8862d4c0b53
Author: Eric Snowberg <eric.snowberg@oracle.com>
Date:   Tue Sep 15 20:49:27 2020 -0400

    certs: Add EFI_CERT_X509_GUID support for dbx entries
    
    The Secure Boot Forbidden Signature Database, dbx, contains a list of now
    revoked signatures and keys previously approved to boot with UEFI Secure
    Boot enabled.  The dbx is capable of containing any number of
    EFI_CERT_X509_SHA256_GUID, EFI_CERT_SHA256_GUID, and EFI_CERT_X509_GUID
    entries.
    
    Currently when EFI_CERT_X509_GUID are contained in the dbx, the entries are
    skipped.
    
    Add support for EFI_CERT_X509_GUID dbx entries. When a EFI_CERT_X509_GUID
    is found, it is added as an asymmetrical key to the .blacklist keyring.
    Anytime the .platform keyring is used, the keys in the .blacklist keyring
    are referenced, if a matching key is found, the key will be rejected.
    
    Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
    Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
    Signed-off-by: David Howells <dhowells@redhat.com>

diff --git a/certs/blacklist.c b/certs/blacklist.c
index 6514f9ebc943..a7f021878a4b 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -100,6 +100,38 @@ int mark_hash_blacklisted(const char *hash)
 	return 0;
 }
 
+int add_key_to_revocation_list(const char *data, size_t size)
+{
+	key_ref_t key;
+
+	key = key_create_or_update(make_key_ref(blacklist_keyring, true),
+				   "asymmetric",
+				   NULL,
+				   data,
+				   size,
+				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
+				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
+
+	if (IS_ERR(key)) {
+		pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
+		return PTR_ERR(key);
+	}
+
+	return 0;
+}
+
+int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
+{
+	int ret;
+
+	ret = validate_trust(pkcs7, blacklist_keyring);
+
+	if (ret == 0)
+		return -EKEYREJECTED;
+
+	return -ENOKEY;
+}
+
 /**
  * is_hash_blacklisted - Determine if a hash is blacklisted
  * @hash: The hash to be checked as a binary blob
diff --git a/certs/blacklist.h b/certs/blacklist.h
index 1efd6fa0dc60..420bb7c86e07 100644
--- a/certs/blacklist.h
+++ b/certs/blacklist.h
@@ -1,3 +1,15 @@
 #include <linux/kernel.h>
+#include <linux/errno.h>
+#include <crypto/pkcs7.h>
 
 extern const char __initconst *const blacklist_hashes[];
+
+#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
+#define validate_trust pkcs7_validate_trust
+#else
+static inline int validate_trust(struct pkcs7_message *pkcs7,
+				 struct key *trust_keyring)
+{
+	return -ENOKEY;
+}
+#endif
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 798291177186..cc165b359ea3 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -241,6 +241,12 @@ int verify_pkcs7_message_sig(const void *data, size_t len,
 			pr_devel("PKCS#7 platform keyring is not available\n");
 			goto error;
 		}
+
+		ret = is_key_on_revocation_list(pkcs7);
+		if (ret != -ENOKEY) {
+			pr_devel("PKCS#7 platform key is on revocation list\n");
+			goto error;
+		}
 	}
 	ret = pkcs7_validate_trust(pkcs7, trusted_keys);
 	if (ret < 0) {
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index fb8b07daa9d1..61f98739e8b1 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -31,11 +31,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
 #define restrict_link_by_builtin_and_secondary_trusted restrict_link_by_builtin_trusted
 #endif
 
+extern struct pkcs7_message *pkcs7;
 #ifdef CONFIG_SYSTEM_BLACKLIST_KEYRING
 extern int mark_hash_blacklisted(const char *hash);
+extern int add_key_to_revocation_list(const char *data, size_t size);
 extern int is_hash_blacklisted(const u8 *hash, size_t hash_len,
 			       const char *type);
 extern int is_binary_blacklisted(const u8 *hash, size_t hash_len);
+extern int is_key_on_revocation_list(struct pkcs7_message *pkcs7);
 #else
 static inline int is_hash_blacklisted(const u8 *hash, size_t hash_len,
 				      const char *type)
@@ -47,6 +50,14 @@ static inline int is_binary_blacklisted(const u8 *hash, size_t hash_len)
 {
 	return 0;
 }
+static inline int add_key_to_revocation_list(const char *data, size_t size)
+{
+	return 0;
+}
+static inline int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
+{
+	return -ENOKEY;
+}
 #endif
 
 #ifdef CONFIG_IMA_BLACKLIST_KEYRING
diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
index c5ba695c10e3..5604bd57c990 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -55,6 +55,15 @@ static __init void uefi_blacklist_binary(const char *source,
 	uefi_blacklist_hash(source, data, len, "bin:", 4);
 }
 
+/*
+ * Add an X509 cert to the revocation list.
+ */
+static __init void uefi_revocation_list_x509(const char *source,
+					     const void *data, size_t len)
+{
+	add_key_to_revocation_list(data, len);
+}
+
 /*
  * Return the appropriate handler for particular signature list types found in
  * the UEFI db and MokListRT tables.
@@ -76,5 +85,7 @@ __init efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_type)
 		return uefi_blacklist_x509_tbs;
 	if (efi_guidcmp(*sig_type, efi_cert_sha256_guid) == 0)
 		return uefi_blacklist_binary;
+	if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
+		return uefi_revocation_list_x509;
 	return 0;
 }


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2021-01-12 17:10   ` David Howells
@ 2021-01-12 19:13     ` Eric Snowberg
  0 siblings, 0 replies; 22+ messages in thread
From: Eric Snowberg @ 2021-01-12 19:13 UTC (permalink / raw)
  To: David Howells
  Cc: dwmw2, Jarkko Sakkinen, herbert, davem, jmorris, serge, nayna,
	Mimi Zohar, erichte, mpe, keyrings, linux-kernel, linux-crypto,
	linux-security-module


> On Jan 12, 2021, at 10:10 AM, David Howells <dhowells@redhat.com> wrote:
> 
> How about the attached?

This looks good to me.

> I've changed the function names to something that I
> think reads better, but otherwise it's the same.

I agree, the function name changes you made sound better.

We are starting to see platforms with KEK signed DBX updates containing
certs like this following boothole, so it would be great if we could get
something like this in.  I also had a follow on series that allowed these
certs to be compiled into the kernel.

https://lkml.org/lkml/2020/9/30/1301

I’d appreciate any feedback on that series as well.

Thanks

> David
> ---
> commit 8913866babb96fcfe452aac6042ca8862d4c0b53
> Author: Eric Snowberg <eric.snowberg@oracle.com>
> Date:   Tue Sep 15 20:49:27 2020 -0400
> 
>    certs: Add EFI_CERT_X509_GUID support for dbx entries
> 
>    The Secure Boot Forbidden Signature Database, dbx, contains a list of now
>    revoked signatures and keys previously approved to boot with UEFI Secure
>    Boot enabled.  The dbx is capable of containing any number of
>    EFI_CERT_X509_SHA256_GUID, EFI_CERT_SHA256_GUID, and EFI_CERT_X509_GUID
>    entries.
> 
>    Currently when EFI_CERT_X509_GUID are contained in the dbx, the entries are
>    skipped.
> 
>    Add support for EFI_CERT_X509_GUID dbx entries. When a EFI_CERT_X509_GUID
>    is found, it is added as an asymmetrical key to the .blacklist keyring.
>    Anytime the .platform keyring is used, the keys in the .blacklist keyring
>    are referenced, if a matching key is found, the key will be rejected.
> 
>    Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>    Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>    Signed-off-by: David Howells <dhowells@redhat.com>
> 
> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index 6514f9ebc943..a7f021878a4b 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c
> @@ -100,6 +100,38 @@ int mark_hash_blacklisted(const char *hash)
> 	return 0;
> }
> 
> +int add_key_to_revocation_list(const char *data, size_t size)
> +{
> +	key_ref_t key;
> +
> +	key = key_create_or_update(make_key_ref(blacklist_keyring, true),
> +				   "asymmetric",
> +				   NULL,
> +				   data,
> +				   size,
> +				   ((KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW),
> +				   KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_BUILT_IN);
> +
> +	if (IS_ERR(key)) {
> +		pr_err("Problem with revocation key (%ld)\n", PTR_ERR(key));
> +		return PTR_ERR(key);
> +	}
> +
> +	return 0;
> +}
> +
> +int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
> +{
> +	int ret;
> +
> +	ret = validate_trust(pkcs7, blacklist_keyring);
> +
> +	if (ret == 0)
> +		return -EKEYREJECTED;
> +
> +	return -ENOKEY;
> +}
> +
> /**
>  * is_hash_blacklisted - Determine if a hash is blacklisted
>  * @hash: The hash to be checked as a binary blob
> diff --git a/certs/blacklist.h b/certs/blacklist.h
> index 1efd6fa0dc60..420bb7c86e07 100644
> --- a/certs/blacklist.h
> +++ b/certs/blacklist.h
> @@ -1,3 +1,15 @@
> #include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <crypto/pkcs7.h>
> 
> extern const char __initconst *const blacklist_hashes[];
> +
> +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> +#define validate_trust pkcs7_validate_trust
> +#else
> +static inline int validate_trust(struct pkcs7_message *pkcs7,
> +				 struct key *trust_keyring)
> +{
> +	return -ENOKEY;
> +}
> +#endif
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 798291177186..cc165b359ea3 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -241,6 +241,12 @@ int verify_pkcs7_message_sig(const void *data, size_t len,
> 			pr_devel("PKCS#7 platform keyring is not available\n");
> 			goto error;
> 		}
> +
> +		ret = is_key_on_revocation_list(pkcs7);
> +		if (ret != -ENOKEY) {
> +			pr_devel("PKCS#7 platform key is on revocation list\n");
> +			goto error;
> +		}
> 	}
> 	ret = pkcs7_validate_trust(pkcs7, trusted_keys);
> 	if (ret < 0) {
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index fb8b07daa9d1..61f98739e8b1 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -31,11 +31,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
> #define restrict_link_by_builtin_and_secondary_trusted restrict_link_by_builtin_trusted
> #endif
> 
> +extern struct pkcs7_message *pkcs7;
> #ifdef CONFIG_SYSTEM_BLACKLIST_KEYRING
> extern int mark_hash_blacklisted(const char *hash);
> +extern int add_key_to_revocation_list(const char *data, size_t size);
> extern int is_hash_blacklisted(const u8 *hash, size_t hash_len,
> 			       const char *type);
> extern int is_binary_blacklisted(const u8 *hash, size_t hash_len);
> +extern int is_key_on_revocation_list(struct pkcs7_message *pkcs7);
> #else
> static inline int is_hash_blacklisted(const u8 *hash, size_t hash_len,
> 				      const char *type)
> @@ -47,6 +50,14 @@ static inline int is_binary_blacklisted(const u8 *hash, size_t hash_len)
> {
> 	return 0;
> }
> +static inline int add_key_to_revocation_list(const char *data, size_t size)
> +{
> +	return 0;
> +}
> +static inline int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
> +{
> +	return -ENOKEY;
> +}
> #endif
> 
> #ifdef CONFIG_IMA_BLACKLIST_KEYRING
> diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
> index c5ba695c10e3..5604bd57c990 100644
> --- a/security/integrity/platform_certs/keyring_handler.c
> +++ b/security/integrity/platform_certs/keyring_handler.c
> @@ -55,6 +55,15 @@ static __init void uefi_blacklist_binary(const char *source,
> 	uefi_blacklist_hash(source, data, len, "bin:", 4);
> }
> 
> +/*
> + * Add an X509 cert to the revocation list.
> + */
> +static __init void uefi_revocation_list_x509(const char *source,
> +					     const void *data, size_t len)
> +{
> +	add_key_to_revocation_list(data, len);
> +}
> +
> /*
>  * Return the appropriate handler for particular signature list types found in
>  * the UEFI db and MokListRT tables.
> @@ -76,5 +85,7 @@ __init efi_element_handler_t get_handler_for_dbx(const efi_guid_t *sig_type)
> 		return uefi_blacklist_x509_tbs;
> 	if (efi_guidcmp(*sig_type, efi_cert_sha256_guid) == 0)
> 		return uefi_blacklist_binary;
> +	if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0)
> +		return uefi_revocation_list_x509;
> 	return 0;
> }
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2021-01-12 14:57   ` David Howells
@ 2021-01-13 20:41     ` Jarkko Sakkinen
  2021-01-14  0:11       ` Eric Snowberg
  0 siblings, 1 reply; 22+ messages in thread
From: Jarkko Sakkinen @ 2021-01-13 20:41 UTC (permalink / raw)
  To: David Howells
  Cc: Eric Snowberg, dwmw2, herbert, davem, jmorris, serge, nayna,
	Mimi Zohar, erichte, mpe, keyrings, linux-kernel, linux-crypto,
	linux-security-module

On Tue, Jan 12, 2021 at 02:57:39PM +0000, David Howells wrote:
> Eric Snowberg <eric.snowberg@oracle.com> wrote:
> 
> > > On Dec 10, 2020, at 2:49 AM, David Howells <dhowells@redhat.com> wrote:
> > > 
> > > Eric Snowberg <eric.snowberg@oracle.com> wrote:
> > > 
> > >> Add support for EFI_CERT_X509_GUID dbx entries. When a EFI_CERT_X509_GUID
> > >> is found, it is added as an asymmetrical key to the .blacklist keyring.
> > >> Anytime the .platform keyring is used, the keys in the .blacklist keyring
> > >> are referenced, if a matching key is found, the key will be rejected.
> > > 
> > > Ummm...  Why this way and not as a blacklist key which takes up less space?
> > > I'm guessing that you're using the key chain matching logic.  We really only
> > > need to blacklist the key IDs.
> > 
> > I implemented it this way so that certs in the dbx would only impact 
> > the .platform keyring. I was under the impression we didn’t want to have 
> > Secure Boot UEFI db/dbx certs dictate keyring functionality within the kernel
> > itself. Meaning if we have a matching dbx cert in any other keyring (builtin,
> > secondary, ima, etc.), it would be allowed. If that is not how you’d like to 
> > see it done, let me know and I’ll make the change.
> 
> I wonder if that is that the right thing to do.  I guess this is a policy
> decision and may depend on the particular user.

Why would you want to allow dbx entry in any keyring?

/Jarkko

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2021-01-13 20:41     ` Jarkko Sakkinen
@ 2021-01-14  0:11       ` Eric Snowberg
  2021-01-15  9:15         ` Jarkko Sakkinen
  0 siblings, 1 reply; 22+ messages in thread
From: Eric Snowberg @ 2021-01-14  0:11 UTC (permalink / raw)
  To: Jarkko Sakkinen, David Howells
  Cc: dwmw2, herbert, davem, jmorris, serge, nayna, Mimi Zohar,
	erichte, mpe, keyrings, linux-kernel, linux-crypto,
	linux-security-module, James.Bottomley


> On Jan 13, 2021, at 1:41 PM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> 
> On Tue, Jan 12, 2021 at 02:57:39PM +0000, David Howells wrote:
>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
>> 
>>>> On Dec 10, 2020, at 2:49 AM, David Howells <dhowells@redhat.com> wrote:
>>>> 
>>>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
>>>> 
>>>>> Add support for EFI_CERT_X509_GUID dbx entries. When a EFI_CERT_X509_GUID
>>>>> is found, it is added as an asymmetrical key to the .blacklist keyring.
>>>>> Anytime the .platform keyring is used, the keys in the .blacklist keyring
>>>>> are referenced, if a matching key is found, the key will be rejected.
>>>> 
>>>> Ummm...  Why this way and not as a blacklist key which takes up less space?
>>>> I'm guessing that you're using the key chain matching logic.  We really only
>>>> need to blacklist the key IDs.
>>> 
>>> I implemented it this way so that certs in the dbx would only impact 
>>> the .platform keyring. I was under the impression we didn’t want to have 
>>> Secure Boot UEFI db/dbx certs dictate keyring functionality within the kernel
>>> itself. Meaning if we have a matching dbx cert in any other keyring (builtin,
>>> secondary, ima, etc.), it would be allowed. If that is not how you’d like to 
>>> see it done, let me know and I’ll make the change.
>> 
>> I wonder if that is that the right thing to do.  I guess this is a policy
>> decision and may depend on the particular user.
> 
> Why would you want to allow dbx entry in any keyring?

Today, DB and MOK certs go into the platform keyring.  These certs are only
referenced during kexec.  They can’t be used for other things like validating
kernel module signatures.  If we follow the same pattern, the DBX and MOKX entries
in the blacklist keyring should only impact kexec. 

Currently, Mickaël Salaün has another outstanding series to allow root to update 
the blacklist keyring.  I assume the use case for this is around certificates used 
within the kernel, for example revoking kernel module signatures.  The question I have
is, should another keyring be introduced?  One that carries DBX and MOKX, which just
correspond to certs/hashes in the platform keyring; this keyring would only be
referenced for kexec, just like the platform keyring is today. Then, the current
blacklist keyring would be used for everything internal to the kernel.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2021-01-14  0:11       ` Eric Snowberg
@ 2021-01-15  9:15         ` Jarkko Sakkinen
  2021-01-15 16:49           ` Eric Snowberg
  0 siblings, 1 reply; 22+ messages in thread
From: Jarkko Sakkinen @ 2021-01-15  9:15 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: Jarkko Sakkinen, David Howells, dwmw2, herbert, davem, jmorris,
	serge, nayna, Mimi Zohar, erichte, mpe, keyrings, linux-kernel,
	linux-crypto, linux-security-module, James.Bottomley

On Wed, Jan 13, 2021 at 05:11:10PM -0700, Eric Snowberg wrote:
> 
> > On Jan 13, 2021, at 1:41 PM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > 
> > On Tue, Jan 12, 2021 at 02:57:39PM +0000, David Howells wrote:
> >> Eric Snowberg <eric.snowberg@oracle.com> wrote:
> >> 
> >>>> On Dec 10, 2020, at 2:49 AM, David Howells <dhowells@redhat.com> wrote:
> >>>> 
> >>>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
> >>>> 
> >>>>> Add support for EFI_CERT_X509_GUID dbx entries. When a EFI_CERT_X509_GUID
> >>>>> is found, it is added as an asymmetrical key to the .blacklist keyring.
> >>>>> Anytime the .platform keyring is used, the keys in the .blacklist keyring
> >>>>> are referenced, if a matching key is found, the key will be rejected.
> >>>> 
> >>>> Ummm...  Why this way and not as a blacklist key which takes up less space?
> >>>> I'm guessing that you're using the key chain matching logic.  We really only
> >>>> need to blacklist the key IDs.
> >>> 
> >>> I implemented it this way so that certs in the dbx would only impact 
> >>> the .platform keyring. I was under the impression we didn’t want to have 
> >>> Secure Boot UEFI db/dbx certs dictate keyring functionality within the kernel
> >>> itself. Meaning if we have a matching dbx cert in any other keyring (builtin,
> >>> secondary, ima, etc.), it would be allowed. If that is not how you’d like to 
> >>> see it done, let me know and I’ll make the change.
> >> 
> >> I wonder if that is that the right thing to do.  I guess this is a policy
> >> decision and may depend on the particular user.
> > 
> > Why would you want to allow dbx entry in any keyring?
> 
> Today, DB and MOK certs go into the platform keyring.  These certs are only
> referenced during kexec.  They can’t be used for other things like validating
> kernel module signatures.  If we follow the same pattern, the DBX and MOKX entries
> in the blacklist keyring should only impact kexec. 
> 
> Currently, Mickaël Salaün has another outstanding series to allow root to update 
> the blacklist keyring.  I assume the use case for this is around certificates used 
> within the kernel, for example revoking kernel module signatures.  The question I have
> is, should another keyring be introduced?  One that carries DBX and MOKX, which just
> correspond to certs/hashes in the platform keyring; this keyring would only be
> referenced for kexec, just like the platform keyring is today. Then, the current
> blacklist keyring would be used for everything internal to the kernel.

Right, I'm following actively that series.

Why couldn't user space drive this process and use that feature to do it?

/Jarkko

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2021-01-15  9:15         ` Jarkko Sakkinen
@ 2021-01-15 16:49           ` Eric Snowberg
  2021-01-20 11:26             ` Jarkko Sakkinen
  0 siblings, 1 reply; 22+ messages in thread
From: Eric Snowberg @ 2021-01-15 16:49 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Jarkko Sakkinen, David Howells, dwmw2, herbert, davem, jmorris,
	serge, nayna, Mimi Zohar, erichte, mpe, keyrings, linux-kernel,
	linux-crypto, linux-security-module, James.Bottomley,
	Eric Snowberg


> On Jan 15, 2021, at 2:15 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Wed, Jan 13, 2021 at 05:11:10PM -0700, Eric Snowberg wrote:
>> 
>>> On Jan 13, 2021, at 1:41 PM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
>>> 
>>> On Tue, Jan 12, 2021 at 02:57:39PM +0000, David Howells wrote:
>>>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
>>>> 
>>>>>> On Dec 10, 2020, at 2:49 AM, David Howells <dhowells@redhat.com> wrote:
>>>>>> 
>>>>>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
>>>>>> 
>>>>>>> Add support for EFI_CERT_X509_GUID dbx entries. When a EFI_CERT_X509_GUID
>>>>>>> is found, it is added as an asymmetrical key to the .blacklist keyring.
>>>>>>> Anytime the .platform keyring is used, the keys in the .blacklist keyring
>>>>>>> are referenced, if a matching key is found, the key will be rejected.
>>>>>> 
>>>>>> Ummm...  Why this way and not as a blacklist key which takes up less space?
>>>>>> I'm guessing that you're using the key chain matching logic.  We really only
>>>>>> need to blacklist the key IDs.
>>>>> 
>>>>> I implemented it this way so that certs in the dbx would only impact 
>>>>> the .platform keyring. I was under the impression we didn’t want to have 
>>>>> Secure Boot UEFI db/dbx certs dictate keyring functionality within the kernel
>>>>> itself. Meaning if we have a matching dbx cert in any other keyring (builtin,
>>>>> secondary, ima, etc.), it would be allowed. If that is not how you’d like to 
>>>>> see it done, let me know and I’ll make the change.
>>>> 
>>>> I wonder if that is that the right thing to do.  I guess this is a policy
>>>> decision and may depend on the particular user.
>>> 
>>> Why would you want to allow dbx entry in any keyring?
>> 
>> Today, DB and MOK certs go into the platform keyring.  These certs are only
>> referenced during kexec.  They can’t be used for other things like validating
>> kernel module signatures.  If we follow the same pattern, the DBX and MOKX entries
>> in the blacklist keyring should only impact kexec. 
>> 
>> Currently, Mickaël Salaün has another outstanding series to allow root to update 
>> the blacklist keyring.  I assume the use case for this is around certificates used 
>> within the kernel, for example revoking kernel module signatures.  The question I have
>> is, should another keyring be introduced?  One that carries DBX and MOKX, which just
>> correspond to certs/hashes in the platform keyring; this keyring would only be
>> referenced for kexec, just like the platform keyring is today. Then, the current
>> blacklist keyring would be used for everything internal to the kernel.
> 
> Right, I'm following actively that series.
> 
> Why couldn't user space drive this process and use that feature to do it?

I could see where the user would want to use both. With Mickaël Salaün’s
series, the blacklist keyring is updated immediately.  However it does
not survive a reboot.  With my patch, the blacklist keyring is updated
during boot, based on what is in the dbx. Neither approach needs a new 
kernel build.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2020-09-16  0:49 [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries Eric Snowberg
  2020-09-16 18:09 ` Jarkko Sakkinen
  2020-12-10  9:49 ` David Howells
@ 2021-01-15 17:21 ` James Bottomley
  2021-01-15 23:01   ` Eric Snowberg
  2 siblings, 1 reply; 22+ messages in thread
From: James Bottomley @ 2021-01-15 17:21 UTC (permalink / raw)
  To: Eric Snowberg, dhowells, dwmw2, jarkko.sakkinen
  Cc: herbert, davem, jmorris, serge, nayna, zohar, erichte, mpe,
	keyrings, linux-kernel, linux-crypto, linux-security-module

On Tue, 2020-09-15 at 20:49 -0400, Eric Snowberg wrote:
> The Secure Boot Forbidden Signature Database, dbx, contains a list of
> now revoked signatures and keys previously approved to boot with UEFI
> Secure Boot enabled.  The dbx is capable of containing any number of
> EFI_CERT_X509_SHA256_GUID, EFI_CERT_SHA256_GUID, and
> EFI_CERT_X509_GUID entries.
> 
> Currently when EFI_CERT_X509_GUID are contained in the dbx, the
> entries are skipped.
> 
> Add support for EFI_CERT_X509_GUID dbx entries. When a
> EFI_CERT_X509_GUID is found, it is added as an asymmetrical key to
> the .blacklist keyring. Anytime the .platform keyring is used, the
> keys in the .blacklist keyring are referenced, if a matching key is
> found, the key will be rejected.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

If you're using shim, as most of our users are, you have no access to
dbx to blacklist certificates.  Plus our security envelope includes the
Mok variables, so you should also be paying attestion to MokListX (or
it's RT equivalent: MokListXRT).

If you add this to the patch, we get something that is mechanistically
complete and which also allows users to add certs to their Mok
blacklist.

James



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2021-01-15 17:21 ` James Bottomley
@ 2021-01-15 23:01   ` Eric Snowberg
  0 siblings, 0 replies; 22+ messages in thread
From: Eric Snowberg @ 2021-01-15 23:01 UTC (permalink / raw)
  To: James Bottomley
  Cc: David Howells, dwmw2, Jarkko Sakkinen, herbert, davem, jmorris,
	serge, nayna, Mimi Zohar, erichte, mpe, keyrings, linux-kernel,
	linux-crypto, linux-security-module


> On Jan 15, 2021, at 10:21 AM, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> On Tue, 2020-09-15 at 20:49 -0400, Eric Snowberg wrote:
>> The Secure Boot Forbidden Signature Database, dbx, contains a list of
>> now revoked signatures and keys previously approved to boot with UEFI
>> Secure Boot enabled.  The dbx is capable of containing any number of
>> EFI_CERT_X509_SHA256_GUID, EFI_CERT_SHA256_GUID, and
>> EFI_CERT_X509_GUID entries.
>> 
>> Currently when EFI_CERT_X509_GUID are contained in the dbx, the
>> entries are skipped.
>> 
>> Add support for EFI_CERT_X509_GUID dbx entries. When a
>> EFI_CERT_X509_GUID is found, it is added as an asymmetrical key to
>> the .blacklist keyring. Anytime the .platform keyring is used, the
>> keys in the .blacklist keyring are referenced, if a matching key is
>> found, the key will be rejected.
>> 
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> 
> If you're using shim, as most of our users are, you have no access to
> dbx to blacklist certificates.  Plus our security envelope includes the
> Mok variables, so you should also be paying attestion to MokListX (or
> it's RT equivalent: MokListXRT).
> 
> If you add this to the patch, we get something that is mechanistically
> complete and which also allows users to add certs to their Mok
> blacklist.

That make sense. I’ll work on a patch to add this ability.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2021-01-15 16:49           ` Eric Snowberg
@ 2021-01-20 11:26             ` Jarkko Sakkinen
  2021-01-20 22:13               ` Eric Snowberg
  0 siblings, 1 reply; 22+ messages in thread
From: Jarkko Sakkinen @ 2021-01-20 11:26 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: Jarkko Sakkinen, David Howells, dwmw2, herbert, davem, jmorris,
	serge, nayna, Mimi Zohar, erichte, mpe, keyrings, linux-kernel,
	linux-crypto, linux-security-module, James.Bottomley

On Fri, Jan 15, 2021 at 09:49:02AM -0700, Eric Snowberg wrote:
> 
> > On Jan 15, 2021, at 2:15 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > On Wed, Jan 13, 2021 at 05:11:10PM -0700, Eric Snowberg wrote:
> >> 
> >>> On Jan 13, 2021, at 1:41 PM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> >>> 
> >>> On Tue, Jan 12, 2021 at 02:57:39PM +0000, David Howells wrote:
> >>>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
> >>>> 
> >>>>>> On Dec 10, 2020, at 2:49 AM, David Howells <dhowells@redhat.com> wrote:
> >>>>>> 
> >>>>>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
> >>>>>> 
> >>>>>>> Add support for EFI_CERT_X509_GUID dbx entries. When a EFI_CERT_X509_GUID
> >>>>>>> is found, it is added as an asymmetrical key to the .blacklist keyring.
> >>>>>>> Anytime the .platform keyring is used, the keys in the .blacklist keyring
> >>>>>>> are referenced, if a matching key is found, the key will be rejected.
> >>>>>> 
> >>>>>> Ummm...  Why this way and not as a blacklist key which takes up less space?
> >>>>>> I'm guessing that you're using the key chain matching logic.  We really only
> >>>>>> need to blacklist the key IDs.
> >>>>> 
> >>>>> I implemented it this way so that certs in the dbx would only impact 
> >>>>> the .platform keyring. I was under the impression we didn’t want to have 
> >>>>> Secure Boot UEFI db/dbx certs dictate keyring functionality within the kernel
> >>>>> itself. Meaning if we have a matching dbx cert in any other keyring (builtin,
> >>>>> secondary, ima, etc.), it would be allowed. If that is not how you’d like to 
> >>>>> see it done, let me know and I’ll make the change.
> >>>> 
> >>>> I wonder if that is that the right thing to do.  I guess this is a policy
> >>>> decision and may depend on the particular user.
> >>> 
> >>> Why would you want to allow dbx entry in any keyring?
> >> 
> >> Today, DB and MOK certs go into the platform keyring.  These certs are only
> >> referenced during kexec.  They can’t be used for other things like validating
> >> kernel module signatures.  If we follow the same pattern, the DBX and MOKX entries
> >> in the blacklist keyring should only impact kexec. 
> >> 
> >> Currently, Mickaël Salaün has another outstanding series to allow root to update 
> >> the blacklist keyring.  I assume the use case for this is around certificates used 
> >> within the kernel, for example revoking kernel module signatures.  The question I have
> >> is, should another keyring be introduced?  One that carries DBX and MOKX, which just
> >> correspond to certs/hashes in the platform keyring; this keyring would only be
> >> referenced for kexec, just like the platform keyring is today. Then, the current
> >> blacklist keyring would be used for everything internal to the kernel.
> > 
> > Right, I'm following actively that series.
> > 
> > Why couldn't user space drive this process and use that feature to do it?
> 
> I could see where the user would want to use both. With Mickaël Salaün’s
> series, the blacklist keyring is updated immediately.  However it does
> not survive a reboot.  With my patch, the blacklist keyring is updated
> during boot, based on what is in the dbx. Neither approach needs a new 
> kernel build.

I don't want to purposely challenge this, but why does it matter
that it doesn't survive the boot? I'm referring here to the golden
principle of kernel defining a mechanism, not policy. User space
can do the population however it wants to for every boot.

E.g. systemd service could do this.

What am I missing here?

/Jarkko

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2021-01-20 11:26             ` Jarkko Sakkinen
@ 2021-01-20 22:13               ` Eric Snowberg
  2021-01-21  0:36                 ` Jarkko Sakkinen
  2021-01-27 11:46                 ` David Howells
  0 siblings, 2 replies; 22+ messages in thread
From: Eric Snowberg @ 2021-01-20 22:13 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Jarkko Sakkinen, David Howells, dwmw2, herbert, davem, jmorris,
	serge, nayna, Mimi Zohar, erichte, mpe, keyrings, linux-kernel,
	linux-crypto, linux-security-module, James.Bottomley,
	Eric Snowberg


> On Jan 20, 2021, at 4:26 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Fri, Jan 15, 2021 at 09:49:02AM -0700, Eric Snowberg wrote:
>> 
>>> On Jan 15, 2021, at 2:15 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>>> 
>>> On Wed, Jan 13, 2021 at 05:11:10PM -0700, Eric Snowberg wrote:
>>>> 
>>>>> On Jan 13, 2021, at 1:41 PM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
>>>>> 
>>>>> On Tue, Jan 12, 2021 at 02:57:39PM +0000, David Howells wrote:
>>>>>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
>>>>>> 
>>>>>>>> On Dec 10, 2020, at 2:49 AM, David Howells <dhowells@redhat.com> wrote:
>>>>>>>> 
>>>>>>>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
>>>>>>>> 
>>>>>>>>> Add support for EFI_CERT_X509_GUID dbx entries. When a EFI_CERT_X509_GUID
>>>>>>>>> is found, it is added as an asymmetrical key to the .blacklist keyring.
>>>>>>>>> Anytime the .platform keyring is used, the keys in the .blacklist keyring
>>>>>>>>> are referenced, if a matching key is found, the key will be rejected.
>>>>>>>> 
>>>>>>>> Ummm...  Why this way and not as a blacklist key which takes up less space?
>>>>>>>> I'm guessing that you're using the key chain matching logic.  We really only
>>>>>>>> need to blacklist the key IDs.
>>>>>>> 
>>>>>>> I implemented it this way so that certs in the dbx would only impact 
>>>>>>> the .platform keyring. I was under the impression we didn’t want to have 
>>>>>>> Secure Boot UEFI db/dbx certs dictate keyring functionality within the kernel
>>>>>>> itself. Meaning if we have a matching dbx cert in any other keyring (builtin,
>>>>>>> secondary, ima, etc.), it would be allowed. If that is not how you’d like to 
>>>>>>> see it done, let me know and I’ll make the change.
>>>>>> 
>>>>>> I wonder if that is that the right thing to do.  I guess this is a policy
>>>>>> decision and may depend on the particular user.
>>>>> 
>>>>> Why would you want to allow dbx entry in any keyring?
>>>> 
>>>> Today, DB and MOK certs go into the platform keyring.  These certs are only
>>>> referenced during kexec.  They can’t be used for other things like validating
>>>> kernel module signatures.  If we follow the same pattern, the DBX and MOKX entries
>>>> in the blacklist keyring should only impact kexec. 
>>>> 
>>>> Currently, Mickaël Salaün has another outstanding series to allow root to update 
>>>> the blacklist keyring.  I assume the use case for this is around certificates used 
>>>> within the kernel, for example revoking kernel module signatures.  The question I have
>>>> is, should another keyring be introduced?  One that carries DBX and MOKX, which just
>>>> correspond to certs/hashes in the platform keyring; this keyring would only be
>>>> referenced for kexec, just like the platform keyring is today. Then, the current
>>>> blacklist keyring would be used for everything internal to the kernel.
>>> 
>>> Right, I'm following actively that series.
>>> 
>>> Why couldn't user space drive this process and use that feature to do it?
>> 
>> I could see where the user would want to use both. With Mickaël Salaün’s
>> series, the blacklist keyring is updated immediately.  However it does
>> not survive a reboot.  With my patch, the blacklist keyring is updated
>> during boot, based on what is in the dbx. Neither approach needs a new 
>> kernel build.
> 
> I don't want to purposely challenge this, but why does it matter
> that it doesn't survive the boot? I'm referring here to the golden
> principle of kernel defining a mechanism, not policy. User space
> can do the population however it wants to for every boot.
> 
> E.g. systemd service could do this.
> 
> What am I missing here?

This change simply adds support for a missing type.  The kernel 
already supports cert and hash entries (EFI_CERT_X509_SHA256_GUID,
EFI_CERT_SHA256_GUID) that originate from the dbx and are loaded 
into the blacklist keyring during boot.  I’m not sure why a cert 
defined with EFI_CERT_X509_GUID should be handled in a different 
manner.

I suppose a user space tool could be created. But wouldn’t what is
currently done in the kernel in this area need to be removed?


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2021-01-20 22:13               ` Eric Snowberg
@ 2021-01-21  0:36                 ` Jarkko Sakkinen
  2021-01-27 11:46                 ` David Howells
  1 sibling, 0 replies; 22+ messages in thread
From: Jarkko Sakkinen @ 2021-01-21  0:36 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: Jarkko Sakkinen, David Howells, dwmw2, herbert, davem, jmorris,
	serge, nayna, Mimi Zohar, erichte, mpe, keyrings, linux-kernel,
	linux-crypto, linux-security-module, James.Bottomley

On Wed, Jan 20, 2021 at 03:13:11PM -0700, Eric Snowberg wrote:
> 
> > On Jan 20, 2021, at 4:26 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > On Fri, Jan 15, 2021 at 09:49:02AM -0700, Eric Snowberg wrote:
> >> 
> >>> On Jan 15, 2021, at 2:15 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >>> 
> >>> On Wed, Jan 13, 2021 at 05:11:10PM -0700, Eric Snowberg wrote:
> >>>> 
> >>>>> On Jan 13, 2021, at 1:41 PM, Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> >>>>> 
> >>>>> On Tue, Jan 12, 2021 at 02:57:39PM +0000, David Howells wrote:
> >>>>>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
> >>>>>> 
> >>>>>>>> On Dec 10, 2020, at 2:49 AM, David Howells <dhowells@redhat.com> wrote:
> >>>>>>>> 
> >>>>>>>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
> >>>>>>>> 
> >>>>>>>>> Add support for EFI_CERT_X509_GUID dbx entries. When a EFI_CERT_X509_GUID
> >>>>>>>>> is found, it is added as an asymmetrical key to the .blacklist keyring.
> >>>>>>>>> Anytime the .platform keyring is used, the keys in the .blacklist keyring
> >>>>>>>>> are referenced, if a matching key is found, the key will be rejected.
> >>>>>>>> 
> >>>>>>>> Ummm...  Why this way and not as a blacklist key which takes up less space?
> >>>>>>>> I'm guessing that you're using the key chain matching logic.  We really only
> >>>>>>>> need to blacklist the key IDs.
> >>>>>>> 
> >>>>>>> I implemented it this way so that certs in the dbx would only impact 
> >>>>>>> the .platform keyring. I was under the impression we didn’t want to have 
> >>>>>>> Secure Boot UEFI db/dbx certs dictate keyring functionality within the kernel
> >>>>>>> itself. Meaning if we have a matching dbx cert in any other keyring (builtin,
> >>>>>>> secondary, ima, etc.), it would be allowed. If that is not how you’d like to 
> >>>>>>> see it done, let me know and I’ll make the change.
> >>>>>> 
> >>>>>> I wonder if that is that the right thing to do.  I guess this is a policy
> >>>>>> decision and may depend on the particular user.
> >>>>> 
> >>>>> Why would you want to allow dbx entry in any keyring?
> >>>> 
> >>>> Today, DB and MOK certs go into the platform keyring.  These certs are only
> >>>> referenced during kexec.  They can’t be used for other things like validating
> >>>> kernel module signatures.  If we follow the same pattern, the DBX and MOKX entries
> >>>> in the blacklist keyring should only impact kexec. 
> >>>> 
> >>>> Currently, Mickaël Salaün has another outstanding series to allow root to update 
> >>>> the blacklist keyring.  I assume the use case for this is around certificates used 
> >>>> within the kernel, for example revoking kernel module signatures.  The question I have
> >>>> is, should another keyring be introduced?  One that carries DBX and MOKX, which just
> >>>> correspond to certs/hashes in the platform keyring; this keyring would only be
> >>>> referenced for kexec, just like the platform keyring is today. Then, the current
> >>>> blacklist keyring would be used for everything internal to the kernel.
> >>> 
> >>> Right, I'm following actively that series.
> >>> 
> >>> Why couldn't user space drive this process and use that feature to do it?
> >> 
> >> I could see where the user would want to use both. With Mickaël Salaün’s
> >> series, the blacklist keyring is updated immediately.  However it does
> >> not survive a reboot.  With my patch, the blacklist keyring is updated
> >> during boot, based on what is in the dbx. Neither approach needs a new 
> >> kernel build.
> > 
> > I don't want to purposely challenge this, but why does it matter
> > that it doesn't survive the boot? I'm referring here to the golden
> > principle of kernel defining a mechanism, not policy. User space
> > can do the population however it wants to for every boot.
> > 
> > E.g. systemd service could do this.
> > 
> > What am I missing here?
> 
> This change simply adds support for a missing type.  The kernel 
> already supports cert and hash entries (EFI_CERT_X509_SHA256_GUID,
> EFI_CERT_SHA256_GUID) that originate from the dbx and are loaded 
> into the blacklist keyring during boot.  I’m not sure why a cert 
> defined with EFI_CERT_X509_GUID should be handled in a different 
> manner.
> 
> I suppose a user space tool could be created. But wouldn’t what is
> currently done in the kernel in this area need to be removed?

Right. I don't think this was a great idea in the first place to
do to the kernel but since it exists, I guess the patch does make
sense.

/Jarkko

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2021-01-20 22:13               ` Eric Snowberg
  2021-01-21  0:36                 ` Jarkko Sakkinen
@ 2021-01-27 11:46                 ` David Howells
  2021-01-27 14:03                   ` Mimi Zohar
  1 sibling, 1 reply; 22+ messages in thread
From: David Howells @ 2021-01-27 11:46 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: dhowells, Eric Snowberg, Jarkko Sakkinen, dwmw2, herbert, davem,
	jmorris, serge, nayna, Mimi Zohar, erichte, mpe, keyrings,
	linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley

Jarkko Sakkinen <jarkko@kernel.org> wrote:

> > I suppose a user space tool could be created. But wouldn’t what is
> > currently done in the kernel in this area need to be removed?
> 
> Right. I don't think this was a great idea in the first place to
> do to the kernel but since it exists, I guess the patch does make
> sense.

This information needs to be loaded from the UEFI tables before the system
starts loading any kernel modules or running any programs (if we do
verification of such, which I think IMA can do).

David


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2021-01-27 11:46                 ` David Howells
@ 2021-01-27 14:03                   ` Mimi Zohar
  2021-01-27 15:41                     ` Eric Snowberg
  2021-01-29 23:27                     ` Jarkko Sakkinen
  0 siblings, 2 replies; 22+ messages in thread
From: Mimi Zohar @ 2021-01-27 14:03 UTC (permalink / raw)
  To: David Howells, Jarkko Sakkinen, linux-integrity
  Cc: Eric Snowberg, Jarkko Sakkinen, dwmw2, herbert, davem, jmorris,
	serge, nayna, erichte, mpe, keyrings, linux-kernel, linux-crypto,
	linux-security-module, James.Bottomley

[Cc'ing linux-integrity]

On Wed, 2021-01-27 at 11:46 +0000, David Howells wrote:
> Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> > > I suppose a user space tool could be created. But wouldn’t what is
> > > currently done in the kernel in this area need to be removed?
> > 
> > Right. I don't think this was a great idea in the first place to
> > do to the kernel but since it exists, I guess the patch does make
> > sense.
> 
> This information needs to be loaded from the UEFI tables before the system
> starts loading any kernel modules or running any programs (if we do
> verification of such, which I think IMA can do).

There needs to a clear distinction between the pre-boot and post-boot
keys.  UEFI has its own trust model, which should be limited to UEFI. 
The .platform keyring was upstreamed and limited to verifying the kexec
kernel image.   Any other usage of the .platform keyring keys is
abusing its intended purpose.

The cover letter says,   "Anytime the .platform keyring is used, the
keys in the .blacklist keyring are referenced, if a matching key is
found, the key will be rejected."   I don't have a problem with loading
the UEFI X509 dbx entries as long as its usage is limited to verifying
the kexec kernel image.

Mimi


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2021-01-27 14:03                   ` Mimi Zohar
@ 2021-01-27 15:41                     ` Eric Snowberg
  2021-01-28  4:13                       ` Nayna
  2021-01-30 10:24                       ` Jarkko Sakkinen
  2021-01-29 23:27                     ` Jarkko Sakkinen
  1 sibling, 2 replies; 22+ messages in thread
From: Eric Snowberg @ 2021-01-27 15:41 UTC (permalink / raw)
  To: Mimi Zohar, David Howells, Jarkko Sakkinen
  Cc: linux-integrity, Jarkko Sakkinen, dwmw2, herbert, davem, jmorris,
	serge, nayna, erichte, mpe, keyrings, linux-kernel, linux-crypto,
	linux-security-module, James.Bottomley, Eric Snowberg


> On Jan 27, 2021, at 7:03 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> [Cc'ing linux-integrity]
> 
> On Wed, 2021-01-27 at 11:46 +0000, David Howells wrote:
>> Jarkko Sakkinen <jarkko@kernel.org> wrote:
>> 
>>>> I suppose a user space tool could be created. But wouldn’t what is
>>>> currently done in the kernel in this area need to be removed?
>>> 
>>> Right. I don't think this was a great idea in the first place to
>>> do to the kernel but since it exists, I guess the patch does make
>>> sense.
>> 
>> This information needs to be loaded from the UEFI tables before the system
>> starts loading any kernel modules or running any programs (if we do
>> verification of such, which I think IMA can do).
> 
> There needs to a clear distinction between the pre-boot and post-boot
> keys.  UEFI has its own trust model, which should be limited to UEFI. 
> The .platform keyring was upstreamed and limited to verifying the kexec
> kernel image.   Any other usage of the .platform keyring keys is
> abusing its intended purpose.
> 
> The cover letter says,   "Anytime the .platform keyring is used, the
> keys in the .blacklist keyring are referenced, if a matching key is
> found, the key will be rejected."   I don't have a problem with loading
> the UEFI X509 dbx entries as long as its usage is limited to verifying
> the kexec kernel image.

Correct, with my patch, when EFI_CERT_X509_GUID entries are found in the
dbx, they will only be used during kexec.  I believe the latest dbx file on 
uefi.org contains three of these entires.

Based on my understanding of why the platform keyring was introduced, 
I intentionally only used these for kexec.  I do question the current 
upstream mainline code though.  Currently, when EFI_CERT_X509_SHA256_GUID
or EFI_CERT_SHA256_GUID entries are found in the dbx, they are applied 
everywhere.  It seems like there should be a dbx revocation keyring 
equivalent to the current platform keyring that is only used for pre-boot. 

If that is a direction you would like to see this go in the future, let
me know, I’d be happy to work on it.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2021-01-27 15:41                     ` Eric Snowberg
@ 2021-01-28  4:13                       ` Nayna
  2021-01-30 10:24                       ` Jarkko Sakkinen
  1 sibling, 0 replies; 22+ messages in thread
From: Nayna @ 2021-01-28  4:13 UTC (permalink / raw)
  To: Eric Snowberg, Mimi Zohar, David Howells, Jarkko Sakkinen
  Cc: linux-integrity, Jarkko Sakkinen, dwmw2, herbert, davem, jmorris,
	serge, nayna, erichte, mpe, keyrings, linux-kernel, linux-crypto,
	linux-security-module, James.Bottomley


On 1/27/21 10:41 AM, Eric Snowberg wrote:
>> On Jan 27, 2021, at 7:03 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>
>> [Cc'ing linux-integrity]
>>
>> On Wed, 2021-01-27 at 11:46 +0000, David Howells wrote:
>>> Jarkko Sakkinen <jarkko@kernel.org> wrote:
>>>
>>>>> I suppose a user space tool could be created. But wouldn’t what is
>>>>> currently done in the kernel in this area need to be removed?
>>>> Right. I don't think this was a great idea in the first place to
>>>> do to the kernel but since it exists, I guess the patch does make
>>>> sense.
>>> This information needs to be loaded from the UEFI tables before the system
>>> starts loading any kernel modules or running any programs (if we do
>>> verification of such, which I think IMA can do).
>> There needs to a clear distinction between the pre-boot and post-boot
>> keys.  UEFI has its own trust model, which should be limited to UEFI.
>> The .platform keyring was upstreamed and limited to verifying the kexec
>> kernel image.   Any other usage of the .platform keyring keys is
>> abusing its intended purpose.
>>
>> The cover letter says,   "Anytime the .platform keyring is used, the
>> keys in the .blacklist keyring are referenced, if a matching key is
>> found, the key will be rejected."   I don't have a problem with loading
>> the UEFI X509 dbx entries as long as its usage is limited to verifying
>> the kexec kernel image.
> Correct, with my patch, when EFI_CERT_X509_GUID entries are found in the
> dbx, they will only be used during kexec.  I believe the latest dbx file on
> uefi.org contains three of these entires.
>
> Based on my understanding of why the platform keyring was introduced,
> I intentionally only used these for kexec.  I do question the current
> upstream mainline code though.  Currently, when EFI_CERT_X509_SHA256_GUID
> or EFI_CERT_SHA256_GUID entries are found in the dbx, they are applied
> everywhere.  It seems like there should be a dbx revocation keyring
> equivalent to the current platform keyring that is only used for pre-boot.
>
> If that is a direction you would like to see this go in the future, let
> me know, I’d be happy to work on it.
>
Yes, as you said, currently blacklist entries from dbx for 
EFI_CERT_X509_SHA256_GUID or EFI_CERT_SHA256_GUID are applied 
everywhere, and does not satisfy the trust model for .platform keyring. 
We should fix this, but changing now might break some existing systems. 
Probably it should be discussed as separate thread from this patchset.

Thanks & Regards,

       - Nayna


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2021-01-27 14:03                   ` Mimi Zohar
  2021-01-27 15:41                     ` Eric Snowberg
@ 2021-01-29 23:27                     ` Jarkko Sakkinen
  1 sibling, 0 replies; 22+ messages in thread
From: Jarkko Sakkinen @ 2021-01-29 23:27 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: David Howells, linux-integrity, Eric Snowberg, Jarkko Sakkinen,
	dwmw2, herbert, davem, jmorris, serge, nayna, erichte, mpe,
	keyrings, linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley

On Wed, Jan 27, 2021 at 09:03:59AM -0500, Mimi Zohar wrote:
> [Cc'ing linux-integrity]
> 
> On Wed, 2021-01-27 at 11:46 +0000, David Howells wrote:
> > Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > > > I suppose a user space tool could be created. But wouldn’t what is
> > > > currently done in the kernel in this area need to be removed?
> > > 
> > > Right. I don't think this was a great idea in the first place to
> > > do to the kernel but since it exists, I guess the patch does make
> > > sense.
> > 
> > This information needs to be loaded from the UEFI tables before the system
> > starts loading any kernel modules or running any programs (if we do
> > verification of such, which I think IMA can do).
> 
> There needs to a clear distinction between the pre-boot and post-boot
> keys.  UEFI has its own trust model, which should be limited to UEFI. 
> The .platform keyring was upstreamed and limited to verifying the kexec
> kernel image.   Any other usage of the .platform keyring keys is
> abusing its intended purpose.
> 
> The cover letter says,   "Anytime the .platform keyring is used, the
> keys in the .blacklist keyring are referenced, if a matching key is
> found, the key will be rejected."   I don't have a problem with loading
> the UEFI X509 dbx entries as long as its usage is limited to verifying
> the kexec kernel image.
> 
> Mimi

Thanks Mimi, this is a valid argument. I agree.

/Jarkko

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries
  2021-01-27 15:41                     ` Eric Snowberg
  2021-01-28  4:13                       ` Nayna
@ 2021-01-30 10:24                       ` Jarkko Sakkinen
  1 sibling, 0 replies; 22+ messages in thread
From: Jarkko Sakkinen @ 2021-01-30 10:24 UTC (permalink / raw)
  To: Eric Snowberg
  Cc: Mimi Zohar, David Howells, linux-integrity, Jarkko Sakkinen,
	dwmw2, herbert, davem, jmorris, serge, nayna, erichte, mpe,
	keyrings, linux-kernel, linux-crypto, linux-security-module,
	James.Bottomley

On Wed, Jan 27, 2021 at 08:41:29AM -0700, Eric Snowberg wrote:
> 
> > On Jan 27, 2021, at 7:03 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > [Cc'ing linux-integrity]
> > 
> > On Wed, 2021-01-27 at 11:46 +0000, David Howells wrote:
> >> Jarkko Sakkinen <jarkko@kernel.org> wrote:
> >> 
> >>>> I suppose a user space tool could be created. But wouldn’t what is
> >>>> currently done in the kernel in this area need to be removed?
> >>> 
> >>> Right. I don't think this was a great idea in the first place to
> >>> do to the kernel but since it exists, I guess the patch does make
> >>> sense.
> >> 
> >> This information needs to be loaded from the UEFI tables before the system
> >> starts loading any kernel modules or running any programs (if we do
> >> verification of such, which I think IMA can do).
> > 
> > There needs to a clear distinction between the pre-boot and post-boot
> > keys.  UEFI has its own trust model, which should be limited to UEFI. 
> > The .platform keyring was upstreamed and limited to verifying the kexec
> > kernel image.   Any other usage of the .platform keyring keys is
> > abusing its intended purpose.
> > 
> > The cover letter says,   "Anytime the .platform keyring is used, the
> > keys in the .blacklist keyring are referenced, if a matching key is
> > found, the key will be rejected."   I don't have a problem with loading
> > the UEFI X509 dbx entries as long as its usage is limited to verifying
> > the kexec kernel image.
> 
> Correct, with my patch, when EFI_CERT_X509_GUID entries are found in the
> dbx, they will only be used during kexec.  I believe the latest dbx file on 
> uefi.org contains three of these entires.
> 
> Based on my understanding of why the platform keyring was introduced, 
> I intentionally only used these for kexec.  I do question the current 
> upstream mainline code though.  Currently, when EFI_CERT_X509_SHA256_GUID
> or EFI_CERT_SHA256_GUID entries are found in the dbx, they are applied 
> everywhere.  It seems like there should be a dbx revocation keyring 
> equivalent to the current platform keyring that is only used for pre-boot. 
> 
> If that is a direction you would like to see this go in the future, let
> me know, I’d be happy to work on it.

I would tend to agree with this.

/Jarkko

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2021-01-30 10:25 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-16  0:49 [PATCH v4] certs: Add EFI_CERT_X509_GUID support for dbx entries Eric Snowberg
2020-09-16 18:09 ` Jarkko Sakkinen
2020-12-10  9:49 ` David Howells
2020-12-10 18:56   ` Eric Snowberg
2021-01-12 14:57   ` David Howells
2021-01-13 20:41     ` Jarkko Sakkinen
2021-01-14  0:11       ` Eric Snowberg
2021-01-15  9:15         ` Jarkko Sakkinen
2021-01-15 16:49           ` Eric Snowberg
2021-01-20 11:26             ` Jarkko Sakkinen
2021-01-20 22:13               ` Eric Snowberg
2021-01-21  0:36                 ` Jarkko Sakkinen
2021-01-27 11:46                 ` David Howells
2021-01-27 14:03                   ` Mimi Zohar
2021-01-27 15:41                     ` Eric Snowberg
2021-01-28  4:13                       ` Nayna
2021-01-30 10:24                       ` Jarkko Sakkinen
2021-01-29 23:27                     ` Jarkko Sakkinen
2021-01-12 17:10   ` David Howells
2021-01-12 19:13     ` Eric Snowberg
2021-01-15 17:21 ` James Bottomley
2021-01-15 23:01   ` Eric Snowberg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).