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

[Jeff Johnson <quic_jjohnson@quicinc.com>]

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?