From 47b2c3fff4932e6fc17ce13d51a43c6969714e20 Mon Sep 17 00:00:00 2001 From: Bilal Amarni Date: Thu, 8 Jun 2017 14:47:26 +0100 Subject: security/keys: add CONFIG_KEYS_COMPAT to Kconfig CONFIG_KEYS_COMPAT is defined in arch-specific Kconfigs and is missing for several 64-bit architectures : mips, parisc, tile. At the moment and for those architectures, calling in 32-bit userspace the keyctl syscall would return an ENOSYS error. This patch moves the CONFIG_KEYS_COMPAT option to security/keys/Kconfig, to make sure the compatibility wrapper is registered by default for any 64-bit architecture as long as it is configured with CONFIG_COMPAT. [DH: Modified to remove arm64 compat enablement also as requested by Eric Biggers] Signed-off-by: Bilal Amarni Signed-off-by: David Howells Reviewed-by: Arnd Bergmann cc: Eric Biggers Signed-off-by: James Morris --- arch/arm64/Kconfig | 4 ---- arch/powerpc/Kconfig | 5 ----- arch/s390/Kconfig | 3 --- arch/sparc/Kconfig | 3 --- arch/x86/Kconfig | 4 ---- security/keys/Kconfig | 4 ++++ 6 files changed, 4 insertions(+), 19 deletions(-) diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 3dcd7ec69bca..b2024db225a9 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -1084,10 +1084,6 @@ config SYSVIPC_COMPAT def_bool y depends on COMPAT && SYSVIPC -config KEYS_COMPAT - def_bool y - depends on COMPAT && KEYS - endmenu menu "Power management options" diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index f7c8f9972f61..83d2e0f43c26 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -1215,11 +1215,6 @@ source "arch/powerpc/Kconfig.debug" source "security/Kconfig" -config KEYS_COMPAT - bool - depends on COMPAT && KEYS - default y - source "crypto/Kconfig" config PPC_LIB_RHEAP diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig index e161fafb495b..6967addc6a89 100644 --- a/arch/s390/Kconfig +++ b/arch/s390/Kconfig @@ -363,9 +363,6 @@ config COMPAT config SYSVIPC_COMPAT def_bool y if COMPAT && SYSVIPC -config KEYS_COMPAT - def_bool y if COMPAT && KEYS - config SMP def_bool y prompt "Symmetric multi-processing support" diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig index b558c9e29de3..5639c9fe5b55 100644 --- a/arch/sparc/Kconfig +++ b/arch/sparc/Kconfig @@ -577,9 +577,6 @@ config SYSVIPC_COMPAT depends on COMPAT && SYSVIPC default y -config KEYS_COMPAT - def_bool y if COMPAT && KEYS - endmenu source "net/Kconfig" diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 4ccfacc7232a..0efb4c9497bc 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -2776,10 +2776,6 @@ config COMPAT_FOR_U64_ALIGNMENT config SYSVIPC_COMPAT def_bool y depends on SYSVIPC - -config KEYS_COMPAT - def_bool y - depends on KEYS endif endmenu diff --git a/security/keys/Kconfig b/security/keys/Kconfig index 6fd95f76bfae..00b7431a8aeb 100644 --- a/security/keys/Kconfig +++ b/security/keys/Kconfig @@ -20,6 +20,10 @@ config KEYS If you are unsure as to whether this is required, answer N. +config KEYS_COMPAT + def_bool y + depends on COMPAT && KEYS + config PERSISTENT_KEYRINGS bool "Enable register of persistent per-UID keyrings" depends on KEYS -- cgit v1.2.3 From 381f20fceba8ea540aef5241a9099f4552700d0c Mon Sep 17 00:00:00 2001 From: Davidlohr Bueso Date: Thu, 8 Jun 2017 14:47:34 +0100 Subject: security: use READ_ONCE instead of deprecated ACCESS_ONCE With the new standardized functions, we can replace all ACCESS_ONCE() calls across relevant security/keyrings/. ACCESS_ONCE() does not work reliably on non-scalar types. For example gcc 4.6 and 4.7 might remove the volatile tag for such accesses during the SRA (scalar replacement of aggregates) step: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145 Update the new calls regardless of if it is a scalar type, this is cleaner than having three alternatives. Signed-off-by: Davidlohr Bueso Signed-off-by: David Howells Signed-off-by: James Morris --- security/keys/keyring.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/security/keys/keyring.c b/security/keys/keyring.c index 4d1678e4586f..de81793f9920 100644 --- a/security/keys/keyring.c +++ b/security/keys/keyring.c @@ -706,7 +706,7 @@ descend_to_keyring: * Non-keyrings avoid the leftmost branch of the root entirely (root * slots 1-15). */ - ptr = ACCESS_ONCE(keyring->keys.root); + ptr = READ_ONCE(keyring->keys.root); if (!ptr) goto not_this_keyring; @@ -720,7 +720,7 @@ descend_to_keyring: if ((shortcut->index_key[0] & ASSOC_ARRAY_FAN_MASK) != 0) goto not_this_keyring; - ptr = ACCESS_ONCE(shortcut->next_node); + ptr = READ_ONCE(shortcut->next_node); node = assoc_array_ptr_to_node(ptr); goto begin_node; } @@ -740,7 +740,7 @@ descend_to_node: if (assoc_array_ptr_is_shortcut(ptr)) { shortcut = assoc_array_ptr_to_shortcut(ptr); smp_read_barrier_depends(); - ptr = ACCESS_ONCE(shortcut->next_node); + ptr = READ_ONCE(shortcut->next_node); BUG_ON(!assoc_array_ptr_is_node(ptr)); } node = assoc_array_ptr_to_node(ptr); @@ -752,7 +752,7 @@ begin_node: ascend_to_node: /* Go through the slots in a node */ for (; slot < ASSOC_ARRAY_FAN_OUT; slot++) { - ptr = ACCESS_ONCE(node->slots[slot]); + ptr = READ_ONCE(node->slots[slot]); if (assoc_array_ptr_is_meta(ptr) && node->back_pointer) goto descend_to_node; @@ -790,13 +790,13 @@ ascend_to_node: /* We've dealt with all the slots in the current node, so now we need * to ascend to the parent and continue processing there. */ - ptr = ACCESS_ONCE(node->back_pointer); + ptr = READ_ONCE(node->back_pointer); slot = node->parent_slot; if (ptr && assoc_array_ptr_is_shortcut(ptr)) { shortcut = assoc_array_ptr_to_shortcut(ptr); smp_read_barrier_depends(); - ptr = ACCESS_ONCE(shortcut->back_pointer); + ptr = READ_ONCE(shortcut->back_pointer); slot = shortcut->parent_slot; } if (!ptr) -- cgit v1.2.3 From 4e880168e9ffb1cdbdb72b3b48ab0324b30c2d62 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Thu, 8 Jun 2017 14:47:49 +0100 Subject: X.509: Fix error code in x509_cert_parse() We forgot to set the error code on this path so it could result in returning NULL which leads to a NULL dereference. Fixes: db6c43bd2132 ("crypto: KEYS: convert public key and digsig asym to the akcipher api") Signed-off-by: Dan Carpenter Signed-off-by: David Howells Signed-off-by: James Morris --- crypto/asymmetric_keys/x509_cert_parser.c | 1 + 1 file changed, 1 insertion(+) diff --git a/crypto/asymmetric_keys/x509_cert_parser.c b/crypto/asymmetric_keys/x509_cert_parser.c index c80765b211cf..dd03fead1ca3 100644 --- a/crypto/asymmetric_keys/x509_cert_parser.c +++ b/crypto/asymmetric_keys/x509_cert_parser.c @@ -102,6 +102,7 @@ struct x509_certificate *x509_cert_parse(const void *data, size_t datalen) } } + ret = -ENOMEM; cert->pub->key = kmemdup(ctx->key, ctx->key_size, GFP_KERNEL); if (!cert->pub->key) goto error_decode; -- cgit v1.2.3 From 41f1c53e0d7d6e79087d5f5e18c467747126a3fc Mon Sep 17 00:00:00 2001 From: Markus Elfring Date: Thu, 8 Jun 2017 14:47:56 +0100 Subject: KEYS: Delete an error message for a failed memory allocation in get_derived_key() Omit an extra message for a memory allocation failure in this function. This issue was detected by using the Coccinelle software. Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf Signed-off-by: Markus Elfring Signed-off-by: David Howells Signed-off-by: James Morris --- security/keys/encrypted-keys/encrypted.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c index 0010955d7876..2ab48eab29a1 100644 --- a/security/keys/encrypted-keys/encrypted.c +++ b/security/keys/encrypted-keys/encrypted.c @@ -385,10 +385,9 @@ static int get_derived_key(u8 *derived_key, enum derived_key_type key_type, derived_buf_len = HASH_SIZE; derived_buf = kzalloc(derived_buf_len, GFP_KERNEL); - if (!derived_buf) { - pr_err("encrypted_key: out of memory\n"); + if (!derived_buf) return -ENOMEM; - } + if (key_type) strcpy(derived_buf, "AUTH_KEY"); else -- cgit v1.2.3 From d636bd9f12a66ea3775c9fabbf3f8e118253467a Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 8 Jun 2017 14:48:03 +0100 Subject: KEYS: put keyring if install_session_keyring_to_cred() fails In join_session_keyring(), if install_session_keyring_to_cred() were to fail, we would leak the keyring reference, just like in the bug fixed by commit 23567fd052a9 ("KEYS: Fix keyring ref leak in join_session_keyring()"). Fortunately this cannot happen currently, but we really should be more careful. Do this by adding and using a new error label at which the keyring reference is dropped. Signed-off-by: Eric Biggers Signed-off-by: David Howells Signed-off-by: James Morris --- security/keys/process_keys.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c index 2217dfec7996..86bced9fdbdf 100644 --- a/security/keys/process_keys.c +++ b/security/keys/process_keys.c @@ -809,15 +809,14 @@ long join_session_keyring(const char *name) ret = PTR_ERR(keyring); goto error2; } else if (keyring == new->session_keyring) { - key_put(keyring); ret = 0; - goto error2; + goto error3; } /* we've got a keyring - now to install it */ ret = install_session_keyring_to_cred(new, keyring); if (ret < 0) - goto error2; + goto error3; commit_creds(new); mutex_unlock(&key_session_mutex); @@ -827,6 +826,8 @@ long join_session_keyring(const char *name) okay: return ret; +error3: + key_put(keyring); error2: mutex_unlock(&key_session_mutex); error: -- cgit v1.2.3 From e9ff56ac352446f55141aaef1553cee662b2e310 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 8 Jun 2017 14:48:10 +0100 Subject: KEYS: encrypted: avoid encrypting/decrypting stack buffers Since v4.9, the crypto API cannot (normally) be used to encrypt/decrypt stack buffers because the stack may be virtually mapped. Fix this for the padding buffers in encrypted-keys by using ZERO_PAGE for the encryption padding and by allocating a temporary heap buffer for the decryption padding. Tested with CONFIG_DEBUG_SG=y: keyctl new_session keyctl add user master "abcdefghijklmnop" @s keyid=$(keyctl add encrypted desc "new user:master 25" @s) datablob="$(keyctl pipe $keyid)" keyctl unlink $keyid keyid=$(keyctl add encrypted desc "load $datablob" @s) datablob2="$(keyctl pipe $keyid)" [ "$datablob" = "$datablob2" ] && echo "Success!" Cc: Andy Lutomirski Cc: Herbert Xu Cc: Mimi Zohar Cc: stable@vger.kernel.org # 4.9+ Signed-off-by: Eric Biggers Signed-off-by: David Howells Signed-off-by: James Morris --- security/keys/encrypted-keys/encrypted.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c index 2ab48eab29a1..d14f1a47a130 100644 --- a/security/keys/encrypted-keys/encrypted.c +++ b/security/keys/encrypted-keys/encrypted.c @@ -479,12 +479,9 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload, struct skcipher_request *req; unsigned int encrypted_datalen; u8 iv[AES_BLOCK_SIZE]; - unsigned int padlen; - char pad[16]; int ret; encrypted_datalen = roundup(epayload->decrypted_datalen, blksize); - padlen = encrypted_datalen - epayload->decrypted_datalen; req = init_skcipher_req(derived_key, derived_keylen); ret = PTR_ERR(req); @@ -492,11 +489,10 @@ static int derived_key_encrypt(struct encrypted_key_payload *epayload, goto out; dump_decrypted_data(epayload); - memset(pad, 0, sizeof pad); sg_init_table(sg_in, 2); sg_set_buf(&sg_in[0], epayload->decrypted_data, epayload->decrypted_datalen); - sg_set_buf(&sg_in[1], pad, padlen); + sg_set_page(&sg_in[1], ZERO_PAGE(0), AES_BLOCK_SIZE, 0); sg_init_table(sg_out, 1); sg_set_buf(sg_out, epayload->encrypted_data, encrypted_datalen); @@ -583,9 +579,14 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload, struct skcipher_request *req; unsigned int encrypted_datalen; u8 iv[AES_BLOCK_SIZE]; - char pad[16]; + u8 *pad; int ret; + /* Throwaway buffer to hold the unused zero padding at the end */ + pad = kmalloc(AES_BLOCK_SIZE, GFP_KERNEL); + if (!pad) + return -ENOMEM; + encrypted_datalen = roundup(epayload->decrypted_datalen, blksize); req = init_skcipher_req(derived_key, derived_keylen); ret = PTR_ERR(req); @@ -593,13 +594,12 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload, goto out; dump_encrypted_data(epayload, encrypted_datalen); - memset(pad, 0, sizeof pad); sg_init_table(sg_in, 1); sg_init_table(sg_out, 2); sg_set_buf(sg_in, epayload->encrypted_data, encrypted_datalen); sg_set_buf(&sg_out[0], epayload->decrypted_data, epayload->decrypted_datalen); - sg_set_buf(&sg_out[1], pad, sizeof pad); + sg_set_buf(&sg_out[1], pad, AES_BLOCK_SIZE); memcpy(iv, epayload->iv, sizeof(iv)); skcipher_request_set_crypt(req, sg_in, sg_out, encrypted_datalen, iv); @@ -611,6 +611,7 @@ static int derived_key_decrypt(struct encrypted_key_payload *epayload, goto out; dump_decrypted_data(epayload); out: + kfree(pad); return ret; } -- cgit v1.2.3 From 794b4bc292f5d31739d89c0202c54e7dc9bc3add Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 8 Jun 2017 14:48:18 +0100 Subject: KEYS: encrypted: fix buffer overread in valid_master_desc() With the 'encrypted' key type it was possible for userspace to provide a data blob ending with a master key description shorter than expected, e.g. 'keyctl add encrypted desc "new x" @s'. When validating such a master key description, validate_master_desc() could read beyond the end of the buffer. Fix this by using strncmp() instead of memcmp(). [Also clean up the code to deduplicate some logic.] Cc: Mimi Zohar Signed-off-by: Eric Biggers Signed-off-by: David Howells Signed-off-by: James Morris --- security/keys/encrypted-keys/encrypted.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c index d14f1a47a130..0f7b95de3b5f 100644 --- a/security/keys/encrypted-keys/encrypted.c +++ b/security/keys/encrypted-keys/encrypted.c @@ -141,23 +141,22 @@ static int valid_ecryptfs_desc(const char *ecryptfs_desc) */ static int valid_master_desc(const char *new_desc, const char *orig_desc) { - if (!memcmp(new_desc, KEY_TRUSTED_PREFIX, KEY_TRUSTED_PREFIX_LEN)) { - if (strlen(new_desc) == KEY_TRUSTED_PREFIX_LEN) - goto out; - if (orig_desc) - if (memcmp(new_desc, orig_desc, KEY_TRUSTED_PREFIX_LEN)) - goto out; - } else if (!memcmp(new_desc, KEY_USER_PREFIX, KEY_USER_PREFIX_LEN)) { - if (strlen(new_desc) == KEY_USER_PREFIX_LEN) - goto out; - if (orig_desc) - if (memcmp(new_desc, orig_desc, KEY_USER_PREFIX_LEN)) - goto out; - } else - goto out; + int prefix_len; + + if (!strncmp(new_desc, KEY_TRUSTED_PREFIX, KEY_TRUSTED_PREFIX_LEN)) + prefix_len = KEY_TRUSTED_PREFIX_LEN; + else if (!strncmp(new_desc, KEY_USER_PREFIX, KEY_USER_PREFIX_LEN)) + prefix_len = KEY_USER_PREFIX_LEN; + else + return -EINVAL; + + if (!new_desc[prefix_len]) + return -EINVAL; + + if (orig_desc && strncmp(new_desc, orig_desc, prefix_len)) + return -EINVAL; + return 0; -out: - return -EINVAL; } /* -- cgit v1.2.3 From 64d107d3acca1565c39c044c459fd18f70943534 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 8 Jun 2017 14:48:25 +0100 Subject: KEYS: encrypted: fix race causing incorrect HMAC calculations The encrypted-keys module was using a single global HMAC transform, which could be rekeyed by multiple threads concurrently operating on different keys, causing incorrect HMAC values to be calculated. Fix this by allocating a new HMAC transform whenever we need to calculate a HMAC. Also simplify things a bit by allocating the shash_desc's using SHASH_DESC_ON_STACK() for both the HMAC and unkeyed hashes. The following script reproduces the bug: keyctl new_session keyctl add user master "abcdefghijklmnop" @s for i in $(seq 2); do ( set -e for j in $(seq 1000); do keyid=$(keyctl add encrypted desc$i "new user:master 25" @s) datablob="$(keyctl pipe $keyid)" keyctl unlink $keyid > /dev/null keyid=$(keyctl add encrypted desc$i "load $datablob" @s) keyctl unlink $keyid > /dev/null done ) & done Output with bug: [ 439.691094] encrypted_key: bad hmac (-22) add_key: Invalid argument add_key: Invalid argument Cc: Mimi Zohar Cc: Herbert Xu Signed-off-by: Eric Biggers Signed-off-by: David Howells Signed-off-by: James Morris --- security/keys/encrypted-keys/encrypted.c | 115 +++++++++---------------------- 1 file changed, 32 insertions(+), 83 deletions(-) diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c index 0f7b95de3b5f..702c80662069 100644 --- a/security/keys/encrypted-keys/encrypted.c +++ b/security/keys/encrypted-keys/encrypted.c @@ -54,13 +54,7 @@ static int blksize; #define MAX_DATA_SIZE 4096 #define MIN_DATA_SIZE 20 -struct sdesc { - struct shash_desc shash; - char ctx[]; -}; - -static struct crypto_shash *hashalg; -static struct crypto_shash *hmacalg; +static struct crypto_shash *hash_tfm; enum { Opt_err = -1, Opt_new, Opt_load, Opt_update @@ -320,53 +314,38 @@ error: return ukey; } -static struct sdesc *alloc_sdesc(struct crypto_shash *alg) -{ - struct sdesc *sdesc; - int size; - - size = sizeof(struct shash_desc) + crypto_shash_descsize(alg); - sdesc = kmalloc(size, GFP_KERNEL); - if (!sdesc) - return ERR_PTR(-ENOMEM); - sdesc->shash.tfm = alg; - sdesc->shash.flags = 0x0; - return sdesc; -} - -static int calc_hmac(u8 *digest, const u8 *key, unsigned int keylen, +static int calc_hash(struct crypto_shash *tfm, u8 *digest, const u8 *buf, unsigned int buflen) { - struct sdesc *sdesc; - int ret; + SHASH_DESC_ON_STACK(desc, tfm); + int err; - sdesc = alloc_sdesc(hmacalg); - if (IS_ERR(sdesc)) { - pr_info("encrypted_key: can't alloc %s\n", hmac_alg); - return PTR_ERR(sdesc); - } + desc->tfm = tfm; + desc->flags = 0; - ret = crypto_shash_setkey(hmacalg, key, keylen); - if (!ret) - ret = crypto_shash_digest(&sdesc->shash, buf, buflen, digest); - kfree(sdesc); - return ret; + err = crypto_shash_digest(desc, buf, buflen, digest); + shash_desc_zero(desc); + return err; } -static int calc_hash(u8 *digest, const u8 *buf, unsigned int buflen) +static int calc_hmac(u8 *digest, const u8 *key, unsigned int keylen, + const u8 *buf, unsigned int buflen) { - struct sdesc *sdesc; - int ret; + struct crypto_shash *tfm; + int err; - sdesc = alloc_sdesc(hashalg); - if (IS_ERR(sdesc)) { - pr_info("encrypted_key: can't alloc %s\n", hash_alg); - return PTR_ERR(sdesc); + tfm = crypto_alloc_shash(hmac_alg, 0, CRYPTO_ALG_ASYNC); + if (IS_ERR(tfm)) { + pr_err("encrypted_key: can't alloc %s transform: %ld\n", + hmac_alg, PTR_ERR(tfm)); + return PTR_ERR(tfm); } - ret = crypto_shash_digest(&sdesc->shash, buf, buflen, digest); - kfree(sdesc); - return ret; + err = crypto_shash_setkey(tfm, key, keylen); + if (!err) + err = calc_hash(tfm, digest, buf, buflen); + crypto_free_shash(tfm); + return err; } enum derived_key_type { ENC_KEY, AUTH_KEY }; @@ -394,7 +373,7 @@ static int get_derived_key(u8 *derived_key, enum derived_key_type key_type, memcpy(derived_buf + strlen(derived_buf) + 1, master_key, master_keylen); - ret = calc_hash(derived_key, derived_buf, derived_buf_len); + ret = calc_hash(hash_tfm, derived_key, derived_buf, derived_buf_len); kfree(derived_buf); return ret; } @@ -998,47 +977,17 @@ struct key_type key_type_encrypted = { }; EXPORT_SYMBOL_GPL(key_type_encrypted); -static void encrypted_shash_release(void) -{ - if (hashalg) - crypto_free_shash(hashalg); - if (hmacalg) - crypto_free_shash(hmacalg); -} - -static int __init encrypted_shash_alloc(void) +static int __init init_encrypted(void) { int ret; - hmacalg = crypto_alloc_shash(hmac_alg, 0, CRYPTO_ALG_ASYNC); - if (IS_ERR(hmacalg)) { - pr_info("encrypted_key: could not allocate crypto %s\n", - hmac_alg); - return PTR_ERR(hmacalg); + hash_tfm = crypto_alloc_shash(hash_alg, 0, CRYPTO_ALG_ASYNC); + if (IS_ERR(hash_tfm)) { + pr_err("encrypted_key: can't allocate %s transform: %ld\n", + hash_alg, PTR_ERR(hash_tfm)); + return PTR_ERR(hash_tfm); } - hashalg = crypto_alloc_shash(hash_alg, 0, CRYPTO_ALG_ASYNC); - if (IS_ERR(hashalg)) { - pr_info("encrypted_key: could not allocate crypto %s\n", - hash_alg); - ret = PTR_ERR(hashalg); - goto hashalg_fail; - } - - return 0; - -hashalg_fail: - crypto_free_shash(hmacalg); - return ret; -} - -static int __init init_encrypted(void) -{ - int ret; - - ret = encrypted_shash_alloc(); - if (ret < 0) - return ret; ret = aes_get_sizes(); if (ret < 0) goto out; @@ -1047,14 +996,14 @@ static int __init init_encrypted(void) goto out; return 0; out: - encrypted_shash_release(); + crypto_free_shash(hash_tfm); return ret; } static void __exit cleanup_encrypted(void) { - encrypted_shash_release(); + crypto_free_shash(hash_tfm); unregister_key_type(&key_type_encrypted); } -- cgit v1.2.3 From 0f534e4a13496b02ae284f50fcb0263f6ea37007 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 8 Jun 2017 14:48:32 +0100 Subject: KEYS: encrypted: use constant-time HMAC comparison MACs should, in general, be compared using crypto_memneq() to prevent timing attacks. Cc: Mimi Zohar Cc: Herbert Xu Signed-off-by: Eric Biggers Signed-off-by: David Howells Signed-off-by: James Morris --- security/keys/encrypted-keys/encrypted.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c index 702c80662069..5c98c2fe03f0 100644 --- a/security/keys/encrypted-keys/encrypted.c +++ b/security/keys/encrypted-keys/encrypted.c @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -534,8 +535,8 @@ static int datablob_hmac_verify(struct encrypted_key_payload *epayload, ret = calc_hmac(digest, derived_key, sizeof derived_key, p, len); if (ret < 0) goto out; - ret = memcmp(digest, epayload->format + epayload->datablob_len, - sizeof digest); + ret = crypto_memneq(digest, epayload->format + epayload->datablob_len, + sizeof(digest)); if (ret) { ret = -EINVAL; dump_hmac("datablob", -- cgit v1.2.3 From 5649645d725c73df4302428ee4e02c869248b4c5 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 8 Jun 2017 14:48:40 +0100 Subject: KEYS: fix dereferencing NULL payload with nonzero length sys_add_key() and the KEYCTL_UPDATE operation of sys_keyctl() allowed a NULL payload with nonzero length to be passed to the key type's ->preparse(), ->instantiate(), and/or ->update() methods. Various key types including asymmetric, cifs.idmap, cifs.spnego, and pkcs7_test did not handle this case, allowing an unprivileged user to trivially cause a NULL pointer dereference (kernel oops) if one of these key types was present. Fix it by doing the copy_from_user() when 'plen' is nonzero rather than when '_payload' is non-NULL, causing the syscall to fail with EFAULT as expected when an invalid buffer is specified. Cc: stable@vger.kernel.org # 2.6.10+ Signed-off-by: Eric Biggers Signed-off-by: David Howells Signed-off-by: James Morris --- security/keys/keyctl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index 447a7d5cee0f..94c2790f8283 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -99,7 +99,7 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type, /* pull the payload in if one was supplied */ payload = NULL; - if (_payload) { + if (plen) { ret = -ENOMEM; payload = kvmalloc(plen, GFP_KERNEL); if (!payload) @@ -324,7 +324,7 @@ long keyctl_update_key(key_serial_t id, /* pull the payload in if one was supplied */ payload = NULL; - if (_payload) { + if (plen) { ret = -ENOMEM; payload = kmalloc(plen, GFP_KERNEL); if (!payload) -- cgit v1.2.3 From 63a0b0509e700717a59f049ec6e4e04e903c7fe2 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 8 Jun 2017 14:48:47 +0100 Subject: KEYS: fix freeing uninitialized memory in key_update() key_update() freed the key_preparsed_payload even if it was not initialized first. This would cause a crash if userspace called keyctl_update() on a key with type like "asymmetric" that has a ->preparse() method but not an ->update() method. Possibly it could even be triggered for other key types by racing with keyctl_setperm() to make the KEY_NEED_WRITE check fail (the permission was already checked, so normally it wouldn't fail there). Reproducer with key type "asymmetric", given a valid cert.der: keyctl new_session keyid=$(keyctl padd asymmetric desc @s < cert.der) keyctl setperm $keyid 0x3f000000 keyctl update $keyid data [ 150.686666] BUG: unable to handle kernel NULL pointer dereference at 0000000000000001 [ 150.687601] IP: asymmetric_key_free_kids+0x12/0x30 [ 150.688139] PGD 38a3d067 [ 150.688141] PUD 3b3de067 [ 150.688447] PMD 0 [ 150.688745] [ 150.689160] Oops: 0000 [#1] SMP [ 150.689455] Modules linked in: [ 150.689769] CPU: 1 PID: 2478 Comm: keyctl Not tainted 4.11.0-rc4-xfstests-00187-ga9f6b6b8cd2f #742 [ 150.690916] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-20170228_101828-anatol 04/01/2014 [ 150.692199] task: ffff88003b30c480 task.stack: ffffc90000350000 [ 150.692952] RIP: 0010:asymmetric_key_free_kids+0x12/0x30 [ 150.693556] RSP: 0018:ffffc90000353e58 EFLAGS: 00010202 [ 150.694142] RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000000004 [ 150.694845] RDX: ffffffff81ee3920 RSI: ffff88003d4b0700 RDI: 0000000000000001 [ 150.697569] RBP: ffffc90000353e60 R08: ffff88003d5d2140 R09: 0000000000000000 [ 150.702483] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000001 [ 150.707393] R13: 0000000000000004 R14: ffff880038a4d2d8 R15: 000000000040411f [ 150.709720] FS: 00007fcbcee35700(0000) GS:ffff88003fd00000(0000) knlGS:0000000000000000 [ 150.711504] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 150.712733] CR2: 0000000000000001 CR3: 0000000039eab000 CR4: 00000000003406e0 [ 150.714487] Call Trace: [ 150.714975] asymmetric_key_free_preparse+0x2f/0x40 [ 150.715907] key_update+0xf7/0x140 [ 150.716560] ? key_default_cmp+0x20/0x20 [ 150.717319] keyctl_update_key+0xb0/0xe0 [ 150.718066] SyS_keyctl+0x109/0x130 [ 150.718663] entry_SYSCALL_64_fastpath+0x1f/0xc2 [ 150.719440] RIP: 0033:0x7fcbce75ff19 [ 150.719926] RSP: 002b:00007ffd5d167088 EFLAGS: 00000206 ORIG_RAX: 00000000000000fa [ 150.720918] RAX: ffffffffffffffda RBX: 0000000000404d80 RCX: 00007fcbce75ff19 [ 150.721874] RDX: 00007ffd5d16785e RSI: 000000002866cd36 RDI: 0000000000000002 [ 150.722827] RBP: 0000000000000006 R08: 000000002866cd36 R09: 00007ffd5d16785e [ 150.723781] R10: 0000000000000004 R11: 0000000000000206 R12: 0000000000404d80 [ 150.724650] R13: 00007ffd5d16784d R14: 00007ffd5d167238 R15: 000000000040411f [ 150.725447] Code: 83 c4 08 31 c0 5b 41 5c 41 5d 41 5e 41 5f 5d c3 66 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 85 ff 74 23 55 48 89 e5 53 48 89 fb <48> 8b 3f e8 06 21 c5 ff 48 8b 7b 08 e8 fd 20 c5 ff 48 89 df e8 [ 150.727489] RIP: asymmetric_key_free_kids+0x12/0x30 RSP: ffffc90000353e58 [ 150.728117] CR2: 0000000000000001 [ 150.728430] ---[ end trace f7f8fe1da2d5ae8d ]--- Fixes: 4d8c0250b841 ("KEYS: Call ->free_preparse() even after ->preparse() returns an error") Cc: stable@vger.kernel.org # 3.17+ Signed-off-by: Eric Biggers Signed-off-by: David Howells Signed-off-by: James Morris --- security/keys/key.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/security/keys/key.c b/security/keys/key.c index 455c04d80bbb..cbae368c0e57 100644 --- a/security/keys/key.c +++ b/security/keys/key.c @@ -966,12 +966,11 @@ int key_update(key_ref_t key_ref, const void *payload, size_t plen) /* the key must be writable */ ret = key_permission(key_ref, KEY_NEED_WRITE); if (ret < 0) - goto error; + return ret; /* attempt to update it if supported */ - ret = -EOPNOTSUPP; if (!key->type->update) - goto error; + return -EOPNOTSUPP; memset(&prep, 0, sizeof(prep)); prep.data = payload; -- cgit v1.2.3 From 57070c850a03ee0cea654fc22cb8032fc3139d39 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 8 Jun 2017 14:48:57 +0100 Subject: KEYS: sanitize add_key() and keyctl() key payloads Before returning from add_key() or one of the keyctl() commands that takes in a key payload, zero the temporary buffer that was allocated to hold the key payload copied from userspace. This may contain sensitive key material that should not be kept around in the slab caches. Signed-off-by: Eric Biggers Signed-off-by: David Howells Signed-off-by: James Morris --- security/keys/keyctl.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index 94c2790f8283..ab0b337c84b4 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -132,7 +132,10 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type, key_ref_put(keyring_ref); error3: - kvfree(payload); + if (payload) { + memzero_explicit(payload, plen); + kvfree(payload); + } error2: kfree(description); error: @@ -347,7 +350,7 @@ long keyctl_update_key(key_serial_t id, key_ref_put(key_ref); error2: - kfree(payload); + kzfree(payload); error: return ret; } @@ -1093,7 +1096,10 @@ long keyctl_instantiate_key_common(key_serial_t id, keyctl_change_reqkey_auth(NULL); error2: - kvfree(payload); + if (payload) { + memzero_explicit(payload, plen); + kvfree(payload); + } error: return ret; } -- cgit v1.2.3 From 6966c74932b328b3d6df92b11b083bfbcd067986 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 8 Jun 2017 14:49:04 +0100 Subject: KEYS: user_defined: sanitize key payloads Zero the payloads of user and logon keys before freeing them. This prevents sensitive key material from being kept around in the slab caches after a key is released. Signed-off-by: Eric Biggers Signed-off-by: David Howells Signed-off-by: James Morris --- security/keys/user_defined.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c index 26605134f17a..3d8c68eba516 100644 --- a/security/keys/user_defined.c +++ b/security/keys/user_defined.c @@ -86,10 +86,18 @@ EXPORT_SYMBOL_GPL(user_preparse); */ void user_free_preparse(struct key_preparsed_payload *prep) { - kfree(prep->payload.data[0]); + kzfree(prep->payload.data[0]); } EXPORT_SYMBOL_GPL(user_free_preparse); +static void user_free_payload_rcu(struct rcu_head *head) +{ + struct user_key_payload *payload; + + payload = container_of(head, struct user_key_payload, rcu); + kzfree(payload); +} + /* * update a user defined key * - the key's semaphore is write-locked @@ -112,7 +120,7 @@ int user_update(struct key *key, struct key_preparsed_payload *prep) prep->payload.data[0] = NULL; if (zap) - kfree_rcu(zap, rcu); + call_rcu(&zap->rcu, user_free_payload_rcu); return ret; } EXPORT_SYMBOL_GPL(user_update); @@ -130,7 +138,7 @@ void user_revoke(struct key *key) if (upayload) { rcu_assign_keypointer(key, NULL); - kfree_rcu(upayload, rcu); + call_rcu(&upayload->rcu, user_free_payload_rcu); } } @@ -143,7 +151,7 @@ void user_destroy(struct key *key) { struct user_key_payload *upayload = key->payload.data[0]; - kfree(upayload); + kzfree(upayload); } EXPORT_SYMBOL_GPL(user_destroy); -- cgit v1.2.3 From a9dd74b252e04b7e41ffe78d418b896b33b70a13 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 8 Jun 2017 14:49:11 +0100 Subject: KEYS: encrypted: sanitize all key material For keys of type "encrypted", consistently zero sensitive key material before freeing it. This was already being done for the decrypted payloads of encrypted keys, but not for the master key and the keys derived from the master key. Out of an abundance of caution and because it is trivial to do so, also zero buffers containing the key payload in encrypted form, although depending on how the encrypted-keys feature is used such information does not necessarily need to be kept secret. Cc: Mimi Zohar Cc: David Safford Signed-off-by: Eric Biggers Signed-off-by: David Howells Signed-off-by: James Morris --- security/keys/encrypted-keys/encrypted.c | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c index 5c98c2fe03f0..bb6324d1ccec 100644 --- a/security/keys/encrypted-keys/encrypted.c +++ b/security/keys/encrypted-keys/encrypted.c @@ -375,7 +375,7 @@ static int get_derived_key(u8 *derived_key, enum derived_key_type key_type, memcpy(derived_buf + strlen(derived_buf) + 1, master_key, master_keylen); ret = calc_hash(hash_tfm, derived_key, derived_buf, derived_buf_len); - kfree(derived_buf); + kzfree(derived_buf); return ret; } @@ -507,6 +507,7 @@ static int datablob_hmac_append(struct encrypted_key_payload *epayload, if (!ret) dump_hmac(NULL, digest, HASH_SIZE); out: + memzero_explicit(derived_key, sizeof(derived_key)); return ret; } @@ -545,6 +546,7 @@ static int datablob_hmac_verify(struct encrypted_key_payload *epayload, dump_hmac("calc", digest, HASH_SIZE); } out: + memzero_explicit(derived_key, sizeof(derived_key)); return ret; } @@ -701,6 +703,7 @@ static int encrypted_key_decrypt(struct encrypted_key_payload *epayload, out: up_read(&mkey->sem); key_put(mkey); + memzero_explicit(derived_key, sizeof(derived_key)); return ret; } @@ -807,13 +810,13 @@ static int encrypted_instantiate(struct key *key, ret = encrypted_init(epayload, key->description, format, master_desc, decrypted_datalen, hex_encoded_iv); if (ret < 0) { - kfree(epayload); + kzfree(epayload); goto out; } rcu_assign_keypointer(key, epayload); out: - kfree(datablob); + kzfree(datablob); return ret; } @@ -822,8 +825,7 @@ static void encrypted_rcu_free(struct rcu_head *rcu) struct encrypted_key_payload *epayload; epayload = container_of(rcu, struct encrypted_key_payload, rcu); - memset(epayload->decrypted_data, 0, epayload->decrypted_datalen); - kfree(epayload); + kzfree(epayload); } /* @@ -881,7 +883,7 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep) rcu_assign_keypointer(key, new_epayload); call_rcu(&epayload->rcu, encrypted_rcu_free); out: - kfree(buf); + kzfree(buf); return ret; } @@ -939,33 +941,26 @@ static long encrypted_read(const struct key *key, char __user *buffer, up_read(&mkey->sem); key_put(mkey); + memzero_explicit(derived_key, sizeof(derived_key)); if (copy_to_user(buffer, ascii_buf, asciiblob_len) != 0) ret = -EFAULT; - kfree(ascii_buf); + kzfree(ascii_buf); return asciiblob_len; out: up_read(&mkey->sem); key_put(mkey); + memzero_explicit(derived_key, sizeof(derived_key)); return ret; } /* - * encrypted_destroy - before freeing the key, clear the decrypted data - * - * Before freeing the key, clear the memory containing the decrypted - * key data. + * encrypted_destroy - clear and free the key's payload */ static void encrypted_destroy(struct key *key) { - struct encrypted_key_payload *epayload = key->payload.data[0]; - - if (!epayload) - return; - - memzero_explicit(epayload->decrypted_data, epayload->decrypted_datalen); - kfree(key->payload.data[0]); + kzfree(key->payload.data[0]); } struct key_type key_type_encrypted = { -- cgit v1.2.3 From ee618b4619b72527aaed765f0f0b74072b281159 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 8 Jun 2017 14:49:18 +0100 Subject: KEYS: trusted: sanitize all key material As the previous patch did for encrypted-keys, zero sensitive any potentially sensitive data related to the "trusted" key type before it is freed. Notably, we were not zeroing the tpm_buf structures in which the actual key is stored for TPM seal and unseal, nor were we zeroing the trusted_key_payload in certain error paths. Cc: Mimi Zohar Cc: David Safford Signed-off-by: Eric Biggers Signed-off-by: David Howells Signed-off-by: James Morris --- security/keys/trusted.c | 50 ++++++++++++++++++++++--------------------------- 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/security/keys/trusted.c b/security/keys/trusted.c index 2ae31c5a87de..435e86e13879 100644 --- a/security/keys/trusted.c +++ b/security/keys/trusted.c @@ -70,7 +70,7 @@ static int TSS_sha1(const unsigned char *data, unsigned int datalen, } ret = crypto_shash_digest(&sdesc->shash, data, datalen, digest); - kfree(sdesc); + kzfree(sdesc); return ret; } @@ -114,7 +114,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key, if (!ret) ret = crypto_shash_final(&sdesc->shash, digest); out: - kfree(sdesc); + kzfree(sdesc); return ret; } @@ -165,7 +165,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key, paramdigest, TPM_NONCE_SIZE, h1, TPM_NONCE_SIZE, h2, 1, &c, 0, 0); out: - kfree(sdesc); + kzfree(sdesc); return ret; } @@ -246,7 +246,7 @@ static int TSS_checkhmac1(unsigned char *buffer, if (memcmp(testhmac, authdata, SHA1_DIGEST_SIZE)) ret = -EINVAL; out: - kfree(sdesc); + kzfree(sdesc); return ret; } @@ -347,7 +347,7 @@ static int TSS_checkhmac2(unsigned char *buffer, if (memcmp(testhmac2, authdata2, SHA1_DIGEST_SIZE)) ret = -EINVAL; out: - kfree(sdesc); + kzfree(sdesc); return ret; } @@ -564,7 +564,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype, *bloblen = storedsize; } out: - kfree(td); + kzfree(td); return ret; } @@ -678,7 +678,7 @@ static int key_seal(struct trusted_key_payload *p, if (ret < 0) pr_info("trusted_key: srkseal failed (%d)\n", ret); - kfree(tb); + kzfree(tb); return ret; } @@ -703,7 +703,7 @@ static int key_unseal(struct trusted_key_payload *p, /* pull migratable flag out of sealed key */ p->migratable = p->key[--p->key_len]; - kfree(tb); + kzfree(tb); return ret; } @@ -1037,12 +1037,12 @@ static int trusted_instantiate(struct key *key, if (!ret && options->pcrlock) ret = pcrlock(options->pcrlock); out: - kfree(datablob); - kfree(options); + kzfree(datablob); + kzfree(options); if (!ret) rcu_assign_keypointer(key, payload); else - kfree(payload); + kzfree(payload); return ret; } @@ -1051,8 +1051,7 @@ static void trusted_rcu_free(struct rcu_head *rcu) struct trusted_key_payload *p; p = container_of(rcu, struct trusted_key_payload, rcu); - memset(p->key, 0, p->key_len); - kfree(p); + kzfree(p); } /* @@ -1094,13 +1093,13 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep) ret = datablob_parse(datablob, new_p, new_o); if (ret != Opt_update) { ret = -EINVAL; - kfree(new_p); + kzfree(new_p); goto out; } if (!new_o->keyhandle) { ret = -EINVAL; - kfree(new_p); + kzfree(new_p); goto out; } @@ -1114,22 +1113,22 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep) ret = key_seal(new_p, new_o); if (ret < 0) { pr_info("trusted_key: key_seal failed (%d)\n", ret); - kfree(new_p); + kzfree(new_p); goto out; } if (new_o->pcrlock) { ret = pcrlock(new_o->pcrlock); if (ret < 0) { pr_info("trusted_key: pcrlock failed (%d)\n", ret); - kfree(new_p); + kzfree(new_p); goto out; } } rcu_assign_keypointer(key, new_p); call_rcu(&p->rcu, trusted_rcu_free); out: - kfree(datablob); - kfree(new_o); + kzfree(datablob); + kzfree(new_o); return ret; } @@ -1158,24 +1157,19 @@ static long trusted_read(const struct key *key, char __user *buffer, for (i = 0; i < p->blob_len; i++) bufp = hex_byte_pack(bufp, p->blob[i]); if ((copy_to_user(buffer, ascii_buf, 2 * p->blob_len)) != 0) { - kfree(ascii_buf); + kzfree(ascii_buf); return -EFAULT; } - kfree(ascii_buf); + kzfree(ascii_buf); return 2 * p->blob_len; } /* - * trusted_destroy - before freeing the key, clear the decrypted data + * trusted_destroy - clear and free the key's payload */ static void trusted_destroy(struct key *key) { - struct trusted_key_payload *p = key->payload.data[0]; - - if (!p) - return; - memset(p->key, 0, p->key_len); - kfree(key->payload.data[0]); + kzfree(key->payload.data[0]); } struct key_type key_type_trusted = { -- cgit v1.2.3 From 0620fddb56dfaf0e1034eeb69d79c73b361debbf Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 8 Jun 2017 14:49:26 +0100 Subject: KEYS: sanitize key structs before freeing While a 'struct key' itself normally does not contain sensitive information, Documentation/security/keys.txt actually encourages this: "Having a payload is not required; and the payload can, in fact, just be a value stored in the struct key itself." In case someone has taken this advice, or will take this advice in the future, zero the key structure before freeing it. We might as well, and as a bonus this could make it a bit more difficult for an adversary to determine which keys have recently been in use. This is safe because the key_jar cache does not use a constructor. Signed-off-by: Eric Biggers Signed-off-by: David Howells Signed-off-by: James Morris --- include/linux/key.h | 1 - security/keys/gc.c | 4 +--- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/include/linux/key.h b/include/linux/key.h index 0c9b93b0d1f7..78e25aabedaf 100644 --- a/include/linux/key.h +++ b/include/linux/key.h @@ -173,7 +173,6 @@ struct key { #ifdef KEY_DEBUGGING unsigned magic; #define KEY_DEBUG_MAGIC 0x18273645u -#define KEY_DEBUG_MAGIC_X 0xf8e9dacbu #endif unsigned long flags; /* status flags (change with bitops) */ diff --git a/security/keys/gc.c b/security/keys/gc.c index 595becc6d0d2..87cb260e4890 100644 --- a/security/keys/gc.c +++ b/security/keys/gc.c @@ -158,9 +158,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys) kfree(key->description); -#ifdef KEY_DEBUGGING - key->magic = KEY_DEBUG_MAGIC_X; -#endif + memzero_explicit(key, sizeof(*key)); kmem_cache_free(key_jar, key); } } -- cgit v1.2.3 From bbe240454d86be95151e0ecfd6ac55fe5ef5a6f5 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 8 Jun 2017 14:49:34 +0100 Subject: KEYS: DH: forbid using digest_null as the KDF hash Requesting "digest_null" in the keyctl_kdf_params caused an infinite loop in kdf_ctr() because the "null" hash has a digest size of 0. Fix it by rejecting hash algorithms with a digest size of 0. Signed-off-by: Eric Biggers Signed-off-by: David Howells Acked-by: Stephan Mueller Signed-off-by: James Morris --- security/keys/dh.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/security/keys/dh.c b/security/keys/dh.c index e603bd912e4c..8abc70ebe22d 100644 --- a/security/keys/dh.c +++ b/security/keys/dh.c @@ -89,6 +89,7 @@ static int kdf_alloc(struct kdf_sdesc **sdesc_ret, char *hashname) struct crypto_shash *tfm; struct kdf_sdesc *sdesc; int size; + int err; /* allocate synchronous hash */ tfm = crypto_alloc_shash(hashname, 0, 0); @@ -97,16 +98,25 @@ static int kdf_alloc(struct kdf_sdesc **sdesc_ret, char *hashname) return PTR_ERR(tfm); } + err = -EINVAL; + if (crypto_shash_digestsize(tfm) == 0) + goto out_free_tfm; + + err = -ENOMEM; size = sizeof(struct shash_desc) + crypto_shash_descsize(tfm); sdesc = kmalloc(size, GFP_KERNEL); if (!sdesc) - return -ENOMEM; + goto out_free_tfm; sdesc->shash.tfm = tfm; sdesc->shash.flags = 0x0; *sdesc_ret = sdesc; return 0; + +out_free_tfm: + crypto_free_shash(tfm); + return err; } static void kdf_dealloc(struct kdf_sdesc *sdesc) -- cgit v1.2.3 From 281590b4221779dbc4a5e2c33c0c5b0239cfe794 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 8 Jun 2017 14:49:42 +0100 Subject: KEYS: DH: don't feed uninitialized "otherinfo" into KDF If userspace called KEYCTL_DH_COMPUTE with kdf_params containing NULL otherinfo but nonzero otherinfolen, the kernel would allocate a buffer for the otherinfo, then feed it into the KDF without initializing it. Fix this by always doing the copy from userspace (which will fail with EFAULT in this scenario). Signed-off-by: Eric Biggers Signed-off-by: David Howells Acked-by: Stephan Mueller Signed-off-by: James Morris --- security/keys/dh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/keys/dh.c b/security/keys/dh.c index 8abc70ebe22d..1c1cac677041 100644 --- a/security/keys/dh.c +++ b/security/keys/dh.c @@ -317,7 +317,7 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params, * Concatenate SP800-56A otherinfo past DH shared secret -- the * input to the KDF is (DH shared secret || otherinfo) */ - if (kdfcopy && kdfcopy->otherinfo && + if (kdfcopy && copy_from_user(kbuf + resultlen, kdfcopy->otherinfo, kdfcopy->otherinfolen) != 0) { ret = -EFAULT; -- cgit v1.2.3 From 0ddd9f1a6b7e5746b52959f645fc66859b805e88 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 8 Jun 2017 14:49:49 +0100 Subject: KEYS: DH: ensure the KDF counter is properly aligned Accessing a 'u8[4]' through a '__be32 *' violates alignment rules. Just make the counter a __be32 instead. Signed-off-by: Eric Biggers Signed-off-by: David Howells Acked-by: Stephan Mueller Signed-off-by: James Morris --- security/keys/dh.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/security/keys/dh.c b/security/keys/dh.c index 1c1cac677041..63ac87d430db 100644 --- a/security/keys/dh.c +++ b/security/keys/dh.c @@ -130,14 +130,6 @@ static void kdf_dealloc(struct kdf_sdesc *sdesc) kzfree(sdesc); } -/* convert 32 bit integer into its string representation */ -static inline void crypto_kw_cpu_to_be32(u32 val, u8 *buf) -{ - __be32 *a = (__be32 *)buf; - - *a = cpu_to_be32(val); -} - /* * Implementation of the KDF in counter mode according to SP800-108 section 5.1 * as well as SP800-56A section 5.8.1 (Single-step KDF). @@ -154,16 +146,14 @@ static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, unsigned int slen, unsigned int h = crypto_shash_digestsize(desc->tfm); int err = 0; u8 *dst_orig = dst; - u32 i = 1; - u8 iteration[sizeof(u32)]; + __be32 counter = cpu_to_be32(1); while (dlen) { err = crypto_shash_init(desc); if (err) goto err; - crypto_kw_cpu_to_be32(i, iteration); - err = crypto_shash_update(desc, iteration, sizeof(u32)); + err = crypto_shash_update(desc, (u8 *)&counter, sizeof(__be32)); if (err) goto err; @@ -189,7 +179,7 @@ static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, unsigned int slen, dlen -= h; dst += h; - i++; + counter = cpu_to_be32(be32_to_cpu(counter) + 1); } } -- cgit v1.2.3 From a77395447b0aeab9473a066ff28fbee01130206b Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 8 Jun 2017 14:49:57 +0100 Subject: KEYS: DH: add __user annotations to keyctl_kdf_params Signed-off-by: Eric Biggers Signed-off-by: David Howells Acked-by: Stephan Mueller Signed-off-by: James Morris --- include/uapi/linux/keyctl.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h index 201c6644b237..ef16df06642a 100644 --- a/include/uapi/linux/keyctl.h +++ b/include/uapi/linux/keyctl.h @@ -70,8 +70,8 @@ struct keyctl_dh_params { }; struct keyctl_kdf_params { - char *hashname; - char *otherinfo; + char __user *hashname; + char __user *otherinfo; __u32 otherinfolen; __u32 __spare[8]; }; -- cgit v1.2.3 From da7798a7b6714cf35ea6084ad6b1fe0497566cf2 Mon Sep 17 00:00:00 2001 From: Loganaden Velvindron Date: Thu, 8 Jun 2017 14:50:04 +0100 Subject: crypto : asymmetric_keys : verify_pefile:zero memory content before freeing Signed-off-by: Loganaden Velvindron Signed-off-by: Yasir Auleear Signed-off-by: David Howells Signed-off-by: James Morris --- crypto/asymmetric_keys/verify_pefile.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crypto/asymmetric_keys/verify_pefile.c b/crypto/asymmetric_keys/verify_pefile.c index 672a94c2c3ff..d178650fd524 100644 --- a/crypto/asymmetric_keys/verify_pefile.c +++ b/crypto/asymmetric_keys/verify_pefile.c @@ -381,7 +381,7 @@ static int pefile_digest_pe(const void *pebuf, unsigned int pelen, } error: - kfree(desc); + kzfree(desc); error_no_desc: crypto_free_shash(tfm); kleave(" = %d", ret); @@ -450,6 +450,6 @@ int verify_pefile_signature(const void *pebuf, unsigned pelen, ret = pefile_digest_pe(pebuf, pelen, &ctx); error: - kfree(ctx.digest); + kzfree(ctx.digest); return ret; } -- cgit v1.2.3 From 7cbe0932c2f2014d6e24e716e79ea3910b468950 Mon Sep 17 00:00:00 2001 From: Mat Martineau Date: Thu, 8 Jun 2017 14:50:11 +0100 Subject: KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP API The initial Diffie-Hellman computation made direct use of the MPI library because the crypto module did not support DH at the time. Now that KPP is implemented, KEYCTL_DH_COMPUTE should use it to get rid of duplicate code and leverage possible hardware acceleration. This fixes an issue whereby the input to the KDF computation would include additional uninitialized memory when the result of the Diffie-Hellman computation was shorter than the input prime number. Signed-off-by: Mat Martineau Signed-off-by: David Howells Signed-off-by: James Morris --- security/keys/Kconfig | 2 +- security/keys/dh.c | 272 +++++++++++++++++++++++++++++++------------------- 2 files changed, 171 insertions(+), 103 deletions(-) diff --git a/security/keys/Kconfig b/security/keys/Kconfig index 00b7431a8aeb..a7a23b5541f8 100644 --- a/security/keys/Kconfig +++ b/security/keys/Kconfig @@ -93,9 +93,9 @@ config ENCRYPTED_KEYS config KEY_DH_OPERATIONS bool "Diffie-Hellman operations on retained keys" depends on KEYS - select MPILIB select CRYPTO select CRYPTO_HASH + select CRYPTO_DH help This option provides support for calculating Diffie-Hellman public keys and shared secrets using values stored as keys diff --git a/security/keys/dh.c b/security/keys/dh.c index 63ac87d430db..4755d4b4f945 100644 --- a/security/keys/dh.c +++ b/security/keys/dh.c @@ -8,34 +8,17 @@ * 2 of the License, or (at your option) any later version. */ -#include #include #include +#include #include #include +#include +#include #include #include "internal.h" -/* - * Public key or shared secret generation function [RFC2631 sec 2.1.1] - * - * ya = g^xa mod p; - * or - * ZZ = yb^xa mod p; - * - * where xa is the local private key, ya is the local public key, g is - * the generator, p is the prime, yb is the remote public key, and ZZ - * is the shared secret. - * - * Both are the same calculation, so g or yb are the "base" and ya or - * ZZ are the "result". - */ -static int do_dh(MPI result, MPI base, MPI xa, MPI p) -{ - return mpi_powm(result, base, xa, p); -} - -static ssize_t mpi_from_key(key_serial_t keyid, size_t maxlen, MPI *mpi) +static ssize_t dh_data_from_key(key_serial_t keyid, void **data) { struct key *key; key_ref_t key_ref; @@ -56,19 +39,17 @@ static ssize_t mpi_from_key(key_serial_t keyid, size_t maxlen, MPI *mpi) status = key_validate(key); if (status == 0) { const struct user_key_payload *payload; + uint8_t *duplicate; payload = user_key_payload_locked(key); - if (maxlen == 0) { - *mpi = NULL; + duplicate = kmemdup(payload->data, payload->datalen, + GFP_KERNEL); + if (duplicate) { + *data = duplicate; ret = payload->datalen; - } else if (payload->datalen <= maxlen) { - *mpi = mpi_read_raw_data(payload->data, - payload->datalen); - if (*mpi) - ret = payload->datalen; } else { - ret = -EINVAL; + ret = -ENOMEM; } } up_read(&key->sem); @@ -79,6 +60,29 @@ error: return ret; } +static void dh_free_data(struct dh *dh) +{ + kzfree(dh->key); + kzfree(dh->p); + kzfree(dh->g); +} + +struct dh_completion { + struct completion completion; + int err; +}; + +static void dh_crypto_done(struct crypto_async_request *req, int err) +{ + struct dh_completion *compl = req->data; + + if (err == -EINPROGRESS) + return; + + compl->err = err; + complete(&compl->completion); +} + struct kdf_sdesc { struct shash_desc shash; char ctx[]; @@ -140,7 +144,7 @@ static void kdf_dealloc(struct kdf_sdesc *sdesc) * 5.8.1.2). */ static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, unsigned int slen, - u8 *dst, unsigned int dlen) + u8 *dst, unsigned int dlen, unsigned int zlen) { struct shash_desc *desc = &sdesc->shash; unsigned int h = crypto_shash_digestsize(desc->tfm); @@ -157,6 +161,22 @@ static int kdf_ctr(struct kdf_sdesc *sdesc, const u8 *src, unsigned int slen, if (err) goto err; + if (zlen && h) { + u8 tmpbuffer[h]; + size_t chunk = min_t(size_t, zlen, h); + memset(tmpbuffer, 0, chunk); + + do { + err = crypto_shash_update(desc, tmpbuffer, + chunk); + if (err) + goto err; + + zlen -= chunk; + chunk = min_t(size_t, zlen, h); + } while (zlen); + } + if (src && slen) { err = crypto_shash_update(desc, src, slen); if (err) @@ -192,7 +212,7 @@ err: static int keyctl_dh_compute_kdf(struct kdf_sdesc *sdesc, char __user *buffer, size_t buflen, - uint8_t *kbuf, size_t kbuflen) + uint8_t *kbuf, size_t kbuflen, size_t lzero) { uint8_t *outbuf = NULL; int ret; @@ -203,7 +223,7 @@ static int keyctl_dh_compute_kdf(struct kdf_sdesc *sdesc, goto err; } - ret = kdf_ctr(sdesc, kbuf, kbuflen, outbuf, buflen); + ret = kdf_ctr(sdesc, kbuf, kbuflen, outbuf, buflen, lzero); if (ret) goto err; @@ -221,21 +241,26 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params, struct keyctl_kdf_params *kdfcopy) { long ret; - MPI base, private, prime, result; - unsigned nbytes; + ssize_t dlen; + int secretlen; + int outlen; struct keyctl_dh_params pcopy; - uint8_t *kbuf; - ssize_t keylen; - size_t resultlen; + struct dh dh_inputs; + struct scatterlist outsg; + struct dh_completion compl; + struct crypto_kpp *tfm; + struct kpp_request *req; + uint8_t *secret; + uint8_t *outbuf; struct kdf_sdesc *sdesc = NULL; if (!params || (!buffer && buflen)) { ret = -EINVAL; - goto out; + goto out1; } if (copy_from_user(&pcopy, params, sizeof(pcopy)) != 0) { ret = -EFAULT; - goto out; + goto out1; } if (kdfcopy) { @@ -244,104 +269,147 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params, if (buflen > KEYCTL_KDF_MAX_OUTPUT_LEN || kdfcopy->otherinfolen > KEYCTL_KDF_MAX_OI_LEN) { ret = -EMSGSIZE; - goto out; + goto out1; } /* get KDF name string */ hashname = strndup_user(kdfcopy->hashname, CRYPTO_MAX_ALG_NAME); if (IS_ERR(hashname)) { ret = PTR_ERR(hashname); - goto out; + goto out1; } /* allocate KDF from the kernel crypto API */ ret = kdf_alloc(&sdesc, hashname); kfree(hashname); if (ret) - goto out; + goto out1; } - /* - * If the caller requests postprocessing with a KDF, allow an - * arbitrary output buffer size since the KDF ensures proper truncation. - */ - keylen = mpi_from_key(pcopy.prime, kdfcopy ? SIZE_MAX : buflen, &prime); - if (keylen < 0 || !prime) { - /* buflen == 0 may be used to query the required buffer size, - * which is the prime key length. - */ - ret = keylen; - goto out; + memset(&dh_inputs, 0, sizeof(dh_inputs)); + + dlen = dh_data_from_key(pcopy.prime, &dh_inputs.p); + if (dlen < 0) { + ret = dlen; + goto out1; } + dh_inputs.p_size = dlen; - /* The result is never longer than the prime */ - resultlen = keylen; + dlen = dh_data_from_key(pcopy.base, &dh_inputs.g); + if (dlen < 0) { + ret = dlen; + goto out2; + } + dh_inputs.g_size = dlen; - keylen = mpi_from_key(pcopy.base, SIZE_MAX, &base); - if (keylen < 0 || !base) { - ret = keylen; - goto error1; + dlen = dh_data_from_key(pcopy.private, &dh_inputs.key); + if (dlen < 0) { + ret = dlen; + goto out2; } + dh_inputs.key_size = dlen; - keylen = mpi_from_key(pcopy.private, SIZE_MAX, &private); - if (keylen < 0 || !private) { - ret = keylen; - goto error2; + secretlen = crypto_dh_key_len(&dh_inputs); + secret = kmalloc(secretlen, GFP_KERNEL); + if (!secret) { + ret = -ENOMEM; + goto out2; + } + ret = crypto_dh_encode_key(secret, secretlen, &dh_inputs); + if (ret) + goto out3; + + tfm = crypto_alloc_kpp("dh", CRYPTO_ALG_TYPE_KPP, 0); + if (IS_ERR(tfm)) { + ret = PTR_ERR(tfm); + goto out3; + } + + ret = crypto_kpp_set_secret(tfm, secret, secretlen); + if (ret) + goto out4; + + outlen = crypto_kpp_maxsize(tfm); + + if (!kdfcopy) { + /* + * When not using a KDF, buflen 0 is used to read the + * required buffer length + */ + if (buflen == 0) { + ret = outlen; + goto out4; + } else if (outlen > buflen) { + ret = -EOVERFLOW; + goto out4; + } } - result = mpi_alloc(0); - if (!result) { + outbuf = kzalloc(kdfcopy ? (outlen + kdfcopy->otherinfolen) : outlen, + GFP_KERNEL); + if (!outbuf) { ret = -ENOMEM; - goto error3; + goto out4; } - /* allocate space for DH shared secret and SP800-56A otherinfo */ - kbuf = kmalloc(kdfcopy ? (resultlen + kdfcopy->otherinfolen) : resultlen, - GFP_KERNEL); - if (!kbuf) { + sg_init_one(&outsg, outbuf, outlen); + + req = kpp_request_alloc(tfm, GFP_KERNEL); + if (!req) { ret = -ENOMEM; - goto error4; + goto out5; } + kpp_request_set_input(req, NULL, 0); + kpp_request_set_output(req, &outsg, outlen); + init_completion(&compl.completion); + kpp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG | + CRYPTO_TFM_REQ_MAY_SLEEP, + dh_crypto_done, &compl); + /* - * Concatenate SP800-56A otherinfo past DH shared secret -- the - * input to the KDF is (DH shared secret || otherinfo) + * For DH, generate_public_key and generate_shared_secret are + * the same calculation */ - if (kdfcopy && - copy_from_user(kbuf + resultlen, kdfcopy->otherinfo, - kdfcopy->otherinfolen) != 0) { - ret = -EFAULT; - goto error5; + ret = crypto_kpp_generate_public_key(req); + if (ret == -EINPROGRESS) { + wait_for_completion(&compl.completion); + ret = compl.err; + if (ret) + goto out6; } - ret = do_dh(result, base, private, prime); - if (ret) - goto error5; - - ret = mpi_read_buffer(result, kbuf, resultlen, &nbytes, NULL); - if (ret != 0) - goto error5; - if (kdfcopy) { - ret = keyctl_dh_compute_kdf(sdesc, buffer, buflen, kbuf, - resultlen + kdfcopy->otherinfolen); - } else { - ret = nbytes; - if (copy_to_user(buffer, kbuf, nbytes) != 0) + /* + * Concatenate SP800-56A otherinfo past DH shared secret -- the + * input to the KDF is (DH shared secret || otherinfo) + */ + if (copy_from_user(outbuf + req->dst_len, kdfcopy->otherinfo, + kdfcopy->otherinfolen) != 0) { ret = -EFAULT; + goto out6; + } + + ret = keyctl_dh_compute_kdf(sdesc, buffer, buflen, outbuf, + req->dst_len + kdfcopy->otherinfolen, + outlen - req->dst_len); + } else if (copy_to_user(buffer, outbuf, req->dst_len) == 0) { + ret = req->dst_len; + } else { + ret = -EFAULT; } -error5: - kzfree(kbuf); -error4: - mpi_free(result); -error3: - mpi_free(private); -error2: - mpi_free(base); -error1: - mpi_free(prime); -out: +out6: + kpp_request_free(req); +out5: + kzfree(outbuf); +out4: + crypto_free_kpp(tfm); +out3: + kzfree(secret); +out2: + dh_free_data(&dh_inputs); +out1: kdf_dealloc(sdesc); return ret; } -- cgit v1.2.3 From 92347cfd62c174ab91ad97dd4bfbaa1d4aa28e67 Mon Sep 17 00:00:00 2001 From: Mark Rutland Date: Thu, 8 Jun 2017 14:47:41 +0100 Subject: KEYS: fix refcount_inc() on zero If a key's refcount is dropped to zero between key_lookup() peeking at the refcount and subsequently attempting to increment it, refcount_inc() will see a zero refcount. Here, refcount_inc() will WARN_ONCE(), and will *not* increment the refcount, which will remain zero. Once key_lookup() drops key_serial_lock, it is possible for the key to be freed behind our back. This patch uses refcount_inc_not_zero() to perform the peek and increment atomically. Fixes: fff292914d3a2f1e ("security, keys: convert key.usage from atomic_t to refcount_t") Signed-off-by: Mark Rutland Signed-off-by: David Howells Cc: David Windsor Cc: Elena Reshetova Cc: Hans Liljestrand Cc: James Morris Cc: Kees Cook Cc: Peter Zijlstra Signed-off-by: James Morris --- security/keys/key.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/security/keys/key.c b/security/keys/key.c index cbae368c0e57..83da68d98b40 100644 --- a/security/keys/key.c +++ b/security/keys/key.c @@ -660,14 +660,11 @@ not_found: goto error; found: - /* pretend it doesn't exist if it is awaiting deletion */ - if (refcount_read(&key->usage) == 0) - goto not_found; - - /* this races with key_put(), but that doesn't matter since key_put() - * doesn't actually change the key + /* A key is allowed to be looked up only if someone still owns a + * reference to it - otherwise it's awaiting the gc. */ - __key_get(key); + if (!refcount_inc_not_zero(&key->usage)) + goto not_found; error: spin_unlock(&key_serial_lock); -- cgit v1.2.3