Message-ID: <20240314202011.GB1132@sol.localdomain>
Date: Thu, 14 Mar 2024 13:20:11 -0700
From: Eric Biggers <ebiggers@...nel.org>
To: James Prestwood <prestwoj@...il.com>
Cc: Jeff Johnson <quic_jjohnson@...cinc.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 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

Powered by blists - more mailing lists