diff mbox

[v6] security/keys: rewrite all of big_key crypto

Message ID 20170917115217.15805-1-Jason@zx2c4.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jason A. Donenfeld Sept. 17, 2017, 11:52 a.m. UTC
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. And, some error paths forgot to zero out sensitive
material, so this patch changes a kfree into a kzfree.

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: Jason A. Donenfeld <Jason@zx2c4.com>
Reviewed-by: Eric Biggers <ebiggers3@gmail.com>
Cc: David 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
---
 security/keys/Kconfig   |   4 +-
 security/keys/big_key.c | 139 ++++++++++++++++++++++--------------------------
 2 files changed, 66 insertions(+), 77 deletions(-)

Changes v5->v6:
  - Fix memory leak.
  - CC->Reviewed-by for Eric.

Comments

David Howells Sept. 19, 2017, 3:38 p.m. UTC | #1
Jason A. Donenfeld <Jason@zx2c4.com> wrote:

> And, some error paths forgot to zero out sensitive
> material, so this patch changes a kfree into a kzfree.

Can you split that out into a separate preparatory patch?

Also, I recommend limiting the lines in your patch description to 75 chars
lest you get people who run checkpatch on everything pestering you ;-)

David
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephan Mueller Sept. 20, 2017, 5:30 a.m. UTC | #2
Am Sonntag, 17. September 2017, 13:52:17 CEST schrieb Jason A. Donenfeld:

Hi Jason,

>   * Use of ECB mode, allowing an attacker to trivially swap blocks or
>     compare identical plaintext blocks.

The use of GCM with the implementtion here is just as challenging. The 
implementation uses a NULL IV. GCM is a very brittle cipher where the 
construction of the IV is of special importance. SP800-38D section 8.2.1 and 
8.2.2 outlines the generation methods of the IV. A collision of keys/IVs is 
fatal. I understand that keys are generated anew each time which makes that 
issue less critical here. However, as user space may see the ciphertext, GCM 
should simply not be used.

A fix could be as easy as to use CCM or one of the authenc() ciphers. Yet, for 
both I am not sure how a zero IV affects the cipher.

The cipher where you do not need to handle the IV at all would be the RFC3394/
SP800-38F keywrapping cipher which is meant for the encryption of key material 
which includes authentication as well. It is available as an skcipher under 
the name of kw(aes). If you want to use it, please be careful that you obtain 
the generated IV to be stored with the plaintext as documented in the comments 
in crypto/keywrap.c


Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason A. Donenfeld Sept. 20, 2017, 10:52 a.m. UTC | #3
On Wed, Sep 20, 2017 at 7:30 AM, Stephan Mueller <smueller@chronox.de> wrote:
> The use of GCM with the implementtion here is just as challenging. The
> implementation uses a NULL IV. GCM is a very brittle cipher where the
> construction of the IV is of special importance. SP800-38D section 8.2.1 and
> 8.2.2 outlines the generation methods of the IV. A collision of keys/IVs is
> fatal. I understand that keys are generated anew each time which makes that
> issue less critical here. However, as user space may see the ciphertext, GCM
> should simply not be used.

This sounds incorrect to me.  Choosing a fresh, random, one-time-use
256-bit key and rolling with a zero nonce is a totally legitimate way
of using GCM. There's no possible reuse of the key stream this way.
However, on the off chance that you know what you're talking about,
could you outline the cryptographic attack you have in mind, or if
that's too difficult, simply link to the relevant paper on eprint?
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephan Mueller Sept. 20, 2017, 1:45 p.m. UTC | #4
Am Mittwoch, 20. September 2017, 12:52:21 CEST schrieb Jason A. Donenfeld:

Hi Jason,

> 
> This sounds incorrect to me.  Choosing a fresh, random, one-time-use
> 256-bit key and rolling with a zero nonce is a totally legitimate way
> of using GCM. There's no possible reuse of the key stream this way.
> However, on the off chance that you know what you're talking about,
> could you outline the cryptographic attack you have in mind, or if
> that's too difficult, simply link to the relevant paper on eprint?

http://csrc.nist.gov/groups/ST/toolkit/BCM/documents/Joux_comments.pdf

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason A. Donenfeld Sept. 20, 2017, 2:01 p.m. UTC | #5
On Wed, Sep 20, 2017 at 3:45 PM, Stephan Mueller <smueller@chronox.de> wrote:
> http://csrc.nist.gov/groups/ST/toolkit/BCM/documents/Joux_comments.pdf

Section 3 shows an attack with repeated nonces, which we don't do here.

Section 4 shows an attack using a non-96-bit nonce, which we also don't do here.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephan Mueller Sept. 20, 2017, 2:06 p.m. UTC | #6
Am Mittwoch, 20. September 2017, 16:01:21 CEST schrieb Jason A. Donenfeld:

Hi Jason,

> 
> Section 3 shows an attack with repeated nonces, which we don't do here.

Maybe I miss a point here, but zero IVs is no repetition of nonces?


Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason A. Donenfeld Sept. 20, 2017, 2:09 p.m. UTC | #7
On Wed, Sep 20, 2017 at 4:06 PM, Stephan Mueller <smueller@chronox.de> wrote:
>> Section 3 shows an attack with repeated nonces, which we don't do here.
>
> Maybe I miss a point here, but zero IVs is no repetition of nonces?

If there's a fresh key each time, then no, it's not a repetition.

This patch uses a fresh random key for every encryption.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason A. Donenfeld Sept. 20, 2017, 2:56 p.m. UTC | #8
On Tue, Sep 19, 2017 at 5:38 PM, David Howells <dhowells@redhat.com> wrote:
> Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
>> And, some error paths forgot to zero out sensitive
>> material, so this patch changes a kfree into a kzfree.
>
> Can you split that out into a separate preparatory patch?
>
> Also, I recommend limiting the lines in your patch description to 75 chars
> lest you get people who run checkpatch on everything pestering you ;-)

No problem. Will split that out, then fix the description, and
resubmit as v7. Coming your way shortly.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index a7a23b5541f8..91eafada3164 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -45,10 +45,8 @@  config BIG_KEYS
 	bool "Large payload keys"
 	depends on KEYS
 	depends on TMPFS
-	depends on (CRYPTO_ANSI_CPRNG = y || CRYPTO_DRBG = y)
 	select CRYPTO_AES
-	select CRYPTO_ECB
-	select CRYPTO_RNG
+	select CRYPTO_GCM
 	help
 	  This option provides support for holding large keys within the kernel
 	  (for example Kerberos ticket caches).  The data may be stored out to
diff --git a/security/keys/big_key.c b/security/keys/big_key.c
index 6acb00f6f22c..e607830b6154 100644
--- a/security/keys/big_key.c
+++ b/security/keys/big_key.c
@@ -1,5 +1,6 @@ 
 /* 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.
  * Written by David Howells (dhowells@redhat.com)
  *
@@ -16,10 +17,10 @@ 
 #include <linux/shmem_fs.h>
 #include <linux/err.h>
 #include <linux/scatterlist.h>
+#include <linux/random.h>
 #include <keys/user-type.h>
 #include <keys/big_key-type.h>
-#include <crypto/rng.h>
-#include <crypto/skcipher.h>
+#include <crypto/aead.h>
 
 /*
  * Layout of key payload words.
@@ -49,7 +50,12 @@  enum big_key_op {
 /*
  * 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
@@ -64,57 +70,62 @@  struct key_type key_type_big_key = {
 	.destroy		= big_key_destroy,
 	.describe		= big_key_describe,
 	.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[] = "ecb(aes)";
+static const char big_key_alg_name[] = "gcm(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_skcipher *big_key_skcipher;
+static struct crypto_aead *big_key_aead;
 
 /*
- * 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)
-{
-	return crypto_rng_get_bytes(big_key_rng, key, ENC_KEY_SIZE);
-}
+static DEFINE_MUTEX(big_key_aead_lock);
 
 /*
  * Encrypt/decrypt big_key data
  */
 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;
-	SKCIPHER_REQUEST_ON_STACK(req, big_key_skcipher);
-
-	if (crypto_skcipher_setkey(big_key_skcipher, key, ENC_KEY_SIZE)) {
+	struct aead_request *aead_req;
+	/* We always use a zero nonce. The reason we can get away with this is
+	 * 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;
 		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)
-		ret = crypto_skcipher_encrypt(req);
+		ret = crypto_aead_encrypt(aead_req);
 	else
-		ret = crypto_skcipher_decrypt(req);
-
-	skcipher_request_zero(req);
-
+		ret = crypto_aead_decrypt(aead_req);
 error:
+	mutex_unlock(&big_key_aead_lock);
+	aead_request_free(aead_req);
 	return ret;
 }
 
@@ -146,16 +157,13 @@  int big_key_preparse(struct key_preparsed_payload *prep)
 		 *
 		 * 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;
 
-		/* prepare aligned data to encrypt */
 		data = kmalloc(enclen, GFP_KERNEL);
 		if (!data)
 			return -ENOMEM;
-
 		memcpy(data, prep->data, datalen);
-		memset(data + datalen, 0x00, enclen - datalen);
 
 		/* generate random key */
 		enckey = kmalloc(ENC_KEY_SIZE, GFP_KERNEL);
@@ -163,13 +171,12 @@  int big_key_preparse(struct key_preparsed_payload *prep)
 			ret = -ENOMEM;
 			goto error;
 		}
-
-		ret = big_key_gen_enckey(enckey);
-		if (ret)
+		ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
+		if (unlikely(ret))
 			goto err_enckey;
 
 		/* 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)
 			goto err_enckey;
 
@@ -195,7 +202,7 @@  int big_key_preparse(struct key_preparsed_payload *prep)
 		*path = file->f_path;
 		path_get(path);
 		fput(file);
-		kfree(data);
+		kzfree(data);
 	} else {
 		/* Just store the data in a buffer */
 		void *data = kmalloc(datalen, GFP_KERNEL);
@@ -211,9 +218,9 @@  int big_key_preparse(struct key_preparsed_payload *prep)
 err_fput:
 	fput(file);
 err_enckey:
-	kfree(enckey);
+	kzfree(enckey);
 error:
-	kfree(data);
+	kzfree(data);
 	return ret;
 }
 
@@ -227,7 +234,7 @@  void big_key_free_preparse(struct key_preparsed_payload *prep)
 
 		path_put(path);
 	}
-	kfree(prep->payload.data[big_key_data]);
+	kzfree(prep->payload.data[big_key_data]);
 }
 
 /*
@@ -259,7 +266,7 @@  void big_key_destroy(struct key *key)
 		path->mnt = NULL;
 		path->dentry = NULL;
 	}
-	kfree(key->payload.data[big_key_data]);
+	kzfree(key->payload.data[big_key_data]);
 	key->payload.data[big_key_data] = NULL;
 }
 
@@ -295,7 +302,7 @@  long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
 		struct file *file;
 		u8 *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;
 
 		data = kmalloc(enclen, GFP_KERNEL);
@@ -328,7 +335,7 @@  long big_key_read(const struct key *key, char __user *buffer, size_t buflen)
 err_fput:
 		fput(file);
 error:
-		kfree(data);
+		kzfree(data);
 	} else {
 		ret = datalen;
 		if (copy_to_user(buffer, key->payload.data[big_key_data],
@@ -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)
 {
-	struct crypto_skcipher *cipher;
-	struct crypto_rng *rng;
 	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 */
-	cipher = crypto_alloc_skcipher(big_key_alg_name, 0, CRYPTO_ALG_ASYNC);
-	if (IS_ERR(cipher)) {
-		ret = PTR_ERR(cipher);
+	big_key_aead = crypto_alloc_aead(big_key_alg_name, 0, CRYPTO_ALG_ASYNC);
+	if (IS_ERR(big_key_aead)) {
+		ret = PTR_ERR(big_key_aead);
 		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);
 	if (ret < 0) {
 		pr_err("Can't register type: %d\n", ret);
-		goto error_cipher;
+		goto free_aead;
 	}
 
 	return 0;
 
-error_cipher:
-	crypto_free_skcipher(big_key_skcipher);
-error_rng:
-	crypto_free_rng(big_key_rng);
+free_aead:
+	crypto_free_aead(big_key_aead);
 	return ret;
 }