Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 14 Jan 2022 21:47:59 -0800
From:   Eric Biggers <ebiggers@kernel.org>
To:     Vitaly Chikunov <vt@altlinux.org>
Cc:     linux-crypto@vger.kernel.org,
        Herbert Xu <herbert@gondor.apana.org.au>,
        keyrings@vger.kernel.org, Denis Kenzior <denkenz@gmail.com>,
        stable@vger.kernel.org
Subject: Re: [PATCH 1/3] crypto: rsa-pkcs1pad - correctly get hash from
 source scatterlist
Message-ID: <YeJgDzSsSepEio6P@sol.localdomain>
References: <20220114081939.218416-1-ebiggers@kernel.org>
 <20220114081939.218416-2-ebiggers@kernel.org>
 <20220115050812.q7o5ij7c3jhloru7@altlinux.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220115050812.q7o5ij7c3jhloru7@altlinux.org>
Precedence: bulk

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