[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

[PATCH v2] crypto: add NULL check to scatterwalk_start

[Stephan Müller]

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?
> 
> > > 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.

The initially suggested fix was wrong in general: cryptlen only defines the length of the ciphertext/plaintext without the AAD. This means, cryptlen can surely be less than AAD.

The culprit is that in case authenc() is invoked from user space with AAD but with a zero plaintext. This in turn caused authenc() to invoke the CBC implementation with that zero plaintext which in turn simply starts a scatterwalk operation on the plaintext. This is in general appropriate as all code will handle zero lengths, except scatterwalk_start. This function assumes that there is always a valid SGL which is not true for for zero length input data.

Granted, we could fix authenc to simply not invoke the CBC operation in the outlined issue. However, I now stumbled over this function for a third time in a row over the last weeks in bugs triggerable via AF_ALG. I suspect that there are many more issues like this lingering in other cipher implementation. Hence, I feel it is prudent to fix an entire class of bugs with this patch.

Ciao
Stephan

---8<---

In edge conditions, scatterwalk_start is invoked with an empty SGL.
Although this is considered a wrong usage of scatterwalk_start, it can
still be invoked. Such invocation can occur if the data to be covered by
the SGL is zero. For example, if the authenc() cipher is invoked with an
empty plaintext, the CBC operation is invoked with an empty plaintext.

This patch fixes (at least) a crash that can be induced via AF_ALG from
unprivileged user space.

It can be argued whether authenc() should be changed to catch this
issue. Yet, this issue in scatterwalk_start was a culprit in other
kernel crash issues that have been fixed before invoking
scatterwalk_start. As this function is constantly being invoked via
AF_ALG from user space, harden the function to avoid a NULL pointer
deference is prudent and even a general fix for these common issues.
This fix therefore covers an entire class of bugs which are hard to
chase down in their own individual cipher implementations.

Fixes: ac02725812cb3 ("crypto: scatterwalk - Inline start/map/done")
CC: <stable@vger.kernel.org>
CC: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Stephan Mueller <smueller@chronox.de>
---
 include/crypto/scatterwalk.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/crypto/scatterwalk.h b/include/crypto/scatterwalk.h
index 880e6be9e95e..0605d44b53bc 100644
--- a/include/crypto/scatterwalk.h
+++ b/include/crypto/scatterwalk.h
@@ -83,7 +83,10 @@ static inline void scatterwalk_start(struct scatter_walk *walk,
 				     struct scatterlist *sg)
 {
 	walk->sg = sg;
-	walk->offset = sg->offset;
+	if (sg)
+		walk->offset = sg->offset;
+	else
+		walk->offset = 0;
 }
 
 static inline void *scatterwalk_map(struct scatter_walk *walk)
-- 
2.13.5

Re: [PATCH v2] crypto: add NULL check to scatterwalk_start

[Stephan Müller]

Am Samstag, 9. September 2017, 00:20:50 CEST schrieb Stephan Müller:

Hi Herbert,

>  	walk->sg = sg;
> -	walk->offset = sg->offset;
> +	if (sg)
> +		walk->offset = sg->offset;
> +	else
> +		walk->offset = 0;
>  }

After running more fuzzing tests, I now cause other types of spurious crashes. 
Do you have any suggestion on how to handle that issue?

Changing skcipher_walk_skcipher with the following instead of the previously 
suggested patch does not help.

	if (!req->cryptlen)
		return 0;

Or do you see authenc() as a special case that does not support zero length 
plaintext/ciphertext?

[ 5420.521073] ------------[ cut here ]------------
[ 5420.521770] kernel BUG at ./include/linux/scatterlist.h:123!
[ 5420.522736] invalid opcode: 0000 [#1] SMP
[ 5420.523723] Modules linked in: ansi_cprng algif_rng ccm algif_skcipher 
des3_ede_x86_64 des_generic algif_hash crypto_user authenc algif_aead af_alg 
ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6 nf_defrag_ipv6 
nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ip_set nfnetlink 
ebtable_nat ebtable_broute bridge stp llc ip6table_mangle ip6table_raw 
ip6table_security iptable_mangle iptable_raw iptable_security ebtable_filter 
ebtables ip6table_filter ip6_tables crct10dif_pclmul crc32_pclmul 
virtio_balloon ghash_clmulni_intel pcspkr i2c_piix4 virtio_net sch_fq_codel 
virtio_console virtio_blk crc32c_intel virtio_pci virtio_ring serio_raw virtio
[ 5420.523723] CPU: 3 PID: 20541 Comm: kcapi Not tainted 4.13.0-rc1+ #483
[ 5420.523723] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.10.2-1.fc26 04/01/2014
[ 5420.523723] task: ffffa384b9ca6800 task.stack: ffffa512c3a9c000
[ 5420.523723] RIP: 0010:shash_ahash_digest+0xc9/0xd0
[ 5420.523723] RSP: 0018:ffffa512c3a9fc38 EFLAGS: 00010286
[ 5420.523723] RAX: 0000000087654321 RBX: ffffa38475fb44b8 RCX: 
ffffa38475fb4010
[ 5420.523723] RDX: 0000000000000000 RSI: ffffa38475fb4508 RDI: 
0000000075fb4088
[ 5420.523723] RBP: ffffa512c3a9fc58 R08: 00000000000147d6 R09: 
0000000000000007
[ 5420.523723] R10: ffffa512c3a9fcb8 R11: ffffffff8211c14d R12: 
ffffa38475fb4508
[ 5420.523723] R13: ffffa384b7e88188 R14: ffffa384b9b98600 R15: 
ffffa38475fb4010
[ 5420.523723] FS:  00007f7b9f535700(0000) GS:ffffa384bfd80000(0000) knlGS:
0000000000000000
[ 5420.523723] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 5420.523723] CR2: 0000000000000000 CR3: 000000007a8ef000 CR4: 
00000000003406e0
[ 5420.523723] Call Trace:
[ 5420.523723]  ? shash_ahash_digest+0xd0/0xd0
[ 5420.523723]  shash_async_digest+0x24/0x30
[ 5420.523723]  crypto_ahash_op+0x29/0x70
[ 5420.523723]  ? printk+0x43/0x4b
[ 5420.523723]  crypto_ahash_digest+0x16/0x20
[ 5420.523723]  crypto_authenc_genicv+0x7b/0xb0 [authenc]
[ 5420.523723]  ? simd_skcipher_encrypt+0xb7/0xc0
[ 5420.523723]  crypto_authenc_encrypt+0xb8/0x180 [authenc]
[ 5420.523723]  aead_recvmsg+0x510/0x5c0 [algif_aead]
[ 5420.523723]  sock_recvmsg+0x3d/0x50
[ 5420.523723]  sock_read_iter+0x86/0xc0
[ 5420.523723]  __vfs_read+0xcb/0x120
[ 5420.523723]  vfs_read+0x8e/0x130
[ 5420.523723]  SyS_read+0x46/0xa0
[ 5420.523723]  do_syscall_64+0x5b/0xc0
[ 5420.523723]  entry_SYSCALL64_slow_path+0x25/0x25
[ 5420.523723] RIP: 0033:0x7f7b9ee43180
[ 5420.523723] RSP: 002b:00007ffd3f975718 EFLAGS: 00000246 ORIG_RAX: 
0000000000000000
[ 5420.523723] RAX: ffffffffffffffda RBX: 0000000000001000 RCX: 
00007f7b9ee43180
[ 5420.523723] RDX: 0000000000001000 RSI: 00007ffd3f976820 RDI: 
0000000000000006
[ 5420.523723] RBP: 0000000000fa701c R08: 0000000000000000 R09: 
0000000000000000
[ 5420.523723] R10: 0000000000000000 R11: 0000000000000246 R12: 
0000000000000000
[ 5420.523723] R13: 00007ffd3f976820 R14: 00007ffd3f976820 R15: 
00007ffd3f975820
[ 5420.523723] Code: 03 35 d4 9a a4 00 48 01 fe 4c 89 e7 e8 71 fa ff ff 41 89 
c5 41 83 ae 80 08 00 00 01 41 f6 44 24 09 02 74 92 e8 c9 c3 41 00 eb 8b <0f> 
0b 0f 0b 0f 1f 00 0f 1f 44 00 00 48 8b 47 20 55 48 8d 77 50 
[ 5420.523723] RIP: shash_ahash_digest+0xc9/0xd0 RSP: ffffa512c3a9fc38



Ciao
Stephan