| Message ID | 20200916004927.64276-1-eric.snowberg@oracle.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v4] certs: Add EFI_CERT_X509_GUID support for dbx entries | expand |
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
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
> 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.
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
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;
}
> 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; > } >
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
> 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.
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
> 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.
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
> 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.
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
> 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?
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
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
[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
> 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.
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
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
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
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; }
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(+)