Date: Thu, 14 Mar 2024 04:52:47 -0700
From: James Prestwood <prestwoj@...il.com>
To: Eric Biggers <ebiggers@...nel.org>,
 Jeff Johnson <quic_jjohnson@...cinc.com>
Cc: Johannes Berg <johannes@...solutions.net>, Karel Balej
 <balejk@...fyz.cz>, dimitri.ledkov@...onical.com,
 alexandre.torgue@...s.st.com, davem@...emloft.net, dhowells@...hat.com,
 herbert@...dor.apana.org.au, keyrings@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-crypto@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-modules@...r.kernel.org,
 linux-stm32@...md-mailman.stormreply.com, mcgrof@...nel.org,
 mcoquelin.stm32@...il.com, linux-wireless@...r.kernel.org,
 netdev@...r.kernel.org, iwd@...ts.linux.dev
Subject: Re: [REGRESSION] Re: [PATCH] crypto: pkcs7: remove sha1 support

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
>

Powered by blists - more mailing lists