| Message ID | 20220114081939.218416-2-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | crypto: more rsa-pkcs1pad fixes | expand |
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, > /* Do the actual verification step. */ > if (memcmp(req_ctx->out_buf + ctx->key_size, out_buf + pos, > req->dst_len) != 0) > -- > 2.34.1
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
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); /* Do the actual verification step. */ if (memcmp(req_ctx->out_buf + ctx->key_size, out_buf + pos, req->dst_len) != 0)