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

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

[Eric Snowberg]

This is the fifth patch series for adding support for 
EFI_CERT_X509_GUID entries [1].  It has been expanded to not only include
dbx entries but also entries in the mokx.  Additionally my series to
preload these certificate [2] has also been included.

This series is based on v5.11-rc4.

[1] https://patchwork.kernel.org/project/linux-security-module/patch/20200916004927.64276-1-eric.snowberg@oracle.com/
[2] https://lore.kernel.org/patchwork/cover/1315485/

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

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


base-commit: 19c329f6808995b142b3966301f217c831e7cf31
-- 
2.18.4

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

[Eric Snowberg]

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.

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>
---
v5: Function name changes done by David Howells
---
 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..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;
 }
-- 
2.18.4

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

[Nayna]

On 1/22/21 1:10 PM, Eric Snowberg wrote:
> 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.
>
> 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>
> ---
> v5: Function name changes done by David Howells
> ---
>   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..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);
> +}

In keeping the naming convention with other functions that blacklist 
hashes, why can't we call these functions:

* uefi_revocation_list_x509() -> uefi_blacklist_x509_cert()
* add_key_to_revocation_list() -> uefi_blacklist_cert()
* is_key_on_revocation_list() -> is_cert_blacklisted()

Thanks & Regards,

      - Nayna

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

[Eric Snowberg]

> On Jan 27, 2021, at 8:54 PM, Nayna <nayna@linux.vnet.ibm.com> wrote:
> 
> 
> On 1/22/21 1:10 PM, Eric Snowberg wrote:
>> 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.
>> 
>> 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>
>> ---
>> v5: Function name changes done by David Howells
>> ---
>>  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..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);
>> +}
> 
> In keeping the naming convention with other functions that blacklist hashes, why can't we call these functions:
> 
> * uefi_revocation_list_x509() -> uefi_blacklist_x509_cert()
> * add_key_to_revocation_list() -> uefi_blacklist_cert()
> * is_key_on_revocation_list() -> is_cert_blacklisted()

The word revocation was used do to the updated Linux coding style:

https://lkml.org/lkml/2020/7/4/229

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

[Nayna]

On 1/27/21 11:11 PM, Eric Snowberg wrote:
>> On Jan 27, 2021, at 8:54 PM, Nayna <nayna@linux.vnet.ibm.com> wrote:
>>
>>
>> On 1/22/21 1:10 PM, Eric Snowberg wrote:
>>> 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.
>>>
>>> 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>
>>> ---
>>> v5: Function name changes done by David Howells
>>> ---
>>>   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..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);
>>> +}
>> In keeping the naming convention with other functions that blacklist hashes, why can't we call these functions:
>>
>> * uefi_revocation_list_x509() -> uefi_blacklist_x509_cert()
>> * add_key_to_revocation_list() -> uefi_blacklist_cert()
>> * is_key_on_revocation_list() -> is_cert_blacklisted()
> The word revocation was used do to the updated Linux coding style:
>
> https://lkml.org/lkml/2020/7/4/229
>
>
Thanks Eric for clarifying. I was confusing it with with the broader 
meaning of revocation i.e. certificate revocation list. To avoid similar 
confusion in the future, I wonder if we should call it as 'blocklist' or 
'denylist' as suggested in the document. This is to avoid conflicts with 
actual CRL support if added in the future. I also wonder if we should 
add the clarification in the patch description.

Thanks & Regards,

        - Nayna

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

[David Howells]

Nayna <nayna@linux.vnet.ibm.com> wrote:

> Thanks Eric for clarifying. I was confusing it with with the broader meaning
> of revocation i.e. certificate revocation list. To avoid similar confusion in
> the future, I wonder if we should call it as 'blocklist' or 'denylist' as
> suggested in the document. This is to avoid conflicts with actual CRL support
> if added in the future. I also wonder if we should add the clarification in
> the patch description.

Reject-list might be better.

David

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

[Eric Snowberg]

> On Jan 28, 2021, at 8:58 AM, David Howells <dhowells@redhat.com> wrote:
> 
> Nayna <nayna@linux.vnet.ibm.com> wrote:
> 
>> Thanks Eric for clarifying. I was confusing it with with the broader meaning
>> of revocation i.e. certificate revocation list. To avoid similar confusion in
>> the future, I wonder if we should call it as 'blocklist' or 'denylist' as
>> suggested in the document. This is to avoid conflicts with actual CRL support
>> if added in the future. I also wonder if we should add the clarification in
>> the patch description.
> 
> Reject-list might be better.

As far as naming goes, I have no preference.  If we can come to an agreement 
on the name, I can change it if needed. Or David, if you want to pull it into
your tree and change the naming again, I’m fine with whatever you pick.  Just
let me know how you would like to handle it.

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

[Eric Snowberg]

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

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 certs/Makefile         |  2 +-
 certs/common.c         | 56 ++++++++++++++++++++++++++++++++++++++++++
 certs/common.h         |  9 +++++++
 certs/system_keyring.c | 49 +++---------------------------------
 4 files changed, 69 insertions(+), 47 deletions(-)
 create mode 100644 certs/common.c
 create mode 100644 certs/common.h

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

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

[Eric Snowberg]

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

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Acked-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 certs/Kconfig                   |  8 ++++++++
 certs/Makefile                  | 18 ++++++++++++++++--
 certs/blacklist.c               | 17 +++++++++++++++++
 certs/revocation_certificates.S | 21 +++++++++++++++++++++
 scripts/Makefile                |  1 +
 5 files changed, 63 insertions(+), 2 deletions(-)
 create mode 100644 certs/revocation_certificates.S

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

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

[Eric Snowberg]

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

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

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 security/integrity/platform_certs/load_uefi.c | 20 +++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

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

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

[David Howells]

Which tree do you envision this going through?  EFI or keyrings - or are you
going to ask Linus to pull it directly?  I can pull it if it should go through
the keyrings tree.

David

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

[Mimi Zohar]

Hi David,

On Thu, 2021-01-28 at 15:16 +0000, David Howells wrote:
> Which tree do you envision this going through?  EFI or keyrings - or are you
> going to ask Linus to pull it directly?  I can pull it if it should go through
> the keyrings tree.

There's one more patch, yet to be posted, which updates
asymmetric_verify().  As long as you're willing to upstream all of the
patches, that's fine with me.

thanks!

Mimi

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

[Mimi Zohar]

On Thu, 2021-01-28 at 10:27 -0500, Mimi Zohar wrote:
> Hi David,
> 
> On Thu, 2021-01-28 at 15:16 +0000, David Howells wrote:
> > Which tree do you envision this going through?  EFI or keyrings - or are you
> > going to ask Linus to pull it directly?  I can pull it if it should go through
> > the keyrings tree.
> 
> There's one more patch, yet to be posted, which updates
> asymmetric_verify().  As long as you're willing to upstream all of the
> patches, that's fine with me.

Oops, wrong thread.  I thought this was Stefan's patch set.

Mimi

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

[Eric Snowberg]

> On Jan 28, 2021, at 8:16 AM, David Howells <dhowells@redhat.com> wrote:
> 
> Which tree do you envision this going through?  EFI or keyrings - or are you
> going to ask Linus to pull it directly?  I can pull it if it should go through
> the keyrings tree.

I was thinking it would go thru your tree, since a majority of the code
is contained within it.

Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]

[David Howells]

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

> This is the fifth patch series for adding support for 
> EFI_CERT_X509_GUID entries [1].  It has been expanded to not only include
> dbx entries but also entries in the mokx.  Additionally my series to
> preload these certificate [2] has also been included.

Okay, I've tentatively applied this to my keys-next branch.  However, it
conflicts minorly with Mickaël Salaün's patches that I've previously merged on
the same branch.  Can you have a look at the merge commit

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

	(the top patch of my keys-next branch)

to see if that is okay by both of you?  If so, can you give it a whirl?

Thanks,
David

Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]

[Mickaël Salaün]

This looks good to me, and it still works for my use case. Eric's
patchset only looks for asymmetric keys in the blacklist keyring, so
even if we use the same keyring we don't look for the same key types. My
patchset only allows blacklist keys (i.e. hashes, not asymmetric keys)
to be added by user space (if authenticated), but because Eric's
asymmetric keys are loaded with KEY_ALLOC_BYPASS_RESTRICTION, it should
be OK for his use case.  There should be no interference between the two
new features, but I find it a bit confusing to have such distinct use of
keys from the same keyring depending on their type.

Regards,
 Mickaël


On 03/02/2021 17:26, David Howells wrote:
> 
> Eric Snowberg <eric.snowberg@oracle.com> wrote:
> 
>> This is the fifth patch series for adding support for 
>> EFI_CERT_X509_GUID entries [1].  It has been expanded to not only include
>> dbx entries but also entries in the mokx.  Additionally my series to
>> preload these certificate [2] has also been included.
> 
> Okay, I've tentatively applied this to my keys-next branch.  However, it
> conflicts minorly with Mickaël Salaün's patches that I've previously merged on
> the same branch.  Can you have a look at the merge commit
> 
> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-next&id=fdbbe7ceeb95090d09c33ce0497e0394c82aa33d
> 
> 	(the top patch of my keys-next branch)
> 
> to see if that is okay by both of you?  If so, can you give it a whirl?
> 
> Thanks,
> David
> 

Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]

[Eric Snowberg]

> On Feb 3, 2021, at 11:49 AM, Mickaël Salaün <mic@digikod.net> wrote:
> 
> This looks good to me, and it still works for my use case. Eric's
> patchset only looks for asymmetric keys in the blacklist keyring, so
> even if we use the same keyring we don't look for the same key types. My
> patchset only allows blacklist keys (i.e. hashes, not asymmetric keys)
> to be added by user space (if authenticated), but because Eric's
> asymmetric keys are loaded with KEY_ALLOC_BYPASS_RESTRICTION, it should
> be OK for his use case.  There should be no interference between the two
> new features, but I find it a bit confusing to have such distinct use of
> keys from the same keyring depending on their type.

I agree, it is a bit confusing.  What is the thought of having a dbx 
keyring, similar to how the platform keyring works?

https://www.spinics.net/lists/linux-security-module/msg40262.html


> On 03/02/2021 17:26, David Howells wrote:
>> 
>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
>> 
>>> This is the fifth patch series for adding support for 
>>> EFI_CERT_X509_GUID entries [1].  It has been expanded to not only include
>>> dbx entries but also entries in the mokx.  Additionally my series to
>>> preload these certificate [2] has also been included.
>> 
>> Okay, I've tentatively applied this to my keys-next branch.  However, it
>> conflicts minorly with Mickaël Salaün's patches that I've previously merged on
>> the same branch.  Can you have a look at the merge commit
>> 
>> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-next&id=fdbbe7ceeb95090d09c33ce0497e0394c82aa33d
>> 
>> 	(the top patch of my keys-next branch)
>> 
>> to see if that is okay by both of you?  If so, can you give it a whirl?


I’m seeing a build error within blacklist_hashes_checked with
one of my configs.

The config is as follows:

$ grep CONFIG_SYSTEM_BLACKLIST_HASH_LIST .config
CONFIG_SYSTEM_BLACKLIST_HASH_LIST=“revocation_list"

$ cat certs/revocation_list
"tbs:1e125ea4f38acb7b29b0c495fd8e7602c2c3353b913811a9da3a2fb505c08a32”

make[1]: *** No rule to make target 'revocation_list', needed by 'certs/blacklist_hashes_checked'.  Stop.

Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]

[Mickaël Salaün]

On 04/02/2021 04:53, Eric Snowberg wrote:
> 
>> On Feb 3, 2021, at 11:49 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>
>> This looks good to me, and it still works for my use case. Eric's
>> patchset only looks for asymmetric keys in the blacklist keyring, so
>> even if we use the same keyring we don't look for the same key types. My
>> patchset only allows blacklist keys (i.e. hashes, not asymmetric keys)
>> to be added by user space (if authenticated), but because Eric's
>> asymmetric keys are loaded with KEY_ALLOC_BYPASS_RESTRICTION, it should
>> be OK for his use case.  There should be no interference between the two
>> new features, but I find it a bit confusing to have such distinct use of
>> keys from the same keyring depending on their type.
> 
> I agree, it is a bit confusing.  What is the thought of having a dbx 
> keyring, similar to how the platform keyring works?
> 
> https://www.spinics.net/lists/linux-security-module/msg40262.html
> 
> 
>> On 03/02/2021 17:26, David Howells wrote:
>>>
>>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
>>>
>>>> This is the fifth patch series for adding support for 
>>>> EFI_CERT_X509_GUID entries [1].  It has been expanded to not only include
>>>> dbx entries but also entries in the mokx.  Additionally my series to
>>>> preload these certificate [2] has also been included.
>>>
>>> Okay, I've tentatively applied this to my keys-next branch.  However, it
>>> conflicts minorly with Mickaël Salaün's patches that I've previously merged on
>>> the same branch.  Can you have a look at the merge commit
>>>
>>> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-next&id=fdbbe7ceeb95090d09c33ce0497e0394c82aa33d
>>>
>>> 	(the top patch of my keys-next branch)
>>>
>>> to see if that is okay by both of you?  If so, can you give it a whirl?
> 
> 
> I’m seeing a build error within blacklist_hashes_checked with
> one of my configs.
> 
> The config is as follows:
> 
> $ grep CONFIG_SYSTEM_BLACKLIST_HASH_LIST .config
> CONFIG_SYSTEM_BLACKLIST_HASH_LIST=“revocation_list"
> 
> $ cat certs/revocation_list
> "tbs:1e125ea4f38acb7b29b0c495fd8e7602c2c3353b913811a9da3a2fb505c08a32”
> 
> make[1]: *** No rule to make target 'revocation_list', needed by 'certs/blacklist_hashes_checked'.  Stop.

It requires an absolute path. This is to align with other variables
using the config_filename macro: CONFIG_SYSTEM_TRUSTED_KEYS,
CONFIG_MODULE_SIG_KEY and now CONFIG_SYSTEM_REVOCATION_KEYS.
Cf. https://lore.kernel.org/lkml/1221725.1607515111@warthog.procyon.org.uk/

We may want to patch scripts/kconfig/streamline_config.pl for both
CONFIG_SYSTEM_REVOCATION_KEYS and CONFIG_SYSTEM_BLACKLIST_HASH_LIST, to
warn user (and exit with an error) if such files are not found.

Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]

[Eric Snowberg]

> On Feb 4, 2021, at 1:26 AM, Mickaël Salaün <mic@digikod.net> wrote:
> 
> 
> On 04/02/2021 04:53, Eric Snowberg wrote:
>> 
>>> On Feb 3, 2021, at 11:49 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>> 
>>> This looks good to me, and it still works for my use case. Eric's
>>> patchset only looks for asymmetric keys in the blacklist keyring, so
>>> even if we use the same keyring we don't look for the same key types. My
>>> patchset only allows blacklist keys (i.e. hashes, not asymmetric keys)
>>> to be added by user space (if authenticated), but because Eric's
>>> asymmetric keys are loaded with KEY_ALLOC_BYPASS_RESTRICTION, it should
>>> be OK for his use case.  There should be no interference between the two
>>> new features, but I find it a bit confusing to have such distinct use of
>>> keys from the same keyring depending on their type.
>> 
>> I agree, it is a bit confusing.  What is the thought of having a dbx 
>> keyring, similar to how the platform keyring works?
>> 
>> https://www.spinics.net/lists/linux-security-module/msg40262.html
>> 
>> 
>>> On 03/02/2021 17:26, David Howells wrote:
>>>> 
>>>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
>>>> 
>>>>> This is the fifth patch series for adding support for 
>>>>> EFI_CERT_X509_GUID entries [1].  It has been expanded to not only include
>>>>> dbx entries but also entries in the mokx.  Additionally my series to
>>>>> preload these certificate [2] has also been included.
>>>> 
>>>> Okay, I've tentatively applied this to my keys-next branch.  However, it
>>>> conflicts minorly with Mickaël Salaün's patches that I've previously merged on
>>>> the same branch.  Can you have a look at the merge commit
>>>> 
>>>> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-next&id=fdbbe7ceeb95090d09c33ce0497e0394c82aa33d
>>>> 
>>>> 	(the top patch of my keys-next branch)
>>>> 
>>>> to see if that is okay by both of you?  If so, can you give it a whirl?
>> 
>> 
>> I’m seeing a build error within blacklist_hashes_checked with
>> one of my configs.
>> 
>> The config is as follows:
>> 
>> $ grep CONFIG_SYSTEM_BLACKLIST_HASH_LIST .config
>> CONFIG_SYSTEM_BLACKLIST_HASH_LIST=“revocation_list"
>> 
>> $ cat certs/revocation_list
>> "tbs:1e125ea4f38acb7b29b0c495fd8e7602c2c3353b913811a9da3a2fb505c08a32”
>> 
>> make[1]: *** No rule to make target 'revocation_list', needed by 'certs/blacklist_hashes_checked'.  Stop.
> 
> It requires an absolute path.

Ok, if I use an absolute path now with CONFIG_SYSTEM_BLACKLIST_HASH_LIST 
it works.

> This is to align with other variables
> using the config_filename macro: CONFIG_SYSTEM_TRUSTED_KEYS,
> CONFIG_MODULE_SIG_KEY and now CONFIG_SYSTEM_REVOCATION_KEYS.

I just did a quick test with CONFIG_SYSTEM_TRUSTED_KEYS. It looks like we 
can use either a relative or absolute path with CONFIG_SYSTEM_TRUSTED_KEYS. 
Shouldn’t this be consistent?

> Cf. https://lore.kernel.org/lkml/1221725.1607515111@warthog.procyon.org.uk/
> 
> We may want to patch scripts/kconfig/streamline_config.pl for both
> CONFIG_SYSTEM_REVOCATION_KEYS and CONFIG_SYSTEM_BLACKLIST_HASH_LIST, to
> warn user (and exit with an error) if such files are not found.

Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]

[Mickaël Salaün]

On 05/02/2021 01:24, Eric Snowberg wrote:
> 
>> On Feb 4, 2021, at 1:26 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>
>>
>> On 04/02/2021 04:53, Eric Snowberg wrote:
>>>
>>>> On Feb 3, 2021, at 11:49 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>>>
>>>> This looks good to me, and it still works for my use case. Eric's
>>>> patchset only looks for asymmetric keys in the blacklist keyring, so
>>>> even if we use the same keyring we don't look for the same key types. My
>>>> patchset only allows blacklist keys (i.e. hashes, not asymmetric keys)
>>>> to be added by user space (if authenticated), but because Eric's
>>>> asymmetric keys are loaded with KEY_ALLOC_BYPASS_RESTRICTION, it should
>>>> be OK for his use case.  There should be no interference between the two
>>>> new features, but I find it a bit confusing to have such distinct use of
>>>> keys from the same keyring depending on their type.
>>>
>>> I agree, it is a bit confusing.  What is the thought of having a dbx 
>>> keyring, similar to how the platform keyring works?
>>>
>>> https://www.spinics.net/lists/linux-security-module/msg40262.html
>>>
>>>
>>>> On 03/02/2021 17:26, David Howells wrote:
>>>>>
>>>>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
>>>>>
>>>>>> This is the fifth patch series for adding support for 
>>>>>> EFI_CERT_X509_GUID entries [1].  It has been expanded to not only include
>>>>>> dbx entries but also entries in the mokx.  Additionally my series to
>>>>>> preload these certificate [2] has also been included.
>>>>>
>>>>> Okay, I've tentatively applied this to my keys-next branch.  However, it
>>>>> conflicts minorly with Mickaël Salaün's patches that I've previously merged on
>>>>> the same branch.  Can you have a look at the merge commit
>>>>>
>>>>> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-next&id=fdbbe7ceeb95090d09c33ce0497e0394c82aa33d
>>>>>
>>>>> 	(the top patch of my keys-next branch)
>>>>>
>>>>> to see if that is okay by both of you?  If so, can you give it a whirl?
>>>
>>>
>>> I’m seeing a build error within blacklist_hashes_checked with
>>> one of my configs.
>>>
>>> The config is as follows:
>>>
>>> $ grep CONFIG_SYSTEM_BLACKLIST_HASH_LIST .config
>>> CONFIG_SYSTEM_BLACKLIST_HASH_LIST=“revocation_list"
>>>
>>> $ cat certs/revocation_list
>>> "tbs:1e125ea4f38acb7b29b0c495fd8e7602c2c3353b913811a9da3a2fb505c08a32”
>>>
>>> make[1]: *** No rule to make target 'revocation_list', needed by 'certs/blacklist_hashes_checked'.  Stop.
>>
>> It requires an absolute path.
> 
> Ok, if I use an absolute path now with CONFIG_SYSTEM_BLACKLIST_HASH_LIST 
> it works.
> 
>> This is to align with other variables
>> using the config_filename macro: CONFIG_SYSTEM_TRUSTED_KEYS,
>> CONFIG_MODULE_SIG_KEY and now CONFIG_SYSTEM_REVOCATION_KEYS.
> 
> I just did a quick test with CONFIG_SYSTEM_TRUSTED_KEYS. It looks like we 
> can use either a relative or absolute path with CONFIG_SYSTEM_TRUSTED_KEYS. 
> Shouldn’t this be consistent?

CONFIG_SYSTEM_TRUSTED_KEYS (and similar config) works with relative path
to $(srctree) not $(srctree)/certs as in your example.

We can make CONFIG_SYSTEM_BLACKLIST_HASH_LIST works with $(srctree) with
this patch:

diff --git a/certs/Makefile b/certs/Makefile
index eb45407ff282..92a233eaa926 100644
--- a/certs/Makefile
+++ b/certs/Makefile
@@ -14,6 +14,8 @@ $(eval $(call config_filename,SYSTEM_BLACKLIST_HASH_LIST))

 $(obj)/blacklist_hashes.o: $(obj)/blacklist_hashes_checked

+CFLAGS_blacklist_hashes.o += -I$(srctree)
+
 targets += blacklist_hashes_checked


> 
>> Cf. https://lore.kernel.org/lkml/1221725.1607515111@warthog.procyon.org.uk/
>>
>> We may want to patch scripts/kconfig/streamline_config.pl for both
>> CONFIG_SYSTEM_REVOCATION_KEYS and CONFIG_SYSTEM_BLACKLIST_HASH_LIST, to
>> warn user (and exit with an error) if such files are not found.
> 

Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]

[Eric Snowberg]

> On Feb 5, 2021, at 3:27 AM, Mickaël Salaün <mic@digikod.net> wrote:
> 
> 
> On 05/02/2021 01:24, Eric Snowberg wrote:
>> 
>>> On Feb 4, 2021, at 1:26 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>> 
>>> 
>>> On 04/02/2021 04:53, Eric Snowberg wrote:
>>>> 
>>>>> On Feb 3, 2021, at 11:49 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>>>> 
>>>>> This looks good to me, and it still works for my use case. Eric's
>>>>> patchset only looks for asymmetric keys in the blacklist keyring, so
>>>>> even if we use the same keyring we don't look for the same key types. My
>>>>> patchset only allows blacklist keys (i.e. hashes, not asymmetric keys)
>>>>> to be added by user space (if authenticated), but because Eric's
>>>>> asymmetric keys are loaded with KEY_ALLOC_BYPASS_RESTRICTION, it should
>>>>> be OK for his use case.  There should be no interference between the two
>>>>> new features, but I find it a bit confusing to have such distinct use of
>>>>> keys from the same keyring depending on their type.
>>>> 
>>>> I agree, it is a bit confusing.  What is the thought of having a dbx 
>>>> keyring, similar to how the platform keyring works?
>>>> 
>>>> https://www.spinics.net/lists/linux-security-module/msg40262.html
>>>> 
>>>> 
>>>>> On 03/02/2021 17:26, David Howells wrote:
>>>>>> 
>>>>>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
>>>>>> 
>>>>>>> This is the fifth patch series for adding support for 
>>>>>>> EFI_CERT_X509_GUID entries [1].  It has been expanded to not only include
>>>>>>> dbx entries but also entries in the mokx.  Additionally my series to
>>>>>>> preload these certificate [2] has also been included.
>>>>>> 
>>>>>> Okay, I've tentatively applied this to my keys-next branch.  However, it
>>>>>> conflicts minorly with Mickaël Salaün's patches that I've previously merged on
>>>>>> the same branch.  Can you have a look at the merge commit
>>>>>> 
>>>>>> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-next&id=fdbbe7ceeb95090d09c33ce0497e0394c82aa33d
>>>>>> 
>>>>>> 	(the top patch of my keys-next branch)
>>>>>> 
>>>>>> to see if that is okay by both of you?  If so, can you give it a whirl?
>>>> 
>>>> 
>>>> I’m seeing a build error within blacklist_hashes_checked with
>>>> one of my configs.
>>>> 
>>>> The config is as follows:
>>>> 
>>>> $ grep CONFIG_SYSTEM_BLACKLIST_HASH_LIST .config
>>>> CONFIG_SYSTEM_BLACKLIST_HASH_LIST=“revocation_list"
>>>> 
>>>> $ cat certs/revocation_list
>>>> "tbs:1e125ea4f38acb7b29b0c495fd8e7602c2c3353b913811a9da3a2fb505c08a32”
>>>> 
>>>> make[1]: *** No rule to make target 'revocation_list', needed by 'certs/blacklist_hashes_checked'.  Stop.
>>> 
>>> It requires an absolute path.
>> 
>> Ok, if I use an absolute path now with CONFIG_SYSTEM_BLACKLIST_HASH_LIST 
>> it works.
>> 
>>> This is to align with other variables
>>> using the config_filename macro: CONFIG_SYSTEM_TRUSTED_KEYS,
>>> CONFIG_MODULE_SIG_KEY and now CONFIG_SYSTEM_REVOCATION_KEYS.
>> 
>> I just did a quick test with CONFIG_SYSTEM_TRUSTED_KEYS. It looks like we 
>> can use either a relative or absolute path with CONFIG_SYSTEM_TRUSTED_KEYS. 
>> Shouldn’t this be consistent?
> 
> CONFIG_SYSTEM_TRUSTED_KEYS (and similar config) works with relative path
> to $(srctree) not $(srctree)/certs as in your example.

Correct, I had "certs" in my relative path.

> We can make CONFIG_SYSTEM_BLACKLIST_HASH_LIST works with $(srctree) with
> this patch:
> 
> diff --git a/certs/Makefile b/certs/Makefile
> index eb45407ff282..92a233eaa926 100644
> --- a/certs/Makefile
> +++ b/certs/Makefile
> @@ -14,6 +14,8 @@ $(eval $(call config_filename,SYSTEM_BLACKLIST_HASH_LIST))
> 
> $(obj)/blacklist_hashes.o: $(obj)/blacklist_hashes_checked
> 
> +CFLAGS_blacklist_hashes.o += -I$(srctree)
> +
> targets += blacklist_hashes_checked

After applying this patch, CONFIG_SYSTEM_BLACKLIST_HASH_LIST now works
like the other filename macros.  It seems like this would be a good
addition.

I have done some additional testing, I am seeing a regression. The blacklist 
keyring is no longer picking up any of the hashes from the dbx during boot. 
I backed out the merge with my changes  (fdbbe7ceeb95090d09c33ce0497e0394c82aa33d) 
and still see the regression.  I then backed out Mickaël merge
(5bf1adccf5c41dbdd51d1f4de220d335d9548598) and it fixes the regression.

On a x86 with the updated dbx from uefi.org, I’d expect to see 234 bin hash entries
in the blacklist keyring.  With the current merged code, there is none.

Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]

[Mickaël Salaün]

On 06/02/2021 02:14, Eric Snowberg wrote:
> 
>> On Feb 5, 2021, at 3:27 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>
>>
>> On 05/02/2021 01:24, Eric Snowberg wrote:
>>>
>>>> On Feb 4, 2021, at 1:26 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>>>
>>>>
>>>> On 04/02/2021 04:53, Eric Snowberg wrote:
>>>>>
>>>>>> On Feb 3, 2021, at 11:49 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>>>>>
>>>>>> This looks good to me, and it still works for my use case. Eric's
>>>>>> patchset only looks for asymmetric keys in the blacklist keyring, so
>>>>>> even if we use the same keyring we don't look for the same key types. My
>>>>>> patchset only allows blacklist keys (i.e. hashes, not asymmetric keys)
>>>>>> to be added by user space (if authenticated), but because Eric's
>>>>>> asymmetric keys are loaded with KEY_ALLOC_BYPASS_RESTRICTION, it should
>>>>>> be OK for his use case.  There should be no interference between the two
>>>>>> new features, but I find it a bit confusing to have such distinct use of
>>>>>> keys from the same keyring depending on their type.
>>>>>
>>>>> I agree, it is a bit confusing.  What is the thought of having a dbx 
>>>>> keyring, similar to how the platform keyring works?
>>>>>
>>>>> https://www.spinics.net/lists/linux-security-module/msg40262.html
>>>>>
>>>>>
>>>>>> On 03/02/2021 17:26, David Howells wrote:
>>>>>>>
>>>>>>> Eric Snowberg <eric.snowberg@oracle.com> wrote:
>>>>>>>
>>>>>>>> This is the fifth patch series for adding support for 
>>>>>>>> EFI_CERT_X509_GUID entries [1].  It has been expanded to not only include
>>>>>>>> dbx entries but also entries in the mokx.  Additionally my series to
>>>>>>>> preload these certificate [2] has also been included.
>>>>>>>
>>>>>>> Okay, I've tentatively applied this to my keys-next branch.  However, it
>>>>>>> conflicts minorly with Mickaël Salaün's patches that I've previously merged on
>>>>>>> the same branch.  Can you have a look at the merge commit
>>>>>>>
>>>>>>> 	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-next&id=fdbbe7ceeb95090d09c33ce0497e0394c82aa33d
>>>>>>>
>>>>>>> 	(the top patch of my keys-next branch)
>>>>>>>
>>>>>>> to see if that is okay by both of you?  If so, can you give it a whirl?
>>>>>
>>>>>
>>>>> I’m seeing a build error within blacklist_hashes_checked with
>>>>> one of my configs.
>>>>>
>>>>> The config is as follows:
>>>>>
>>>>> $ grep CONFIG_SYSTEM_BLACKLIST_HASH_LIST .config
>>>>> CONFIG_SYSTEM_BLACKLIST_HASH_LIST=“revocation_list"
>>>>>
>>>>> $ cat certs/revocation_list
>>>>> "tbs:1e125ea4f38acb7b29b0c495fd8e7602c2c3353b913811a9da3a2fb505c08a32”
>>>>>
>>>>> make[1]: *** No rule to make target 'revocation_list', needed by 'certs/blacklist_hashes_checked'.  Stop.
>>>>
>>>> It requires an absolute path.
>>>
>>> Ok, if I use an absolute path now with CONFIG_SYSTEM_BLACKLIST_HASH_LIST 
>>> it works.
>>>
>>>> This is to align with other variables
>>>> using the config_filename macro: CONFIG_SYSTEM_TRUSTED_KEYS,
>>>> CONFIG_MODULE_SIG_KEY and now CONFIG_SYSTEM_REVOCATION_KEYS.
>>>
>>> I just did a quick test with CONFIG_SYSTEM_TRUSTED_KEYS. It looks like we 
>>> can use either a relative or absolute path with CONFIG_SYSTEM_TRUSTED_KEYS. 
>>> Shouldn’t this be consistent?
>>
>> CONFIG_SYSTEM_TRUSTED_KEYS (and similar config) works with relative path
>> to $(srctree) not $(srctree)/certs as in your example.
> 
> Correct, I had "certs" in my relative path.
> 
>> We can make CONFIG_SYSTEM_BLACKLIST_HASH_LIST works with $(srctree) with
>> this patch:
>>
>> diff --git a/certs/Makefile b/certs/Makefile
>> index eb45407ff282..92a233eaa926 100644
>> --- a/certs/Makefile
>> +++ b/certs/Makefile
>> @@ -14,6 +14,8 @@ $(eval $(call config_filename,SYSTEM_BLACKLIST_HASH_LIST))
>>
>> $(obj)/blacklist_hashes.o: $(obj)/blacklist_hashes_checked
>>
>> +CFLAGS_blacklist_hashes.o += -I$(srctree)
>> +
>> targets += blacklist_hashes_checked
> 
> After applying this patch, CONFIG_SYSTEM_BLACKLIST_HASH_LIST now works
> like the other filename macros.  It seems like this would be a good
> addition.

I'll send a patch with this.

> 
> I have done some additional testing, I am seeing a regression. The blacklist 
> keyring is no longer picking up any of the hashes from the dbx during boot. 
> I backed out the merge with my changes  (fdbbe7ceeb95090d09c33ce0497e0394c82aa33d) 
> and still see the regression.  I then backed out Mickaël merge
> (5bf1adccf5c41dbdd51d1f4de220d335d9548598) and it fixes the regression.
> 
> On a x86 with the updated dbx from uefi.org, I’d expect to see 234 bin hash entries
> in the blacklist keyring.  With the current merged code, there is none.

Hum, I missed a part in refactoring (commit
f78e50c8f750c0ac6767ac1ed006360cf77c56c4). :/
Could you please test the following patch?

diff --git a/certs/blacklist.c b/certs/blacklist.c
index 07c592ae5307..f998a2e85ddc 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -197,13 +197,16 @@ int mark_hash_blacklisted(const u8 *hash, size_t
hash_len,
                enum blacklist_hash_type hash_type)
 {
        const char *buffer;
+       int err;

        buffer = get_raw_hash(hash, hash_len, hash_type);
        if (IS_ERR(buffer))
                return PTR_ERR(buffer);
+       err = mark_raw_hash_blacklisted(buffer);
        kfree(buffer);
-       return 0;
+       return err;
 }


Is it possible to test these kind of dbx blacklist with Qemu?

Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]

[Eric Snowberg]

> On Feb 6, 2021, at 11:30 AM, Mickaël Salaün <mic@digikod.net> wrote:
> 
> On 06/02/2021 02:14, Eric Snowberg wrote:
> 
>> I have done some additional testing, I am seeing a regression. The blacklist 
>> keyring is no longer picking up any of the hashes from the dbx during boot. 
>> I backed out the merge with my changes  (fdbbe7ceeb95090d09c33ce0497e0394c82aa33d) 
>> and still see the regression.  I then backed out Mickaël merge
>> (5bf1adccf5c41dbdd51d1f4de220d335d9548598) and it fixes the regression.
>> 
>> On a x86 with the updated dbx from uefi.org, I’d expect to see 234 bin hash entries
>> in the blacklist keyring.  With the current merged code, there is none.
> 
> Hum, I missed a part in refactoring (commit
> f78e50c8f750c0ac6767ac1ed006360cf77c56c4). :/
> Could you please test the following patch?
> 
> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index 07c592ae5307..f998a2e85ddc 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c
> @@ -197,13 +197,16 @@ int mark_hash_blacklisted(const u8 *hash, size_t
> hash_len,
>                enum blacklist_hash_type hash_type)
> {
>        const char *buffer;
> +       int err;
> 
>        buffer = get_raw_hash(hash, hash_len, hash_type);
>        if (IS_ERR(buffer))
>                return PTR_ERR(buffer);
> +       err = mark_raw_hash_blacklisted(buffer);
>        kfree(buffer);
> -       return 0;
> +       return err;
> }

I applied this patch, it works better, but there is still a regression. 
Most of the hashes show up in the blacklist keyring now.  However some 
do not, here is what I see in the log during boot:

[    2.321876] blacklist: Problem blacklisting hash (-13)
[    2.322729] blacklist: Problem blacklisting hash (-13)
[    2.323549] blacklist: Problem blacklisting hash (-13)
[    2.324369] blacklist: Problem blacklisting hash (-13)

> Is it possible to test these kind of dbx blacklist with Qemu?

Yes, just use OVMF. 

Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]

[Mickaël Salaün]

On 09/02/2021 00:05, Eric Snowberg wrote:
> 
>> On Feb 6, 2021, at 11:30 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>
>> On 06/02/2021 02:14, Eric Snowberg wrote:
>>
>>> I have done some additional testing, I am seeing a regression. The blacklist 
>>> keyring is no longer picking up any of the hashes from the dbx during boot. 
>>> I backed out the merge with my changes  (fdbbe7ceeb95090d09c33ce0497e0394c82aa33d) 
>>> and still see the regression.  I then backed out Mickaël merge
>>> (5bf1adccf5c41dbdd51d1f4de220d335d9548598) and it fixes the regression.
>>>
>>> On a x86 with the updated dbx from uefi.org, I’d expect to see 234 bin hash entries
>>> in the blacklist keyring.  With the current merged code, there is none.
>>
>> Hum, I missed a part in refactoring (commit
>> f78e50c8f750c0ac6767ac1ed006360cf77c56c4). :/
>> Could you please test the following patch?
>>
>> diff --git a/certs/blacklist.c b/certs/blacklist.c
>> index 07c592ae5307..f998a2e85ddc 100644
>> --- a/certs/blacklist.c
>> +++ b/certs/blacklist.c
>> @@ -197,13 +197,16 @@ int mark_hash_blacklisted(const u8 *hash, size_t
>> hash_len,
>>                enum blacklist_hash_type hash_type)
>> {
>>        const char *buffer;
>> +       int err;
>>
>>        buffer = get_raw_hash(hash, hash_len, hash_type);
>>        if (IS_ERR(buffer))
>>                return PTR_ERR(buffer);
>> +       err = mark_raw_hash_blacklisted(buffer);
>>        kfree(buffer);
>> -       return 0;
>> +       return err;
>> }
> 
> I applied this patch, it works better, but there is still a regression. 
> Most of the hashes show up in the blacklist keyring now.  However some 
> do not, here is what I see in the log during boot:
> 
> [    2.321876] blacklist: Problem blacklisting hash (-13)
> [    2.322729] blacklist: Problem blacklisting hash (-13)
> [    2.323549] blacklist: Problem blacklisting hash (-13)
> [    2.324369] blacklist: Problem blacklisting hash (-13)
> 
>> Is it possible to test these kind of dbx blacklist with Qemu?
> 
> Yes, just use OVMF. 
> 

My changes (with the fix) don't change the previous semantic. I just
tested without my changes and with my changes (and the fix), and I get
the same result: 184 bin hashes with
https://uefi.org/sites/default/files/resources/dbxupdate_x64.bin

Could you please re-test and if there is still an issue bisect and share
the certificates causing this issue?

David, do you want me to send the two new patches or an updated full
patch series?

Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]

[Mickaël Salaün]

On 09/02/2021 22:53, Mickaël Salaün wrote:
> 
> On 09/02/2021 00:05, Eric Snowberg wrote:
>>
>>> On Feb 6, 2021, at 11:30 AM, Mickaël Salaün <mic@digikod.net> wrote:
>>>
>>> On 06/02/2021 02:14, Eric Snowberg wrote:
>>>
>>>> I have done some additional testing, I am seeing a regression. The blacklist 
>>>> keyring is no longer picking up any of the hashes from the dbx during boot. 
>>>> I backed out the merge with my changes  (fdbbe7ceeb95090d09c33ce0497e0394c82aa33d) 
>>>> and still see the regression.  I then backed out Mickaël merge
>>>> (5bf1adccf5c41dbdd51d1f4de220d335d9548598) and it fixes the regression.
>>>>
>>>> On a x86 with the updated dbx from uefi.org, I’d expect to see 234 bin hash entries
>>>> in the blacklist keyring.  With the current merged code, there is none.
>>>
>>> Hum, I missed a part in refactoring (commit
>>> f78e50c8f750c0ac6767ac1ed006360cf77c56c4). :/
>>> Could you please test the following patch?
>>>
>>> diff --git a/certs/blacklist.c b/certs/blacklist.c
>>> index 07c592ae5307..f998a2e85ddc 100644
>>> --- a/certs/blacklist.c
>>> +++ b/certs/blacklist.c
>>> @@ -197,13 +197,16 @@ int mark_hash_blacklisted(const u8 *hash, size_t
>>> hash_len,
>>>                enum blacklist_hash_type hash_type)
>>> {
>>>        const char *buffer;
>>> +       int err;
>>>
>>>        buffer = get_raw_hash(hash, hash_len, hash_type);
>>>        if (IS_ERR(buffer))
>>>                return PTR_ERR(buffer);
>>> +       err = mark_raw_hash_blacklisted(buffer);
>>>        kfree(buffer);
>>> -       return 0;
>>> +       return err;
>>> }
>>
>> I applied this patch, it works better, but there is still a regression. 
>> Most of the hashes show up in the blacklist keyring now.  However some 
>> do not, here is what I see in the log during boot:
>>
>> [    2.321876] blacklist: Problem blacklisting hash (-13)
>> [    2.322729] blacklist: Problem blacklisting hash (-13)
>> [    2.323549] blacklist: Problem blacklisting hash (-13)
>> [    2.324369] blacklist: Problem blacklisting hash (-13)
>>
>>> Is it possible to test these kind of dbx blacklist with Qemu?
>>
>> Yes, just use OVMF. 
>>
> 
> My changes (with the fix) don't change the previous semantic. I just
> tested without my changes and with my changes (and the fix), and I get
> the same result: 184 bin hashes with
> https://uefi.org/sites/default/files/resources/dbxupdate_x64.bin
> 
> Could you please re-test and if there is still an issue bisect and share
> the certificates causing this issue?
> 
> David, do you want me to send the two new patches or an updated full
> patch series?
> 

I found the issue and fixed it in a new patch series:
https://lore.kernel.org/lkml/20210210120410.471693-1-mic@digikod.net/

Re: Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]

[David Howells]

Hi Eric, Mickaël,

Do we have a consensus on this?  From what's written here, I don't think I can
ask Linus to pull the merge of your two branches.  I feel that I probably need
to push Eric's first as that fixes a CVE if I can't offer a merge.

David

Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]

[Mickaël Salaün]

Hi David,

The only commit causing issues is commit f78e50c8f750 ("certs: Factor
out the blacklist hash creation"). I think my last patch fix the issue,
and I'm testing with the UEFI DBX, but I don't understand why this
change would have an impact. In the meantime you can push Eric's commits
first, I'll adapt my changes.

 Mickaël


On 09/02/2021 14:14, David Howells wrote:
> 
> Hi Eric, Mickaël,
> 
> Do we have a consensus on this?  From what's written here, I don't think I can
> ask Linus to pull the merge of your two branches.  I feel that I probably need
> to push Eric's first as that fixes a CVE if I can't offer a merge.
> 
> David
> 

Re: Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]

[David Howells]

Mickaël Salaün <mic@digikod.net> wrote:

> The only commit causing issues is commit f78e50c8f750 ("certs: Factor
> out the blacklist hash creation"). I think my last patch fix the issue,
> and I'm testing with the UEFI DBX, but I don't understand why this
> change would have an impact. In the meantime you can push Eric's commits
> first, I'll adapt my changes.

Okay.  In that case, I've dropped your branch from my keys-next branch for the
moment and remerged Eric's branch.

David

Re: Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]

[Jarkko Sakkinen]

On Tue, Feb 09, 2021 at 01:14:06PM +0000, David Howells wrote:
> 
> Hi Eric, Mickaël,
> 
> Do we have a consensus on this?  From what's written here, I don't think I can
> ask Linus to pull the merge of your two branches.  I feel that I probably need
> to push Eric's first as that fixes a CVE if I can't offer a merge.
> 
> David

Would it be possible to compose a single unified patch set?

It's also somewhat distracting to review both separately.

/Jarkko

Re: Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]

[David Howells]

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

> > On Feb 3, 2021, at 11:49 AM, Mickaël Salaün <mic@digikod.net> wrote:
> > 
> > This looks good to me, and it still works for my use case. Eric's
> > patchset only looks for asymmetric keys in the blacklist keyring, so
> > even if we use the same keyring we don't look for the same key types. My
> > patchset only allows blacklist keys (i.e. hashes, not asymmetric keys)
> > to be added by user space (if authenticated), but because Eric's
> > asymmetric keys are loaded with KEY_ALLOC_BYPASS_RESTRICTION, it should
> > be OK for his use case.  There should be no interference between the two
> > new features, but I find it a bit confusing to have such distinct use of
> > keys from the same keyring depending on their type.
> 
> I agree, it is a bit confusing.  What is the thought of having a dbx 
> keyring, similar to how the platform keyring works?
> 
> https://www.spinics.net/lists/linux-security-module/msg40262.html

That would be fine by me.

David