From 03dab869b7b239c4e013ec82aea22e181e441cfc Mon Sep 17 00:00:00 2001 From: David Howells Date: Wed, 26 Oct 2016 15:01:54 +0100 Subject: KEYS: Fix short sprintf buffer in /proc/keys show function This fixes CVE-2016-7042. Fix a short sprintf buffer in proc_keys_show(). If the gcc stack protector is turned on, this can cause a panic due to stack corruption. The problem is that xbuf[] is not big enough to hold a 64-bit timeout rendered as weeks: (gdb) p 0xffffffffffffffffULL/(60*60*24*7) $2 = 30500568904943 That's 14 chars plus NUL, not 11 chars plus NUL. Expand the buffer to 16 chars. I think the unpatched code apparently works if the stack-protector is not enabled because on a 32-bit machine the buffer won't be overflowed and on a 64-bit machine there's a 64-bit aligned pointer at one side and an int that isn't checked again on the other side. The panic incurred looks something like: Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ffffffff81352ebe CPU: 0 PID: 1692 Comm: reproducer Not tainted 4.7.2-201.fc24.x86_64 #1 Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011 0000000000000086 00000000fbbd2679 ffff8800a044bc00 ffffffff813d941f ffffffff81a28d58 ffff8800a044bc98 ffff8800a044bc88 ffffffff811b2cb6 ffff880000000010 ffff8800a044bc98 ffff8800a044bc30 00000000fbbd2679 Call Trace: [] dump_stack+0x63/0x84 [] panic+0xde/0x22a [] ? proc_keys_show+0x3ce/0x3d0 [] __stack_chk_fail+0x19/0x30 [] proc_keys_show+0x3ce/0x3d0 [] ? key_validate+0x50/0x50 [] ? key_default_cmp+0x20/0x20 [] seq_read+0x2cc/0x390 [] proc_reg_read+0x42/0x70 [] __vfs_read+0x37/0x150 [] ? security_file_permission+0xa0/0xc0 [] vfs_read+0x96/0x130 [] SyS_read+0x55/0xc0 [] entry_SYSCALL_64_fastpath+0x1a/0xa4 Reported-by: Ondrej Kozina Signed-off-by: David Howells Tested-by: Ondrej Kozina cc: stable@vger.kernel.org Signed-off-by: James Morris --- security/keys/proc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/keys/proc.c b/security/keys/proc.c index f0611a6368cd..b9f531c9e4fa 100644 --- a/security/keys/proc.c +++ b/security/keys/proc.c @@ -181,7 +181,7 @@ static int proc_keys_show(struct seq_file *m, void *v) struct timespec now; unsigned long timo; key_ref_t key_ref, skey_ref; - char xbuf[12]; + char xbuf[16]; int rc; struct keyring_search_context ctx = { -- cgit v1.2.3 From 7df3e59c3d1df4f87fe874c7956ef7a3d2f4d5fb Mon Sep 17 00:00:00 2001 From: David Howells Date: Wed, 26 Oct 2016 15:02:01 +0100 Subject: KEYS: Sort out big_key initialisation big_key has two separate initialisation functions, one that registers the key type and one that registers the crypto. If the key type fails to register, there's no problem if the crypto registers successfully because there's no way to reach the crypto except through the key type. However, if the key type registers successfully but the crypto does not, big_key_rng and big_key_blkcipher may end up set to NULL - but the code neither checks for this nor unregisters the big key key type. Furthermore, since the key type is registered before the crypto, it is theoretically possible for the kernel to try adding a big_key before the crypto is set up, leading to the same effect. Fix this by merging big_key_crypto_init() and big_key_init() and calling the resulting function late. If they're going to be encrypted, we shouldn't be creating big_keys before we have the facilities to do the encryption available. The key type registration is also moved after the crypto initialisation. The fix also includes message printing on failure. If the big_key type isn't correctly set up, simply doing: dd if=/dev/zero bs=4096 count=1 | keyctl padd big_key a @s ought to cause an oops. Fixes: 13100a72f40f5748a04017e0ab3df4cf27c809ef ('Security: Keys: Big keys stored encrypted') Signed-off-by: David Howells cc: Peter Hlavaty cc: Kirill Marinushkin cc: Artem Savkov cc: stable@vger.kernel.org Signed-off-by: James Morris --- security/keys/big_key.c | 59 +++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/security/keys/big_key.c b/security/keys/big_key.c index c0b3030b5634..835c1ab30d01 100644 --- a/security/keys/big_key.c +++ b/security/keys/big_key.c @@ -9,6 +9,7 @@ * 2 of the Licence, or (at your option) any later version. */ +#define pr_fmt(fmt) "big_key: "fmt #include #include #include @@ -341,44 +342,48 @@ error: */ static int __init big_key_init(void) { - return register_key_type(&key_type_big_key); -} - -/* - * Initialize big_key crypto and RNG algorithms - */ -static int __init big_key_crypto_init(void) -{ - int ret = -EINVAL; + struct crypto_skcipher *cipher; + struct crypto_rng *rng; + int ret; - /* init RNG */ - big_key_rng = crypto_alloc_rng(big_key_rng_name, 0, 0); - if (IS_ERR(big_key_rng)) { - big_key_rng = NULL; - return -EFAULT; + rng = crypto_alloc_rng(big_key_rng_name, 0, 0); + if (IS_ERR(rng)) { + pr_err("Can't alloc rng: %ld\n", PTR_ERR(rng)); + return PTR_ERR(rng); } + big_key_rng = rng; + /* seed RNG */ - ret = crypto_rng_reset(big_key_rng, NULL, crypto_rng_seedsize(big_key_rng)); - if (ret) - goto error; + ret = crypto_rng_reset(rng, NULL, crypto_rng_seedsize(rng)); + if (ret) { + pr_err("Can't reset rng: %d\n", ret); + goto error_rng; + } /* init block cipher */ - big_key_skcipher = crypto_alloc_skcipher(big_key_alg_name, - 0, CRYPTO_ALG_ASYNC); - if (IS_ERR(big_key_skcipher)) { - big_key_skcipher = NULL; - ret = -EFAULT; - goto error; + cipher = crypto_alloc_skcipher(big_key_alg_name, 0, CRYPTO_ALG_ASYNC); + if (IS_ERR(cipher)) { + ret = PTR_ERR(cipher); + pr_err("Can't alloc crypto: %d\n", ret); + goto error_rng; + } + + big_key_skcipher = cipher; + + ret = register_key_type(&key_type_big_key); + if (ret < 0) { + pr_err("Can't register type: %d\n", ret); + goto error_cipher; } return 0; -error: +error_cipher: + crypto_free_skcipher(big_key_skcipher); +error_rng: crypto_free_rng(big_key_rng); - big_key_rng = NULL; return ret; } -device_initcall(big_key_init); -late_initcall(big_key_crypto_init); +late_initcall(big_key_init); -- cgit v1.2.3 From 31e6ec4519c0fe0ee4a2f6ba3ab278e9506b9500 Mon Sep 17 00:00:00 2001 From: Artem Savkov Date: Wed, 26 Oct 2016 15:02:09 +0100 Subject: security/keys: make BIG_KEYS dependent on stdrng. Since BIG_KEYS can't be compiled as module it requires one of the "stdrng" providers to be compiled into kernel. Otherwise big_key_crypto_init() fails on crypto_alloc_rng step and next dereference of big_key_skcipher (e.g. in big_key_preparse()) results in a NULL pointer dereference. Fixes: 13100a72f40f5748a04017e0ab3df4cf27c809ef ('Security: Keys: Big keys stored encrypted') Signed-off-by: Artem Savkov Signed-off-by: David Howells cc: Stephan Mueller cc: Kirill Marinushkin cc: stable@vger.kernel.org Signed-off-by: James Morris --- security/keys/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/keys/Kconfig b/security/keys/Kconfig index f826e8739023..d942c7c2bc0a 100644 --- a/security/keys/Kconfig +++ b/security/keys/Kconfig @@ -41,7 +41,7 @@ config BIG_KEYS bool "Large payload keys" depends on KEYS depends on TMPFS - select CRYPTO + depends on (CRYPTO_ANSI_CPRNG = y || CRYPTO_DRBG = y) select CRYPTO_AES select CRYPTO_ECB select CRYPTO_RNG -- cgit v1.2.3