[REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Karel Balej]

Dimitri, Johannes,

ever since upgrading to Linux v6.7 I am unable to connect to a 802.1X
wireless network (specifically, eduroam). In my dmesg, the following
messages appear:

	[   68.161621] wlan0: authenticate with xx:xx:xx:xx:xx:xx (local address=xx:xx:xx:xx:xx:xx)
	[   68.163733] wlan0: send auth to xx:xx:xx:xx:xx:xx (try 1/3)
	[   68.165773] wlan0: authenticated
	[   68.166785] wlan0: associate with xx:xx:xx:xx:xx:xx (try 1/3)
	[   68.168498] wlan0: RX AssocResp from xx:xx:xx:xx:xx:xx (capab=0x1411 status=0 aid=4)
	[   68.172445] wlan0: associated
	[   68.204956] wlan0: Limiting TX power to 23 (23 - 0) dBm as advertised by xx:xx:xx:xx:xx:xx
	[   70.262032] wlan0: deauthenticated from xx:xx:xx:xx:xx:xx (Reason: 23=IEEE8021X_FAILED)
	[   73.065966] wlan0: authenticate with xx:xx:xx:xx:xx:xx (local address=xx:xx:xx:xx:xx:xx)
	[   73.068006] wlan0: send auth to xx:xx:xx:xx:xx:xx (try 1/3)
	[   73.070166] wlan0: authenticated
	[   73.070756] wlan0: associate with xx:xx:xx:xx:xx:xx (try 1/3)
	[   73.072807] wlan0: RX AssocResp from xx:xx:xx:xx:xx:xx (capab=0x1411 status=0 aid=4)
	[   73.076676] wlan0: associated
	[   73.120396] wlan0: Limiting TX power to 23 (23 - 0) dBm as advertised by xx:xx:xx:xx:xx:xx
	[   75.148376] wlan0: deauthenticating from xx:xx:xx:xx:xx:xx by local choice (Reason: 23=IEEE8021X_FAILED)
	[   77.718016] wlan0: authenticate with xx:xx:xx:xx:xx:xx (local address=xx:xx:xx:xx:xx:xx)
	[   77.720137] wlan0: send auth to xx:xx:xx:xx:xx:xx (try 1/3)
	[   77.722670] wlan0: authenticated
	[   77.724737] wlan0: associate with xx:xx:xx:xx:xx:xx (try 1/3)
	[   77.726172] wlan0: RX AssocResp from xx:xx:xx:xx:xx:xx (capab=0x1411 status=0 aid=4)
	[   77.730822] wlan0: associated
	[   77.830763] wlan0: Limiting TX power to 23 (23 - 0) dBm as advertised by xx:xx:xx:xx:xx:xx
	[   79.784199] wlan0: deauthenticating from xx:xx:xx:xx:xx:xx by local choice (Reason: 23=IEEE8021X_FAILED)

The connection works fine with v6.6 and I have bisected the problem to
the revision introduced by this patch (16ab7cb5825f mainline).

My wireless kernel driver is iwlwifi and I use iwd. I started the bisect
with a config copied from my distribution package [1].

Would you please help me with this? Please let me know if I forgot to
mention something which could be helpful in resolving this.

[1] https://raw.githubusercontent.com/void-linux/void-packages/master/srcpkgs/linux6.6/files/x86_64-dotconfig

Thank you very much, kind regards,
K. B.

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Johannes Berg]

Not sure why you're CC'ing the world, but I guess adding a few more
doesn't hurt ...

On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
> 
>  and I use iwd

This is your problem, the wireless stack in the kernel doesn't use any
kernel crypto code for 802.1X.

I suppose iwd wants to use the kernel infrastructure but has no
fallbacks to other implementations.

johannes

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[James Prestwood]

Hi,

On 3/13/24 1:56 AM, Johannes Berg wrote:
> Not sure why you're CC'ing the world, but I guess adding a few more
> doesn't hurt ...
>
> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
>>   and I use iwd
> This is your problem, the wireless stack in the kernel doesn't use any
> kernel crypto code for 802.1X.

Yes, the wireless stack has zero bearing on the issue. I think that's 
what you meant by "problem".

IWD has used the kernel crypto API forever which was abruptly broken, 
that is the problem.

The original commit says it was to remove support for sha1 signed kernel 
modules, but it did more than that and broke the keyctl API.

>
> I suppose iwd wants to use the kernel infrastructure but has no
> fallbacks to other implementations.
> johannes
>

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Eric Biggers]

On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:
> Hi,
> 
> On 3/13/24 1:56 AM, Johannes Berg wrote:
> > Not sure why you're CC'ing the world, but I guess adding a few more
> > doesn't hurt ...
> > 
> > On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
> > >   and I use iwd
> > This is your problem, the wireless stack in the kernel doesn't use any
> > kernel crypto code for 802.1X.
> 
> Yes, the wireless stack has zero bearing on the issue. I think that's what
> you meant by "problem".
> 
> IWD has used the kernel crypto API forever which was abruptly broken, that
> is the problem.
> 
> The original commit says it was to remove support for sha1 signed kernel
> modules, but it did more than that and broke the keyctl API.
> 

Which specific API is iwd using that is relevant here?
I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
and grepped for keyctl and AF_ALG, but there are no matches.

- Eric

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[James Prestwood]

Hi,

On 3/13/24 12:44 PM, Eric Biggers wrote:
> On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:
>> Hi,
>>
>> On 3/13/24 1:56 AM, Johannes Berg wrote:
>>> Not sure why you're CC'ing the world, but I guess adding a few more
>>> doesn't hurt ...
>>>
>>> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
>>>>    and I use iwd
>>> This is your problem, the wireless stack in the kernel doesn't use any
>>> kernel crypto code for 802.1X.
>> Yes, the wireless stack has zero bearing on the issue. I think that's what
>> you meant by "problem".
>>
>> IWD has used the kernel crypto API forever which was abruptly broken, that
>> is the problem.
>>
>> The original commit says it was to remove support for sha1 signed kernel
>> modules, but it did more than that and broke the keyctl API.
>>
> Which specific API is iwd using that is relevant here?
> I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
> and grepped for keyctl and AF_ALG, but there are no matches.

IWD uses ELL for its crypto, which uses the AF_ALG API:

https://git.kernel.org/pub/scm/libs/ell/ell.git/

I believe the failure is when calling:

KEYCTL_PKEY_QUERY enc="x962" hash="sha1"

 From logs Michael posted on the IWD list, the ELL API that fails is:

l_key_get_info (ell.git/ell/key.c:416)

Thanks,

James

>
> - Eric

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Eric Biggers]

On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote:
> Hi,
> 
> On 3/13/24 12:44 PM, Eric Biggers wrote:
> > On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:
> > > Hi,
> > > 
> > > On 3/13/24 1:56 AM, Johannes Berg wrote:
> > > > Not sure why you're CC'ing the world, but I guess adding a few more
> > > > doesn't hurt ...
> > > > 
> > > > On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
> > > > >    and I use iwd
> > > > This is your problem, the wireless stack in the kernel doesn't use any
> > > > kernel crypto code for 802.1X.
> > > Yes, the wireless stack has zero bearing on the issue. I think that's what
> > > you meant by "problem".
> > > 
> > > IWD has used the kernel crypto API forever which was abruptly broken, that
> > > is the problem.
> > > 
> > > The original commit says it was to remove support for sha1 signed kernel
> > > modules, but it did more than that and broke the keyctl API.
> > > 
> > Which specific API is iwd using that is relevant here?
> > I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
> > and grepped for keyctl and AF_ALG, but there are no matches.
> 
> IWD uses ELL for its crypto, which uses the AF_ALG API:
> 
> https://git.kernel.org/pub/scm/libs/ell/ell.git/

Thanks for pointing out that the relevant code is really in that separate
repository.  Note, it seems that keyctl() is the problem here, not AF_ALG.  The
blamed commit didn't change anything for AF_ALG.

> I believe the failure is when calling:
> 
> KEYCTL_PKEY_QUERY enc="x962" hash="sha1"
> 
> From logs Michael posted on the IWD list, the ELL API that fails is:
> 
> l_key_get_info (ell.git/ell/key.c:416)

Okay, I guess that's what's actually causing the problem.  KEYCTL_PKEY_* are a
weird set of APIs where userspace can ask the kernel to do asymmetric key
operations.  It's unclear why they exist, as the same functionality is available
in userspace crypto libraries.

I suppose that the blamed commit, or at least part of it, will need to be
reverted to keep these weird keyctls working.

For the future, why doesn't iwd just use a userspace crypto library such as
OpenSSL?

- Eric

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[James Prestwood]

Hi,

On 3/13/24 1:22 PM, Eric Biggers wrote:
> On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote:
>> Hi,
>>
>> On 3/13/24 12:44 PM, Eric Biggers wrote:
>>> On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:
>>>> Hi,
>>>>
>>>> On 3/13/24 1:56 AM, Johannes Berg wrote:
>>>>> Not sure why you're CC'ing the world, but I guess adding a few more
>>>>> doesn't hurt ...
>>>>>
>>>>> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
>>>>>>     and I use iwd
>>>>> This is your problem, the wireless stack in the kernel doesn't use any
>>>>> kernel crypto code for 802.1X.
>>>> Yes, the wireless stack has zero bearing on the issue. I think that's what
>>>> you meant by "problem".
>>>>
>>>> IWD has used the kernel crypto API forever which was abruptly broken, that
>>>> is the problem.
>>>>
>>>> The original commit says it was to remove support for sha1 signed kernel
>>>> modules, but it did more than that and broke the keyctl API.
>>>>
>>> Which specific API is iwd using that is relevant here?
>>> I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
>>> and grepped for keyctl and AF_ALG, but there are no matches.
>> IWD uses ELL for its crypto, which uses the AF_ALG API:
>>
>> https://git.kernel.org/pub/scm/libs/ell/ell.git/
> Thanks for pointing out that the relevant code is really in that separate
> repository.  Note, it seems that keyctl() is the problem here, not AF_ALG.  The
> blamed commit didn't change anything for AF_ALG.
>
>> I believe the failure is when calling:
>>
>> KEYCTL_PKEY_QUERY enc="x962" hash="sha1"
>>
>>  From logs Michael posted on the IWD list, the ELL API that fails is:
>>
>> l_key_get_info (ell.git/ell/key.c:416)
> Okay, I guess that's what's actually causing the problem.  KEYCTL_PKEY_* are a
> weird set of APIs where userspace can ask the kernel to do asymmetric key
> operations.  It's unclear why they exist, as the same functionality is available
> in userspace crypto libraries.
>
> I suppose that the blamed commit, or at least part of it, will need to be
> reverted to keep these weird keyctls working.
>
> For the future, why doesn't iwd just use a userspace crypto library such as
> OpenSSL?

I was not around when the original decision was made, but a few reasons 
I know we don't use openSSL:

  - IWD has virtually zero dependencies.

  - OpenSSL + friends are rather large libraries.

  - AF_ALG has transparent hardware acceleration (not sure if openSSL 
does too).

Another consideration is once you support openSSL someone wants wolfSSL, 
then boringSSL etc. Even if users implement support it just becomes a 
huge burden to carry for the project. Just look at wpa_supplicant's 
src/crypto/ folder, nearly 40k LOC in there, compared to ELL's crypto 
modules which is ~5k. You have to sort out all the nitty gritty details 
of each library, and provide a common driver/API for the core code, 
differences between openssl versions, the list goes on.

Thanks,

James


>
> - Eric

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Eric Biggers]

On Wed, Mar 13, 2024 at 02:17:29PM -0700, James Prestwood wrote:
> Hi,
> 
> On 3/13/24 1:22 PM, Eric Biggers wrote:
> > On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote:
> > > Hi,
> > > 
> > > On 3/13/24 12:44 PM, Eric Biggers wrote:
> > > > On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:
> > > > > Hi,
> > > > > 
> > > > > On 3/13/24 1:56 AM, Johannes Berg wrote:
> > > > > > Not sure why you're CC'ing the world, but I guess adding a few more
> > > > > > doesn't hurt ...
> > > > > > 
> > > > > > On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
> > > > > > >     and I use iwd
> > > > > > This is your problem, the wireless stack in the kernel doesn't use any
> > > > > > kernel crypto code for 802.1X.
> > > > > Yes, the wireless stack has zero bearing on the issue. I think that's what
> > > > > you meant by "problem".
> > > > > 
> > > > > IWD has used the kernel crypto API forever which was abruptly broken, that
> > > > > is the problem.
> > > > > 
> > > > > The original commit says it was to remove support for sha1 signed kernel
> > > > > modules, but it did more than that and broke the keyctl API.
> > > > > 
> > > > Which specific API is iwd using that is relevant here?
> > > > I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
> > > > and grepped for keyctl and AF_ALG, but there are no matches.
> > > IWD uses ELL for its crypto, which uses the AF_ALG API:
> > > 
> > > https://git.kernel.org/pub/scm/libs/ell/ell.git/
> > Thanks for pointing out that the relevant code is really in that separate
> > repository.  Note, it seems that keyctl() is the problem here, not AF_ALG.  The
> > blamed commit didn't change anything for AF_ALG.
> > 
> > > I believe the failure is when calling:
> > > 
> > > KEYCTL_PKEY_QUERY enc="x962" hash="sha1"
> > > 
> > >  From logs Michael posted on the IWD list, the ELL API that fails is:
> > > 
> > > l_key_get_info (ell.git/ell/key.c:416)
> > Okay, I guess that's what's actually causing the problem.  KEYCTL_PKEY_* are a
> > weird set of APIs where userspace can ask the kernel to do asymmetric key
> > operations.  It's unclear why they exist, as the same functionality is available
> > in userspace crypto libraries.
> > 
> > I suppose that the blamed commit, or at least part of it, will need to be
> > reverted to keep these weird keyctls working.
> > 
> > For the future, why doesn't iwd just use a userspace crypto library such as
> > OpenSSL?
> 
> I was not around when the original decision was made, but a few reasons I
> know we don't use openSSL:
> 
>  - IWD has virtually zero dependencies.

Depending on something in the kernel does not eliminate a dependency; it just
adds that particular kernel UAPI to your list of dependencies.  The reason that
we're having this discussion in the first place is because iwd is depending on
an obscure kernel UAPI that is not well defined.  Historically it's been hard to
avoid "breaking" changes in these crypto-related UAPIs because of the poor
design where a huge number of algorithms are potentially supported, but the list
is undocumented and it varies from one system to another based on configuration.
Also due to their obscurity many kernel developers don't know that these UAPIs
even exist.  (The reaction when someone finds out is usually "Why!?")

It may be worth looking at if iwd should make a different choice for this
dependency.  It's understandable to blame dependencies when things go wrong, but
at the same time the choice of dependency is very much a choice, and some
choices can be more technically sound and cause fewer problems than others...

>  - OpenSSL + friends are rather large libraries.

The Linux kernel is also large, and it's made larger by having to support
obsolete crypto algorithms for backwards compatibility with iwd.

>  - AF_ALG has transparent hardware acceleration (not sure if openSSL does
> too).

OpenSSL takes advantage of CPU-based hardware acceleration, e.g. AES-NI.

> Another consideration is once you support openSSL someone wants wolfSSL,
> then boringSSL etc. Even if users implement support it just becomes a huge
> burden to carry for the project. Just look at wpa_supplicant's src/crypto/
> folder, nearly 40k LOC in there, compared to ELL's crypto modules which is
> ~5k. You have to sort out all the nitty gritty details of each library, and
> provide a common driver/API for the core code, differences between openssl
> versions, the list goes on.

What is the specific functionality that you're actually relying on that you
think would need 40K lines of code to replace, even using OpenSSL?  I see you
are using KEYCTL_PKEY_*, but what specifically are you using them for?  What
operations are being performed, and with which algorithms and key formats?
Also, is the kernel behavior that you're relying on documented anywhere?  There
are man pages for those keyctls, but they don't say anything about any
particular hash algorithm, SHA-1 or otherwise, being supported.

- Eric

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Jeff Johnson]

On 3/13/2024 3:10 PM, Eric Biggers wrote:
> On Wed, Mar 13, 2024 at 02:17:29PM -0700, James Prestwood wrote:
>> Hi,
>>
>> On 3/13/24 1:22 PM, Eric Biggers wrote:
>>> On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote:
>>>> Hi,
>>>>
>>>> On 3/13/24 12:44 PM, Eric Biggers wrote:
>>>>> On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 3/13/24 1:56 AM, Johannes Berg wrote:
>>>>>>> Not sure why you're CC'ing the world, but I guess adding a few more
>>>>>>> doesn't hurt ...
>>>>>>>
>>>>>>> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
>>>>>>>>     and I use iwd
>>>>>>> This is your problem, the wireless stack in the kernel doesn't use any
>>>>>>> kernel crypto code for 802.1X.
>>>>>> Yes, the wireless stack has zero bearing on the issue. I think that's what
>>>>>> you meant by "problem".
>>>>>>
>>>>>> IWD has used the kernel crypto API forever which was abruptly broken, that
>>>>>> is the problem.
>>>>>>
>>>>>> The original commit says it was to remove support for sha1 signed kernel
>>>>>> modules, but it did more than that and broke the keyctl API.
>>>>>>
>>>>> Which specific API is iwd using that is relevant here?
>>>>> I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
>>>>> and grepped for keyctl and AF_ALG, but there are no matches.
>>>> IWD uses ELL for its crypto, which uses the AF_ALG API:
>>>>
>>>> https://git.kernel.org/pub/scm/libs/ell/ell.git/
>>> Thanks for pointing out that the relevant code is really in that separate
>>> repository.  Note, it seems that keyctl() is the problem here, not AF_ALG.  The
>>> blamed commit didn't change anything for AF_ALG.
>>>
>>>> I believe the failure is when calling:
>>>>
>>>> KEYCTL_PKEY_QUERY enc="x962" hash="sha1"
>>>>
>>>>  From logs Michael posted on the IWD list, the ELL API that fails is:
>>>>
>>>> l_key_get_info (ell.git/ell/key.c:416)
>>> Okay, I guess that's what's actually causing the problem.  KEYCTL_PKEY_* are a
>>> weird set of APIs where userspace can ask the kernel to do asymmetric key
>>> operations.  It's unclear why they exist, as the same functionality is available
>>> in userspace crypto libraries.
>>>
>>> I suppose that the blamed commit, or at least part of it, will need to be
>>> reverted to keep these weird keyctls working.
>>>
>>> For the future, why doesn't iwd just use a userspace crypto library such as
>>> OpenSSL?
>>
>> I was not around when the original decision was made, but a few reasons I
>> know we don't use openSSL:
>>
>>  - IWD has virtually zero dependencies.
> 
> Depending on something in the kernel does not eliminate a dependency; it just
> adds that particular kernel UAPI to your list of dependencies.  The reason that
> we're having this discussion in the first place is because iwd is depending on
> an obscure kernel UAPI that is not well defined.  Historically it's been hard to
> avoid "breaking" changes in these crypto-related UAPIs because of the poor
> design where a huge number of algorithms are potentially supported, but the list
> is undocumented and it varies from one system to another based on configuration.
> Also due to their obscurity many kernel developers don't know that these UAPIs
> even exist.  (The reaction when someone finds out is usually "Why!?")
> 
> It may be worth looking at if iwd should make a different choice for this
> dependency.  It's understandable to blame dependencies when things go wrong, but
> at the same time the choice of dependency is very much a choice, and some
> choices can be more technically sound and cause fewer problems than others...
> 
>>  - OpenSSL + friends are rather large libraries.
> 
> The Linux kernel is also large, and it's made larger by having to support
> obsolete crypto algorithms for backwards compatibility with iwd.
> 
>>  - AF_ALG has transparent hardware acceleration (not sure if openSSL does
>> too).
> 
> OpenSSL takes advantage of CPU-based hardware acceleration, e.g. AES-NI.
> 
>> Another consideration is once you support openSSL someone wants wolfSSL,
>> then boringSSL etc. Even if users implement support it just becomes a huge
>> burden to carry for the project. Just look at wpa_supplicant's src/crypto/
>> folder, nearly 40k LOC in there, compared to ELL's crypto modules which is
>> ~5k. You have to sort out all the nitty gritty details of each library, and
>> provide a common driver/API for the core code, differences between openssl
>> versions, the list goes on.
> 
> What is the specific functionality that you're actually relying on that you
> think would need 40K lines of code to replace, even using OpenSSL?  I see you
> are using KEYCTL_PKEY_*, but what specifically are you using them for?  What
> operations are being performed, and with which algorithms and key formats?
> Also, is the kernel behavior that you're relying on documented anywhere?  There
> are man pages for those keyctls, but they don't say anything about any
> particular hash algorithm, SHA-1 or otherwise, being supported.

<https://lore.kernel.org/all/CA+55aFxW7NMAMvYhkvz1UPbUTUJewRt6Yb51QAx5RtrWOwjebg@mail.gmail.com/>
"And we simply do not break user space."
-Linus Torvalds

Is this no longer applicable?

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Eric Biggers]

On Wed, Mar 13, 2024 at 03:51:10PM -0700, Jeff Johnson wrote:
> On 3/13/2024 3:10 PM, Eric Biggers wrote:
> > On Wed, Mar 13, 2024 at 02:17:29PM -0700, James Prestwood wrote:
> >> Hi,
> >>
> >> On 3/13/24 1:22 PM, Eric Biggers wrote:
> >>> On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote:
> >>>> Hi,
> >>>>
> >>>> On 3/13/24 12:44 PM, Eric Biggers wrote:
> >>>>> On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 3/13/24 1:56 AM, Johannes Berg wrote:
> >>>>>>> Not sure why you're CC'ing the world, but I guess adding a few more
> >>>>>>> doesn't hurt ...
> >>>>>>>
> >>>>>>> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
> >>>>>>>>     and I use iwd
> >>>>>>> This is your problem, the wireless stack in the kernel doesn't use any
> >>>>>>> kernel crypto code for 802.1X.
> >>>>>> Yes, the wireless stack has zero bearing on the issue. I think that's what
> >>>>>> you meant by "problem".
> >>>>>>
> >>>>>> IWD has used the kernel crypto API forever which was abruptly broken, that
> >>>>>> is the problem.
> >>>>>>
> >>>>>> The original commit says it was to remove support for sha1 signed kernel
> >>>>>> modules, but it did more than that and broke the keyctl API.
> >>>>>>
> >>>>> Which specific API is iwd using that is relevant here?
> >>>>> I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
> >>>>> and grepped for keyctl and AF_ALG, but there are no matches.
> >>>> IWD uses ELL for its crypto, which uses the AF_ALG API:
> >>>>
> >>>> https://git.kernel.org/pub/scm/libs/ell/ell.git/
> >>> Thanks for pointing out that the relevant code is really in that separate
> >>> repository.  Note, it seems that keyctl() is the problem here, not AF_ALG.  The
> >>> blamed commit didn't change anything for AF_ALG.
> >>>
> >>>> I believe the failure is when calling:
> >>>>
> >>>> KEYCTL_PKEY_QUERY enc="x962" hash="sha1"
> >>>>
> >>>>  From logs Michael posted on the IWD list, the ELL API that fails is:
> >>>>
> >>>> l_key_get_info (ell.git/ell/key.c:416)
> >>> Okay, I guess that's what's actually causing the problem.  KEYCTL_PKEY_* are a
> >>> weird set of APIs where userspace can ask the kernel to do asymmetric key
> >>> operations.  It's unclear why they exist, as the same functionality is available
> >>> in userspace crypto libraries.
> >>>
> >>> I suppose that the blamed commit, or at least part of it, will need to be
> >>> reverted to keep these weird keyctls working.
> >>>
> >>> For the future, why doesn't iwd just use a userspace crypto library such as
> >>> OpenSSL?
> >>
> >> I was not around when the original decision was made, but a few reasons I
> >> know we don't use openSSL:
> >>
> >>  - IWD has virtually zero dependencies.
> > 
> > Depending on something in the kernel does not eliminate a dependency; it just
> > adds that particular kernel UAPI to your list of dependencies.  The reason that
> > we're having this discussion in the first place is because iwd is depending on
> > an obscure kernel UAPI that is not well defined.  Historically it's been hard to
> > avoid "breaking" changes in these crypto-related UAPIs because of the poor
> > design where a huge number of algorithms are potentially supported, but the list
> > is undocumented and it varies from one system to another based on configuration.
> > Also due to their obscurity many kernel developers don't know that these UAPIs
> > even exist.  (The reaction when someone finds out is usually "Why!?")
> > 
> > It may be worth looking at if iwd should make a different choice for this
> > dependency.  It's understandable to blame dependencies when things go wrong, but
> > at the same time the choice of dependency is very much a choice, and some
> > choices can be more technically sound and cause fewer problems than others...
> > 
> >>  - OpenSSL + friends are rather large libraries.
> > 
> > The Linux kernel is also large, and it's made larger by having to support
> > obsolete crypto algorithms for backwards compatibility with iwd.
> > 
> >>  - AF_ALG has transparent hardware acceleration (not sure if openSSL does
> >> too).
> > 
> > OpenSSL takes advantage of CPU-based hardware acceleration, e.g. AES-NI.
> > 
> >> Another consideration is once you support openSSL someone wants wolfSSL,
> >> then boringSSL etc. Even if users implement support it just becomes a huge
> >> burden to carry for the project. Just look at wpa_supplicant's src/crypto/
> >> folder, nearly 40k LOC in there, compared to ELL's crypto modules which is
> >> ~5k. You have to sort out all the nitty gritty details of each library, and
> >> provide a common driver/API for the core code, differences between openssl
> >> versions, the list goes on.
> > 
> > What is the specific functionality that you're actually relying on that you
> > think would need 40K lines of code to replace, even using OpenSSL?  I see you
> > are using KEYCTL_PKEY_*, but what specifically are you using them for?  What
> > operations are being performed, and with which algorithms and key formats?
> > Also, is the kernel behavior that you're relying on documented anywhere?  There
> > are man pages for those keyctls, but they don't say anything about any
> > particular hash algorithm, SHA-1 or otherwise, being supported.
> 
> <https://lore.kernel.org/all/CA+55aFxW7NMAMvYhkvz1UPbUTUJewRt6Yb51QAx5RtrWOwjebg@mail.gmail.com/>
> "And we simply do not break user space."
> -Linus Torvalds
> 
> Is this no longer applicable?
> 

As I said, the commit, or at least the part of it that broke iwd (it's not clear
that it's the whole commit), needs to be reverted.

I just hope that, simultaneously, the iwd developers will consider improving the
design of iwd to avoid this type of recurring issue in the future.  After all,
this may be the only real chance for such a discussion before the next time iwd
breaks.

Also, part of the reason I'm asking about what functionality that iwd is relying
on is so that, if necessary, it can be properly documented and supported...

If we don't know what we are supporting, it is very hard to support it.

- Eric

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Eric Biggers]

On Wed, Mar 13, 2024 at 04:06:11PM -0700, Eric Biggers wrote:
> On Wed, Mar 13, 2024 at 03:51:10PM -0700, Jeff Johnson wrote:
> > On 3/13/2024 3:10 PM, Eric Biggers wrote:
> > > On Wed, Mar 13, 2024 at 02:17:29PM -0700, James Prestwood wrote:
> > >> Hi,
> > >>
> > >> On 3/13/24 1:22 PM, Eric Biggers wrote:
> > >>> On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote:
> > >>>> Hi,
> > >>>>
> > >>>> On 3/13/24 12:44 PM, Eric Biggers wrote:
> > >>>>> On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:
> > >>>>>> Hi,
> > >>>>>>
> > >>>>>> On 3/13/24 1:56 AM, Johannes Berg wrote:
> > >>>>>>> Not sure why you're CC'ing the world, but I guess adding a few more
> > >>>>>>> doesn't hurt ...
> > >>>>>>>
> > >>>>>>> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
> > >>>>>>>>     and I use iwd
> > >>>>>>> This is your problem, the wireless stack in the kernel doesn't use any
> > >>>>>>> kernel crypto code for 802.1X.
> > >>>>>> Yes, the wireless stack has zero bearing on the issue. I think that's what
> > >>>>>> you meant by "problem".
> > >>>>>>
> > >>>>>> IWD has used the kernel crypto API forever which was abruptly broken, that
> > >>>>>> is the problem.
> > >>>>>>
> > >>>>>> The original commit says it was to remove support for sha1 signed kernel
> > >>>>>> modules, but it did more than that and broke the keyctl API.
> > >>>>>>
> > >>>>> Which specific API is iwd using that is relevant here?
> > >>>>> I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
> > >>>>> and grepped for keyctl and AF_ALG, but there are no matches.
> > >>>> IWD uses ELL for its crypto, which uses the AF_ALG API:
> > >>>>
> > >>>> https://git.kernel.org/pub/scm/libs/ell/ell.git/
> > >>> Thanks for pointing out that the relevant code is really in that separate
> > >>> repository.  Note, it seems that keyctl() is the problem here, not AF_ALG.  The
> > >>> blamed commit didn't change anything for AF_ALG.
> > >>>
> > >>>> I believe the failure is when calling:
> > >>>>
> > >>>> KEYCTL_PKEY_QUERY enc="x962" hash="sha1"
> > >>>>
> > >>>>  From logs Michael posted on the IWD list, the ELL API that fails is:
> > >>>>
> > >>>> l_key_get_info (ell.git/ell/key.c:416)
> > >>> Okay, I guess that's what's actually causing the problem.  KEYCTL_PKEY_* are a
> > >>> weird set of APIs where userspace can ask the kernel to do asymmetric key
> > >>> operations.  It's unclear why they exist, as the same functionality is available
> > >>> in userspace crypto libraries.
> > >>>
> > >>> I suppose that the blamed commit, or at least part of it, will need to be
> > >>> reverted to keep these weird keyctls working.
> > >>>
> > >>> For the future, why doesn't iwd just use a userspace crypto library such as
> > >>> OpenSSL?
> > >>
> > >> I was not around when the original decision was made, but a few reasons I
> > >> know we don't use openSSL:
> > >>
> > >>  - IWD has virtually zero dependencies.
> > > 
> > > Depending on something in the kernel does not eliminate a dependency; it just
> > > adds that particular kernel UAPI to your list of dependencies.  The reason that
> > > we're having this discussion in the first place is because iwd is depending on
> > > an obscure kernel UAPI that is not well defined.  Historically it's been hard to
> > > avoid "breaking" changes in these crypto-related UAPIs because of the poor
> > > design where a huge number of algorithms are potentially supported, but the list
> > > is undocumented and it varies from one system to another based on configuration.
> > > Also due to their obscurity many kernel developers don't know that these UAPIs
> > > even exist.  (The reaction when someone finds out is usually "Why!?")
> > > 
> > > It may be worth looking at if iwd should make a different choice for this
> > > dependency.  It's understandable to blame dependencies when things go wrong, but
> > > at the same time the choice of dependency is very much a choice, and some
> > > choices can be more technically sound and cause fewer problems than others...
> > > 
> > >>  - OpenSSL + friends are rather large libraries.
> > > 
> > > The Linux kernel is also large, and it's made larger by having to support
> > > obsolete crypto algorithms for backwards compatibility with iwd.
> > > 
> > >>  - AF_ALG has transparent hardware acceleration (not sure if openSSL does
> > >> too).
> > > 
> > > OpenSSL takes advantage of CPU-based hardware acceleration, e.g. AES-NI.
> > > 
> > >> Another consideration is once you support openSSL someone wants wolfSSL,
> > >> then boringSSL etc. Even if users implement support it just becomes a huge
> > >> burden to carry for the project. Just look at wpa_supplicant's src/crypto/
> > >> folder, nearly 40k LOC in there, compared to ELL's crypto modules which is
> > >> ~5k. You have to sort out all the nitty gritty details of each library, and
> > >> provide a common driver/API for the core code, differences between openssl
> > >> versions, the list goes on.
> > > 
> > > What is the specific functionality that you're actually relying on that you
> > > think would need 40K lines of code to replace, even using OpenSSL?  I see you
> > > are using KEYCTL_PKEY_*, but what specifically are you using them for?  What
> > > operations are being performed, and with which algorithms and key formats?
> > > Also, is the kernel behavior that you're relying on documented anywhere?  There
> > > are man pages for those keyctls, but they don't say anything about any
> > > particular hash algorithm, SHA-1 or otherwise, being supported.
> > 
> > <https://lore.kernel.org/all/CA+55aFxW7NMAMvYhkvz1UPbUTUJewRt6Yb51QAx5RtrWOwjebg@mail.gmail.com/>
> > "And we simply do not break user space."
> > -Linus Torvalds
> > 
> > Is this no longer applicable?
> > 
> 
> As I said, the commit, or at least the part of it that broke iwd (it's not clear
> that it's the whole commit), needs to be reverted.
> 
> I just hope that, simultaneously, the iwd developers will consider improving the
> design of iwd to avoid this type of recurring issue in the future.  After all,
> this may be the only real chance for such a discussion before the next time iwd
> breaks.
> 
> Also, part of the reason I'm asking about what functionality that iwd is relying
> on is so that, if necessary, it can be properly documented and supported...
> 
> If we don't know what we are supporting, it is very hard to support it.

I ended up just sending out a patch that does a full revert:
https://lore.kernel.org/linux-crypto/20240313233227.56391-1-ebiggers@kernel.org

Again, regardless of that, these issues with iwd have been recurring, and it is
still worth discussing the best way from a technical perspective to prevent them
from happening in the future...  There's a fairly clear path to achieve that.

- Eric

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[James Prestwood]

Hi,

On 3/13/24 4:06 PM, Eric Biggers wrote:
> On Wed, Mar 13, 2024 at 03:51:10PM -0700, Jeff Johnson wrote:
>> On 3/13/2024 3:10 PM, Eric Biggers wrote:
>>> On Wed, Mar 13, 2024 at 02:17:29PM -0700, James Prestwood wrote:
>>>> Hi,
>>>>
>>>> On 3/13/24 1:22 PM, Eric Biggers wrote:
>>>>> On Wed, Mar 13, 2024 at 01:12:54PM -0700, James Prestwood wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 3/13/24 12:44 PM, Eric Biggers wrote:
>>>>>>> On Wed, Mar 13, 2024 at 10:26:06AM -0700, James Prestwood wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 3/13/24 1:56 AM, Johannes Berg wrote:
>>>>>>>>> Not sure why you're CC'ing the world, but I guess adding a few more
>>>>>>>>> doesn't hurt ...
>>>>>>>>>
>>>>>>>>> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
>>>>>>>>>>      and I use iwd
>>>>>>>>> This is your problem, the wireless stack in the kernel doesn't use any
>>>>>>>>> kernel crypto code for 802.1X.
>>>>>>>> Yes, the wireless stack has zero bearing on the issue. I think that's what
>>>>>>>> you meant by "problem".
>>>>>>>>
>>>>>>>> IWD has used the kernel crypto API forever which was abruptly broken, that
>>>>>>>> is the problem.
>>>>>>>>
>>>>>>>> The original commit says it was to remove support for sha1 signed kernel
>>>>>>>> modules, but it did more than that and broke the keyctl API.
>>>>>>>>
>>>>>>> Which specific API is iwd using that is relevant here?
>>>>>>> I cloned https://kernel.googlesource.com/pub/scm/network/wireless/iwd
>>>>>>> and grepped for keyctl and AF_ALG, but there are no matches.
>>>>>> IWD uses ELL for its crypto, which uses the AF_ALG API:
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/libs/ell/ell.git/
>>>>> Thanks for pointing out that the relevant code is really in that separate
>>>>> repository.  Note, it seems that keyctl() is the problem here, not AF_ALG.  The
>>>>> blamed commit didn't change anything for AF_ALG.
>>>>>
>>>>>> I believe the failure is when calling:
>>>>>>
>>>>>> KEYCTL_PKEY_QUERY enc="x962" hash="sha1"
>>>>>>
>>>>>>   From logs Michael posted on the IWD list, the ELL API that fails is:
>>>>>>
>>>>>> l_key_get_info (ell.git/ell/key.c:416)
>>>>> Okay, I guess that's what's actually causing the problem.  KEYCTL_PKEY_* are a
>>>>> weird set of APIs where userspace can ask the kernel to do asymmetric key
>>>>> operations.  It's unclear why they exist, as the same functionality is available
>>>>> in userspace crypto libraries.
>>>>>
>>>>> I suppose that the blamed commit, or at least part of it, will need to be
>>>>> reverted to keep these weird keyctls working.
>>>>>
>>>>> For the future, why doesn't iwd just use a userspace crypto library such as
>>>>> OpenSSL?
>>>> I was not around when the original decision was made, but a few reasons I
>>>> know we don't use openSSL:
>>>>
>>>>   - IWD has virtually zero dependencies.
>>> Depending on something in the kernel does not eliminate a dependency; it just
>>> adds that particular kernel UAPI to your list of dependencies.  The reason that
>>> we're having this discussion in the first place is because iwd is depending on
>>> an obscure kernel UAPI that is not well defined.  Historically it's been hard to
>>> avoid "breaking" changes in these crypto-related UAPIs because of the poor
>>> design where a huge number of algorithms are potentially supported, but the list
>>> is undocumented and it varies from one system to another based on configuration.
>>> Also due to their obscurity many kernel developers don't know that these UAPIs
>>> even exist.  (The reaction when someone finds out is usually "Why!?")
>>>
>>> It may be worth looking at if iwd should make a different choice for this
>>> dependency.  It's understandable to blame dependencies when things go wrong, but
>>> at the same time the choice of dependency is very much a choice, and some
>>> choices can be more technically sound and cause fewer problems than others...
>>>
>>>>   - OpenSSL + friends are rather large libraries.
>>> The Linux kernel is also large, and it's made larger by having to support
>>> obsolete crypto algorithms for backwards compatibility with iwd.
>>>
>>>>   - AF_ALG has transparent hardware acceleration (not sure if openSSL does
>>>> too).
>>> OpenSSL takes advantage of CPU-based hardware acceleration, e.g. AES-NI.
>>>
>>>> Another consideration is once you support openSSL someone wants wolfSSL,
>>>> then boringSSL etc. Even if users implement support it just becomes a huge
>>>> burden to carry for the project. Just look at wpa_supplicant's src/crypto/
>>>> folder, nearly 40k LOC in there, compared to ELL's crypto modules which is
>>>> ~5k. You have to sort out all the nitty gritty details of each library, and
>>>> provide a common driver/API for the core code, differences between openssl
>>>> versions, the list goes on.
>>> What is the specific functionality that you're actually relying on that you
>>> think would need 40K lines of code to replace, even using OpenSSL?  I see you
>>> are using KEYCTL_PKEY_*, but what specifically are you using them for?  What
>>> operations are being performed, and with which algorithms and key formats?
>>> Also, is the kernel behavior that you're relying on documented anywhere?  There
>>> are man pages for those keyctls, but they don't say anything about any
>>> particular hash algorithm, SHA-1 or otherwise, being supported.
>> <https://lore.kernel.org/all/CA+55aFxW7NMAMvYhkvz1UPbUTUJewRt6Yb51QAx5RtrWOwjebg@mail.gmail.com/>
>> "And we simply do not break user space."
>> -Linus Torvalds
>>
>> Is this no longer applicable?
>>
> As I said, the commit, or at least the part of it that broke iwd (it's not clear
> that it's the whole commit), needs to be reverted.
>
> I just hope that, simultaneously, the iwd developers will consider improving the
> design of iwd to avoid this type of recurring issue in the future.  After all,
> this may be the only real chance for such a discussion before the next time iwd
> breaks.
>
> Also, part of the reason I'm asking about what functionality that iwd is relying
> on is so that, if necessary, it can be properly documented and supported...
>
> If we don't know what we are supporting, it is very hard to support it.

IWD uses AF_ALG/keyctl for _all_ its crypto, cipher, and checksum needs. 
Anything that wifi requires as far as crypto goes IWD uses the kernel, 
except ECC is the only exception. The entire list of crypto requirements 
(for full support at least) for IWD is here:

https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/tools/test_runner_kernel_config

For KEYCTL_PKEY_* specifically we use it for all asymmetric crypto 
operations, (query), encrypt, decrypt, sign, verify.

I'll be honest, the AF_ALG/keyctl support in ELL was mostly done by the 
time I started working on IWD so I was not aware the documentation was 
so poor. That is an entirely separate issue than this IMO, and I'm happy 
to help with getting docs updated to include a proper list of supported 
features. In addition maybe some automated testing that gets run on 
kernel builds which actually exercises this API so it doesn't get 
accidentally get broken in the future? Docs/tests IMO are the proper 
"fix" here, not telling someone to stop using an API that has existed a 
long time.

I'm also not entirely sure why this stuff continues to be removed from 
the kernel. First MD4, then it got reverted, then this (now reverted, 
thanks). Both cases there was not clear justification of why it was 
being removed.

Thanks,

James

>
> - Eric
>

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[James Bottomley]

On Thu, 2024-03-14 at 04:52 -0700, James Prestwood wrote:
> I'm also not entirely sure why this stuff continues to be removed
> from the kernel. First MD4, then it got reverted, then this (now
> reverted, thanks). Both cases there was not clear justification of
> why it was  being removed.

I think this is some misunderstanding of the NIST and FIPS requirements
with regards to hashes, ciphers and bits of security.  The bottom line
is that neither NIST nor FIPS requires the removal of the sha1
algorithm at all.  Both of them still support it for HMAC (for now). 
In addition, the FIPS requirement is only that you not *issue* sha1
hashed signatures.  FIPS still allows you to verify legacy signatures
with sha1 as the signing hash (for backwards compatibility reasons). 
Enterprises with no legacy and no HMAC requirements *may* remove the
hash, but it's not mandated.

So *removing* sha1 from the certificate code was the wrong thing to do.
We should have configurably prevented using sha1 as the algorithm for
new signatures but kept it for signature verification.

Can we please get this sorted out before 2025, because next up is the
FIPS requirement to move to at least 128 bits of security which will
see RSA2048 deprecated in a similar way: We should refuse to issue
RSA2048 signatures, but will still be allowed to verify them for legacy
reasons.

James

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Eric Biggers]

On Thu, Mar 14, 2024 at 04:52:47AM -0700, James Prestwood wrote:
> IWD uses AF_ALG/keyctl for _all_ its crypto, cipher, and checksum needs.
> Anything that wifi requires as far as crypto goes IWD uses the kernel,
> except ECC is the only exception. The entire list of crypto requirements
> (for full support at least) for IWD is here:
> 
> https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/tools/test_runner_kernel_config

That's quite an extensive list, and it's not documented in the iwd README.
Don't you get bug reports from users who are running a kernel that's missing one
of those options?

> For KEYCTL_PKEY_* specifically we use it for all asymmetric crypto
> operations, (query), encrypt, decrypt, sign, verify.
> 
> I'll be honest, the AF_ALG/keyctl support in ELL was mostly done by the time
> I started working on IWD so I was not aware the documentation was so poor.
> That is an entirely separate issue than this IMO, and I'm happy to help with
> getting docs updated to include a proper list of supported features. In
> addition maybe some automated testing that gets run on kernel builds which
> actually exercises this API so it doesn't get accidentally get broken in the
> future? Docs/tests IMO are the proper "fix" here, not telling someone to
> stop using an API that has existed a long time.

I looked into the history, and it seems the KEYCTL_PKEY_* APIs were added as a
collaboration between the iwd developers and the kernel keyrings maintainer.
So, as far as I can tell, it's not that the kernel had an existing API that iwd
started using.  It's that iwd got some APIs added to the kernel for themselves.
KEYCTL_PKEY_* don't seem to have been adopted elsewhere; Debian Code Search
doesn't return any notable results.  keyctl does provide a command-line
interface to them, but I can't find any users of the keyctl commands either.

Then, everyone disappeared and it got dumped on the next generation of kernel
developers, who often don't know that this API even exists.  And since the API
is also poorly specified and difficult to maintain (e.g., changing a seemingly
unrelated part of the kernel can break it), the results are predictable...  And
of course the only thing that breaks is iwd, since it's the only user.

It would be worth taking a step back and looking at the overall system
architecture here.  Is this the best way to ensure a reliable wireless
experience for Linux users?

Maybe it's time to admit that KEYCTL_PKEY_* was basically an experiment, and a
different direction (e.g. using OpenSSL) should be taken...

(Another issue with the kernel keyrings stuff is that provides a significant
attack surface for the kernel to be exploited.)

If you do decide to continue with the status quo, it may be necessary for the
iwd developers to take a more active role in maintaining this API in order to
ensure it continues working properly for you.

AF_ALG is on *slightly* firmer ground since it's been around for longer, is
properly part of the crypto subsystem, and has a few other users.  Unfortunately
it still suffers from the same issues though, just to a slightly lesser degree.

> I'm also not entirely sure why this stuff continues to be removed from the
> kernel. First MD4, then it got reverted, then this (now reverted, thanks).
> Both cases there was not clear justification of why it was being removed.

These algorithms are insecure, and it's likely that the author of these commits
thought that there were no remaining users and nothing would break.  Removing
them is a worthy goal for code maintenance purposes and to avoid providing
insecure options that could accidentally be used.  The AF_ALG and KEYCTL_PKEY_*
APIs are very easy to overlook and I suspect that the author of these commits
did not know about them.  These APIs are rarely used, not well specified, the
availability of them and specific algorithms varies by kernel configuration, and
userspace only uses a subset of the algorithms in the kernel's museum of crypto
primitives anyway.  So it's plausible that there are algorithms that no one is
using or that at least there is a fallback for, so can be safely removed...

- Eric

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Ard Biesheuvel]

On Thu, 14 Mar 2024 at 21:20, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Thu, Mar 14, 2024 at 04:52:47AM -0700, James Prestwood wrote:
> > IWD uses AF_ALG/keyctl for _all_ its crypto, cipher, and checksum needs.
> > Anything that wifi requires as far as crypto goes IWD uses the kernel,
> > except ECC is the only exception. The entire list of crypto requirements
> > (for full support at least) for IWD is here:
> >
> > https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/tools/test_runner_kernel_config
>
> That's quite an extensive list, and it's not documented in the iwd README.
> Don't you get bug reports from users who are running a kernel that's missing one
> of those options?
>
> > For KEYCTL_PKEY_* specifically we use it for all asymmetric crypto
> > operations, (query), encrypt, decrypt, sign, verify.
> >
> > I'll be honest, the AF_ALG/keyctl support in ELL was mostly done by the time
> > I started working on IWD so I was not aware the documentation was so poor.
> > That is an entirely separate issue than this IMO, and I'm happy to help with
> > getting docs updated to include a proper list of supported features. In
> > addition maybe some automated testing that gets run on kernel builds which
> > actually exercises this API so it doesn't get accidentally get broken in the
> > future? Docs/tests IMO are the proper "fix" here, not telling someone to
> > stop using an API that has existed a long time.
>
> I looked into the history, and it seems the KEYCTL_PKEY_* APIs were added as a
> collaboration between the iwd developers and the kernel keyrings maintainer.
> So, as far as I can tell, it's not that the kernel had an existing API that iwd
> started using.  It's that iwd got some APIs added to the kernel for themselves.
> KEYCTL_PKEY_* don't seem to have been adopted elsewhere; Debian Code Search
> doesn't return any notable results.  keyctl does provide a command-line
> interface to them, but I can't find any users of the keyctl commands either.
>
> Then, everyone disappeared and it got dumped on the next generation of kernel
> developers, who often don't know that this API even exists.  And since the API
> is also poorly specified and difficult to maintain (e.g., changing a seemingly
> unrelated part of the kernel can break it), the results are predictable...  And
> of course the only thing that breaks is iwd, since it's the only user.
>
> It would be worth taking a step back and looking at the overall system
> architecture here.  Is this the best way to ensure a reliable wireless
> experience for Linux users?
>
> Maybe it's time to admit that KEYCTL_PKEY_* was basically an experiment, and a
> different direction (e.g. using OpenSSL) should be taken...
>
> (Another issue with the kernel keyrings stuff is that provides a significant
> attack surface for the kernel to be exploited.)
>
> If you do decide to continue with the status quo, it may be necessary for the
> iwd developers to take a more active role in maintaining this API in order to
> ensure it continues working properly for you.
>
> AF_ALG is on *slightly* firmer ground since it's been around for longer, is
> properly part of the crypto subsystem, and has a few other users.  Unfortunately
> it still suffers from the same issues though, just to a slightly lesser degree.
>

We dropped MD4 because there are no users in the kernel. It is not the
kernel's job to run code on behalf of user space if it does not
require any privileges and can therefore execute in user space
directly.

The fact that AF_ALG permits this is a huge oversight on the part of
the kernel community, and a major maintenance burden. The point of
AF_ALG was to expose hardware crypto accelerators (which are shared
resources that /need/ to be managed by the kernel) to user space, and
we inadvertently ended up allowing the kernel's pure-software
algorithms to be used in the same way.

The fact that we even added APIs to the kernel to accommodate iwd is
even worse. It means system call overhead (which has become worse due
to all the speculation mitigations) to execute some code that could
execute in user space just as well, which is a bad idea for other
reasons too (for instance, accelerated crypto that uses SIMD in the
kernel disables preemption on many architectures, resulting in
scheduling jitter)

Note that in the case of iwd, it is unlikely that the use of AF_ALG
could ever result in meaningful use of hardware accelerators: today's
wireless interfaces don't use software crypto for the bulk of the data
(i.e., the packets themselves) and the wireless key exchange protocols
etc are unlikely to be supported in generic crypto accelerators, and
even if they were, the latency would likely result in worse
performance overall than a software implementation.

So iwd's deliberate choice to use the kernel as a crypto library is
severely misguided. I have made the same point 4 years ago when I
replaced iwd's use of the kernel's ecb(arc4) code with a suitable
software implementation (3 files changed, 53 insertions, 40
deletions). Of course, replacing other algorithms will take more work
than that, but it is the only sensible approach. We all know the cat
is out of the bag when it comes to AF_ALG, and we simply have to
retain all those broken algorithms as executable code at the kernel's
privileged execution level, just in case some user space is still
around that relies on it. But that doesn't mean we cannot be very
clear about our preferred way forward.

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Michael Yartys]

Hi

This came in via the iwd mailing list, and I would like to add some small a detail as I also experience this issue on my university eduroam network. I've verified that the certificate chain doesn't contain SHA-1 signed certificates, so the update breaks more than just SHA-1.

Michael




On Wednesday, March 13th, 2024 at 09:56, Johannes Berg <johannes@sipsolutions.net> wrote:

> 
> 
> Not sure why you're CC'ing the world, but I guess adding a few more
> doesn't hurt ...
> 
> On Wed, 2024-03-13 at 09:50 +0100, Karel Balej wrote:
> 
> > and I use iwd
> 
> 
> This is your problem, the wireless stack in the kernel doesn't use any
> kernel crypto code for 802.1X.
> 
> I suppose iwd wants to use the kernel infrastructure but has no
> fallbacks to other implementations.
> 
> johannes

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Karel Balej]

Thank you all for your feedback so far.

Since it seems that this really is a regression on the kernel side, let
me add the appropriate list to Cc and tag this:

#regzbot introduced: 16ab7cb5825f

Best regards,
K. B.

Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

[Karel Balej]

#regzbot title: SHA1 support removal breaks iwd's ability to connect to eduroam
#regzbot monitor: https://lore.kernel.org/all/20240313233227.56391-1-ebiggers@kernel.org/
#regzbot monitor: https://lore.kernel.org/all/CZSHRUIJ4RKL.34T4EASV5DNJM@matfyz.cz/
#regzbot link: https://lore.kernel.org/iwd/njvxKaPo_CBxsQGaNSRHj8xOSxzk1_j_K-minIe4GCKUMB1qxJT8nPk9SGmfqg7Aepm_5dO7FEofYIYP1g15R9V5dJ0F8bN6O4VthSjzu1g=@yartys.no/

Sorry for the tracking mess...