Re: Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]
On Tue, Feb 09, 2021 at 01:14:06PM +, David Howells wrote: > > Hi Eric, Mickaël, > > Do we have a consensus on this? From what's written here, I don't think I can > ask Linus to pull the merge of your two branches. I feel that I probably need > to push Eric's first as that fixes a CVE if I can't offer a merge. > > David Would it be possible to compose a single unified patch set? It's also somewhat distracting to review both separately. /Jarkko
Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]
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 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-...@digikod.net/
Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]
On 09/02/2021 00:05, Eric Snowberg wrote:
>
>> On Feb 6, 2021, at 11:30 AM, Mickaël Salaün wrote:
>>
>> On 06/02/2021 02:14, Eric Snowberg wrote:
>>
>>> I have done some additional testing, I am seeing a regression. The
>>> blacklist
>>> keyring is no longer picking up any of the hashes from the dbx during boot.
>>> I backed out the merge with my changes
>>> (fdbbe7ceeb95090d09c33ce0497e0394c82aa33d)
>>> and still see the regression. I then backed out Mickaël merge
>>> (5bf1adccf5c41dbdd51d1f4de220d335d9548598) and it fixes the regression.
>>>
>>> On a x86 with the updated dbx from uefi.org, I’d expect to see 234 bin hash
>>> entries
>>> in the blacklist keyring. With the current merged code, there is none.
>>
>> Hum, I missed a part in refactoring (commit
>> f78e50c8f750c0ac6767ac1ed006360cf77c56c4). :/
>> Could you please test the following patch?
>>
>> diff --git a/certs/blacklist.c b/certs/blacklist.c
>> index 07c592ae5307..f998a2e85ddc 100644
>> --- a/certs/blacklist.c
>> +++ b/certs/blacklist.c
>> @@ -197,13 +197,16 @@ int mark_hash_blacklisted(const u8 *hash, size_t
>> hash_len,
>>enum blacklist_hash_type hash_type)
>> {
>>const char *buffer;
>> + int err;
>>
>>buffer = get_raw_hash(hash, hash_len, hash_type);
>>if (IS_ERR(buffer))
>>return PTR_ERR(buffer);
>> + err = mark_raw_hash_blacklisted(buffer);
>>kfree(buffer);
>> - return 0;
>> + return err;
>> }
>
> I applied this patch, it works better, but there is still a regression.
> Most of the hashes show up in the blacklist keyring now. However some
> do not, here is what I see in the log during boot:
>
> [2.321876] blacklist: Problem blacklisting hash (-13)
> [2.322729] blacklist: Problem blacklisting hash (-13)
> [2.323549] blacklist: Problem blacklisting hash (-13)
> [2.324369] blacklist: Problem blacklisting hash (-13)
>
>> Is it possible to test these kind of dbx blacklist with Qemu?
>
> Yes, just use OVMF.
>
My changes (with the fix) don't change the previous semantic. I just
tested without my changes and with my changes (and the fix), and I get
the same result: 184 bin hashes with
https://uefi.org/sites/default/files/resources/dbxupdate_x64.bin
Could you please re-test and if there is still an issue bisect and share
the certificates causing this issue?
David, do you want me to send the two new patches or an updated full
patch series?
Re: Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]
Mickaël Salaün wrote:
> The only commit causing issues is commit f78e50c8f750 ("certs: Factor
> out the blacklist hash creation"). I think my last patch fix the issue,
> and I'm testing with the UEFI DBX, but I don't understand why this
> change would have an impact. In the meantime you can push Eric's commits
> first, I'll adapt my changes.
Okay. In that case, I've dropped your branch from my keys-next branch for the
moment and remerged Eric's branch.
David
Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]
Hi David,
The only commit causing issues is commit f78e50c8f750 ("certs: Factor
out the blacklist hash creation"). I think my last patch fix the issue,
and I'm testing with the UEFI DBX, but I don't understand why this
change would have an impact. In the meantime you can push Eric's commits
first, I'll adapt my changes.
Mickaël
On 09/02/2021 14:14, David Howells wrote:
>
> Hi Eric, Mickaël,
>
> Do we have a consensus on this? From what's written here, I don't think I can
> ask Linus to pull the merge of your two branches. I feel that I probably need
> to push Eric's first as that fixes a CVE if I can't offer a merge.
>
> David
>
Re: Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]
Hi Eric, Mickaël, Do we have a consensus on this? From what's written here, I don't think I can ask Linus to pull the merge of your two branches. I feel that I probably need to push Eric's first as that fixes a CVE if I can't offer a merge. David
Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]
> On Feb 6, 2021, at 11:30 AM, Mickaël Salaün wrote:
>
> On 06/02/2021 02:14, Eric Snowberg wrote:
>
>> I have done some additional testing, I am seeing a regression. The blacklist
>> keyring is no longer picking up any of the hashes from the dbx during boot.
>> I backed out the merge with my changes
>> (fdbbe7ceeb95090d09c33ce0497e0394c82aa33d)
>> and still see the regression. I then backed out Mickaël merge
>> (5bf1adccf5c41dbdd51d1f4de220d335d9548598) and it fixes the regression.
>>
>> On a x86 with the updated dbx from uefi.org, I’d expect to see 234 bin hash
>> entries
>> in the blacklist keyring. With the current merged code, there is none.
>
> Hum, I missed a part in refactoring (commit
> f78e50c8f750c0ac6767ac1ed006360cf77c56c4). :/
> Could you please test the following patch?
>
> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index 07c592ae5307..f998a2e85ddc 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c
> @@ -197,13 +197,16 @@ int mark_hash_blacklisted(const u8 *hash, size_t
> hash_len,
>enum blacklist_hash_type hash_type)
> {
>const char *buffer;
> + int err;
>
>buffer = get_raw_hash(hash, hash_len, hash_type);
>if (IS_ERR(buffer))
>return PTR_ERR(buffer);
> + err = mark_raw_hash_blacklisted(buffer);
>kfree(buffer);
> - return 0;
> + return err;
> }
I applied this patch, it works better, but there is still a regression.
Most of the hashes show up in the blacklist keyring now. However some
do not, here is what I see in the log during boot:
[2.321876] blacklist: Problem blacklisting hash (-13)
[2.322729] blacklist: Problem blacklisting hash (-13)
[2.323549] blacklist: Problem blacklisting hash (-13)
[2.324369] blacklist: Problem blacklisting hash (-13)
> Is it possible to test these kind of dbx blacklist with Qemu?
Yes, just use OVMF.
Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]
On 06/02/2021 02:14, Eric Snowberg wrote: > >> On Feb 5, 2021, at 3:27 AM, Mickaël Salaün wrote: >> >> >> On 05/02/2021 01:24, Eric Snowberg wrote: >>> On Feb 4, 2021, at 1:26 AM, Mickaël Salaün wrote: On 04/02/2021 04:53, Eric Snowberg wrote: > >> On Feb 3, 2021, at 11:49 AM, Mickaël Salaün 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 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=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,
Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]
> On Feb 5, 2021, at 3:27 AM, Mickaël Salaün wrote: > > > On 05/02/2021 01:24, Eric Snowberg wrote: >> >>> On Feb 4, 2021, at 1:26 AM, Mickaël Salaün wrote: >>> >>> >>> On 04/02/2021 04:53, Eric Snowberg wrote: > On Feb 3, 2021, at 11:49 AM, Mickaël Salaün 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 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=fdbbe7ceeb95090d09c33ce0497e0394c82aa33d >> >> (the top patch of my keys-next branch) >> >> to see if that is okay by both of you? If so, can you give it a whirl? I’m seeing a build error within blacklist_hashes_checked with one of my configs. The config is as follows: $ grep CONFIG_SYSTEM_BLACKLIST_HASH_LIST .config CONFIG_SYSTEM_BLACKLIST_HASH_LIST=“revocation_list" $ cat certs/revocation_list "tbs:1e125ea4f38acb7b29b0c495fd8e7602c2c3353b913811a9da3a2fb505c08a32” make[1]: *** No rule to make target 'revocation_list', needed by 'certs/blacklist_hashes_checked'. Stop. >>> >>> It requires an absolute path. >> >> Ok, if I use an absolute path now with CONFIG_SYSTEM_BLACKLIST_HASH_LIST >> it works. >> >>> This is to align with other variables >>> using the config_filename macro: CONFIG_SYSTEM_TRUSTED_KEYS, >>> CONFIG_MODULE_SIG_KEY and now CONFIG_SYSTEM_REVOCATION_KEYS. >> >> I just did a quick test with CONFIG_SYSTEM_TRUSTED_KEYS. It looks like we >> can use either a relative or absolute path with CONFIG_SYSTEM_TRUSTED_KEYS. >> Shouldn’t this be consistent? > > CONFIG_SYSTEM_TRUSTED_KEYS (and similar config) works with relative path > to $(srctree) not $(srctree)/certs as in your example. Correct, I had "certs" in my relative path. > We can make CONFIG_SYSTEM_BLACKLIST_HASH_LIST works with $(srctree) with > this patch: > > diff --git a/certs/Makefile b/certs/Makefile > index eb45407ff282..92a233eaa926 100644 > --- a/certs/Makefile > +++ b/certs/Makefile > @@ -14,6 +14,8 @@ $(eval $(call config_filename,SYSTEM_BLACKLIST_HASH_LIST)) > > $(obj)/blacklist_hashes.o: $(obj)/blacklist_hashes_checked > > +CFLAGS_blacklist_hashes.o += -I$(srctree) > + > targets += blacklist_hashes_checked After applying this patch, CONFIG_SYSTEM_BLACKLIST_HASH_LIST now works like the other filename macros. It seems like this would be a good addition. I have done some additional testing, I am seeing a regression. The blacklist keyring is no longer picking up any of the hashes from the dbx during boot. I backed out the merge with my changes (fdbbe7ceeb95090d09c33ce0497e0394c82aa33d) and still see the regression. I then backed out Mickaël merge (5bf1adccf5c41dbdd51d1f4de220d335d9548598) and it fixes the regression. On a x86 with the updated dbx from uefi.org, I’d expect to see 234 bin hash entries in the blacklist keyring. With the current merged code, there is none.
Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]
On 05/02/2021 01:24, Eric Snowberg wrote: > >> On Feb 4, 2021, at 1:26 AM, Mickaël Salaün wrote: >> >> >> On 04/02/2021 04:53, Eric Snowberg wrote: >>> On Feb 3, 2021, at 11:49 AM, Mickaël Salaün 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 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=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.1607515...@warthog.procyon.org.uk/ >> >> We may want to patch scripts/kconfig/streamline_config.pl for both >> CONFIG_SYSTEM_REVOCATION_KEYS and CONFIG_SYSTEM_BLACKLIST_HASH_LIST, to >> warn user (and exit with an error) if such files are not found. >
Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]
> On Feb 4, 2021, at 1:26 AM, Mickaël Salaün wrote: > > > On 04/02/2021 04:53, Eric Snowberg wrote: >> >>> On Feb 3, 2021, at 11:49 AM, Mickaël Salaün 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 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=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.1607515...@warthog.procyon.org.uk/ > > We may want to patch scripts/kconfig/streamline_config.pl for both > CONFIG_SYSTEM_REVOCATION_KEYS and CONFIG_SYSTEM_BLACKLIST_HASH_LIST, to > warn user (and exit with an error) if such files are not found.
Re: Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]
Eric Snowberg wrote: > > On Feb 3, 2021, at 11:49 AM, Mickaël Salaün 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
Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]
On 04/02/2021 04:53, Eric Snowberg wrote: > >> On Feb 3, 2021, at 11:49 AM, Mickaël Salaün 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 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=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.1607515...@warthog.procyon.org.uk/ We may want to patch scripts/kconfig/streamline_config.pl for both CONFIG_SYSTEM_REVOCATION_KEYS and CONFIG_SYSTEM_BLACKLIST_HASH_LIST, to warn user (and exit with an error) if such files are not found.
Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]
> On Feb 3, 2021, at 11:49 AM, Mickaël Salaün 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 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=fdbbe7ceeb95090d09c33ce0497e0394c82aa33d >> >> (the top patch of my keys-next branch) >> >> to see if that is okay by both of you? If so, can you give it a whirl? I’m seeing a build error within blacklist_hashes_checked with one of my configs. The config is as follows: $ grep CONFIG_SYSTEM_BLACKLIST_HASH_LIST .config CONFIG_SYSTEM_BLACKLIST_HASH_LIST=“revocation_list" $ cat certs/revocation_list "tbs:1e125ea4f38acb7b29b0c495fd8e7602c2c3353b913811a9da3a2fb505c08a32” make[1]: *** No rule to make target 'revocation_list', needed by 'certs/blacklist_hashes_checked'. Stop.
Re: Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]
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 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=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 >
Conflict with Mickaël Salaün's blacklist patches [was [PATCH v5 0/4] Add EFI_CERT_X509_GUID support for dbx/mokx entries]
Eric Snowberg 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=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
