crypto: rsa-pkcs1pad fixes
Subsystems (4)
v2 cover crypto: rsa-pkcs1pad fixes
This series fixes some bugs in rsa-pkcs1pad.
Changed v1 => v2:
- Added patch "crypto: rsa-pkcs1pad - only allow with rsa" (previously was a standalone patch)
- Added patch "crypto: rsa-pkcs1pad - restore signature length check"
Eric Biggers (5):
crypto: rsa-pkcs1pad - only allow with rsa
crypto: rsa-pkcs1pad - correctly get hash from source scatterlist
crypto: rsa-pkcs1pad - restore signature length check
crypto: rsa-pkcs1pad - fix buffer overread in
pkcs1pad_verify_complete()
crypto: rsa-pkcs1pad - use clearer variable names
crypto/rsa-pkcs1pad.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-)
Signature · 1 line
2.34.1
PATCH v2 1/5 crypto: rsa-pkcs1pad - only allow with rsa
From: Eric Biggers <[email protected]>
The pkcs1pad template can be instantiated with an arbitrary akcipher algorithm, which doesn't make sense; it is specifically an RSA padding scheme. Make it check that the underlying algorithm really is RSA.
--- crypto/rsa-pkcs1pad.c | 5 +++++ 1 file changed, 5 insertions(+)
crypto/rsa-pkcs1pad.c +5 -0
@@ -621,6 +621,11 @@ static int pkcs1pad_create(struct crypto_template *tmpl, struct rtattr **tb) rsa_alg =crypto_spawn_akcipher_alg (&ctx->spawn); +if (strcmp (rsa_alg->base.cra_name,"rsa" ) !=0 ) { + err = -EINVAL ; + goto err_free_inst; + } + err = -ENAMETOOLONG ; hash_name =crypto_attr_alg_name (tb[2 ]);if (IS_ERR (hash_name)) {
Signature · 1 line
2.34.1
PATCH v2 2/5 crypto: rsa-pkcs1pad - correctly get hash from source scatterlist
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
Commit c7381b012872 ("crypto: akcipher - new verify API for public key algorithms") changed akcipher_alg::verify to take in both the signature and the actual hash and do the signature verification, rather than just return the hash expected by the signature as was the case before. To do this, it implemented a hack where the signature and hash are concatenated with each other in one scatterlist.
Obviously, for this to work correctly, akcipher_alg::verify needs to correctly extract the two items from the scatterlist it is given. Unfortunately, it doesn't correctly extract the hash in the case where the signature is longer than the RSA key size, as it assumes that the signature's length is equal to the RSA key size. This causes a prefix of the hash, or even the entire hash, to be taken from the *signature*.
(Note, the case of a signature longer than the RSA key size should not be allowed in the first place; a separate patch will fix that.)
It is unclear whether the resulting scheme has any useful security properties.
Fix this by correctly extracting the hash from the scatterlist.
--- crypto/rsa-pkcs1pad.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
crypto/rsa-pkcs1pad.c +1 -1
@@ -495,7 +495,7 @@ static int pkcs1pad_verify_complete(struct akcipher_request *req, int err)sg_nents_for_len (req->src, req->src_len + req->dst_len), req_ctx->out_buf + ctx->key_size, - req->dst_len, ctx->key_size); + req->dst_len, req->src_len);/* Do the actual verification step. */ if (memcmp (req_ctx->out_buf + ctx->key_size, out_buf + pos, req->dst_len) !=0 )
Signature · 1 line
2.34.1
PATCH v2 3/5 crypto: rsa-pkcs1pad - restore signature length check
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
RSA PKCS#1 v1.5 signatures are required to be the same length as the RSA key size. RFC8017 specifically requires the verifier to check this
(https://datatracker.ietf.org/doc/html/rfc8017#section-8.2.2).
Commit a49de377e051 ("crypto: Add hash param to pkcs1pad") changed the kernel to allow longer signatures, but didn't explain this part of the change; it seems to be unrelated to the rest of the commit.
Revert this change, since it doesn't appear to be correct.
We can be pretty sure that no one is relying on overly-long signatures (which would have to be front-padded with zeroes) being supported, given that they would have been broken since commit c7381b012872 ("crypto: akcipher - new verify API for public key algorithms").
--- crypto/rsa-pkcs1pad.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
crypto/rsa-pkcs1pad.c +1 -1
@@ -538,7 +538,7 @@ static int pkcs1pad_verify(struct akcipher_request *req)if (WARN_ON (req->dst) ||WARN_ON (!req->dst_len) || - !ctx->key_size || req->src_len < ctx->key_size) + !ctx->key_size || req->src_len != ctx->key_size)return -EINVAL ; req_ctx->out_buf =kmalloc (ctx->key_size + req->dst_len,GFP_KERNEL );
Signature · 1 line
2.34.1
PATCH v2 4/5 crypto: rsa-pkcs1pad - fix buffer overread in pkcs1pad_verify_complete()
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
Before checking whether the expected digest_info is present, we need to check that there are enough bytes remaining.
--- crypto/rsa-pkcs1pad.c | 2 ++ 1 file changed, 2 insertions(+)
crypto/rsa-pkcs1pad.c +2 -0
@@ -476,6 +476,8 @@ static int pkcs1pad_verify_complete(struct akcipher_request *req, int err) pos++;if (digest_info) { +if (digest_info->size > dst_len - pos) + goto done;if (crypto_memneq (out_buf + pos, digest_info->data, digest_info->size)) goto done;
Signature · 1 line
2.34.1
PATCH v2 5/5 crypto: rsa-pkcs1pad - use clearer variable names
In reply to Eric BiggersFrom: Eric Biggers <[email protected]>
The new convention for akcipher_alg::verify makes it unclear which values are the lengths of the signature and digest. Add local variables to make it clearer what is going on.
Also rename the digest_size variable in pkcs1pad_sign(), as it is actually the digest *info* size, not the digest size which is different.
--- crypto/rsa-pkcs1pad.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)
crypto/rsa-pkcs1pad.c +16 -15
@@ -385,15 +385,15 @@ static int pkcs1pad_sign(struct akcipher_request *req)struct pkcs1pad_inst_ctx *ictx =akcipher_instance_ctx (inst);const struct rsa_asn1_template *digest_info = ictx->digest_info;int err; -unsigned int ps_end, digest_size =0 ; +unsigned int ps_end, digest_info_size =0 ;if (!ctx->key_size)return -EINVAL ;if (digest_info) - digest_size = digest_info->size; + digest_info_size = digest_info->size; -if (req->src_len + digest_size > ctx->key_size -11 ) +if (req->src_len + digest_info_size > ctx->key_size -11 )return -EOVERFLOW ;if (req->dst_len < ctx->key_size) {
@@ -406,7 +406,7 @@ static int pkcs1pad_sign(struct akcipher_request *req)if (!req_ctx->in_buf)return -ENOMEM ; - ps_end = ctx->key_size - digest_size - req->src_len -2 ; + ps_end = ctx->key_size - digest_info_size - req->src_len -2 ; req_ctx->in_buf[0 ] =0x01 ;memset (req_ctx->in_buf +1 ,0xff , ps_end -1 ); req_ctx->in_buf[ps_end] =0x00 ;
@@ -441,6 +441,8 @@ static int pkcs1pad_verify_complete(struct akcipher_request *req, int err)struct akcipher_instance *inst =akcipher_alg_instance (tfm);struct pkcs1pad_inst_ctx *ictx =akcipher_instance_ctx (inst);const struct rsa_asn1_template *digest_info = ictx->digest_info; +const unsigned int sig_size = req->src_len; +const unsigned int digest_size = req->dst_len;unsigned int dst_len;unsigned int pos;u8 *out_buf;
@@ -487,20 +489,19 @@ static int pkcs1pad_verify_complete(struct akcipher_request *req, int err) err =0 ; -if (req->dst_len != dst_len - pos) { +if (digest_size != dst_len - pos) { err = -EKEYREJECTED ; req->dst_len = dst_len - pos; goto done; }/* Extract appended digest. */ sg_pcopy_to_buffer (req->src, -sg_nents_for_len (req->src, - req->src_len + req->dst_len), +sg_nents_for_len (req->src, sig_size + digest_size), req_ctx->out_buf + ctx->key_size, - req->dst_len, req->src_len); + digest_size, sig_size);/* Do the actual verification step. */ if (memcmp (req_ctx->out_buf + ctx->key_size, out_buf + pos, - req->dst_len) !=0 ) + digest_size) !=0 ) err = -EKEYREJECTED ; done:kfree_sensitive (req_ctx->out_buf);
@@ -536,14 +537,15 @@ static int pkcs1pad_verify(struct akcipher_request *req)struct crypto_akcipher *tfm =crypto_akcipher_reqtfm (req);struct pkcs1pad_ctx *ctx =akcipher_tfm_ctx (tfm);struct pkcs1pad_request *req_ctx =akcipher_request_ctx (req); +const unsigned int sig_size = req->src_len; +const unsigned int digest_size = req->dst_len;int err; -if (WARN_ON (req->dst) || -WARN_ON (!req->dst_len) || - !ctx->key_size || req->src_len != ctx->key_size) +if (WARN_ON (req->dst) ||WARN_ON (!digest_size) || + !ctx->key_size || sig_size != ctx->key_size)return -EINVAL ; - req_ctx->out_buf =kmalloc (ctx->key_size + req->dst_len,GFP_KERNEL ); + req_ctx->out_buf =kmalloc (ctx->key_size + digest_size,GFP_KERNEL );if (!req_ctx->out_buf)return -ENOMEM ;
@@ -556,8 +558,7 @@ static int pkcs1pad_verify(struct akcipher_request *req)/* Reuse input buffer, output to a new buffer */ akcipher_request_set_crypt (&req_ctx->child_req, req->src, - req_ctx->out_sg, req->src_len, - ctx->key_size); + req_ctx->out_sg, sig_size, ctx->key_size); err =crypto_akcipher_encrypt (&req_ctx->child_req);if (err != -EINPROGRESS && err != -EBUSY )
Signature · 1 line
2.34.1
Herbert Xu
maintainerQuoted 15 lines · Eric Biggers
This series fixes some bugs in rsa-pkcs1pad.
Changed v1 => v2:
- Added patch "crypto: rsa-pkcs1pad - only allow with rsa" (previously was a standalone patch)
- Added patch "crypto: rsa-pkcs1pad - restore signature length check"Eric Biggers (5):
crypto: rsa-pkcs1pad - only allow with rsa
crypto: rsa-pkcs1pad - correctly get hash from source scatterlist
crypto: rsa-pkcs1pad - restore signature length check
crypto: rsa-pkcs1pad - fix buffer overread in
pkcs1pad_verify_complete()
crypto: rsa-pkcs1pad - use clearer variable names
Quoted 2 lines
crypto/rsa-pkcs1pad.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-)
All applied. Thanks.
Signature · 3 lines
Email: Herbert Xu <[email protected]> Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt