On Sat, Jan 15, 2022 at 08:08:12AM +0300, Vitaly Chikunov wrote:
Eric,
On Fri, Jan 14, 2022 at 12:19:37AM -0800, Eric Biggers wrote:
From: Eric Biggers ebiggers@google.com
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*.
It is unclear whether the resulting scheme has any useful security properties.
Fix this by correctly extracting the hash from the scatterlist.
Fixes: c7381b012872 ("crypto: akcipher - new verify API for public key algorithms") Cc: stable@vger.kernel.org # v5.2+ Signed-off-by: Eric Biggers ebiggers@google.com
crypto/rsa-pkcs1pad.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c index 1b3545781425..7b223adebabf 100644 --- a/crypto/rsa-pkcs1pad.c +++ b/crypto/rsa-pkcs1pad.c @@ -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);Reviewed-by: Vitaly Chikunov vt@altlinux.org
Reviewing this I noticed that while req->src_len is checked in pkcs1pad_verify() to be not shorter than ctx->key_size it's never checked to be not longer. Signatures longer than RSA modulus N (which is ctx->key_size) are still invalid (RFC8017 8.2.2). (So, assumption they are equal was in accord with the standard, but not with the current codebase.)
I suggest to add this check too while we at it.
There was such check before, but it was removed in a49de377e051 ("crypto: Add hash param to pkcs1pad") for an unknown reason:
- if (!ctx->key_size || req->src_len != ctx->key_size)
- if (!ctx->key_size || req->src_len < ctx->key_size) return -EINVAL;
Thanks,
Yes, after sending this out I was looking at the PKCS#1 v1.5 encoding specification, and I had noticed that too:
"1. Length checking: If the length of the signature S is not k octets, output 'invalid signature' and stop."
I agree that we should enforce that too, although it's curious that commit a49de377e051 removed that check. Hopefully that was just a mistake and not something that someone was actually relying on. I'll send a separate patch for that; I think it should be separate from this patch.
- Eric