Eric,
On Fri, Jan 14, 2022 at 12:19:37AM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx> # v5.2+
> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
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,
> /* Do the actual verification step. */
> if (memcmp(req_ctx->out_buf + ctx->key_size, out_buf + pos,
> req->dst_len) != 0)
> --
> 2.34.1
