Commit 428490e3 authored by Jason A. Donenfeld's avatar Jason A. Donenfeld Committed by David Howells

security/keys: rewrite all of big_key crypto

This started out as just replacing the use of crypto/rng with
get_random_bytes_wait, 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.

It makes no sense to seed crypto/rng at boot time and then keep
using it like this, when in fact there's already get_random_bytes_wait,
which can ensure there's enough entropy and be a much more standard way
of generating keys. Since this sensitive material is being stored
untrusted, using ECB and no authentication is simply not okay at all. I
find it surprising and a bit horrifying that this code even made it past
basic crypto review, which perhaps points to some larger issues. 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.

So, to summarize, this commit fixes the following vulnerabilities:

  * Low entropy key generation, allowing an attacker to potentially
    guess or predict keys.
  * 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.
Signed-off-by: default avatarJason A. Donenfeld <Jason@zx2c4.com>
Reviewed-by: default avatarEric Biggers <ebiggers3@gmail.com>
Signed-off-by: default avatarDavid Howells <dhowells@redhat.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Kirill Marinushkin <k.marinushkin@gmail.com>
Cc: security@kernel.org
Cc: stable@vger.kernel.org
parent 91080180
...@@ -45,10 +45,8 @@ config BIG_KEYS ...@@ -45,10 +45,8 @@ config BIG_KEYS
bool "Large payload keys" bool "Large payload keys"
depends on KEYS depends on KEYS
depends on TMPFS depends on TMPFS
depends on (CRYPTO_ANSI_CPRNG = y || CRYPTO_DRBG = y)
select CRYPTO_AES select CRYPTO_AES
select CRYPTO_ECB select CRYPTO_GCM
select CRYPTO_RNG
help help
This option provides support for holding large keys within the kernel This option provides support for holding large keys within the kernel
(for example Kerberos ticket caches). The data may be stored out to (for example Kerberos ticket caches). The data may be stored out to
......
/* Large capacity key type /* Large capacity key type
* *
* Copyright (C) 2017 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
* Copyright (C) 2013 Red Hat, Inc. All Rights Reserved. * Copyright (C) 2013 Red Hat, Inc. All Rights Reserved.
* Written by David Howells (dhowells@redhat.com) * Written by David Howells (dhowells@redhat.com)
* *
...@@ -16,10 +17,10 @@ ...@@ -16,10 +17,10 @@
#include <linux/shmem_fs.h> #include <linux/shmem_fs.h>
#include <linux/err.h> #include <linux/err.h>
#include <linux/scatterlist.h> #include <linux/scatterlist.h>
#include <linux/random.h>
#include <keys/user-type.h> #include <keys/user-type.h>
#include <keys/big_key-type.h> #include <keys/big_key-type.h>
#include <crypto/rng.h> #include <crypto/aead.h>
#include <crypto/skcipher.h>
/* /*
* Layout of key payload words. * Layout of key payload words.
...@@ -49,7 +50,12 @@ enum big_key_op { ...@@ -49,7 +50,12 @@ enum big_key_op {
/* /*
* Key size for big_key data encryption * Key size for big_key data encryption
*/ */
#define ENC_KEY_SIZE 16 #define ENC_KEY_SIZE 32
/*
* Authentication tag length
*/
#define ENC_AUTHTAG_SIZE 16
/* /*
* big_key defined keys take an arbitrary string as the description and an * big_key defined keys take an arbitrary string as the description and an
...@@ -64,57 +70,62 @@ struct key_type key_type_big_key = { ...@@ -64,57 +70,62 @@ struct key_type key_type_big_key = {
.destroy = big_key_destroy, .destroy = big_key_destroy,
.describe = big_key_describe, .describe = big_key_describe,
.read = big_key_read, .read = big_key_read,
/* no ->update(); don't add it without changing big_key_crypt() nonce */
}; };
/* /*
* Crypto names for big_key data encryption * Crypto names for big_key data authenticated encryption
*/ */
static const char big_key_rng_name[] = "stdrng"; static const char big_key_alg_name[] = "gcm(aes)";
static const char big_key_alg_name[] = "ecb(aes)";
/* /*
* Crypto algorithms for big_key data encryption * Crypto algorithms for big_key data authenticated encryption
*/ */
static struct crypto_rng *big_key_rng; static struct crypto_aead *big_key_aead;
static struct crypto_skcipher *big_key_skcipher;
/* /*
* Generate random key to encrypt big_key data * Since changing the key affects the entire object, we need a mutex.
*/ */
static inline int big_key_gen_enckey(u8 *key) static DEFINE_MUTEX(big_key_aead_lock);
{
return crypto_rng_get_bytes(big_key_rng, key, ENC_KEY_SIZE);
}
/* /*
* Encrypt/decrypt big_key data * Encrypt/decrypt big_key data
*/ */
static int big_key_crypt(enum big_key_op op, u8 *data, size_t datalen, u8 *key) static int big_key_crypt(enum big_key_op op, u8 *data, size_t datalen, u8 *key)
{ {
int ret = -EINVAL; int ret;
struct scatterlist sgio; struct scatterlist sgio;
SKCIPHER_REQUEST_ON_STACK(req, big_key_skcipher); struct aead_request *aead_req;
/* We always use a zero nonce. The reason we can get away with this is
if (crypto_skcipher_setkey(big_key_skcipher, key, ENC_KEY_SIZE)) { * because we're using a different randomly generated key for every
* different encryption. Notably, too, key_type_big_key doesn't define
* an .update function, so there's no chance we'll wind up reusing the
* key to encrypt updated data. Simply put: one key, one encryption.
*/
u8 zero_nonce[crypto_aead_ivsize(big_key_aead)];
aead_req = aead_request_alloc(big_key_aead, GFP_KERNEL);
if (!aead_req)
return -ENOMEM;
memset(zero_nonce, 0, sizeof(zero_nonce));
sg_init_one(&sgio, data, datalen + (op == BIG_KEY_ENC ? ENC_AUTHTAG_SIZE : 0));
aead_request_set_crypt(aead_req, &sgio, &sgio, datalen, zero_nonce);
aead_request_set_callback(aead_req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
aead_request_set_ad(aead_req, 0);
mutex_lock(&big_key_aead_lock);
if (crypto_aead_setkey(big_key_aead, key, ENC_KEY_SIZE)) {
ret = -EAGAIN; ret = -EAGAIN;
goto error; goto error;
} }
skcipher_request_set_tfm(req, big_key_skcipher);
skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP,
NULL, NULL);
sg_init_one(&sgio, data, datalen);
skcipher_request_set_crypt(req, &sgio, &sgio, datalen, NULL);
if (op == BIG_KEY_ENC) if (op == BIG_KEY_ENC)
ret = crypto_skcipher_encrypt(req); ret = crypto_aead_encrypt(aead_req);
else else
ret = crypto_skcipher_decrypt(req); ret = crypto_aead_decrypt(aead_req);
skcipher_request_zero(req);
error: error:
mutex_unlock(&big_key_aead_lock);
aead_request_free(aead_req);
return ret; return ret;
} }
...@@ -146,16 +157,13 @@ int big_key_preparse(struct key_preparsed_payload *prep) ...@@ -146,16 +157,13 @@ int big_key_preparse(struct key_preparsed_payload *prep)
* *
* File content is stored encrypted with randomly generated key. * File content is stored encrypted with randomly generated key.
*/ */
size_t enclen = ALIGN(datalen, crypto_skcipher_blocksize(big_key_skcipher)); size_t enclen = datalen + ENC_AUTHTAG_SIZE;
loff_t pos = 0; loff_t pos = 0;
/* prepare aligned data to encrypt */
data = kmalloc(enclen, GFP_KERNEL); data = kmalloc(enclen, GFP_KERNEL);
if (!data) if (!data)
return -ENOMEM; return -ENOMEM;
memcpy(data, prep->data, datalen); memcpy(data, prep->data, datalen);
memset(data + datalen, 0x00, enclen - datalen);
/* generate random key */ /* generate random key */
enckey = kmalloc(ENC_KEY_SIZE, GFP_KERNEL); enckey = kmalloc(ENC_KEY_SIZE, GFP_KERNEL);
...@@ -163,13 +171,12 @@ int big_key_preparse(struct key_preparsed_payload *prep) ...@@ -163,13 +171,12 @@ int big_key_preparse(struct key_preparsed_payload *prep)
ret = -ENOMEM; ret = -ENOMEM;
goto error; goto error;
} }
ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
ret = big_key_gen_enckey(enckey); if (unlikely(ret))
if (ret)
goto err_enckey; goto err_enckey;
/* encrypt aligned data */ /* encrypt aligned data */
ret = big_key_crypt(BIG_KEY_ENC, data, enclen, enckey); ret = big_key_crypt(BIG_KEY_ENC, data, datalen, enckey);
if (ret) if (ret)
goto err_enckey; goto err_enckey;
...@@ -295,7 +302,7 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen) ...@@ -295,7 +302,7 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
struct file *file; struct file *file;
u8 *data; u8 *data;
u8 *enckey = (u8 *)key->payload.data[big_key_data]; u8 *enckey = (u8 *)key->payload.data[big_key_data];
size_t enclen = ALIGN(datalen, crypto_skcipher_blocksize(big_key_skcipher)); size_t enclen = datalen + ENC_AUTHTAG_SIZE;
loff_t pos = 0; loff_t pos = 0;
data = kmalloc(enclen, GFP_KERNEL); data = kmalloc(enclen, GFP_KERNEL);
...@@ -344,47 +351,31 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen) ...@@ -344,47 +351,31 @@ long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
*/ */
static int __init big_key_init(void) static int __init big_key_init(void)
{ {
struct crypto_skcipher *cipher;
struct crypto_rng *rng;
int ret; int ret;
rng = crypto_alloc_rng(big_key_rng_name, 0, 0);
if (IS_ERR(rng)) {
pr_err("Can't alloc rng: %ld\n", PTR_ERR(rng));
return PTR_ERR(rng);
}
big_key_rng = rng;
/* seed RNG */
ret = crypto_rng_reset(rng, NULL, crypto_rng_seedsize(rng));
if (ret) {
pr_err("Can't reset rng: %d\n", ret);
goto error_rng;
}
/* init block cipher */ /* init block cipher */
cipher = crypto_alloc_skcipher(big_key_alg_name, 0, CRYPTO_ALG_ASYNC); big_key_aead = crypto_alloc_aead(big_key_alg_name, 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(cipher)) { if (IS_ERR(big_key_aead)) {
ret = PTR_ERR(cipher); ret = PTR_ERR(big_key_aead);
pr_err("Can't alloc crypto: %d\n", ret); pr_err("Can't alloc crypto: %d\n", ret);
goto error_rng; return ret;
}
ret = crypto_aead_setauthsize(big_key_aead, ENC_AUTHTAG_SIZE);
if (ret < 0) {
pr_err("Can't set crypto auth tag len: %d\n", ret);
goto free_aead;
} }
big_key_skcipher = cipher;
ret = register_key_type(&key_type_big_key); ret = register_key_type(&key_type_big_key);
if (ret < 0) { if (ret < 0) {
pr_err("Can't register type: %d\n", ret); pr_err("Can't register type: %d\n", ret);
goto error_cipher; goto free_aead;
} }
return 0; return 0;
error_cipher: free_aead:
crypto_free_skcipher(big_key_skcipher); crypto_free_aead(big_key_aead);
error_rng:
crypto_free_rng(big_key_rng);
return ret; return ret;
} }
......
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