| Message ID | 3d74d6134f4f87a90ebe0a37cb06c6ec144ceef7.1738521533.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ecdsa KEYCTL_PKEY_QUERY fixes | expand |
On Sun, Feb 02, 2025 at 08:00:53PM +0100, Lukas Wunner wrote: > KEYCTL_PKEY_QUERY system calls for ecdsa keys return the key size as > max_enc_size and max_dec_size, even though such keys cannot be used for > encryption/decryption. They're exclusively for signature generation or > verification. > > Only rsa keys with pkcs1 encoding can also be used for encryption or > decryption. > > Return 0 instead for ecdsa keys (as well as ecrdsa keys). > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > --- > crypto/asymmetric_keys/public_key.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) I think we should discuss who is using these user-space APIs before doing any more work on them. The in-kernel asymmetric crypto code is not safe against side-channel attacks. As there are no in-kernel users of private-key functionality, we should consider getting rid of private key support completely. As it stands the only user is this user-space API. Cheers,
On Sun, Feb 09, 2025 at 05:58:07PM +0800, Herbert Xu wrote: > On Sun, Feb 02, 2025 at 08:00:53PM +0100, Lukas Wunner wrote: > > KEYCTL_PKEY_QUERY system calls for ecdsa keys return the key size as > > max_enc_size and max_dec_size, even though such keys cannot be used for > > encryption/decryption. They're exclusively for signature generation or > > verification. > > > > Only rsa keys with pkcs1 encoding can also be used for encryption or > > decryption. > > > > Return 0 instead for ecdsa keys (as well as ecrdsa keys). > > I think we should discuss who is using these user-space APIs > before doing any more work on them. The in-kernel asymmetric > crypto code is not safe against side-channel attacks. As there > are no in-kernel users of private-key functionality, we should > consider getting rid of private key support completely. > > As it stands the only user is this user-space API. Personally I am not using this user-space API, so I don't really have a dog in this fight. I just noticed the incorrect output for KEYCTL_PKEY_QUERY and thought it might be better if it's fixed. One user of this API is the Embedded Linux Library, which in turn is used by Intel Wireless Daemon: https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/key.c https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/src/eap-tls.c Basically IWD seems to be invoking the kernel's Key Retention Service for EAP authentication. It's still maintained and known to have active users, so removing the user-space keyctl ABI would definitely cause breakage. I've just checked for other reverse dependencies of the "libell0" package on Debian, it lists "bluez" and "mptcpd" but looking at their source code reveals they're not using the l_key_*() functions, so they would not be affected by removal. There's a keyring package for go, so I suppose there may be go applications out there using it: https://pkg.go.dev/pault.ag/go/keyring Then there's the keyutils library... https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/keyutils.git ...and listing the reverse dependencies for "libkeyutils1" on Debian reveals a slew of packages which are using it: gdm3 samba-libs sssd-common python3-keyutils nfs-common ndctl mokutil kstart libkrb5-3 kafs-client ima-evm-utils ceph-common libecryptfs1 ecryptfs-utils cifs-utils And "python3-keyutils" in turn has this reverse dependency: udiskie Finally, folks at cloudflare praised the kernel's Key Retention Service and encouraged everyone to use it... :) https://blog.cloudflare.com/the-linux-kernel-key-retention-service-and-why-you-should-use-it-in-your-next-application/ In short, it doesn't seem trivial to drop this user-space API. Thanks, Lukas
On Sun, Feb 9, 2025 at 11:29 AM Lukas Wunner <lukas@wunner.de> wrote: > > On Sun, Feb 09, 2025 at 05:58:07PM +0800, Herbert Xu wrote: > > On Sun, Feb 02, 2025 at 08:00:53PM +0100, Lukas Wunner wrote: > > > KEYCTL_PKEY_QUERY system calls for ecdsa keys return the key size as > > > max_enc_size and max_dec_size, even though such keys cannot be used for > > > encryption/decryption. They're exclusively for signature generation or > > > verification. > > > > > > Only rsa keys with pkcs1 encoding can also be used for encryption or > > > decryption. > > > > > > Return 0 instead for ecdsa keys (as well as ecrdsa keys). > > > > I think we should discuss who is using these user-space APIs > > before doing any more work on them. The in-kernel asymmetric > > crypto code is not safe against side-channel attacks. As there > > are no in-kernel users of private-key functionality, we should > > consider getting rid of private key support completely. > > > > As it stands the only user is this user-space API. Please don't! Keyrings + asymmetric crypto is a great building block for secure architectures. If anything we want more of this, not less. We can get rid of various ssh-agents and anything that tries to keep cryptographic material in a separate address space. It is the most straightforward way to avoid heartbleed-like vulnerabilities [1]. Before this we had to design whole solutions just to separate private keys from network facing code [2]. Now, in-kernel RSA implementation is indeed a downside, but again - one could swap internal implementations and provide their own. We have an internal BoringSSL-based in-kernel crypto driver (which I hope to open source one day) which avoids this problem. I remember there was also some work to expose TPMs through the keyrings API, which would solve this problem as well [3]. In general this API allows adopting various platform crypto chips very easily and should be encouraged. I made a presentation explaining why this API is much better for TPMs, for example, rather than directly using /dev/tpm from userspace [4]. On the topic of better RSA implementation: last year we've been working with folks from a company called Cryspen with the hope to produce better and even formally-verified RSA and ECDSA implementations for the Linux kernel (based on their HACL open source library). We got pretty good results [5] for RSA: tl;dr signing is faster than the current in-kernel code, but verification is slower (not a problem as we can use verification from the in-kernel implementation as we don't care about side channels there). Unfortunately the work was deprioritised this year, but if there is enough interest from the kernel community (and hopefully support to make the code more "kernel-integration friendly) I can try to make a case to re-prioritise this again. > Personally I am not using this user-space API, so I don't really > have a dog in this fight. I just noticed the incorrect output > for KEYCTL_PKEY_QUERY and thought it might be better if it's fixed. > > One user of this API is the Embedded Linux Library, which in turn > is used by Intel Wireless Daemon: > > https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/key.c > https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/src/eap-tls.c > > Basically IWD seems to be invoking the kernel's Key Retention Service for > EAP authentication. It's still maintained and known to have active users, > so removing the user-space keyctl ABI would definitely cause breakage. > > I've just checked for other reverse dependencies of the "libell0" package > on Debian, it lists "bluez" and "mptcpd" but looking at their source code > reveals they're not using the l_key_*() functions, so they would not be > affected by removal. > > There's a keyring package for go, so I suppose there may be go applications > out there using it: > > https://pkg.go.dev/pault.ag/go/keyring > > Then there's the keyutils library... > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/keyutils.git > > ...and listing the reverse dependencies for "libkeyutils1" on Debian > reveals a slew of packages which are using it: > > gdm3 samba-libs sssd-common python3-keyutils nfs-common ndctl > mokutil kstart libkrb5-3 kafs-client ima-evm-utils ceph-common > libecryptfs1 ecryptfs-utils cifs-utils > > And "python3-keyutils" in turn has this reverse dependency: > > udiskie > > Finally, folks at cloudflare praised the kernel's Key Retention Service > and encouraged everyone to use it... :) > > https://blog.cloudflare.com/the-linux-kernel-key-retention-service-and-why-you-should-use-it-in-your-next-application/ > > In short, it doesn't seem trivial to drop this user-space API. > > Thanks, > > Lukas Thanks, Ignat [1]: https://heartbleed.com/ [2]: https://blog.cloudflare.com/keyless-ssl-the-nitty-gritty-technical-details/ [3]: https://lore.kernel.org/lkml/97dd7485-51bf-4e47-83ab-957710fc2182@linux.ibm.com/T/ [4]: https://youtu.be/g8b4K5FQUj8?si=yY8mkoRuyE_SKBjh [5]: https://md.cryspen.com/cf_hacs_kernel
On Sun, Feb 09, 2025 at 12:29:54PM +0100, Lukas Wunner wrote: > > One user of this API is the Embedded Linux Library, which in turn > is used by Intel Wireless Daemon: > > https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/key.c > https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/src/eap-tls.c Surely this doesn't use the private key part of the API, does it? While I intensely dislike the entire API being there, it's only the private key part that I really want to remove. Thanks,
On Mon, Feb 10, 2025 at 03:54:45PM +0800, Herbert Xu wrote: > On Sun, Feb 09, 2025 at 12:29:54PM +0100, Lukas Wunner wrote: > > One user of this API is the Embedded Linux Library, which in turn > > is used by Intel Wireless Daemon: > > > > https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/key.c > > https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/src/eap-tls.c > > Surely this doesn't use the private key part of the API, does it? It does use the private key part: It takes advantage of the kernel's Key Retention Service for EAP-TLS, which generally uses mutual authentication. E.g. clients authenticate against a wireless hotspot. Hence it does invoke KEYCTL_PKEY_SIGN and KEYCTL_PKEY_ENCRYPT (with private keys, obviously). > While I intensely dislike the entire API being there, it's only the > private key part that I really want to remove. Note that the patches proposed here only touch the KEYCTL_PKEY_QUERY interface, which is used for public keys as well. Thanks, Lukas
On Mon, Feb 10, 2025 at 07:53:57PM +0100, Lukas Wunner wrote: > It takes advantage of the kernel's Key Retention Service for EAP-TLS, > which generally uses mutual authentication. E.g. clients authenticate > against a wireless hotspot. Hence it does invoke KEYCTL_PKEY_SIGN and > KEYCTL_PKEY_ENCRYPT (with private keys, obviously). Sorry, I meant KEYCTL_PKEY_DECRYPT. ^^
On Mon, Feb 10, 2025 at 07:53:57PM +0100, Lukas Wunner wrote: > > It does use the private key part: > > It takes advantage of the kernel's Key Retention Service for EAP-TLS, > which generally uses mutual authentication. E.g. clients authenticate > against a wireless hotspot. Hence it does invoke KEYCTL_PKEY_SIGN and > KEYCTL_PKEY_ENCRYPT (with private keys, obviously). Well if it wishes to keep this going, then someone will have to step up and maintain these algorithms and make them secure against side-channel attacks. In the absence of that, this functionality should be removed from the kernel. Cheers,
On Mon, Feb 10, 2025 at 07:53:57PM +0100, Lukas Wunner wrote: > > > > https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/key.c > > > https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/src/eap-tls.c > > > > Surely this doesn't use the private key part of the API, does it? > > It does use the private key part: > > It takes advantage of the kernel's Key Retention Service for EAP-TLS, > which generally uses mutual authentication. E.g. clients authenticate > against a wireless hotspot. Hence it does invoke KEYCTL_PKEY_SIGN and > KEYCTL_PKEY_ENCRYPT (with private keys, obviously). Does it really? I grepped the whole iwd git tree and the only use of private key functionality is to check that it matches the public key, IOW it encrypts a piece of text and then decrypts it again to check whether they match. It doesn't make use of any other private key functionality AFAICS. Cheers,
On Sun, Feb 16, 2025 at 12:19:44PM +0800, Herbert Xu wrote: > On Mon, Feb 10, 2025 at 07:53:57PM +0100, Lukas Wunner wrote: > > > > https://git.kernel.org/pub/scm/libs/ell/ell.git/tree/ell/key.c > > > > https://git.kernel.org/pub/scm/network/wireless/iwd.git/tree/src/eap-tls.c > > > > > > Surely this doesn't use the private key part of the API, does it? > > > > It does use the private key part: > > > > It takes advantage of the kernel's Key Retention Service for EAP-TLS, > > which generally uses mutual authentication. E.g. clients authenticate > > against a wireless hotspot. Hence it does invoke KEYCTL_PKEY_SIGN and > > KEYCTL_PKEY_ENCRYPT (with private keys, obviously). > > Does it really? I grepped the whole iwd git tree and the only > use of private key functionality is to check that it matches > the public key, IOW it encrypts a piece of text and then decrypts > it again to check whether they match. > > It doesn't make use of any other private key functionality AFAICS. __eap_handle_request() [iwd src/eap.c] eap->method->handle_request() eap_tls_common_handle_request() [iwd src/eap-tls-common.c] l_tls_handle_rx() [ell ell/tls-record.c] tls_handle_ciphertext() tls_handle_plaintext() tls_handle_message() [ell ell/tls.c] tls_handle_handshake() tls_handle_server_hello_done() tls_send_certificate_verify() tls->pending.cipher_suite->signature->sign tls_rsa_sign() [ell ell/tls-suites.c] l_key_sign() [ell ell/key.c] eds_common() kernel_key_eds() syscall(__NR_keyctl, KEYCTL_PKEY_SIGN, ...) ... where tls_handle_server_hello_done() performs client authentication per RFC 8446 sec 4.6.2: "When the client has sent the "post_handshake_auth" extension (see Section 4.2.6), a server MAY request client authentication at any time after the handshake has completed by sending a CertificateRequest message. The client MUST respond with the appropriate Authentication messages (see Section 4.4). If the client chooses to authenticate, it MUST send Certificate, CertificateVerify, and Finished." https://datatracker.ietf.org/doc/html/rfc8446#section-4.6.2 I think the best option at this point isn't to aim for removal but to wait for Cloudflare to beat their out-of-tree implementation (which apparently isn't susceptible to side channel attacks) into shape so that it can be upstreamed. Thanks, Lukas
On Sun, Feb 16, 2025 at 11:45:04AM +0100, Lukas Wunner wrote: > > I think the best option at this point isn't to aim for removal > but to wait for Cloudflare to beat their out-of-tree implementation > (which apparently isn't susceptible to side channel attacks) > into shape so that it can be upstreamed. I don't think having a one-off fix is sufficient. We need someone who can maintain this for the long term. Are you willing to do this? Cheers,
On Sun, Mar 02, 2025 at 03:47:50PM +0800, Herbert Xu wrote: > On Sun, Feb 16, 2025 at 11:45:04AM +0100, Lukas Wunner wrote: > > I think the best option at this point isn't to aim for removal > > but to wait for Cloudflare to beat their out-of-tree implementation > > (which apparently isn't susceptible to side channel attacks) > > into shape so that it can be upstreamed. > > I don't think having a one-off fix is sufficient. We need someone > who can maintain this for the long term. Are you willing to do this? In principle, yes. Which files are we talking about exactly? Thanks, Lukas
On Sun, Mar 02, 2025 at 10:25:34AM +0100, Lukas Wunner wrote: > > In principle, yes. Great! > Which files are we talking about exactly? So it's basically everything under crypto/asymmetric_keys + the algorithms that they use. Cheers,
diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c index bf165d321440..dd44a966947f 100644 --- a/crypto/asymmetric_keys/public_key.c +++ b/crypto/asymmetric_keys/public_key.c @@ -188,6 +188,8 @@ static int software_key_query(const struct kernel_pkey_params *params, ptr = pkey_pack_u32(ptr, pkey->paramlen); memcpy(ptr, pkey->params, pkey->paramlen); + memset(info, 0, sizeof(*info)); + if (issig) { sig = crypto_alloc_sig(alg_name, 0, 0); if (IS_ERR(sig)) { @@ -211,6 +213,9 @@ static int software_key_query(const struct kernel_pkey_params *params, info->supported_ops |= KEYCTL_SUPPORTS_SIGN; if (strcmp(params->encoding, "pkcs1") == 0) { + info->max_enc_size = len; + info->max_dec_size = len; + info->supported_ops |= KEYCTL_SUPPORTS_ENCRYPT; if (pkey->key_is_private) info->supported_ops |= KEYCTL_SUPPORTS_DECRYPT; @@ -232,6 +237,8 @@ static int software_key_query(const struct kernel_pkey_params *params, len = crypto_akcipher_maxsize(tfm); info->max_sig_size = len; info->max_data_size = len; + info->max_enc_size = len; + info->max_dec_size = len; info->supported_ops = KEYCTL_SUPPORTS_ENCRYPT; if (pkey->key_is_private) @@ -239,8 +246,6 @@ static int software_key_query(const struct kernel_pkey_params *params, } info->key_size = len * 8; - info->max_enc_size = len; - info->max_dec_size = len; ret = 0;