KASAN: use-after-free Read in aead_recvmsg

Commit Message

Eric Biggers Nov. 28, 2017, 7:29 a.m. UTC

On Tue, Nov 28, 2017 at 07:30:46AM +0100, Stephan Mueller wrote:
> Am Montag, 27. November 2017, 23:43:08 CET schrieb Eric Biggers:
> 
> Hi Eric,
> 
> > No, that doesn't help.  I tested v4.15-rc1 with all the extra commits from
> > crypto-2.6.git/master applied:
> > 
> > 	crypto: algif_aead - skip SGL entries with NULL page
> > 	crypto: af_alg - remove locking in async callback
> > 	crypto: skcipher - Fix skcipher_walk_aead_common
> > 
> > Did you use the .config the bot provided?  It's possible the bug is only
> > noticable with KASAN enabled.
> 
> Not so far, but the bug seemed to be there without my patch and then gone 
> after testing it with my patch. It seems not.
> 
> I will use your config then.
> 

Sometimes you have to reboot to get the reproducer to work, because the bug has
to do with referencing counting of the "null skcipher" which is a global
resource.  Here's a patch that fixes it, it seems:

---8<---

From 453b54793e843c0d5b8fd2d5e33fcc5427ec038e Mon Sep 17 00:00:00 2001
From: Eric Biggers <[e-mail]>
Date: Mon, 27 Nov 2017 23:23:05 -0800
Subject: [PATCH] crypto: algif_aead - fix reference counting of null skcipher

In the AEAD interface for AF_ALG, the reference to the "null skcipher"
held by each tfm was being dropped in the wrong place -- when each
af_alg_ctx was freed instead of when the aead_tfm was freed.  As
discovered by syzkaller, a specially crafted program could use this to
cause the null skcipher to be freed while it is still in use.

Fix it by dropping the reference in the right place.

Fixes: 72548b093ee3 ("crypto: algif_aead - copy AAD from src to dst")
Reported-by: syzbot <[e-mail]>
Cc: <[e-mail]> # v4.14+
Signed-off-by: Eric Biggers <[e-mail]>
---
 crypto/algif_aead.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stephan Mueller Nov. 28, 2017, 7:43 a.m. UTC | #1

Am Dienstag, 28. November 2017, 08:29:44 CET schrieb Eric Biggers:

Hi Eric,

> 
> Sometimes you have to reboot to get the reproducer to work, because the bug
> has to do with referencing counting of the "null skcipher" which is a
> global resource.  Here's a patch that fixes it, it seems:
> 
> ---8<---
> 
> From 453b54793e843c0d5b8fd2d5e33fcc5427ec038e Mon Sep 17 00:00:00 2001
> From: Eric Biggers <[e-mail]>
> Date: Mon, 27 Nov 2017 23:23:05 -0800
> Subject: [PATCH] crypto: algif_aead - fix reference counting of null
> skcipher
> 
> In the AEAD interface for AF_ALG, the reference to the "null skcipher"
> held by each tfm was being dropped in the wrong place -- when each
> af_alg_ctx was freed instead of when the aead_tfm was freed.  As
> discovered by syzkaller, a specially crafted program could use this to
> cause the null skcipher to be freed while it is still in use.
> 
> Fix it by dropping the reference in the right place.
> 
> Fixes: 72548b093ee3 ("crypto: algif_aead - copy AAD from src to dst")
> Reported-by: syzbot <[e-mail]>
> Cc: <[e-mail]> # v4.14+
> Signed-off-by: Eric Biggers <[e-mail]>

Yes, absolutely -- the null cipher is allocated together with the AEAD cipher 
and should be freed together with the AEAD cipher.

Thanks a lot.

Reviewed-by: Stephan Mueller <[e-mail]>

Ciao
Stephan

Herbert Xu Nov. 29, 2017, 5:23 a.m. UTC | #2

On Mon, Nov 27, 2017 at 11:29:44PM -0800, Eric Biggers wrote:
>
> >From 453b54793e843c0d5b8fd2d5e33fcc5427ec038e Mon Sep 17 00:00:00 2001
> From: Eric Biggers <[e-mail]>
> Date: Mon, 27 Nov 2017 23:23:05 -0800
> Subject: [PATCH] crypto: algif_aead - fix reference counting of null skcipher
> 
> In the AEAD interface for AF_ALG, the reference to the "null skcipher"
> held by each tfm was being dropped in the wrong place -- when each
> af_alg_ctx was freed instead of when the aead_tfm was freed.  As
> discovered by syzkaller, a specially crafted program could use this to
> cause the null skcipher to be freed while it is still in use.
> 
> Fix it by dropping the reference in the right place.
> 
> Fixes: 72548b093ee3 ("crypto: algif_aead - copy AAD from src to dst")
> Reported-by: syzbot <[e-mail]>
> Cc: <[e-mail]> # v4.14+
> Signed-off-by: Eric Biggers <[e-mail]>

Patch applied.  Thanks.

Patch

diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
index 805f485ddf1b..48b34e9c6834 100644
--- a/crypto/algif_aead.c
+++ b/crypto/algif_aead.c
@@ -503,6 +503,7 @@  static void aead_release(void *private)
 	struct aead_tfm *tfm = private;
 
 	crypto_free_aead(tfm->aead);
+	crypto_put_default_null_skcipher2();
 	kfree(tfm);
 }
 
@@ -535,7 +536,6 @@  static void aead_sock_destruct(struct sock *sk)
 	unsigned int ivlen = crypto_aead_ivsize(tfm);
 
 	af_alg_pull_tsgl(sk, ctx->used, NULL, 0);
-	crypto_put_default_null_skcipher2();
 	sock_kzfree_s(sk, ctx->iv, ivlen);
 	sock_kfree_s(sk, ctx, ctx->len);
 	af_alg_release_parent(sk);