security/keys: rewrite all of big_key crypto
Subsystems (2)
PATCH v2 1/1 security/keys: rewrite all of big_key crypto
This started out as just replacing the use of crypto/rng with get_random_bytes, so that we wouldn't use bad randomness at boot time. But, upon looking further, it appears that there were even deeper underlying cryptographic problems, and that this seems to have been committed with very little crypto review. So, I rewrote the whole thing, trying to keep to the conventions introduced by the previous author, to fix these cryptographic flaws.
Since this sensitive material is being stored untrusted, using ECB and no authentication is simply not okay at all. I find it surprising that this code even made it past basic crypto review, though perhaps there's something I'm missing. This patch moves from using AES-ECB to using AES-GCM. Since keys are uniquely generated each time, we can set the nonce to zero. There was also a race condition in which the same key would be reused at the same time in different threads. A mutex fixes this issue now. And, some error paths forgot to zero out sensitive material, so this patch changes a kfree into a kzfree.
So, to summarize, this commit fixes the following vulnerabilities:
* Unauthenticated encryption, allowing an attacker to modify the cipher text in particular ways in order to manipulate the plaintext, which is is even more frightening considering the next point.
* Use of ECB mode, allowing an attacker to trivially swap blocks or compare identical plaintext blocks.
* Key re-use.
* Faulty memory zeroing.
---
Changes v1->v2:
- The code has now been tested, and it works.
- Style fixups.
- Memory zeroring.
- Use mutex to prevent key reuse race condition.
- Useful comments.
- Kconfig simplification.
- We no longer rely on get_random_bytes_wait, even though we really should, because that hasn't yet been merged. When it is merged, I'll submit a follow-up patch.
security/keys/Kconfig | 4 +- security/keys/big_key.c | 129 ++++++++++++++++++++++-------------------------- 2 files changed, 61 insertions(+), 72 deletions(-)
security/keys/Kconfig +1 -3
@@ -41,10 +41,8 @@ config BIG_KEYS bool "Large payload keys" depends on KEYS depends on TMPFS - depends on (CRYPTO_ANSI_CPRNG = y || CRYPTO_DRBG = y) select CRYPTO_AES - select CRYPTO_ECB - select CRYPTO_RNG + select CRYPTO_GCM help This option provides support for holding large keys within the kernel (for example Kerberos ticket caches). The data may be stored out to
security/keys/big_key.c +60 -69
226 lines not on this page · raw message
Signature · 1 line
2.13.0
Hi Jason,
Quoted 2 lines · Jason A. Donenfeld
issue now. And, some error paths forgot to zero out sensitive material, so this patch changes a kfree into a kzfree.
There are other places in big_key.c that should be doing kzfree() instead of kfree(). Sorry, I actually recently did a patchset that fixed this for major parts of the keyrings API, but I skipped the big_key type because it was one of the more obscure key types (and frankly I have no idea what, if anything, actually uses it). Probably the switch to kzfree() should be its own patch since it's a separate logical change.
security/keys/big_key.c static int big_key_crypt(enum big_key_op op, u8 *data, size_t datalen, u8 *key) · line 101
Quoted 10 lines
{
security/keys/big_key.c +4
-3int ret = -EINVAL ;struct scatterlist sgio; -SKCIPHER_REQUEST_ON_STACK (req, big_key_skcipher); - -if (crypto_skcipher_setkey (big_key_skcipher, key,ENC_KEY_SIZE )) { +u8 req_on_stack[sizeof (struct aead_request ) + +crypto_aead_reqsize (big_key_aead)]; +struct aead_request *aead_req = (struct aead_request *)req_on_stack; +
The crypto API with CONFIG_VMAP_STACK=y and CONFIG_DEBUG_SG=y is unhappy with using an aead_request on the stack, because it can't create a scatterlist from it. It will need to be on the heap instead. (Or else the crypto API fixed.)
# cat .vimrc | keyctl padd big_key fred @s [ 43.687347] ------------[ cut here ]------------ [ 43.692592] kernel BUG at ./include/linux/scatterlist.h:140! [ 43.697527] invalid opcode: 0000 [#1] SMP [ 43.700843] CPU: 0 PID: 219 Comm: keyctl Not tainted 4.12.0-rc4-next-20170606-xfstests-00003-gc6e36366c198 #120 [ 43.707361] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-20170228_101828-anatol 04/01/2014 [ 43.712167] task: ffff9ff6f97d8340 task.stack: ffffbd460049c000 [ 43.714259] RIP: 0010:crypto_gcm_init_common+0x234/0x260 [ 43.715786] RSP: 0018:ffffbd460049fac8 EFLAGS: 00010246 [ 43.717031] RAX: 0000000000000000 RBX: ffffbd460049fba8 RCX: 0000000000000028 [ 43.718688] RDX: 00001d4f4049fbb8 RSI: 000000000000001d RDI: ffffbd468049fbb8 [ 43.720245] RBP: ffffbd460049fb00 R08: ffffbd460049fbd8 R09: ffffbd460049fbd8 [ 43.721473] R10: 0000000000000000 R11: 0000000000000000 R12: ffffbd460049fbb8 [ 43.722704] R13: 000000000000092f R14: ffffbd460049fb58 R15: ffffbd460049fba8 [ 43.723927] FS: 00007f04df2f8700(0000) GS:ffff9ff6fe200000(0000) knlGS:0000000000000000 [ 43.725262] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 43.726065] CR2: 00007f04dec1a4c0 CR3: 000000003957e000 CR4: 00000000003406f0 [ 43.727059] Call Trace: [ 43.727414] crypto_gcm_encrypt+0x37/0xe0 [ 43.727896] big_key_crypt+0x106/0x160 [ 43.728324] big_key_preparse+0xc0/0x1d0 [ 43.728784] ? down_read+0x68/0xa0 [ 43.729183] key_create_or_update+0x13f/0x440 [ 43.729688] SyS_add_key+0xa1/0x1d0 [ 43.730260] entry_SYSCALL_64_fastpath+0x1f/0xbe [ 43.730851] RIP: 0033:0x7f04dec22f19 [ 43.731253] RSP: 002b:00007ffd622a0c88 EFLAGS: 00000206 ORIG_RAX: 00000000000000f8 [ 43.732321] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f04dec22f19 [ 43.733274] RDX: 0000000000606260 RSI: 00007ffd622a2866 RDI: 00007ffd622a285e [ 43.734192] RBP: 0000000000000000 R08: 00000000fffffffd R09: 000000000000001e [ 43.735026] R10: 000000000000092f R11: 0000000000000206 R12: 0000000000100000 [ 43.735855] R13: 00007ffd622a0ca8 R14: 00007ffd622a0df8 R15: 0000000000404606 [ 43.736690] Code: c8 01 48 89 83 d8 00 00 00 48 83 c4 10 5b 41 5c 41 5d 41 5e 41 5f 5d c3 48 c7 c0 00 00 00 80 48 2b 05 69 36 74 00 e9 43 ff ff ff <0f> 0b 0f 0b 0f 0b 0f 0b 0f 0b 0f 0b 0f 0b 0f 0b 48 c7 45 c8 01 [ 43.738740] RIP: crypto_gcm_init_common+0x234/0x260 RSP: ffffbd460049fac8 [ 43.739402] ---[ end trace 6df3f493a6e39d76 ]--- Segmentation fault
Quoted 13 lines · Eric Biggers
Hi Jason,
Quoted 2 lines · Jason A. Donenfeld
issue now. And, some error paths forgot to zero out sensitive material, so this patch changes a kfree into a kzfree.
There are other places in
big_key.cthat should be doingkzfree()instead ofkfree(). Sorry, I actually recently did a patchset that fixed this for major parts of the keyrings API, but I skipped thebig_keytype because it was one of the more obscure key types (and frankly I have no idea what, if anything, actually uses it). Probably the switch tokzfree()should be its own patch since it's a separate logical change.
Quoted 14 lines
security/keys/big_key.c static int big_key_crypt(enum big_key_op op, u8 *data, size_t datalen, u8 *key) · line 101
Quoted 10 lines
{
security/keys/big_key.c +4
-3int ret = -EINVAL ;struct scatterlist sgio; -SKCIPHER_REQUEST_ON_STACK (req, big_key_skcipher); - -if (crypto_skcipher_setkey (big_key_skcipher, key,ENC_KEY_SIZE )) { +u8 req_on_stack[sizeof (struct aead_request ) + +crypto_aead_reqsize (big_key_aead)]; +struct aead_request *aead_req = (struct aead_request *)req_on_stack; +The crypto API with
CONFIG_VMAP_STACK=yandCONFIG_DEBUG_SG=yis unhappy with using anaead_requeston the stack, because it can't create a scatterlist from it. It will need to be on the heap instead. (Or else the crypto API fixed.)
You should not create AEAD requests on the stack. The only reason skcipher requests can be created on the stack is for backwards compatibility with the sync blkcipher interface.
For new code you should always allocate the request using aead_request_alloc.
Thanks,
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
David Howells
maintainerQuoted 2 lines · Eric Biggers
but I skipped the
big_keytype because it was one of the more obscure key types (and frankly I have no idea what, if anything, actually uses it)
Kerberos can use it.
David