Re: [RFC PATCH v3] akcipher: Introduce verify_rsa/verify for public key algorithms
On Fri, Jan 25, 2019 at 02:18:09PM +0300, Vitaly Chikunov wrote: > > Well, if we allowed to reuse dst* fields why not just put digest over > dst scatterlist? That would be much simpler. Please see below. > > The advantage of having it in one scatterlist is that for those > > users that already have the two pieces together could simply provide > > a single SG element (I don't know how likely that is in practice > > though). > > > > For others you would simply tack on an extra SG element. 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 v3] akcipher: Introduce verify_rsa/verify for public key algorithms
On Fri, Jan 25, 2019 at 06:09:29PM +0800, Herbert Xu wrote: > On Fri, Jan 18, 2019 at 11:58:46PM +0300, Vitaly Chikunov wrote: > > 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 sufficient. > > > > Make improved .verify() call which gets hash value as parameter and > > produce complete signature check without any output besides status. > > > > RSA-centric drivers have replaced verify() with verify_rsa() which > > have old semantic and which they still should implement (if they want > > pkcs1pad to work). 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. > > > > For pkcs1pad crypto_akcipher_verify_rsa() is introduced which directly > > calls .verify_rsa() for its backend. Without this api PKCS1 can not be > > implemented. > > > > Tested on x86_64. > > > > Signed-off-by: Vitaly Chikunov > > Thanks for working on this! > > We have been here before. We changed the AEAD interface in a way > that is not dissimilar to what you want to do here. > > So I think the basic plan should be: While time goes, I got another simpler idea how we should settle this: 1. Since we are know that by nature RSA sign/verify is just encrypt/decrypt, and since sign/verify calls should not be used directly by any normal users: - Remove sign/verify calls from all existing RSA backends. 2. pkcs1pad should use encrypt/decrypt API for its low level purposes (instead of sign/verify API, which now would be not implemented by them), and provide sign (same as before) and new verify (returning only status). Thus, we avoid verify_rsa call altogether while remaining its functionality, and skipping conversion step. > 1) Implement new top-level verify, alongside existing verify_rsa. > 2) For existing drivers implement a wrapper over verify_rsa. > 3) Convert *all* existing users to the new verify API. > 4) Remove top-level verify_rsa API. > 5) Convert existing drivers over to verify API. > 6) Remove verify_rsa completely. > > > +int crypto_akcipher_verify(struct akcipher_request *req, > > + const unsigned char *digest, unsigned int digest_len) > > We should not add new fields outside of akcipher_request. So > these fields need to go inside it. However, I think we don't > actually need two new fields. They could both go into the src > scatterlist. All we need is a new field, say textlen to indicate > where the text stops and where the hash starts. We could also > overlay textlen over dstlen as it would now be unused for verify. Well, if we allowed to reuse dst* fields why not just put digest over dst scatterlist? That would be much simpler. Thanks, > The advantage of having it in one scatterlist is that for those > users that already have the two pieces together could simply provide > a single SG element (I don't know how likely that is in practice > though). > > For others you would simply tack on an extra SG element. > > Thanks, > -- > Email: Herbert Xu > Home Page: http://gondor.apana.org.au/~herbert/ > PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [RFC PATCH v3] akcipher: Introduce verify_rsa/verify for public key algorithms
On Fri, Jan 18, 2019 at 11:58:46PM +0300, Vitaly Chikunov wrote: > 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 sufficient. > > Make improved .verify() call which gets hash value as parameter and > produce complete signature check without any output besides status. > > RSA-centric drivers have replaced verify() with verify_rsa() which > have old semantic and which they still should implement (if they want > pkcs1pad to work). 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. > > For pkcs1pad crypto_akcipher_verify_rsa() is introduced which directly > calls .verify_rsa() for its backend. Without this api PKCS1 can not be > implemented. > > Tested on x86_64. > > Signed-off-by: Vitaly Chikunov Thanks for working on this! We have been here before. We changed the AEAD interface in a way that is not dissimilar to what you want to do here. So I think the basic plan should be: 1) Implement new top-level verify, alongside existing verify_rsa. 2) For existing drivers implement a wrapper over verify_rsa. 3) Convert *all* existing users to the new verify API. 4) Remove top-level verify_rsa API. 5) Convert existing drivers over to verify API. 6) Remove verify_rsa completely. > +int crypto_akcipher_verify(struct akcipher_request *req, > +const unsigned char *digest, unsigned int digest_len) We should not add new fields outside of akcipher_request. So these fields need to go inside it. However, I think we don't actually need two new fields. They could both go into the src scatterlist. All we need is a new field, say textlen to indicate where the text stops and where the hash starts. We could also overlay textlen over dstlen as it would now be unused for verify. The advantage of having it in one scatterlist is that for those users that already have the two pieces together could simply provide a single SG element (I don't know how likely that is in practice though). For others you would simply tack on an extra SG element. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[RFC PATCH v3] akcipher: Introduce verify_rsa/verify for public key algorithms
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 sufficient.
Make improved .verify() call which gets hash value as parameter and
produce complete signature check without any output besides status.
RSA-centric drivers have replaced verify() with verify_rsa() which
have old semantic and which they still should implement (if they want
pkcs1pad to work). 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.
For pkcs1pad crypto_akcipher_verify_rsa() is introduced which directly
calls .verify_rsa() for its backend. Without this api PKCS1 can not be
implemented.
Tested on x86_64.
Signed-off-by: Vitaly Chikunov
---
This should be applied over cryptodev tree.
Changes since v2:
- `output` is factored out from public_key_verify_signature() into
crypto_akcipher_verify().
- in crypto_akcipher_verify_rsa() -ENOSYS error is added for robustness (if,
in the future, some RSA driver will not implement this api).
- api descriptions are updated to be more clear.
Changes since v1:
- complete rework to the different approach and should be treated as
a new patch.
crypto/akcipher.c | 60 +++
crypto/asymmetric_keys/public_key.c | 32 ++
crypto/rsa-pkcs1pad.c | 4 +-
crypto/rsa.c | 2 +-
crypto/testmgr.c | 43 ++-
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 | 34 ++-
9 files changed, 117 insertions(+), 64 deletions(-)
diff --git a/crypto/akcipher.c b/crypto/akcipher.c
index 0cbeae137e0a..95f207b2eb12 100644
--- a/crypto/akcipher.c
+++ b/crypto/akcipher.c
@@ -25,6 +25,66 @@
#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) ||
+ WARN_ON(req->dst || req->dst_len))
+ return -EINVAL;
+
+ crypto_stats_get(calg);
+ if (alg->verify_rsa) {
+ struct scatterlist output_sg;
+ void *output;
+ unsigned int outlen;
+
+ outlen = crypto_akcipher_maxsize(tfm);
+ ret = -EKEYREJECTED;
+ if (WARN_ON(outlen < digest_len))
+ goto out;
+ ret = -ENOMEM;
+ output = kmalloc(outlen, GFP_KERNEL);
+ if (!output)
+ goto out;
+ sg_init_one(_sg, output, outlen);
+ akcipher_request_set_crypt(req, req->src, _sg,
+ req->src_len, outlen);
+
+ /* 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 &&
+ (req->dst_len != digest_len ||
+memcmp(digest, output, digest_len)))
+ ret = -EKEYREJECTED;
+ kfree(output);
+ } 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
