Re: [PATCH] certs: Add EFI_CERT_X509_GUID support for dbx entries

2021-01-13 Thread James Bottomley
On Wed, 2021-01-13 at 13:40 +, David Howells wrote:
> Hi Linus,
> 
> Are you willing to take this between merge windows - or does it need
> to wait for the next merge window?  It's not technically a bug fix to
> the kernel, but it does have a CVE attached to it.
> 
> Note that I've also updated Jarkko's address in his Reviewed-by since
> his Intel address no longer works.

Sorry, late to the party.

I suppose I lost the argument that we shouldn't really be trusting any
certs from db when shim is in operation because they're all EFI binary
signing ones and will usually simply be the microsoft certificate and
possibly an OEM platform one and we're usually pivoting the root of
trust to the certificates in the MokList.

However, if we are going to do this, we should also be blacklisting the
certificates in MokListX which the OS sees through MokListXRT.  Since
MokListX is an essential piece of our revocation infrastructure it
should have been mentioned in the CVE but wasn't for some reason.

James




Re: [PATCH] certs: Add EFI_CERT_X509_GUID support for dbx entries

2021-01-13 Thread David Howells
David Howells  wrote:

> This fixes CVE-2020-26541.

Note that I added the CVE number, not Eric.

David



[PATCH] certs: Add EFI_CERT_X509_GUID support for dbx entries

2021-01-13 Thread David Howells
Hi Linus,

Are you willing to take this between merge windows - or does it need to wait
for the next merge window?  It's not technically a bug fix to the kernel, but
it does have a CVE attached to it.

Note that I've also updated Jarkko's address in his Reviewed-by since his
Intel address no longer works.

David
---
commit b5f71d4461d6d09463b2ce8bc4fc150ea1c385c0
Author: Eric Snowberg 
Date:   Tue Sep 15 20:49:27 2020 -0400

certs: Add EFI_CERT_X509_GUID support for dbx entries

This fixes CVE-2020-26541.

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

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

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

[DH: I've changed the names of the new functions with Eric's approval]

Signed-off-by: Eric Snowberg 
Reviewed-by: Jarkko Sakkinen 
Signed-off-by: David Howells 

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 
+#include 
+#include 
 
 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_revoca