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@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,
>
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
