lkml.org 
[lkml]   [2017]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Subject[PATCH 09/23] KEYS: encrypted: fix race causing incorrect HMAC calculations
    From
    Date
    From: Eric Biggers <ebiggers@google.com>

    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: Eric Biggers <ebiggers@google.com>
    Signed-off-by: David Howells <dhowells@redhat.com>
    ---

    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);
    }

    \
     
     \ /
      Last update: 2017-06-12 01:29    [W:3.462 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site