Re: [RFC PATCH v2] akcipher: Introduce verify_rsa/verify for public key algorithms
On Fri, Jan 18, 2019 at 11:41:00PM +0300, Vitaly Chikunov wrote: > > a) RSA verify works differently (is it just disguised encrypt), > b) We have separate wrapper module for it (pkcs1pad). Thus: > > Old API can not be removed. In other words, we can not replace > .verify_rsa with .verify in these drivers or PKCS1 will not work. > > We can replace .verify_rsa with .verify in pkcs1pad, but there is no > need for that if we stay with two API calls, which we can't avoid. I think having two API calls during a transition period is fine. But it must not be the long-term outcome. In order to keep existing drivers working, I think we should make the API wrap the legacy verify_rsa and implement verify directly on top of it. IOW the driver remains unchanged for now but the crypto API code should provide a verify API that is implemented on top of the driver's verify_rsa call. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [RFC PATCH v2] akcipher: Introduce verify_rsa/verify for public key algorithms
David, On Wed, Jan 16, 2019 at 09:27:19PM +0300, Vitaly Chikunov wrote: > On Wed, Jan 16, 2019 at 05:12:20PM +, David Howells wrote: > > Umm... What do I apply this patch to? > > This should go over "crypto: testmgr - split akcipher tests by a key type" > which I sent at 20190107 to linux-crypto. Sorry for the mess. > > > In your modified public_key_verify_signature(): > > > > > - sg_init_one(_sg, output, outlen); > > > - akcipher_request_set_crypt(req, _sg, _sg, sig->s_size, > > > + sg_init_one(_sg, output, outlen); > > > + akcipher_request_set_crypt(req, _sg, _sg, sig->s_size, > > > outlen); > > > > Why is the output necessary? It was there for the decoded hash to be placed > > in prior to comparison - but now that's not necessary. > > Agreed. I prepared the patch which factors `output' into public_key_verify_signature(). Also, I try to elucidate my arguments below. > > > - ret = crypto_wait_req(crypto_akcipher_verify(req), ); > > > + ret = crypto_wait_req(crypto_akcipher_verify(req, sig->digest, > > > + sig->digest_size), ); > > > > I see sig->digest is passed in here. Should it be passed in in place of > > output_sg above? In short, it's passed as parameter to not clutter `struct akcipher_request' and to distinguish it from encrypt/decrypt scatterlists. New 'verify' op requires very different parameter set than encrypt, decrypt, sign. This difference is now most illustrative in testmgr (see in the next patch). > (I tried different approaches such as passing additional arguments to > `akcipher_request_set_crypt' or having additional setter like > `akcipher_request_set_aux' just to set value of digest and that should > be used just for verify op.) > > I thought passing input parameter in `struct akcipher_request' in the > field called 'dst' could be confusing for users. So this should be an > additional parameter in the request which is never used by any other caller. > Also, it was unknown if this should be scatterlist or not (I choose that > it should not). When it's separate argument to crypto_akcipher_verify() > call user is forced to set it, and there is no cluttering of `struct > akcipher_request' (which is designed to handle just encryption/decryption) > with not needed auxiliary parameters, and because it's very separated > from request parameters which all scatterlists and all other calls > arguments usually are not scatterlists, so this looked like similar > approach to what others do. > > > > - inst->alg.verify = pkcs1pad_verify; > > > + inst->alg.verify_rsa = pkcs1pad_verify; > > > > Is there a reason that pkcs1pad_verify() can't do the comparison? > > If you agree that we have two callbacks for a full and a partial > verification (I assume you do), why should pkcs1pad_verify do a full > verification if it's allowed to do just partial one, and it's RSA > cipher which have special partial verification designed for them. > > So I decided that pkcs1pad_verify should implement verify_rsa api only > and this is beneficial for having minimal code change and somewhat > backward compatible. > > > > - .verify = rsa_verify, > > > + .verify_rsa = rsa_verify, > > > > Likewise verify_rsa()? > > > > Granted, this might involve pkcs1pad_verify() dressing up the signature in > > the > > appropriate wrappings and passing it along to verify_rsa() to do the actual > > comparison there (ie. what pkcs1pad_verify_complete() does). a) RSA verify works differently (is it just disguised encrypt), b) We have separate wrapper module for it (pkcs1pad). Thus: Old API can not be removed. In other words, we can not replace .verify_rsa with .verify in these drivers or PKCS1 will not work. We can replace .verify_rsa with .verify in pkcs1pad, but there is no need for that if we stay with two API calls, which we can't avoid. > If we stay with the two api calls (verify and verify_rsa) pkcs1pad_verify > does not need to do any verification leaving it to the akcipher core. > > There is only potential "problem" that pkcs1pad code will not be able to > use other akciphers besides rsa family (implementing only verify_rsa), > but I assumed this is not needed (since only RSA is actually using > PKCS1) and maybe even beneficial restriction. > > > > - .verify = caam_rsa_enc, > > > + .verify_rsa = caam_rsa_enc, > > > > I presume this is the reason - because this reuses its encrypt operation > > directly. But could this instead perform the comparison upon completion, > > say > > in rsa_pub_done()? No, or pkcs1pad will not work. Thanks, > > > > > - .verify = qat_rsa_enc, > > > + .verify_rsa = qat_rsa_enc, > > > > Again, this could do the comparison, say, in qat_rsa_cb(). > > Abandoning idea with two api calls (full verify and partial verify_rsa) > will require me to modify code for all these drivers for devices that I > don't have and can't test. So, I choose approach with less new code. > > If you think that partial
Re: [RFC PATCH v2] akcipher: Introduce verify_rsa/verify for public key algorithms
David, On Wed, Jan 16, 2019 at 05:12:20PM +, David Howells wrote: > Umm... What do I apply this patch to? This should go over "crypto: testmgr - split akcipher tests by a key type" which I sent at 20190107 to linux-crypto. Sorry for the mess. > In your modified public_key_verify_signature(): > > > - sg_init_one(_sg, output, outlen); > > - akcipher_request_set_crypt(req, _sg, _sg, sig->s_size, > > + sg_init_one(_sg, output, outlen); > > + akcipher_request_set_crypt(req, _sg, _sg, sig->s_size, > >outlen); > > Why is the output necessary? It was there for the decoded hash to be placed > in prior to comparison - but now that's not necessary. Agreed. > > - ret = crypto_wait_req(crypto_akcipher_verify(req), ); > > + ret = crypto_wait_req(crypto_akcipher_verify(req, sig->digest, > > +sig->digest_size), ); > > I see sig->digest is passed in here. Should it be passed in in place of > output_sg above? (I tried different approaches such as passing additional arguments to `akcipher_request_set_crypt' or having additional setter like `akcipher_request_set_aux' just to set value of digest and that should be used just for verify op.) I thought passing input parameter in `struct akcipher_request' in the field called 'dst' could be confusing for users. So this should be an additional parameter in the request which is never used by any other caller. Also, it was unknown if this should be scatterlist or not (I choose that it should not). When it's separate argument to crypto_akcipher_verify() call user is forced to set it, and there is no cluttering of `struct akcipher_request' (which is designed to handle just encryption/decryption) with not needed auxiliary parameters, and because it's very separated from request parameters which all scatterlists and all other calls arguments usually are not scatterlists, so this looked like similar approach to what others do. > > - inst->alg.verify = pkcs1pad_verify; > > + inst->alg.verify_rsa = pkcs1pad_verify; > > Is there a reason that pkcs1pad_verify() can't do the comparison? If you agree that we have two callbacks for a full and a partial verification (I assume you do), why should pkcs1pad_verify do a full verification if it's allowed to do just partial one, and it's RSA cipher which have special partial verification designed for them. So I decided that pkcs1pad_verify should implement verify_rsa api only and this is beneficial for having minimal code change and somewhat backward compatible. > > - .verify = rsa_verify, > > + .verify_rsa = rsa_verify, > > Likewise verify_rsa()? > > Granted, this might involve pkcs1pad_verify() dressing up the signature in the > appropriate wrappings and passing it along to verify_rsa() to do the actual > comparison there (ie. what pkcs1pad_verify_complete() does). If we stay with the two api calls (verify and verify_rsa) pkcs1pad_verify does not need to do any verification leaving it to the akcipher core. There is only potential "problem" that pkcs1pad code will not be able to use other akciphers besides rsa family (implementing only verify_rsa), but I assumed this is not needed (since only RSA is actually using PKCS1) and maybe even beneficial restriction. > > - .verify = caam_rsa_enc, > > + .verify_rsa = caam_rsa_enc, > > I presume this is the reason - because this reuses its encrypt operation > directly. But could this instead perform the comparison upon completion, say > in rsa_pub_done()? > > > - .verify = qat_rsa_enc, > > + .verify_rsa = qat_rsa_enc, > > Again, this could do the comparison, say, in qat_rsa_cb(). Abandoning idea with two api calls (full verify and partial verify_rsa) will require me to modify code for all these drivers for devices that I don't have and can't test. So, I choose approach with less new code. If you think that partial verify api should be completely removed that change will require much bigger rework. Please tell if you would prefer that. Thanks,
Re: [RFC PATCH v2] akcipher: Introduce verify_rsa/verify for public key algorithms
Umm... What do I apply this patch to? In your modified public_key_verify_signature(): > - sg_init_one(_sg, output, outlen); > - akcipher_request_set_crypt(req, _sg, _sg, sig->s_size, > + sg_init_one(_sg, output, outlen); > + akcipher_request_set_crypt(req, _sg, _sg, sig->s_size, > outlen); Why is the output necessary? It was there for the decoded hash to be placed in prior to comparison - but now that's not necessary. > - ret = crypto_wait_req(crypto_akcipher_verify(req), ); > + ret = crypto_wait_req(crypto_akcipher_verify(req, sig->digest, > + sig->digest_size), ); I see sig->digest is passed in here. Should it be passed in in place of output_sg above? > - inst->alg.verify = pkcs1pad_verify; > + inst->alg.verify_rsa = pkcs1pad_verify; Is there a reason that pkcs1pad_verify() can't do the comparison? > - .verify = rsa_verify, > + .verify_rsa = rsa_verify, Likewise verify_rsa()? Granted, this might involve pkcs1pad_verify() dressing up the signature in the appropriate wrappings and passing it along to verify_rsa() to do the actual comparison there (ie. what pkcs1pad_verify_complete() does). > - .verify = caam_rsa_enc, > + .verify_rsa = caam_rsa_enc, I presume this is the reason - because this reuses its encrypt operation directly. But could this instead perform the comparison upon completion, say in rsa_pub_done()? > - .verify = qat_rsa_enc, > + .verify_rsa = qat_rsa_enc, Again, this could do the comparison, say, in qat_rsa_cb(). David
[RFC PATCH v2] akcipher: Introduce verify_rsa/verify for public key algorithms
To my opinion this new version may fit suggestions of Herbert and David - we
only
have single general top level crypto_akcipher_verify() call, but two low level
->verify() and ->verify_rsa() calls. Final signature verification is moved from
each caller of crypto_akcipher_verify() into crypto_akcipher_verify() itself.
There is remained crypto_akcipher_verify_rsa() just for rsa specific (pkcs1)
code (it seems that we are isolating from calling directly ->verify_rsa()).
Does it looks good?
Original/new commit message:
Previous akcipher .verify() just `decrypts' (using RSA encrypt which is
using public key) signature to uncover message hash, which was then
compared in upper level public_key_verify_signature() with the expected
hash value, which itself was never passed into verify().
This approach was incompatible with EC-DSA family of algorithms,
because, to verify a signature EC-DSA algorithm also needs a hash value
as input; then it's used (together with a signature divided into halves
`r||s') to produce a witness value, which is then compared with `r' to
determine if the signature is correct. Thus, for EC-DSA, nor
requirements of .verify() itself, nor its output expectations in
public_key_verify_signature() wasn't satisfied.
Make improved .verify() call which gets hash value as parameter and
produce complete signature check (without any output besides status.
Other rsa-centric drivers have replaced verify() with verify_rsa() which
have old semantic. If akcipher have .verify_rsa() callback, it will be
used for a partial verification, which then is finished in
crypto_akcipher_verify().
Now for the top level verification only crypto_akcipher_verify() needs
to be called.
crypto_akcipher_verify_rsa() is introduced which directly calls
.verify_rsa() for use by PKCS1 driver to call its backend.
Signed-off-by: Vitaly Chikunov
---
crypto/akcipher.c | 53 +++
crypto/asymmetric_keys/public_key.c | 19 +++---
crypto/rsa-pkcs1pad.c | 4 +-
crypto/rsa.c | 2 +-
crypto/testmgr.c | 5 ++-
drivers/crypto/caam/caampkc.c | 2 +-
drivers/crypto/ccp/ccp-crypto-rsa.c | 2 +-
drivers/crypto/qat/qat_common/qat_asym_algs.c | 2 +-
include/crypto/akcipher.h | 21 +++
9 files changed, 81 insertions(+), 29 deletions(-)
diff --git a/crypto/akcipher.c b/crypto/akcipher.c
index 0cbeae137e0a..2e247cbf5115 100644
--- a/crypto/akcipher.c
+++ b/crypto/akcipher.c
@@ -25,6 +25,59 @@
#include
#include "internal.h"
+/**
+ * crypto_akcipher_verify() - Invoke public key signature verification
+ *
+ * Function invokes the specific public key signature verification operation
+ * for a given public key algorithm.
+ *
+ * @req: asymmetric key request
+ * @digest:expected hash value
+ * @digest_len:hash length
+ *
+ * Return: zero on verification success; error code in case of error.
+ */
+int crypto_akcipher_verify(struct akcipher_request *req,
+ const unsigned char *digest, unsigned int digest_len)
+{
+ struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req);
+ struct akcipher_alg *alg = crypto_akcipher_alg(tfm);
+ struct crypto_alg *calg = tfm->base.__crt_alg;
+ int ret;
+
+ if (WARN_ON(!digest || !digest_len))
+ return -EINVAL;
+
+ crypto_stats_get(calg);
+ if (alg->verify_rsa) {
+ u8 output[HASH_MAX_DIGESTSIZE];
+
+ /* Perform the verification calculation. This doesn't actually
+* do the verification, but rather calculates the hash expected
+* by the signature and returns that to us.
+*/
+ ret = crypto_akcipher_verify_rsa(req);
+ if (ret)
+ goto out;
+ if (req->dst_len != digest_len ||
+ WARN_ON(req->dst_len > sizeof(output))) {
+ ret = -EKEYREJECTED;
+ goto out;
+ }
+ sg_copy_to_buffer(req->dst,
+ sg_nents_for_len(req->dst, digest_len),
+ output, digest_len);
+ /* Do final verification step. */
+ if (memcmp(digest, output, digest_len))
+ ret = -EKEYREJECTED;
+ } else
+ ret = alg->verify(req, digest, digest_len);
+out:
+ crypto_stats_akcipher_verify(ret, calg);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(crypto_akcipher_verify);
+
#ifdef CONFIG_NET
static int crypto_akcipher_report(struct sk_buff *skb, struct crypto_alg *alg)
{
diff --git a/crypto/asymmetric_keys/public_key.c
b/crypto/asymmetric_keys/public_key.c
index f5d85b47fcc6..1f86d22548f0 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -227,7 +227,7 @@
