Messages in this thread | | | Subject | Re: [PATCH v5 1/4] certs: Add EFI_CERT_X509_GUID support for dbx entries | From | Nayna <> | Date | Thu, 28 Jan 2021 10:35:09 -0500 |
| |
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
| |