Commit d76155ae authored by Eric Biggers's avatar Eric Biggers Committed by David Howells

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 <zohar@linux.vnet.ibm.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
Signed-off-by: default avatarDavid Howells <dhowells@redhat.com>
parent f2173234
...@@ -54,13 +54,7 @@ static int blksize; ...@@ -54,13 +54,7 @@ static int blksize;
#define MAX_DATA_SIZE 4096 #define MAX_DATA_SIZE 4096
#define MIN_DATA_SIZE 20 #define MIN_DATA_SIZE 20
struct sdesc { static struct crypto_shash *hash_tfm;
struct shash_desc shash;
char ctx[];
};
static struct crypto_shash *hashalg;
static struct crypto_shash *hmacalg;
enum { enum {
Opt_err = -1, Opt_new, Opt_load, Opt_update 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 ...@@ -320,53 +314,38 @@ static struct key *request_user_key(const char *master_desc, const u8 **master_k
return ukey; return ukey;
} }
static struct sdesc *alloc_sdesc(struct crypto_shash *alg) static int calc_hash(struct crypto_shash *tfm, u8 *digest,
{
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,
const u8 *buf, unsigned int buflen) const u8 *buf, unsigned int buflen)
{ {
struct sdesc *sdesc; SHASH_DESC_ON_STACK(desc, tfm);
int ret; int err;
sdesc = alloc_sdesc(hmacalg); desc->tfm = tfm;
if (IS_ERR(sdesc)) { desc->flags = 0;
pr_info("encrypted_key: can't alloc %s\n", hmac_alg);
return PTR_ERR(sdesc);
}
ret = crypto_shash_setkey(hmacalg, key, keylen); err = crypto_shash_digest(desc, buf, buflen, digest);
if (!ret) shash_desc_zero(desc);
ret = crypto_shash_digest(&sdesc->shash, buf, buflen, digest); return err;
kfree(sdesc);
return ret;
} }
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; struct crypto_shash *tfm;
int ret; int err;
sdesc = alloc_sdesc(hashalg); tfm = crypto_alloc_shash(hmac_alg, 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(sdesc)) { if (IS_ERR(tfm)) {
pr_info("encrypted_key: can't alloc %s\n", hash_alg); pr_err("encrypted_key: can't alloc %s transform: %ld\n",
return PTR_ERR(sdesc); hmac_alg, PTR_ERR(tfm));
return PTR_ERR(tfm);
} }
ret = crypto_shash_digest(&sdesc->shash, buf, buflen, digest); err = crypto_shash_setkey(tfm, key, keylen);
kfree(sdesc); if (!err)
return ret; err = calc_hash(tfm, digest, buf, buflen);
crypto_free_shash(tfm);
return err;
} }
enum derived_key_type { ENC_KEY, AUTH_KEY }; 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, ...@@ -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, memcpy(derived_buf + strlen(derived_buf) + 1, master_key,
master_keylen); 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); kfree(derived_buf);
return ret; return ret;
} }
...@@ -998,47 +977,17 @@ struct key_type key_type_encrypted = { ...@@ -998,47 +977,17 @@ struct key_type key_type_encrypted = {
}; };
EXPORT_SYMBOL_GPL(key_type_encrypted); EXPORT_SYMBOL_GPL(key_type_encrypted);
static void encrypted_shash_release(void) static int __init init_encrypted(void)
{
if (hashalg)
crypto_free_shash(hashalg);
if (hmacalg)
crypto_free_shash(hmacalg);
}
static int __init encrypted_shash_alloc(void)
{ {
int ret; int ret;
hmacalg = crypto_alloc_shash(hmac_alg, 0, CRYPTO_ALG_ASYNC); hash_tfm = crypto_alloc_shash(hash_alg, 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(hmacalg)) { if (IS_ERR(hash_tfm)) {
pr_info("encrypted_key: could not allocate crypto %s\n", pr_err("encrypted_key: can't allocate %s transform: %ld\n",
hmac_alg); hash_alg, PTR_ERR(hash_tfm));
return PTR_ERR(hmacalg); 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(); ret = aes_get_sizes();
if (ret < 0) if (ret < 0)
goto out; goto out;
...@@ -1047,14 +996,14 @@ static int __init init_encrypted(void) ...@@ -1047,14 +996,14 @@ static int __init init_encrypted(void)
goto out; goto out;
return 0; return 0;
out: out:
encrypted_shash_release(); crypto_free_shash(hash_tfm);
return ret; return ret;
} }
static void __exit cleanup_encrypted(void) static void __exit cleanup_encrypted(void)
{ {
encrypted_shash_release(); crypto_free_shash(hash_tfm);
unregister_key_type(&key_type_encrypted); unregister_key_type(&key_type_encrypted);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment