| Message ID | 20220729165954.991-1-ignat@cloudflare.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: akcipher - default implementations for setting private/public keys | expand |
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,
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
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,
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
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,
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.
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);
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(+) -- 2.36.1