Message-ID: <20240313234014.GE1111@sol.localdomain>
Date: Wed, 13 Mar 2024 16:40:14 -0700
From: Eric Biggers <ebiggers@...nel.org>
To: Jeff Johnson <quic_jjohnson@...cinc.com>
Cc: James Prestwood <prestwoj@...il.com>,
	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

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

Powered by blists - more mailing lists