| Message ID | 20190116164703.9267-1-vt@altlinux.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [RFC,v2] akcipher: Introduce verify_rsa/verify for public key algorithms | expand |
Umm... What do I apply this patch to? In your modified public_key_verify_signature(): > - sg_init_one(&digest_sg, output, outlen); > - akcipher_request_set_crypt(req, &sig_sg, &digest_sg, sig->s_size, > + sg_init_one(&output_sg, output, outlen); > + akcipher_request_set_crypt(req, &sig_sg, &output_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), &cwait); > + ret = crypto_wait_req(crypto_akcipher_verify(req, sig->digest, > + sig->digest_size), &cwait); 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
David, On Wed, Jan 16, 2019 at 05:12:20PM +0000, 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(&digest_sg, output, outlen); > > - akcipher_request_set_crypt(req, &sig_sg, &digest_sg, sig->s_size, > > + sg_init_one(&output_sg, output, outlen); > > + akcipher_request_set_crypt(req, &sig_sg, &output_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), &cwait); > > + ret = crypto_wait_req(crypto_akcipher_verify(req, sig->digest, > > + sig->digest_size), &cwait); > > 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,
David, On Wed, Jan 16, 2019 at 09:27:19PM +0300, Vitaly Chikunov wrote: > On Wed, Jan 16, 2019 at 05:12:20PM +0000, 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(&digest_sg, output, outlen); > > > - akcipher_request_set_crypt(req, &sig_sg, &digest_sg, sig->s_size, > > > + sg_init_one(&output_sg, output, outlen); > > > + akcipher_request_set_crypt(req, &sig_sg, &output_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), &cwait); > > > + ret = crypto_wait_req(crypto_akcipher_verify(req, sig->digest, > > > + sig->digest_size), &cwait); > > > > 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 verify api should be completely removed that > change will require much bigger rework. Please tell if you would prefer > that. > > Thanks,
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,
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 <crypto/internal/akcipher.h> #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 @@ int public_key_verify_signature(const struct public_key *pkey, struct crypto_wait cwait; struct crypto_akcipher *tfm; struct akcipher_request *req; - struct scatterlist sig_sg, digest_sg; + struct scatterlist sig_sg, output_sg; char alg_name[CRYPTO_MAX_ALG_NAME]; void *output; unsigned int outlen; @@ -270,27 +270,18 @@ int public_key_verify_signature(const struct public_key *pkey, goto error_free_req; sg_init_one(&sig_sg, sig->s, sig->s_size); - sg_init_one(&digest_sg, output, outlen); - akcipher_request_set_crypt(req, &sig_sg, &digest_sg, sig->s_size, + sg_init_one(&output_sg, output, outlen); + akcipher_request_set_crypt(req, &sig_sg, &output_sg, sig->s_size, outlen); crypto_init_wait(&cwait); akcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG | CRYPTO_TFM_REQ_MAY_SLEEP, crypto_req_done, &cwait); - - /* 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_wait_req(crypto_akcipher_verify(req), &cwait); + ret = crypto_wait_req(crypto_akcipher_verify(req, sig->digest, + sig->digest_size), &cwait); if (ret) goto out_free_output; - /* Do the actual verification step. */ - if (req->dst_len != sig->digest_size || - memcmp(sig->digest, output, sig->digest_size) != 0) - ret = -EKEYREJECTED; - out_free_output: kfree(output); error_free_req: diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c index cfc04e15fd97..f82d70724229 100644 --- a/crypto/rsa-pkcs1pad.c +++ b/crypto/rsa-pkcs1pad.c @@ -550,7 +550,7 @@ static int pkcs1pad_verify(struct akcipher_request *req) req_ctx->out_sg, req->src_len, ctx->key_size); - err = crypto_akcipher_verify(&req_ctx->child_req); + err = crypto_akcipher_verify_rsa(&req_ctx->child_req); if (err != -EINPROGRESS && err != -EBUSY) return pkcs1pad_verify_complete(req, err); @@ -674,7 +674,7 @@ static int pkcs1pad_create(struct crypto_template *tmpl, struct rtattr **tb) inst->alg.encrypt = pkcs1pad_encrypt; inst->alg.decrypt = pkcs1pad_decrypt; inst->alg.sign = pkcs1pad_sign; - inst->alg.verify = pkcs1pad_verify; + inst->alg.verify_rsa = pkcs1pad_verify; inst->alg.set_pub_key = pkcs1pad_set_pub_key; inst->alg.set_priv_key = pkcs1pad_set_priv_key; inst->alg.max_size = pkcs1pad_get_max_size; diff --git a/crypto/rsa.c b/crypto/rsa.c index 4167980c243d..42df7d0c915c 100644 --- a/crypto/rsa.c +++ b/crypto/rsa.c @@ -354,7 +354,7 @@ static struct akcipher_alg rsa = { .encrypt = rsa_enc, .decrypt = rsa_dec, .sign = rsa_sign, - .verify = rsa_verify, + .verify_rsa = rsa_verify, .set_priv_key = rsa_set_priv_key, .set_pub_key = rsa_set_pub_key, .max_size = rsa_max_size, diff --git a/crypto/testmgr.c b/crypto/testmgr.c index 1ce53f8058d2..ed59408bf30b 100644 --- a/crypto/testmgr.c +++ b/crypto/testmgr.c @@ -2303,7 +2303,7 @@ static int test_akcipher_one(struct crypto_akcipher *tfm, err = crypto_wait_req(vecs->siggen_sigver_test ? /* Run asymmetric signature verification */ - crypto_akcipher_verify(req) : + crypto_akcipher_verify(req, c, c_size) : /* Run asymmetric encrypt */ crypto_akcipher_encrypt(req), &wait); if (err) { @@ -2317,7 +2317,8 @@ static int test_akcipher_one(struct crypto_akcipher *tfm, goto free_all; } /* verify that encrypted message is equal to expected */ - if (memcmp(c, outbuf_enc, c_size)) { + if (!vecs->siggen_sigver_test && + memcmp(c, outbuf_enc, c_size)) { pr_err("alg: akcipher: %s test failed. Invalid output\n", op); hexdump(outbuf_enc, c_size); err = -EINVAL; diff --git a/drivers/crypto/caam/caampkc.c b/drivers/crypto/caam/caampkc.c index 77ab28a2811a..e06b4c9a89e2 100644 --- a/drivers/crypto/caam/caampkc.c +++ b/drivers/crypto/caam/caampkc.c @@ -995,7 +995,7 @@ static struct akcipher_alg caam_rsa = { .encrypt = caam_rsa_enc, .decrypt = caam_rsa_dec, .sign = caam_rsa_dec, - .verify = caam_rsa_enc, + .verify_rsa = caam_rsa_enc, .set_pub_key = caam_rsa_set_pub_key, .set_priv_key = caam_rsa_set_priv_key, .max_size = caam_rsa_max_size, diff --git a/drivers/crypto/ccp/ccp-crypto-rsa.c b/drivers/crypto/ccp/ccp-crypto-rsa.c index 05850dfd7940..fc669b1bb328 100644 --- a/drivers/crypto/ccp/ccp-crypto-rsa.c +++ b/drivers/crypto/ccp/ccp-crypto-rsa.c @@ -215,7 +215,7 @@ static struct akcipher_alg ccp_rsa_defaults = { .encrypt = ccp_rsa_encrypt, .decrypt = ccp_rsa_decrypt, .sign = ccp_rsa_decrypt, - .verify = ccp_rsa_encrypt, + .verify_rsa = ccp_rsa_encrypt, .set_pub_key = ccp_rsa_setpubkey, .set_priv_key = ccp_rsa_setprivkey, .max_size = ccp_rsa_maxsize, diff --git a/drivers/crypto/qat/qat_common/qat_asym_algs.c b/drivers/crypto/qat/qat_common/qat_asym_algs.c index 320e7854b4ee..a6c7ff572970 100644 --- a/drivers/crypto/qat/qat_common/qat_asym_algs.c +++ b/drivers/crypto/qat/qat_common/qat_asym_algs.c @@ -1301,7 +1301,7 @@ static struct akcipher_alg rsa = { .encrypt = qat_rsa_enc, .decrypt = qat_rsa_dec, .sign = qat_rsa_dec, - .verify = qat_rsa_enc, + .verify_rsa = qat_rsa_enc, .set_pub_key = qat_rsa_setpubkey, .set_priv_key = qat_rsa_setprivkey, .max_size = qat_rsa_max_size, diff --git a/include/crypto/akcipher.h b/include/crypto/akcipher.h index 2d690494568c..1bb8ba74b95d 100644 --- a/include/crypto/akcipher.h +++ b/include/crypto/akcipher.h @@ -55,10 +55,12 @@ struct crypto_akcipher { * algorithm. In case of error, where the dst_len was insufficient, * the req->dst_len will be updated to the size required for the * operation - * @verify: Function performs a sign operation as defined by public key + * @verify_rsa: Function performs a partial verify operation as defined by RSA * algorithm. In case of error, where the dst_len was insufficient, * the req->dst_len will be updated to the size required for the * operation + * @verify: Function performs a complete verify operation as defined by public + * key algorithm. Requires digest value as input parameter. * @encrypt: Function performs an encrypt operation as defined by public key * algorithm. In case of error, where the dst_len was insufficient, * the req->dst_len will be updated to the size required for the @@ -91,7 +93,9 @@ struct crypto_akcipher { */ struct akcipher_alg { int (*sign)(struct akcipher_request *req); - int (*verify)(struct akcipher_request *req); + int (*verify_rsa)(struct akcipher_request *req); + int (*verify)(struct akcipher_request *req, const u8 *digest, + unsigned int digest_len); int (*encrypt)(struct akcipher_request *req); int (*decrypt)(struct akcipher_request *req); int (*set_pub_key)(struct crypto_akcipher *tfm, const void *key, @@ -343,16 +347,15 @@ static inline int crypto_akcipher_sign(struct akcipher_request *req) } /** - * crypto_akcipher_verify() - Invoke public key verify operation + * crypto_akcipher_verify_rsa() - Invoke partial RSA verify operation * - * Function invokes the specific public key verify operation for a given - * public key algorithm + * Function invokes partial verify operation for a RSA algorithm * * @req: asymmetric key request * * Return: zero on success; error code in case of error */ -static inline int crypto_akcipher_verify(struct akcipher_request *req) +static inline int crypto_akcipher_verify_rsa(struct akcipher_request *req) { struct crypto_akcipher *tfm = crypto_akcipher_reqtfm(req); struct akcipher_alg *alg = crypto_akcipher_alg(tfm); @@ -360,7 +363,7 @@ static inline int crypto_akcipher_verify(struct akcipher_request *req) int ret; crypto_stats_get(calg); - ret = alg->verify(req); + ret = alg->verify_rsa(req); crypto_stats_akcipher_verify(ret, calg); return ret; } @@ -406,4 +409,8 @@ static inline int crypto_akcipher_set_priv_key(struct crypto_akcipher *tfm, return alg->set_priv_key(tfm, key, keylen); } + +int crypto_akcipher_verify(struct akcipher_request *req, + const unsigned char *digest, + unsigned int digest_len); #endif
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 <vt@altlinux.org> --- 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(-)