Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752342AbdFHNz2 (ORCPT ); Thu, 8 Jun 2017 09:55:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43276 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752059AbdFHNsn (ORCPT ); Thu, 8 Jun 2017 09:48:43 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com A6A3DDC91C Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=dhowells@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com A6A3DDC91C Organization: Red Hat UK Ltd. Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SI4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 3798903 Subject: [PATCH 09/23] KEYS: encrypted: fix race causing incorrect HMAC calculations From: David Howells To: jmorris@namei.org Cc: Herbert Xu , Eric Biggers , linux-kernel@vger.kernel.org, dhowells@redhat.com, linux-security-module@vger.kernel.org, keyrings@vger.kernel.org, Mimi Zohar Date: Thu, 08 Jun 2017 14:48:25 +0100 Message-ID: <149692970541.11452.13378862411770263162.stgit@warthog.procyon.org.uk> In-Reply-To: <149692963884.11452.7673998701432248814.stgit@warthog.procyon.org.uk> References: <149692963884.11452.7673998701432248814.stgit@warthog.procyon.org.uk> User-Agent: StGit/0.17.1-dirty MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 08 Jun 2017 13:48:27 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5971 Lines: 216 From: Eric Biggers 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 --- 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 @@ static struct key *request_user_key(const char *master_desc, const u8 **master_k 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); }