[PATCH] crypto: akcipher - default implementations for setting private/public keys

[PATCH] crypto: akcipher - default implementations for setting private/public keys

[Ignat Korchagin]

Many akcipher implementations (like ECDSA) support only signature
verifications only, so they don't have all callbacks defined.

Commit 78a0324f4a53 ("crypto: akcipher - default implementations for
request callbacks") introduced default callbacks for sign/verify
operations, which just return an error code.

However, these are not enough, because before calling sign/verify the
caller would likely call set_priv_key/set_pub_key first on the
instantiated transform (as the in-kernel testmgr does). These functions do
not have default stubs, so the kernel crashes, when trying to set a
private key on an akcipher, which doesn't support signature generation.

I've noticed this, when trying to add a KAT vector for ECDSA signature to
the testmgr.

With this patch the testmgr returns an error in dmesg (as it should)
instead of crashing the kernel NULL ptr dereference.

Fixes: 78a0324f4a53 ("crypto: akcipher - default implementations for request callbacks")
Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
---
 crypto/akcipher.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/crypto/akcipher.c b/crypto/akcipher.c
index f866085c8a4a..fc4db0c6ca33 100644
--- a/crypto/akcipher.c
+++ b/crypto/akcipher.c
@@ -120,6 +120,12 @@ static int akcipher_default_op(struct akcipher_request *req)
 	return -ENOSYS;
 }

+static int akcipher_default_set_key(struct crypto_akcipher *tfm,
+				     const void *key, unsigned int keylen)
+{
+	return -ENOSYS;
+}
+
 int crypto_register_akcipher(struct akcipher_alg *alg)
 {
 	struct crypto_alg *base = &alg->base;
@@ -132,6 +138,10 @@ int crypto_register_akcipher(struct akcipher_alg *alg)
 		alg->encrypt = akcipher_default_op;
 	if (!alg->decrypt)
 		alg->decrypt = akcipher_default_op;
+	if (!alg->set_priv_key)
+		alg->set_priv_key = akcipher_default_set_key;
+	if (!alg->set_pub_key)
+		alg->set_pub_key = akcipher_default_set_key;

 	akcipher_prepare_alg(alg);
 	return crypto_register_alg(base);
--
2.36.1

Re: [PATCH] crypto: akcipher - default implementations for setting private/public keys

[Herbert Xu]

On Fri, Jul 29, 2022 at 05:59:54PM +0100, Ignat Korchagin wrote:
>
> @@ -132,6 +138,10 @@ int crypto_register_akcipher(struct akcipher_alg *alg)
>  		alg->encrypt = akcipher_default_op;
>  	if (!alg->decrypt)
>  		alg->decrypt = akcipher_default_op;
> +	if (!alg->set_priv_key)
> +		alg->set_priv_key = akcipher_default_set_key;
> +	if (!alg->set_pub_key)
> +		alg->set_pub_key = akcipher_default_set_key;

Under what circumstances could we have an algorithm without a
set_pub_key function?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: akcipher - default implementations for setting private/public keys

[Ignat Korchagin]

On Fri, Aug 19, 2022 at 10:54 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Fri, Jul 29, 2022 at 05:59:54PM +0100, Ignat Korchagin wrote:
> >
> > @@ -132,6 +138,10 @@ int crypto_register_akcipher(struct akcipher_alg *alg)
> >               alg->encrypt = akcipher_default_op;
> >       if (!alg->decrypt)
> >               alg->decrypt = akcipher_default_op;
> > +     if (!alg->set_priv_key)
> > +             alg->set_priv_key = akcipher_default_set_key;
> > +     if (!alg->set_pub_key)
> > +             alg->set_pub_key = akcipher_default_set_key;
>
> Under what circumstances could we have an algorithm without a
> set_pub_key function?

I can only elaborate here as I didn't encounter any real-world
use-cases, but may assume some limited crypto hardware device, which
may somehow "encourage" doing public key operations in software and
providing only "private-key" operations due to its limited resources.

Ignat

> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: akcipher - default implementations for setting private/public keys

[Herbert Xu]

On Mon, Aug 29, 2022 at 11:48:23AM +0100, Ignat Korchagin wrote:
>
> I can only elaborate here as I didn't encounter any real-world
> use-cases, but may assume some limited crypto hardware device, which
> may somehow "encourage" doing public key operations in software and
> providing only "private-key" operations due to its limited resources.

In general if a hardware is missing a piece of the functinoality
required by the API then it should implement a software fallback.

The only time such a NULL helper would make sense if an algorithm
had no public key.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: akcipher - default implementations for setting private/public keys

[Ignat Korchagin]

On Tue, Aug 30, 2022 at 10:00 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Mon, Aug 29, 2022 at 11:48:23AM +0100, Ignat Korchagin wrote:
> >
> > I can only elaborate here as I didn't encounter any real-world
> > use-cases, but may assume some limited crypto hardware device, which
> > may somehow "encourage" doing public key operations in software and
> > providing only "private-key" operations due to its limited resources.
>
> In general if a hardware is missing a piece of the functinoality
> required by the API then it should implement a software fallback.
>
> The only time such a NULL helper would make sense if an algorithm
> had no public key.

I vaguely remember some initial research in quantum-resistant
signatures, which used HMAC for "signing" thus don't have any public
keys. But it is way beyond my expertise to comment on the practicality
and availability of such schemes.

I'm more concerned here about a buggy "third-party" RSA driver, which
may not implement the callback and which gets prioritised by the
framework, thus giving the ability to trigger a NULL-ptr dereference
from userspace via keyctl(2). I think the Crypto API framework should
be a bit more robust to handle such a case, but I also understand that
there are a lot of "if"s in this scenario and we can say it is up to
crypto driver not to be buggy. Therefore, consider my opinion as not
strong and I can post a v2, which does not provide a default stub for
set_pub_key, if you prefer.

Ignat

Re: [PATCH] crypto: akcipher - default implementations for setting private/public keys

[Herbert Xu]

On Tue, Aug 30, 2022 at 11:48:23AM +0100, Ignat Korchagin wrote:
>
> I vaguely remember some initial research in quantum-resistant
> signatures, which used HMAC for "signing" thus don't have any public
> keys. But it is way beyond my expertise to comment on the practicality
> and availability of such schemes.

We could always add this again should an algorithm requiring
it be introduced.

> I'm more concerned here about a buggy "third-party" RSA driver, which
> may not implement the callback and which gets prioritised by the
> framework, thus giving the ability to trigger a NULL-ptr dereference
> from userspace via keyctl(2). I think the Crypto API framework should
> be a bit more robust to handle such a case, but I also understand that
> there are a lot of "if"s in this scenario and we can say it is up to
> crypto driver not to be buggy. Therefore, consider my opinion as not
> strong and I can post a v2, which does not provide a default stub for
> set_pub_key, if you prefer.

If you're concerned with buggy algorithms/drivers, we should
ensure that the self-tests catch this.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: akcipher - default implementations for setting private/public keys

[Ignat Korchagin]

On Wed, Aug 31, 2022 at 9:56 AM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Tue, Aug 30, 2022 at 11:48:23AM +0100, Ignat Korchagin wrote:
> >
> > I vaguely remember some initial research in quantum-resistant
> > signatures, which used HMAC for "signing" thus don't have any public
> > keys. But it is way beyond my expertise to comment on the practicality
> > and availability of such schemes.
>
> We could always add this again should an algorithm requiring
> it be introduced.
>
> > I'm more concerned here about a buggy "third-party" RSA driver, which
> > may not implement the callback and which gets prioritised by the
> > framework, thus giving the ability to trigger a NULL-ptr dereference
> > from userspace via keyctl(2). I think the Crypto API framework should
> > be a bit more robust to handle such a case, but I also understand that
> > there are a lot of "if"s in this scenario and we can say it is up to
> > crypto driver not to be buggy. Therefore, consider my opinion as not
> > strong and I can post a v2, which does not provide a default stub for
> > set_pub_key, if you prefer.
>
> If you're concerned with buggy algorithms/drivers, we should
> ensure that the self-tests catch this.

So it does currently, because I caught it via testmgr. Will send a v2
without a default set_pub_key.