crypto: authenc - cryptlen must be at least AAD len

PATCH In review Stephan Müller 2 participants 9 messages Sep 6, 2017 – Sep 8, 2017

Subsystems (2)

Stable branch Crypto API

lore

Share & tools

lore

Stephan Müller

author

2017-09-06 19:22 UTC #0 lore raw

PATCH 1/1 crypto: authenc - cryptlen must be at least AAD len

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 <[email protected]>

CC Herbert Xu <[email protected]>

Signed-off-by Stephan Mueller <[email protected]>

---
 crypto/authenc.c | 3 +++
 1 file changed, 3 insertions(+)

crypto/authenc.c +3 -0

index 875470b0e026..21e202fc32c1 100644
@@ -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;

Signature · 1 line

2.13.5

Stephan Müller

author

2017-09-06 20:10 UTC #1 lore raw

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

Hi Herbert,

Quoted 7 lines

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

Herbert Xu

maintainer

2017-09-07 03:54 UTC #2 lore raw

Quoted 9 lines · Stephan Müller

Am Mittwoch, 6. September 2017, 21:22:44 CEST schrieb Stephan Müller:
Hi Herbert,
Quoted 5 lines
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.

Quoted 5 lines

Quoted 2 lines
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,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Stephan Müller

author

2017-09-07 05:48 UTC #3 lore raw

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

Hi Herbert,

Quoted 6 lines

Quoted 4 lines
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;

...

Quoted 2 lines

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

Herbert Xu

maintainer

2017-09-07 06:01 UTC #4 lore raw

Quoted 10 lines · Stephan Müller

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?

Quoted 6 lines

Quoted 2 lines
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,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Stephan Mueller

author

2017-09-07 06:04 UTC #5 lore raw

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

Hi Herbert,

Quoted 8 lines · Stephan Müller

Quoted 7 lines
There is already such check:
static inline int crypto_aead_decrypt(struct aead_request *req) {
        struct crypto_aead *aead = crypto_aead_reqtfm(req);

Quoted 7 lines

Quoted 5 lines
        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.

Quoted 9 lines

Quoted 7 lines
Quoted 3 lines
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?

Quoted 2 lines

Cheers,

Ciao
Stephan

Herbert Xu

maintainer

2017-09-07 06:09 UTC #6 lore raw

Quoted 3 lines · Stephan Mueller

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 [email protected], just add a Cc: tag in the patch description and it'll automatically go to stable when it hits mainline.

Cheers,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Stephan Müller

author

2017-09-08 22:20 UTC #7 lore raw

PATCH v2 1/1 crypto: add NULL check to scatterwalk_start

In reply to Herbert Xu

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

Hi Herbert,

Quoted 16 lines · Stephan Müller

Quoted 5 lines
There is already such check:
static inline int crypto_aead_decrypt(struct aead_request *req) {
Quoted 7 lines
        struct crypto_aead *aead = crypto_aead_reqtfm(req);
        
        if (req->cryptlen < crypto_aead_authsize(aead))
        
                return -EINVAL;
...
That doesn't check assoclen, does it?

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
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 <[email protected]>

CC Herbert Xu <[email protected]>

Signed-off-by Stephan Mueller <[email protected]>

---
 include/crypto/scatterwalk.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

include/crypto/scatterwalk.h +4 -1

index 880e6be9e95e..0605d44b53bc 100644
@@ -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)

Signature · 1 line

2.13.5

Stephan Müller

author

2017-09-08 23:01 UTC #8 lore raw

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

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

Hi Herbert,

include/crypto/scatterwalk.h static inline void scatterwalk_start(struct scatter_walk *walk, · line 90

Quoted 7 lines

include/crypto/scatterwalk.h +4 -1

 	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