[PATCH] crypto: authenc - cryptlen must be at least AAD len

[PATCH] crypto: authenc - cryptlen must be at least AAD len

[Stephan Müller]

With AF_ALG, AAD len and cryptlen can be set freely by unprivileged
user space. The cipher implementation must therefore validate the input
data for sanity. For AEAD ciphers, this implies that cryptlen must be
at least as large as AAD size.

This fixes a kernel crash that can be triggered via AF_ALG detected by
the fuzzing test implemented with libkcapi.

CC: <stable@vger.kernel.org>
CC: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 crypto/authenc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/crypto/authenc.c b/crypto/authenc.c
index 875470b0e026..21e202fc32c1 100644
--- a/crypto/authenc.c
+++ b/crypto/authenc.c
@@ -209,6 +209,9 @@ static int crypto_authenc_encrypt(struct aead_request *req)
 	struct scatterlist *src, *dst;
 	int err;
 
+	if (req->assoclen > cryptlen)
+		return -EINVAL;
+
 	src = scatterwalk_ffwd(areq_ctx->src, req->src, req->assoclen);
 	dst = src;
 
-- 
2.13.5

Re: [PATCH] crypto: authenc - cryptlen must be at least AAD len

[Stephan Müller]

Am Mittwoch, 6. September 2017, 21:22:44 CEST schrieb Stephan M�ller:

Hi Herbert,

> With AF_ALG, AAD len and cryptlen can be set freely by unprivileged
> user space. The cipher implementation must therefore validate the input
> data for sanity. For AEAD ciphers, this implies that cryptlen must be
> at least as large as AAD size.
> 
> This fixes a kernel crash that can be triggered via AF_ALG detected by
> the fuzzing test implemented with libkcapi.

What is your opinion: should this check be rather added to crypto_aead_encrypt 
(similar to a sanity check found in crypto_aead_decrypt)?

Ciao
Stephan

Re: [PATCH] crypto: authenc - cryptlen must be at least AAD len

[Herbert Xu]

On Wed, Sep 06, 2017 at 10:10:08PM +0200, Stephan M�ller wrote:
> Am Mittwoch, 6. September 2017, 21:22:44 CEST schrieb Stephan M�ller:
> 
> Hi Herbert,
> 
> > With AF_ALG, AAD len and cryptlen can be set freely by unprivileged
> > user space. The cipher implementation must therefore validate the input
> > data for sanity. For AEAD ciphers, this implies that cryptlen must be
> > at least as large as AAD size.
> > 
> > This fixes a kernel crash that can be triggered via AF_ALG detected by
> > the fuzzing test implemented with libkcapi.
> 
> What is your opinion: should this check be rather added to crypto_aead_encrypt 
> (similar to a sanity check found in crypto_aead_decrypt)?

Doesn't this apply to decryption as well? Perhaps we can simply
truncate assoclen in aead_request_set_ad.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: authenc - cryptlen must be at least AAD len

[Stephan Müller]

Am Donnerstag, 7. September 2017, 05:54:05 CEST schrieb Herbert Xu:

Hi Herbert,
> > 
> > What is your opinion: should this check be rather added to
> > crypto_aead_encrypt (similar to a sanity check found in
> > crypto_aead_decrypt)?
> 
> Doesn't this apply to decryption as well?

There is already such check:

static inline int crypto_aead_decrypt(struct aead_request *req)
{
        struct crypto_aead *aead = crypto_aead_reqtfm(req);

        if (req->cryptlen < crypto_aead_authsize(aead))
                return -EINVAL;
...

> Perhaps we can simply
> truncate assoclen in aead_request_set_ad.

I am not sure that would work because at the time we set the AAD len, we may 
not yet have cryptlen. I.e. aead_request_set_ad may be called before 
aead_request_set_crypt.


Ciao
Stephan

Re: [PATCH] crypto: authenc - cryptlen must be at least AAD len

[Herbert Xu]

On Thu, Sep 07, 2017 at 07:48:53AM +0200, Stephan M�ller wrote:
>
> There is already such check:
> 
> static inline int crypto_aead_decrypt(struct aead_request *req)
> {
>         struct crypto_aead *aead = crypto_aead_reqtfm(req);
> 
>         if (req->cryptlen < crypto_aead_authsize(aead))
>                 return -EINVAL;
> ...

That doesn't check assoclen, does it?

> > Perhaps we can simply
> > truncate assoclen in aead_request_set_ad.
> 
> I am not sure that would work because at the time we set the AAD len, we may 
> not yet have cryptlen. I.e. aead_request_set_ad may be called before 
> aead_request_set_crypt.

We can add the truncation in both places.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: authenc - cryptlen must be at least AAD len

[Stephan Mueller]

Am Donnerstag, 7. September 2017, 08:01:08 CEST schrieb Herbert Xu:

Hi Herbert,

> On Thu, Sep 07, 2017 at 07:48:53AM +0200, Stephan M�ller wrote:
> > There is already such check:
> > 
> > static inline int crypto_aead_decrypt(struct aead_request *req)
> > {
> > 
> >         struct crypto_aead *aead = crypto_aead_reqtfm(req);
> >         
> >         if (req->cryptlen < crypto_aead_authsize(aead))
> >         
> >                 return -EINVAL;
> > 
> > ...
> 
> That doesn't check assoclen, does it?

Right, I mixed up the tag and the AAD, sorry for that.
> 
> > > Perhaps we can simply
> > > truncate assoclen in aead_request_set_ad.
> > 
> > I am not sure that would work because at the time we set the AAD len, we
> > may not yet have cryptlen. I.e. aead_request_set_ad may be called before
> > aead_request_set_crypt.
> 
> We can add the truncation in both places.

I sill send a new patch -- shall I first send it excluding stable so that we 
can review it before bothering the stable folks?
> 
> Cheers,



Ciao
Stephan

Re: [PATCH] crypto: authenc - cryptlen must be at least AAD len

[Herbert Xu]

On Thu, Sep 07, 2017 at 08:04:25AM +0200, Stephan Mueller wrote:
>
> I sill send a new patch -- shall I first send it excluding stable so that we 
> can review it before bothering the stable folks?

You don't need to cc the email to stable@vger.kernel.org, just add
a Cc: tag in the patch description and it'll automatically go to
stable when it hits mainline.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt