| Message ID | 20231010212240.61637-1-dimitri.ledkov@canonical.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | crypto: pkcs7: remove sha1 support | expand |
On Tue, Oct 10, 2023 at 10:22:38PM +0100, Dimitri John Ledkov wrote: > Removes support for sha1 signed kernel modules, importing sha1 signed > x.509 certificates. > > rsa-pkcs1pad keeps sha1 padding support, which seems to be used by > virtio driver. > > sha1 remains available as there are many drivers and subsystems using > it. Note only hmac(sha1) with secret keys remains cryptographically > secure. > > In the kernel there are filesystems, IMA, tpm/pcr that appear to be > using sha1. Maybe they can all start to be slowly upgraded to > something else i.e. blake3, ParallelHash, SHAKE256 as needed. > > Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@canonical.com> > --- > crypto/asymmetric_keys/mscode_parser.c | 3 - > crypto/asymmetric_keys/pkcs7_parser.c | 4 -- > crypto/asymmetric_keys/public_key.c | 3 +- > crypto/asymmetric_keys/signature.c | 2 +- > crypto/asymmetric_keys/x509_cert_parser.c | 8 --- > crypto/testmgr.h | 80 ----------------------- > include/linux/oid_registry.h | 4 -- > kernel/module/Kconfig | 5 -- > 8 files changed, 2 insertions(+), 107 deletions(-) Patch applied. Thanks.
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.
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
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 >
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
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
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.
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
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
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
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
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?
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
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
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 >
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
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
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.
#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...
diff --git a/crypto/asymmetric_keys/mscode_parser.c b/crypto/asymmetric_keys/mscode_parser.c index 690405ebe7..6416bded0e 100644 --- a/crypto/asymmetric_keys/mscode_parser.c +++ b/crypto/asymmetric_keys/mscode_parser.c @@ -75,9 +75,6 @@ int mscode_note_digest_algo(void *context, size_t hdrlen, oid = look_up_OID(value, vlen); switch (oid) { - case OID_sha1: - ctx->digest_algo = "sha1"; - break; case OID_sha256: ctx->digest_algo = "sha256"; break; diff --git a/crypto/asymmetric_keys/pkcs7_parser.c b/crypto/asymmetric_keys/pkcs7_parser.c index cf4caab962..ab647cb4d7 100644 --- a/crypto/asymmetric_keys/pkcs7_parser.c +++ b/crypto/asymmetric_keys/pkcs7_parser.c @@ -227,9 +227,6 @@ int pkcs7_sig_note_digest_algo(void *context, size_t hdrlen, struct pkcs7_parse_context *ctx = context; switch (ctx->last_oid) { - case OID_sha1: - ctx->sinfo->sig->hash_algo = "sha1"; - break; case OID_sha256: ctx->sinfo->sig->hash_algo = "sha256"; break; @@ -272,7 +269,6 @@ int pkcs7_sig_note_pkey_algo(void *context, size_t hdrlen, ctx->sinfo->sig->pkey_algo = "rsa"; ctx->sinfo->sig->encoding = "pkcs1"; break; - case OID_id_ecdsa_with_sha1: case OID_id_ecdsa_with_sha224: case OID_id_ecdsa_with_sha256: case OID_id_ecdsa_with_sha384: diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c index abeecb8329..5bf0452c17 100644 --- a/crypto/asymmetric_keys/public_key.c +++ b/crypto/asymmetric_keys/public_key.c @@ -116,8 +116,7 @@ software_key_determine_akcipher(const struct public_key *pkey, */ if (!hash_algo) return -EINVAL; - if (strcmp(hash_algo, "sha1") != 0 && - strcmp(hash_algo, "sha224") != 0 && + if (strcmp(hash_algo, "sha224") != 0 && strcmp(hash_algo, "sha256") != 0 && strcmp(hash_algo, "sha384") != 0 && strcmp(hash_algo, "sha512") != 0) diff --git a/crypto/asymmetric_keys/signature.c b/crypto/asymmetric_keys/signature.c index 2deff81f8a..398983be77 100644 --- a/crypto/asymmetric_keys/signature.c +++ b/crypto/asymmetric_keys/signature.c @@ -115,7 +115,7 @@ EXPORT_SYMBOL_GPL(decrypt_blob); * Sign the specified data blob using the private key specified by params->key. * The signature is wrapped in an encoding if params->encoding is specified * (eg. "pkcs1"). If the encoding needs to know the digest type, this can be - * passed through params->hash_algo (eg. "sha1"). + * passed through params->hash_algo (eg. "sha512"). * * Returns the length of the data placed in the signature buffer or an error. */ diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c index 2c30928621..68ef1ffbbe 100644 --- a/crypto/asymmetric_keys/x509_cert_parser.c +++ b/crypto/asymmetric_keys/x509_cert_parser.c @@ -198,10 +198,6 @@ int x509_note_sig_algo(void *context, size_t hdrlen, unsigned char tag, default: return -ENOPKG; /* Unsupported combination */ - case OID_sha1WithRSAEncryption: - ctx->cert->sig->hash_algo = "sha1"; - goto rsa_pkcs1; - case OID_sha256WithRSAEncryption: ctx->cert->sig->hash_algo = "sha256"; goto rsa_pkcs1; @@ -218,10 +214,6 @@ int x509_note_sig_algo(void *context, size_t hdrlen, unsigned char tag, ctx->cert->sig->hash_algo = "sha224"; goto rsa_pkcs1; - case OID_id_ecdsa_with_sha1: - ctx->cert->sig->hash_algo = "sha1"; - goto ecdsa; - case OID_id_ecdsa_with_sha224: ctx->cert->sig->hash_algo = "sha224"; goto ecdsa; diff --git a/crypto/testmgr.h b/crypto/testmgr.h index 3cfe91e2d1..98f83c32e0 100644 --- a/crypto/testmgr.h +++ b/crypto/testmgr.h @@ -653,30 +653,6 @@ static const struct akcipher_testvec rsa_tv_template[] = { static const struct akcipher_testvec ecdsa_nist_p192_tv_template[] = { { .key = - "\x04\xf7\x46\xf8\x2f\x15\xf6\x22\x8e\xd7\x57\x4f\xcc\xe7\xbb\xc1" - "\xd4\x09\x73\xcf\xea\xd0\x15\x07\x3d\xa5\x8a\x8a\x95\x43\xe4\x68" - "\xea\xc6\x25\xc1\xc1\x01\x25\x4c\x7e\xc3\x3c\xa6\x04\x0a\xe7\x08" - "\x98", - .key_len = 49, - .params = - "\x30\x13\x06\x07\x2a\x86\x48\xce\x3d\x02\x01\x06\x08\x2a\x86\x48" - "\xce\x3d\x03\x01\x01", - .param_len = 21, - .m = - "\xcd\xb9\xd2\x1c\xb7\x6f\xcd\x44\xb3\xfd\x63\xea\xa3\x66\x7f\xae" - "\x63\x85\xe7\x82", - .m_size = 20, - .algo = OID_id_ecdsa_with_sha1, - .c = - "\x30\x35\x02\x19\x00\xba\xe5\x93\x83\x6e\xb6\x3b\x63\xa0\x27\x91" - "\xc6\xf6\x7f\xc3\x09\xad\x59\xad\x88\x27\xd6\x92\x6b\x02\x18\x10" - "\x68\x01\x9d\xba\xce\x83\x08\xef\x95\x52\x7b\xa0\x0f\xe4\x18\x86" - "\x80\x6f\xa5\x79\x77\xda\xd0", - .c_size = 55, - .public_key_vec = true, - .siggen_sigver_test = true, - }, { - .key = "\x04\xb6\x4b\xb1\xd1\xac\xba\x24\x8f\x65\xb2\x60\x00\x90\xbf\xbd" "\x78\x05\x73\xe9\x79\x1d\x6f\x7c\x0b\xd2\xc3\x93\xa7\x28\xe1\x75" "\xf7\xd5\x95\x1d\x28\x10\xc0\x75\x50\x5c\x1a\x4f\x3f\x8f\xa5\xee" @@ -780,32 +756,6 @@ static const struct akcipher_testvec ecdsa_nist_p192_tv_template[] = { static const struct akcipher_testvec ecdsa_nist_p256_tv_template[] = { { .key = - "\x04\xb9\x7b\xbb\xd7\x17\x64\xd2\x7e\xfc\x81\x5d\x87\x06\x83\x41" - "\x22\xd6\x9a\xaa\x87\x17\xec\x4f\x63\x55\x2f\x94\xba\xdd\x83\xe9" - "\x34\x4b\xf3\xe9\x91\x13\x50\xb6\xcb\xca\x62\x08\xe7\x3b\x09\xdc" - "\xc3\x63\x4b\x2d\xb9\x73\x53\xe4\x45\xe6\x7c\xad\xe7\x6b\xb0\xe8" - "\xaf", - .key_len = 65, - .params = - "\x30\x13\x06\x07\x2a\x86\x48\xce\x3d\x02\x01\x06\x08\x2a\x86\x48" - "\xce\x3d\x03\x01\x07", - .param_len = 21, - .m = - "\xc2\x2b\x5f\x91\x78\x34\x26\x09\x42\x8d\x6f\x51\xb2\xc5\xaf\x4c" - "\x0b\xde\x6a\x42", - .m_size = 20, - .algo = OID_id_ecdsa_with_sha1, - .c = - "\x30\x46\x02\x21\x00\xf9\x25\xce\x9f\x3a\xa6\x35\x81\xcf\xd4\xe7" - "\xb7\xf0\x82\x56\x41\xf7\xd4\xad\x8d\x94\x5a\x69\x89\xee\xca\x6a" - "\x52\x0e\x48\x4d\xcc\x02\x21\x00\xd7\xe4\xef\x52\x66\xd3\x5b\x9d" - "\x8a\xfa\x54\x93\x29\xa7\x70\x86\xf1\x03\x03\xf3\x3b\xe2\x73\xf7" - "\xfb\x9d\x8b\xde\xd4\x8d\x6f\xad", - .c_size = 72, - .public_key_vec = true, - .siggen_sigver_test = true, - }, { - .key = "\x04\x8b\x6d\xc0\x33\x8e\x2d\x8b\x67\xf5\xeb\xc4\x7f\xa0\xf5\xd9" "\x7b\x03\xa5\x78\x9a\xb5\xea\x14\xe4\x23\xd0\xaf\xd7\x0e\x2e\xa0" "\xc9\x8b\xdb\x95\xf8\xb3\xaf\xac\x00\x2c\x2c\x1f\x7a\xfd\x95\x88" @@ -916,36 +866,6 @@ static const struct akcipher_testvec ecdsa_nist_p256_tv_template[] = { static const struct akcipher_testvec ecdsa_nist_p384_tv_template[] = { { - .key = /* secp384r1(sha1) */ - "\x04\x89\x25\xf3\x97\x88\xcb\xb0\x78\xc5\x72\x9a\x14\x6e\x7a\xb1" - "\x5a\xa5\x24\xf1\x95\x06\x9e\x28\xfb\xc4\xb9\xbe\x5a\x0d\xd9\x9f" - "\xf3\xd1\x4d\x2d\x07\x99\xbd\xda\xa7\x66\xec\xbb\xea\xba\x79\x42" - "\xc9\x34\x89\x6a\xe7\x0b\xc3\xf2\xfe\x32\x30\xbe\xba\xf9\xdf\x7e" - "\x4b\x6a\x07\x8e\x26\x66\x3f\x1d\xec\xa2\x57\x91\x51\xdd\x17\x0e" - "\x0b\x25\xd6\x80\x5c\x3b\xe6\x1a\x98\x48\x91\x45\x7a\x73\xb0\xc3" - "\xf1", - .key_len = 97, - .params = - "\x30\x10\x06\x07\x2a\x86\x48\xce\x3d\x02\x01\x06\x05\x2b\x81\x04" - "\x00\x22", - .param_len = 18, - .m = - "\x12\x55\x28\xf0\x77\xd5\xb6\x21\x71\x32\x48\xcd\x28\xa8\x25\x22" - "\x3a\x69\xc1\x93", - .m_size = 20, - .algo = OID_id_ecdsa_with_sha1, - .c = - "\x30\x66\x02\x31\x00\xf5\x0f\x24\x4c\x07\x93\x6f\x21\x57\x55\x07" - "\x20\x43\x30\xde\xa0\x8d\x26\x8e\xae\x63\x3f\xbc\x20\x3a\xc6\xf1" - "\x32\x3c\xce\x70\x2b\x78\xf1\x4c\x26\xe6\x5b\x86\xcf\xec\x7c\x7e" - "\xd0\x87\xd7\xd7\x6e\x02\x31\x00\xcd\xbb\x7e\x81\x5d\x8f\x63\xc0" - "\x5f\x63\xb1\xbe\x5e\x4c\x0e\xa1\xdf\x28\x8c\x1b\xfa\xf9\x95\x88" - "\x74\xa0\x0f\xbf\xaf\xc3\x36\x76\x4a\xa1\x59\xf1\x1c\xa4\x58\x26" - "\x79\x12\x2a\xb7\xc5\x15\x92\xc5", - .c_size = 104, - .public_key_vec = true, - .siggen_sigver_test = true, - }, { .key = /* secp384r1(sha224) */ "\x04\x69\x6c\xcf\x62\xee\xd0\x0d\xe5\xb5\x2f\x70\x54\xcf\x26\xa0" "\xd9\x98\x8d\x92\x2a\xab\x9b\x11\xcb\x48\x18\xa1\xa9\x0d\xd5\x18" diff --git a/include/linux/oid_registry.h b/include/linux/oid_registry.h index 4d04fa5d1e..8b79e55cfc 100644 --- a/include/linux/oid_registry.h +++ b/include/linux/oid_registry.h @@ -17,12 +17,10 @@ * build_OID_registry.pl to generate the data for look_up_OID(). */ enum OID { - OID_id_dsa_with_sha1, /* 1.2.840.10030.4.3 */ OID_id_dsa, /* 1.2.840.10040.4.1 */ OID_id_ecPublicKey, /* 1.2.840.10045.2.1 */ OID_id_prime192v1, /* 1.2.840.10045.3.1.1 */ OID_id_prime256v1, /* 1.2.840.10045.3.1.7 */ - OID_id_ecdsa_with_sha1, /* 1.2.840.10045.4.1 */ OID_id_ecdsa_with_sha224, /* 1.2.840.10045.4.3.1 */ OID_id_ecdsa_with_sha256, /* 1.2.840.10045.4.3.2 */ OID_id_ecdsa_with_sha384, /* 1.2.840.10045.4.3.3 */ @@ -30,7 +28,6 @@ enum OID { /* PKCS#1 {iso(1) member-body(2) us(840) rsadsi(113549) pkcs(1) pkcs-1(1)} */ OID_rsaEncryption, /* 1.2.840.113549.1.1.1 */ - OID_sha1WithRSAEncryption, /* 1.2.840.113549.1.1.5 */ OID_sha256WithRSAEncryption, /* 1.2.840.113549.1.1.11 */ OID_sha384WithRSAEncryption, /* 1.2.840.113549.1.1.12 */ OID_sha512WithRSAEncryption, /* 1.2.840.113549.1.1.13 */ @@ -67,7 +64,6 @@ enum OID { OID_PKU2U, /* 1.3.5.1.5.2.7 */ OID_Scram, /* 1.3.6.1.5.5.14 */ OID_certAuthInfoAccess, /* 1.3.6.1.5.5.7.1.1 */ - OID_sha1, /* 1.3.14.3.2.26 */ OID_id_ansip384r1, /* 1.3.132.0.34 */ OID_sha256, /* 2.16.840.1.101.3.4.2.1 */ OID_sha384, /* 2.16.840.1.101.3.4.2.2 */ diff --git a/kernel/module/Kconfig b/kernel/module/Kconfig index 33a2e991f6..19a53d5e77 100644 --- a/kernel/module/Kconfig +++ b/kernel/module/Kconfig @@ -236,10 +236,6 @@ choice possible to load a signed module containing the algorithm to check the signature on that module. -config MODULE_SIG_SHA1 - bool "Sign modules with SHA-1" - select CRYPTO_SHA1 - config MODULE_SIG_SHA224 bool "Sign modules with SHA-224" select CRYPTO_SHA256 @@ -261,7 +257,6 @@ endchoice config MODULE_SIG_HASH string depends on MODULE_SIG || IMA_APPRAISE_MODSIG - default "sha1" if MODULE_SIG_SHA1 default "sha224" if MODULE_SIG_SHA224 default "sha256" if MODULE_SIG_SHA256 default "sha384" if MODULE_SIG_SHA384
Removes support for sha1 signed kernel modules, importing sha1 signed x.509 certificates. rsa-pkcs1pad keeps sha1 padding support, which seems to be used by virtio driver. sha1 remains available as there are many drivers and subsystems using it. Note only hmac(sha1) with secret keys remains cryptographically secure. In the kernel there are filesystems, IMA, tpm/pcr that appear to be using sha1. Maybe they can all start to be slowly upgraded to something else i.e. blake3, ParallelHash, SHAKE256 as needed. Signed-off-by: Dimitri John Ledkov <dimitri.ledkov@canonical.com> --- crypto/asymmetric_keys/mscode_parser.c | 3 - crypto/asymmetric_keys/pkcs7_parser.c | 4 -- crypto/asymmetric_keys/public_key.c | 3 +- crypto/asymmetric_keys/signature.c | 2 +- crypto/asymmetric_keys/x509_cert_parser.c | 8 --- crypto/testmgr.h | 80 ----------------------- include/linux/oid_registry.h | 4 -- kernel/module/Kconfig | 5 -- 8 files changed, 2 insertions(+), 107 deletions(-)