| Message ID | 20210122181054.32635-1-eric.snowberg@oracle.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add EFI_CERT_X509_GUID support for dbx/mokx entries | expand |
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
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
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
> 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.
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
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 >
> 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.
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.
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
> 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.
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. >
> 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.
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?
> 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.
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
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
>
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
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?
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/
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