All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-16 13:00 ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-16 13:00 UTC (permalink / raw)
  To: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, ebiggers3
  Cc: Jason A. Donenfeld, Herbert Xu, Kirill Marinushkin,
	Ard Biesheuvel, Ilhan Gurel, security, stable

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>
Cc: David Howells <dhowells@redhat.com>
Cc: Eric Biggers <ebiggers3@gmail.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Kirill Marinushkin <k.marinushkin@gmail.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ilhan Gurel <ilhan.gurel@gmail.com>
Cc: security@kernel.org
Cc: stable@vger.kernel.org
---
 security/keys/Kconfig   |   4 +-
 security/keys/big_key.c | 139 ++++++++++++++++++++++--------------------------
 2 files changed, 65 insertions(+), 78 deletions(-)

Changes v3->v4:
  - Patchset resurrected from the dead. Just like the original use of ECB,
    which this patch fixes, crypto-related things are sometimes neglected,
    unfortunately.
  - Rebased on top of 4.13+ changes.
  - Now that get_random_bytes_wait has been merged, we can actually use
    it, just like in v1 of this 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..36682686f8c2 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,10 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 			ret = -ENOMEM;
 			goto error;
 		}
-
-		ret = big_key_gen_enckey(enckey);
-		if (ret)
-			goto err_enckey;
+		get_random_bytes_wait(enckey, ENC_KEY_SIZE);
 
 		/* 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 +200,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 +216,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 +232,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 +264,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 +300,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 +333,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 +349,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;
 }
 
-- 
2.14.1


^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-16 13:00 ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-16 13:00 UTC (permalink / raw)
  To: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, ebiggers3
  Cc: Jason A. Donenfeld, Herbert Xu, Kirill Marinushkin,
	Ard Biesheuvel, Ilhan Gurel, security, stable

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>
Cc: David Howells <dhowells@redhat.com>
Cc: Eric Biggers <ebiggers3@gmail.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Kirill Marinushkin <k.marinushkin@gmail.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ilhan Gurel <ilhan.gurel@gmail.com>
Cc: security@kernel.org
Cc: stable@vger.kernel.org
---
 security/keys/Kconfig   |   4 +-
 security/keys/big_key.c | 139 ++++++++++++++++++++++--------------------------
 2 files changed, 65 insertions(+), 78 deletions(-)

Changes v3->v4:
  - Patchset resurrected from the dead. Just like the original use of ECB,
    which this patch fixes, crypto-related things are sometimes neglected,
    unfortunately.
  - Rebased on top of 4.13+ changes.
  - Now that get_random_bytes_wait has been merged, we can actually use
    it, just like in v1 of this 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..36682686f8c2 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,10 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 			ret = -ENOMEM;
 			goto error;
 		}
-
-		ret = big_key_gen_enckey(enckey);
-		if (ret)
-			goto err_enckey;
+		get_random_bytes_wait(enckey, ENC_KEY_SIZE);
 
 		/* 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 +200,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 +216,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 +232,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 +264,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 +300,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 +333,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 +349,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;
 }
 
-- 
2.14.1

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-16 13:00 ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-16 13:00 UTC (permalink / raw)
  To: linux-security-module

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>
Cc: David Howells <dhowells@redhat.com>
Cc: Eric Biggers <ebiggers3@gmail.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Kirill Marinushkin <k.marinushkin@gmail.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ilhan Gurel <ilhan.gurel@gmail.com>
Cc: security at kernel.org
Cc: stable at vger.kernel.org
---
 security/keys/Kconfig   |   4 +-
 security/keys/big_key.c | 139 ++++++++++++++++++++++--------------------------
 2 files changed, 65 insertions(+), 78 deletions(-)

Changes v3->v4:
  - Patchset resurrected from the dead. Just like the original use of ECB,
    which this patch fixes, crypto-related things are sometimes neglected,
    unfortunately.
  - Rebased on top of 4.13+ changes.
  - Now that get_random_bytes_wait has been merged, we can actually use
    it, just like in v1 of this 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..36682686f8c2 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 at 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,10 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 			ret = -ENOMEM;
 			goto error;
 		}
-
-		ret = big_key_gen_enckey(enckey);
-		if (ret)
-			goto err_enckey;
+		get_random_bytes_wait(enckey, ENC_KEY_SIZE);
 
 		/* 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 +200,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 +216,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 +232,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 +264,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 +300,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 +333,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 +349,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;
 }
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info@ http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-16 13:00 ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-16 13:00 UTC (permalink / raw)
  To: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, ebiggers3
  Cc: Jason A. Donenfeld, Herbert Xu, Kirill Marinushkin,
	Ard Biesheuvel, Ilhan Gurel, security, stable

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>
Cc: David Howells <dhowells@redhat.com>
Cc: Eric Biggers <ebiggers3@gmail.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Kirill Marinushkin <k.marinushkin@gmail.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ilhan Gurel <ilhan.gurel@gmail.com>
Cc: security@kernel.org
Cc: stable@vger.kernel.org
---
 security/keys/Kconfig   |   4 +-
 security/keys/big_key.c | 139 ++++++++++++++++++++++--------------------------
 2 files changed, 65 insertions(+), 78 deletions(-)

Changes v3->v4:
  - Patchset resurrected from the dead. Just like the original use of ECB,
    which this patch fixes, crypto-related things are sometimes neglected,
    unfortunately.
  - Rebased on top of 4.13+ changes.
  - Now that get_random_bytes_wait has been merged, we can actually use
    it, just like in v1 of this 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..36682686f8c2 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,10 @@ int big_key_preparse(struct key_preparsed_payload *prep)
 			ret = -ENOMEM;
 			goto error;
 		}
-
-		ret = big_key_gen_enckey(enckey);
-		if (ret)
-			goto err_enckey;
+		get_random_bytes_wait(enckey, ENC_KEY_SIZE);
 
 		/* 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 +200,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 +216,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 +232,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 +264,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 +300,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 +333,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 +349,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;
 }
 
-- 
2.14.1

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v5] security/keys: rewrite all of big_key crypto
  2017-09-16 13:00 ` Jason A. Donenfeld
  (?)
  (?)
@ 2017-09-16 13:05   ` Jason A. Donenfeld
  -1 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-16 13:05 UTC (permalink / raw)
  To: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, ebiggers3
  Cc: Jason A. Donenfeld, Herbert Xu, Kirill Marinushkin, security, stable

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>
Cc: David Howells <dhowells@redhat.com>
Cc: Eric Biggers <ebiggers3@gmail.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 | 141 +++++++++++++++++++++++-------------------------
 2 files changed, 67 insertions(+), 78 deletions(-)

Changes v4->v5:
 - Fix get_random_bytes_wait ret value derp.

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..26d74721b6b6 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)
-			goto err_enckey;
+		ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
+		if (unlikely(ret))
+			goto error;
 
 		/* 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;
 }
 
-- 
2.14.1


^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v5] security/keys: rewrite all of big_key crypto
@ 2017-09-16 13:05   ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-16 13:05 UTC (permalink / raw)
  To: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, ebiggers3
  Cc: Jason A. Donenfeld, Herbert Xu, Kirill Marinushkin, security, stable

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>
Cc: David Howells <dhowells@redhat.com>
Cc: Eric Biggers <ebiggers3@gmail.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 | 141 +++++++++++++++++++++++-------------------------
 2 files changed, 67 insertions(+), 78 deletions(-)

Changes v4->v5:
 - Fix get_random_bytes_wait ret value derp.

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..26d74721b6b6 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)
-			goto err_enckey;
+		ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
+		if (unlikely(ret))
+			goto error;
 
 		/* 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;
 }
 
-- 
2.14.1

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v5] security/keys: rewrite all of big_key crypto
@ 2017-09-16 13:05   ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-16 13:05 UTC (permalink / raw)
  To: linux-security-module

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>
Cc: David Howells <dhowells@redhat.com>
Cc: Eric Biggers <ebiggers3@gmail.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Kirill Marinushkin <k.marinushkin@gmail.com>
Cc: security at kernel.org
Cc: stable at vger.kernel.org
---
 security/keys/Kconfig   |   4 +-
 security/keys/big_key.c | 141 +++++++++++++++++++++++-------------------------
 2 files changed, 67 insertions(+), 78 deletions(-)

Changes v4->v5:
 - Fix get_random_bytes_wait ret value derp.

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..26d74721b6b6 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 at 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)
-			goto err_enckey;
+		ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
+		if (unlikely(ret))
+			goto error;
 
 		/* 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;
 }
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info@ http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] [PATCH v5] security/keys: rewrite all of big_key crypto
@ 2017-09-16 13:05   ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-16 13:05 UTC (permalink / raw)
  To: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, ebiggers3
  Cc: Jason A. Donenfeld, Herbert Xu, Kirill Marinushkin, security, stable

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>
Cc: David Howells <dhowells@redhat.com>
Cc: Eric Biggers <ebiggers3@gmail.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 | 141 +++++++++++++++++++++++-------------------------
 2 files changed, 67 insertions(+), 78 deletions(-)

Changes v4->v5:
 - Fix get_random_bytes_wait ret value derp.

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..26d74721b6b6 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)
-			goto err_enckey;
+		ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
+		if (unlikely(ret))
+			goto error;
 
 		/* 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;
 }
 
-- 
2.14.1

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v5] security/keys: rewrite all of big_key crypto
  2017-09-16 13:05   ` Jason A. Donenfeld
  (?)
  (?)
@ 2017-09-17  6:04     ` Eric Biggers
  -1 siblings, 0 replies; 79+ messages in thread
From: Eric Biggers @ 2017-09-17  6:04 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, Herbert Xu, Kirill Marinushkin, security, stable

Hi Jason,

On Sat, Sep 16, 2017 at 03:05:33PM +0200, Jason A. Donenfeld wrote:
> -
> -		ret = big_key_gen_enckey(enckey);
> -		if (ret)
> -			goto err_enckey;
> +		ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
> +		if (unlikely(ret))
> +			goto error;

This should jump to 'err_enckey', otherwise it will leak 'enckey'.

Otherwise the changes all look good; after fixing the above, feel free to add my
Reviewed-by.  Yes, AES-GCM is the right choice here.  It is, however, almost
certainly the case that if someone can modify your swap partition, they can
already own your system in many other ways, so the "authenticated" portion of
"authenticated encryption" may not actually buy much in this situation :-)

The patch is a little long and perhaps should be split into several patches,
each of which fixes one bug; but see what David thinks.

I should also note, that while there definitely were some inadmissible bugs
here, the support for encrypting big_key's was only added recently, in the v4.7
kernel.  And obviously not encrypting at all is at least as much as a
"vulnerability" as using weak encryption.  I'm also a little skeptical that
people actually care enough about big_key's for it to be worthwhile to mark a
rewrite like this for stable, though I suppose it wouldn't be *too* hard to at
least cherry-pick this to 4.9 if you wanted.  (There is a small conflict so
you'd have to send the backport yourself after this goes into mainline.)

Eric

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v5] security/keys: rewrite all of big_key crypto
@ 2017-09-17  6:04     ` Eric Biggers
  0 siblings, 0 replies; 79+ messages in thread
From: Eric Biggers @ 2017-09-17  6:04 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, Herbert Xu, Kirill Marinushkin, security, stable

Hi Jason,

On Sat, Sep 16, 2017 at 03:05:33PM +0200, Jason A. Donenfeld wrote:
> -
> -		ret = big_key_gen_enckey(enckey);
> -		if (ret)
> -			goto err_enckey;
> +		ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
> +		if (unlikely(ret))
> +			goto error;

This should jump to 'err_enckey', otherwise it will leak 'enckey'.

Otherwise the changes all look good; after fixing the above, feel free to add my
Reviewed-by.  Yes, AES-GCM is the right choice here.  It is, however, almost
certainly the case that if someone can modify your swap partition, they can
already own your system in many other ways, so the "authenticated" portion of
"authenticated encryption" may not actually buy much in this situation :-)

The patch is a little long and perhaps should be split into several patches,
each of which fixes one bug; but see what David thinks.

I should also note, that while there definitely were some inadmissible bugs
here, the support for encrypting big_key's was only added recently, in the v4.7
kernel.  And obviously not encrypting at all is at least as much as a
"vulnerability" as using weak encryption.  I'm also a little skeptical that
people actually care enough about big_key's for it to be worthwhile to mark a
rewrite like this for stable, though I suppose it wouldn't be *too* hard to at
least cherry-pick this to 4.9 if you wanted.  (There is a small conflict so
you'd have to send the backport yourself after this goes into mainline.)

Eric

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v5] security/keys: rewrite all of big_key crypto
@ 2017-09-17  6:04     ` Eric Biggers
  0 siblings, 0 replies; 79+ messages in thread
From: Eric Biggers @ 2017-09-17  6:04 UTC (permalink / raw)
  To: linux-security-module

Hi Jason,

On Sat, Sep 16, 2017 at 03:05:33PM +0200, Jason A. Donenfeld wrote:
> -
> -		ret = big_key_gen_enckey(enckey);
> -		if (ret)
> -			goto err_enckey;
> +		ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
> +		if (unlikely(ret))
> +			goto error;

This should jump to 'err_enckey', otherwise it will leak 'enckey'.

Otherwise the changes all look good; after fixing the above, feel free to add my
Reviewed-by.  Yes, AES-GCM is the right choice here.  It is, however, almost
certainly the case that if someone can modify your swap partition, they can
already own your system in many other ways, so the "authenticated" portion of
"authenticated encryption" may not actually buy much in this situation :-)

The patch is a little long and perhaps should be split into several patches,
each of which fixes one bug; but see what David thinks.

I should also note, that while there definitely were some inadmissible bugs
here, the support for encrypting big_key's was only added recently, in the v4.7
kernel.  And obviously not encrypting at all is at least as much as a
"vulnerability" as using weak encryption.  I'm also a little skeptical that
people actually care enough about big_key's for it to be worthwhile to mark a
rewrite like this for stable, though I suppose it wouldn't be *too* hard to at
least cherry-pick this to 4.9 if you wanted.  (There is a small conflict so
you'd have to send the backport yourself after this goes into mainline.)

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v5] security/keys: rewrite all of big_key crypto
@ 2017-09-17  6:04     ` Eric Biggers
  0 siblings, 0 replies; 79+ messages in thread
From: Eric Biggers @ 2017-09-17  6:04 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, Herbert Xu, Kirill Marinushkin, security, stable

Hi Jason,

On Sat, Sep 16, 2017 at 03:05:33PM +0200, Jason A. Donenfeld wrote:
> -
> -		ret = big_key_gen_enckey(enckey);
> -		if (ret)
> -			goto err_enckey;
> +		ret = get_random_bytes_wait(enckey, ENC_KEY_SIZE);
> +		if (unlikely(ret))
> +			goto error;

This should jump to 'err_enckey', otherwise it will leak 'enckey'.

Otherwise the changes all look good; after fixing the above, feel free to add my
Reviewed-by.  Yes, AES-GCM is the right choice here.  It is, however, almost
certainly the case that if someone can modify your swap partition, they can
already own your system in many other ways, so the "authenticated" portion of
"authenticated encryption" may not actually buy much in this situation :-)

The patch is a little long and perhaps should be split into several patches,
each of which fixes one bug; but see what David thinks.

I should also note, that while there definitely were some inadmissible bugs
here, the support for encrypting big_key's was only added recently, in the v4.7
kernel.  And obviously not encrypting at all is at least as much as a
"vulnerability" as using weak encryption.  I'm also a little skeptical that
people actually care enough about big_key's for it to be worthwhile to mark a
rewrite like this for stable, though I suppose it wouldn't be *too* hard to at
least cherry-pick this to 4.9 if you wanted.  (There is a small conflict so
you'd have to send the backport yourself after this goes into mainline.)

Eric

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v5] security/keys: rewrite all of big_key crypto
  2017-09-17  6:04     ` Eric Biggers
  (?)
  (?)
@ 2017-09-17 11:50       ` Jason A. Donenfeld
  -1 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-17 11:50 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Herbert Xu, Kirill Marinushkin, security, stable

On Sun, Sep 17, 2017 at 8:04 AM, Eric Biggers <ebiggers3@gmail.com> wrote:
> This should jump to 'err_enckey', otherwise it will leak 'enckey'.

Yikes, good catch, thanks!

>
> Otherwise the changes all look good; after fixing the above, feel free to add my
> Reviewed-by.

Ack.

> Yes, AES-GCM is the right choice here.  It is, however, almost
> certainly the case that if someone can modify your swap partition, they can
> already own your system in many other ways, so the "authenticated" portion of
> "authenticated encryption" may not actually buy much in this situation :-)

True, though this is slightly different from people putting their
big_keys on NFS, I guess (but who would do such a thing anyway?).

> The patch is a little long and perhaps should be split into several patches,
> each of which fixes one bug; but see what David thinks.

Meh, it's a rewrite, so sure, it's long. I saw a bunch of bugs and
rewrote the whole thing rather than going one-by-one with the bugs,
which would have produced a series. Personally I'd prefer to keep it
like this than spending the time needling away with git and extra
commit messages and struggling to make them each build separately.
Seems like very very little gain for the time required to do that
right. *shrug* Will wait to hear from David.

> I should also note, that while there definitely were some inadmissible bugs
> here, the support for encrypting big_key's was only added recently, in the v4.7
> kernel.  And obviously not encrypting at all is at least as much as a
> "vulnerability" as using weak encryption.

Yea that's fair. I'm mostly just running around like a headless
chicken after seeing ECB and wondering how this got committed in the
first place, but nobody really takes chickens seriously. But yes, I
suppose one way of considering this is just to say, "big_keys did not
use encryption before 4.14, even though the code was really
complicated."

> I'm also a little skeptical that
> people actually care enough about big_key's for it to be worthwhile to mark a
> rewrite like this for stable, though I suppose it wouldn't be *too* hard to at
> least cherry-pick this to 4.9 if you wanted.  (There is a small conflict so
> you'd have to send the backport yourself after this goes into mainline.)

I imagine the conflicts will be moving back from get_random_bytes_wait
to get_random_bytes and maybe the loff_t/kernel_{read,write} thing. Is
this what you had in mind too? I should be able to handle those fairly
easily and send it to Greg manually after this lands.

Thanks again for the review, very appreciated.

Regards,
Jason

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v5] security/keys: rewrite all of big_key crypto
@ 2017-09-17 11:50       ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-17 11:50 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Herbert Xu, Kirill Marinushkin, security, stable

On Sun, Sep 17, 2017 at 8:04 AM, Eric Biggers <ebiggers3@gmail.com> wrote:
> This should jump to 'err_enckey', otherwise it will leak 'enckey'.

Yikes, good catch, thanks!

>
> Otherwise the changes all look good; after fixing the above, feel free to add my
> Reviewed-by.

Ack.

> Yes, AES-GCM is the right choice here.  It is, however, almost
> certainly the case that if someone can modify your swap partition, they can
> already own your system in many other ways, so the "authenticated" portion of
> "authenticated encryption" may not actually buy much in this situation :-)

True, though this is slightly different from people putting their
big_keys on NFS, I guess (but who would do such a thing anyway?).

> The patch is a little long and perhaps should be split into several patches,
> each of which fixes one bug; but see what David thinks.

Meh, it's a rewrite, so sure, it's long. I saw a bunch of bugs and
rewrote the whole thing rather than going one-by-one with the bugs,
which would have produced a series. Personally I'd prefer to keep it
like this than spending the time needling away with git and extra
commit messages and struggling to make them each build separately.
Seems like very very little gain for the time required to do that
right. *shrug* Will wait to hear from David.

> I should also note, that while there definitely were some inadmissible bugs
> here, the support for encrypting big_key's was only added recently, in the v4.7
> kernel.  And obviously not encrypting at all is at least as much as a
> "vulnerability" as using weak encryption.

Yea that's fair. I'm mostly just running around like a headless
chicken after seeing ECB and wondering how this got committed in the
first place, but nobody really takes chickens seriously. But yes, I
suppose one way of considering this is just to say, "big_keys did not
use encryption before 4.14, even though the code was really
complicated."

> I'm also a little skeptical that
> people actually care enough about big_key's for it to be worthwhile to mark a
> rewrite like this for stable, though I suppose it wouldn't be *too* hard to at
> least cherry-pick this to 4.9 if you wanted.  (There is a small conflict so
> you'd have to send the backport yourself after this goes into mainline.)

I imagine the conflicts will be moving back from get_random_bytes_wait
to get_random_bytes and maybe the loff_t/kernel_{read,write} thing. Is
this what you had in mind too? I should be able to handle those fairly
easily and send it to Greg manually after this lands.

Thanks again for the review, very appreciated.

Regards,
Jason

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v5] security/keys: rewrite all of big_key crypto
@ 2017-09-17 11:50       ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-17 11:50 UTC (permalink / raw)
  To: linux-security-module

On Sun, Sep 17, 2017 at 8:04 AM, Eric Biggers <ebiggers3@gmail.com> wrote:
> This should jump to 'err_enckey', otherwise it will leak 'enckey'.

Yikes, good catch, thanks!

>
> Otherwise the changes all look good; after fixing the above, feel free to add my
> Reviewed-by.

Ack.

> Yes, AES-GCM is the right choice here.  It is, however, almost
> certainly the case that if someone can modify your swap partition, they can
> already own your system in many other ways, so the "authenticated" portion of
> "authenticated encryption" may not actually buy much in this situation :-)

True, though this is slightly different from people putting their
big_keys on NFS, I guess (but who would do such a thing anyway?).

> The patch is a little long and perhaps should be split into several patches,
> each of which fixes one bug; but see what David thinks.

Meh, it's a rewrite, so sure, it's long. I saw a bunch of bugs and
rewrote the whole thing rather than going one-by-one with the bugs,
which would have produced a series. Personally I'd prefer to keep it
like this than spending the time needling away with git and extra
commit messages and struggling to make them each build separately.
Seems like very very little gain for the time required to do that
right. *shrug* Will wait to hear from David.

> I should also note, that while there definitely were some inadmissible bugs
> here, the support for encrypting big_key's was only added recently, in the v4.7
> kernel.  And obviously not encrypting at all is at least as much as a
> "vulnerability" as using weak encryption.

Yea that's fair. I'm mostly just running around like a headless
chicken after seeing ECB and wondering how this got committed in the
first place, but nobody really takes chickens seriously. But yes, I
suppose one way of considering this is just to say, "big_keys did not
use encryption before 4.14, even though the code was really
complicated."

> I'm also a little skeptical that
> people actually care enough about big_key's for it to be worthwhile to mark a
> rewrite like this for stable, though I suppose it wouldn't be *too* hard to at
> least cherry-pick this to 4.9 if you wanted.  (There is a small conflict so
> you'd have to send the backport yourself after this goes into mainline.)

I imagine the conflicts will be moving back from get_random_bytes_wait
to get_random_bytes and maybe the loff_t/kernel_{read,write} thing. Is
this what you had in mind too? I should be able to handle those fairly
easily and send it to Greg manually after this lands.

Thanks again for the review, very appreciated.

Regards,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v5] security/keys: rewrite all of big_key crypto
@ 2017-09-17 11:50       ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-17 11:50 UTC (permalink / raw)
  To: Eric Biggers
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Herbert Xu, Kirill Marinushkin, security, stable

On Sun, Sep 17, 2017 at 8:04 AM, Eric Biggers <ebiggers3@gmail.com> wrote:
> This should jump to 'err_enckey', otherwise it will leak 'enckey'.

Yikes, good catch, thanks!

>
> Otherwise the changes all look good; after fixing the above, feel free to add my
> Reviewed-by.

Ack.

> Yes, AES-GCM is the right choice here.  It is, however, almost
> certainly the case that if someone can modify your swap partition, they can
> already own your system in many other ways, so the "authenticated" portion of
> "authenticated encryption" may not actually buy much in this situation :-)

True, though this is slightly different from people putting their
big_keys on NFS, I guess (but who would do such a thing anyway?).

> The patch is a little long and perhaps should be split into several patches,
> each of which fixes one bug; but see what David thinks.

Meh, it's a rewrite, so sure, it's long. I saw a bunch of bugs and
rewrote the whole thing rather than going one-by-one with the bugs,
which would have produced a series. Personally I'd prefer to keep it
like this than spending the time needling away with git and extra
commit messages and struggling to make them each build separately.
Seems like very very little gain for the time required to do that
right. *shrug* Will wait to hear from David.

> I should also note, that while there definitely were some inadmissible bugs
> here, the support for encrypting big_key's was only added recently, in the v4.7
> kernel.  And obviously not encrypting at all is at least as much as a
> "vulnerability" as using weak encryption.

Yea that's fair. I'm mostly just running around like a headless
chicken after seeing ECB and wondering how this got committed in the
first place, but nobody really takes chickens seriously. But yes, I
suppose one way of considering this is just to say, "big_keys did not
use encryption before 4.14, even though the code was really
complicated."

> I'm also a little skeptical that
> people actually care enough about big_key's for it to be worthwhile to mark a
> rewrite like this for stable, though I suppose it wouldn't be *too* hard to at
> least cherry-pick this to 4.9 if you wanted.  (There is a small conflict so
> you'd have to send the backport yourself after this goes into mainline.)

I imagine the conflicts will be moving back from get_random_bytes_wait
to get_random_bytes and maybe the loff_t/kernel_{read,write} thing. Is
this what you had in mind too? I should be able to handle those fairly
easily and send it to Greg manually after this lands.

Thanks again for the review, very appreciated.

Regards,
Jason

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v6] security/keys: rewrite all of big_key crypto
  2017-09-17 11:50       ` Jason A. Donenfeld
  (?)
  (?)
@ 2017-09-17 11:52         ` Jason A. Donenfeld
  -1 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-17 11:52 UTC (permalink / raw)
  To: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, ebiggers3
  Cc: Jason A. Donenfeld, Herbert Xu, Kirill Marinushkin, security, stable

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.

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


^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-17 11:52         ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-17 11:52 UTC (permalink / raw)
  To: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, ebiggers3
  Cc: Jason A. Donenfeld, Herbert Xu, Kirill Marinushkin, security, stable

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.

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

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-17 11:52         ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-17 11:52 UTC (permalink / raw)
  To: linux-security-module

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 at kernel.org
Cc: stable at 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.

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 at 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;
 }
 
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info@ http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-17 11:52         ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-17 11:52 UTC (permalink / raw)
  To: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, ebiggers3
  Cc: Jason A. Donenfeld, Herbert Xu, Kirill Marinushkin, security, stable

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.

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

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v4] security/keys: rewrite all of big_key crypto
  2017-09-16 13:00 ` Jason A. Donenfeld
  (?)
  (?)
@ 2017-09-18  8:49   ` Stephan Mueller
  -1 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-18  8:49 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, ebiggers3, Herbert Xu, Kirill Marinushkin,
	Ard Biesheuvel, Ilhan Gurel, security, stable

Am Samstag, 16. September 2017, 15:00:34 CEST schrieb Jason A. Donenfeld:

Hi Jason,

> This started out as just replacing the use of crypto/rng with
> get_random_bytes_wait, 

This change is a challenge. The use of the kernel crypto API's DRNG has been 
made to allow FIPS 140-2 compliance. Otherwise, the entire key generation 
logic will not be using the right(TM) DRNG. Thus, I would not suggest to 
replace that for a stable tree.

Note, I am currently working on a pluggable DRNG apporach for /dev/random and 
/dev/urandom to be able to get rid of the use of the kernel crypto API's DRNG 
API. It is ready and I will air that solution shortly. Yet, it needs work to 
be integrated upstream (and approval from Ted Tso).

Ciao
Stephan

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-18  8:49   ` Stephan Mueller
  0 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-18  8:49 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, ebiggers3, Herbert Xu, Kirill Marinushkin,
	Ard Biesheuvel, Ilhan Gurel, security, stable

Am Samstag, 16. September 2017, 15:00:34 CEST schrieb Jason A. Donenfeld:

Hi Jason,

> This started out as just replacing the use of crypto/rng with
> get_random_bytes_wait, 

This change is a challenge. The use of the kernel crypto API's DRNG has been 
made to allow FIPS 140-2 compliance. Otherwise, the entire key generation 
logic will not be using the right(TM) DRNG. Thus, I would not suggest to 
replace that for a stable tree.

Note, I am currently working on a pluggable DRNG apporach for /dev/random and 
/dev/urandom to be able to get rid of the use of the kernel crypto API's DRNG 
API. It is ready and I will air that solution shortly. Yet, it needs work to 
be integrated upstream (and approval from Ted Tso).

Ciao
Stephan

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-18  8:49   ` Stephan Mueller
  0 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-18  8:49 UTC (permalink / raw)
  To: linux-security-module

Am Samstag, 16. September 2017, 15:00:34 CEST schrieb Jason A. Donenfeld:

Hi Jason,

> This started out as just replacing the use of crypto/rng with
> get_random_bytes_wait, 

This change is a challenge. The use of the kernel crypto API's DRNG has been 
made to allow FIPS 140-2 compliance. Otherwise, the entire key generation 
logic will not be using the right(TM) DRNG. Thus, I would not suggest to 
replace that for a stable tree.

Note, I am currently working on a pluggable DRNG apporach for /dev/random and 
/dev/urandom to be able to get rid of the use of the kernel crypto API's DRNG 
API. It is ready and I will air that solution shortly. Yet, it needs work to 
be integrated upstream (and approval from Ted Tso).

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-18  8:49   ` Stephan Mueller
  0 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-18  8:49 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, ebiggers3, Herbert Xu, Kirill Marinushkin,
	Ard Biesheuvel, Ilhan Gurel, security, stable

Am Samstag, 16. September 2017, 15:00:34 CEST schrieb Jason A. Donenfeld:

Hi Jason,

> This started out as just replacing the use of crypto/rng with
> get_random_bytes_wait, 

This change is a challenge. The use of the kernel crypto API's DRNG has been 
made to allow FIPS 140-2 compliance. Otherwise, the entire key generation 
logic will not be using the right(TM) DRNG. Thus, I would not suggest to 
replace that for a stable tree.

Note, I am currently working on a pluggable DRNG apporach for /dev/random and 
/dev/urandom to be able to get rid of the use of the kernel crypto API's DRNG 
API. It is ready and I will air that solution shortly. Yet, it needs work to 
be integrated upstream (and approval from Ted Tso).

Ciao
Stephan

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
  2017-09-18  8:49   ` Stephan Mueller
  (?)
@ 2017-09-18  9:04     ` Greg KH
  -1 siblings, 0 replies; 79+ messages in thread
From: Greg KH @ 2017-09-18  9:04 UTC (permalink / raw)
  To: Stephan Mueller
  Cc: Jason A. Donenfeld, linux-security-module, keyrings,
	kernel-hardening, linux-kernel, dhowells, ebiggers3, Herbert Xu,
	Kirill Marinushkin, Ard Biesheuvel, Ilhan Gurel, security,
	stable

On Mon, Sep 18, 2017 at 10:49:56AM +0200, Stephan Mueller wrote:
> Am Samstag, 16. September 2017, 15:00:34 CEST schrieb Jason A. Donenfeld:
> 
> Hi Jason,
> 
> > This started out as just replacing the use of crypto/rng with
> > get_random_bytes_wait, 
> 
> This change is a challenge. The use of the kernel crypto API's DRNG has been 
> made to allow FIPS 140-2 compliance. Otherwise, the entire key generation 
> logic will not be using the right(TM) DRNG. Thus, I would not suggest to 
> replace that for a stable tree.

Why not?  What is the issue here, there is only one "DRNG" in the kernel
now (and probably for a long time...)

> Note, I am currently working on a pluggable DRNG apporach for /dev/random and 
> /dev/urandom to be able to get rid of the use of the kernel crypto API's DRNG 
> API. It is ready and I will air that solution shortly. Yet, it needs work to 
> be integrated upstream (and approval from Ted Tso).

We don't postpone work for potential future patches that might or might
not ever happen or get merged.  That's how NetBSD died...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-18  9:04     ` Greg KH
  0 siblings, 0 replies; 79+ messages in thread
From: Greg KH @ 2017-09-18  9:04 UTC (permalink / raw)
  To: Stephan Mueller
  Cc: Jason A. Donenfeld, linux-security-module, keyrings,
	kernel-hardening, linux-kernel, dhowells, ebiggers3, Herbert Xu,
	Kirill Marinushkin, Ard Biesheuvel, Ilhan Gurel, security,
	stable

On Mon, Sep 18, 2017 at 10:49:56AM +0200, Stephan Mueller wrote:
> Am Samstag, 16. September 2017, 15:00:34 CEST schrieb Jason A. Donenfeld:
> 
> Hi Jason,
> 
> > This started out as just replacing the use of crypto/rng with
> > get_random_bytes_wait, 
> 
> This change is a challenge. The use of the kernel crypto API's DRNG has been 
> made to allow FIPS 140-2 compliance. Otherwise, the entire key generation 
> logic will not be using the right(TM) DRNG. Thus, I would not suggest to 
> replace that for a stable tree.

Why not?  What is the issue here, there is only one "DRNG" in the kernel
now (and probably for a long time...)

> Note, I am currently working on a pluggable DRNG apporach for /dev/random and 
> /dev/urandom to be able to get rid of the use of the kernel crypto API's DRNG 
> API. It is ready and I will air that solution shortly. Yet, it needs work to 
> be integrated upstream (and approval from Ted Tso).

We don't postpone work for potential future patches that might or might
not ever happen or get merged.  That's how NetBSD died...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-18  9:04     ` Greg KH
  0 siblings, 0 replies; 79+ messages in thread
From: Greg KH @ 2017-09-18  9:04 UTC (permalink / raw)
  To: linux-security-module

On Mon, Sep 18, 2017 at 10:49:56AM +0200, Stephan Mueller wrote:
> Am Samstag, 16. September 2017, 15:00:34 CEST schrieb Jason A. Donenfeld:
> 
> Hi Jason,
> 
> > This started out as just replacing the use of crypto/rng with
> > get_random_bytes_wait, 
> 
> This change is a challenge. The use of the kernel crypto API's DRNG has been 
> made to allow FIPS 140-2 compliance. Otherwise, the entire key generation 
> logic will not be using the right(TM) DRNG. Thus, I would not suggest to 
> replace that for a stable tree.

Why not?  What is the issue here, there is only one "DRNG" in the kernel
now (and probably for a long time...)

> Note, I am currently working on a pluggable DRNG apporach for /dev/random and 
> /dev/urandom to be able to get rid of the use of the kernel crypto API's DRNG 
> API. It is ready and I will air that solution shortly. Yet, it needs work to 
> be integrated upstream (and approval from Ted Tso).

We don't postpone work for potential future patches that might or might
not ever happen or get merged.  That's how NetBSD died...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
  2017-09-18  9:04     ` Greg KH
  (?)
@ 2017-09-18  9:12       ` Stephan Mueller
  -1 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-18  9:12 UTC (permalink / raw)
  To: kernel-hardening
  Cc: Greg KH, Jason A. Donenfeld, linux-security-module, keyrings,
	linux-kernel, dhowells, ebiggers3, Herbert Xu,
	Kirill Marinushkin, Ard Biesheuvel, Ilhan Gurel, security,
	stable

Am Montag, 18. September 2017, 11:04:55 CEST schrieb Greg KH:

Hi Greg,

> On Mon, Sep 18, 2017 at 10:49:56AM +0200, Stephan Mueller wrote:
> > Am Samstag, 16. September 2017, 15:00:34 CEST schrieb Jason A. Donenfeld:
> > 
> > Hi Jason,
> > 
> > > This started out as just replacing the use of crypto/rng with
> > > get_random_bytes_wait,
> > 
> > This change is a challenge. The use of the kernel crypto API's DRNG has
> > been made to allow FIPS 140-2 compliance. Otherwise, the entire key
> > generation logic will not be using the right(TM) DRNG. Thus, I would not
> > suggest to replace that for a stable tree.
> 
> Why not?

An SP800-90A-compliant DRNG must be used in those circumstances.

> What is the issue here, there is only one "DRNG" in the kernel
> now (and probably for a long time...)

There are more DRNGs implemented in the kernel crypto API (see crypto/drbg.c 
or crypto/ansi-cprng.c).
> 
> > Note, I am currently working on a pluggable DRNG apporach for /dev/random
> > and /dev/urandom to be able to get rid of the use of the kernel crypto
> > API's DRNG API. It is ready and I will air that solution shortly. Yet, it
> > needs work to be integrated upstream (and approval from Ted Tso).
> 
> We don't postpone work for potential future patches that might or might
> not ever happen or get merged.  That's how NetBSD died...

Then I would recommend to leave it as is or hear complaints from other users.

Ciao
Stephan

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-18  9:12       ` Stephan Mueller
  0 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-18  9:12 UTC (permalink / raw)
  To: kernel-hardening
  Cc: Greg KH, Jason A. Donenfeld, linux-security-module, keyrings,
	linux-kernel, dhowells, ebiggers3, Herbert Xu,
	Kirill Marinushkin, Ard Biesheuvel, Ilhan Gurel, security,
	stable

Am Montag, 18. September 2017, 11:04:55 CEST schrieb Greg KH:

Hi Greg,

> On Mon, Sep 18, 2017 at 10:49:56AM +0200, Stephan Mueller wrote:
> > Am Samstag, 16. September 2017, 15:00:34 CEST schrieb Jason A. Donenfeld:
> > 
> > Hi Jason,
> > 
> > > This started out as just replacing the use of crypto/rng with
> > > get_random_bytes_wait,
> > 
> > This change is a challenge. The use of the kernel crypto API's DRNG has
> > been made to allow FIPS 140-2 compliance. Otherwise, the entire key
> > generation logic will not be using the right(TM) DRNG. Thus, I would not
> > suggest to replace that for a stable tree.
> 
> Why not?

An SP800-90A-compliant DRNG must be used in those circumstances.

> What is the issue here, there is only one "DRNG" in the kernel
> now (and probably for a long time...)

There are more DRNGs implemented in the kernel crypto API (see crypto/drbg.c 
or crypto/ansi-cprng.c).
> 
> > Note, I am currently working on a pluggable DRNG apporach for /dev/random
> > and /dev/urandom to be able to get rid of the use of the kernel crypto
> > API's DRNG API. It is ready and I will air that solution shortly. Yet, it
> > needs work to be integrated upstream (and approval from Ted Tso).
> 
> We don't postpone work for potential future patches that might or might
> not ever happen or get merged.  That's how NetBSD died...

Then I would recommend to leave it as is or hear complaints from other users.

Ciao
Stephan

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-18  9:12       ` Stephan Mueller
  0 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-18  9:12 UTC (permalink / raw)
  To: linux-security-module

Am Montag, 18. September 2017, 11:04:55 CEST schrieb Greg KH:

Hi Greg,

> On Mon, Sep 18, 2017 at 10:49:56AM +0200, Stephan Mueller wrote:
> > Am Samstag, 16. September 2017, 15:00:34 CEST schrieb Jason A. Donenfeld:
> > 
> > Hi Jason,
> > 
> > > This started out as just replacing the use of crypto/rng with
> > > get_random_bytes_wait,
> > 
> > This change is a challenge. The use of the kernel crypto API's DRNG has
> > been made to allow FIPS 140-2 compliance. Otherwise, the entire key
> > generation logic will not be using the right(TM) DRNG. Thus, I would not
> > suggest to replace that for a stable tree.
> 
> Why not?

An SP800-90A-compliant DRNG must be used in those circumstances.

> What is the issue here, there is only one "DRNG" in the kernel
> now (and probably for a long time...)

There are more DRNGs implemented in the kernel crypto API (see crypto/drbg.c 
or crypto/ansi-cprng.c).
> 
> > Note, I am currently working on a pluggable DRNG apporach for /dev/random
> > and /dev/urandom to be able to get rid of the use of the kernel crypto
> > API's DRNG API. It is ready and I will air that solution shortly. Yet, it
> > needs work to be integrated upstream (and approval from Ted Tso).
> 
> We don't postpone work for potential future patches that might or might
> not ever happen or get merged.  That's how NetBSD died...

Then I would recommend to leave it as is or hear complaints from other users.

Ciao
Stephan
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v4] security/keys: rewrite all of big_key crypto
  2017-09-18  8:49   ` Stephan Mueller
  (?)
  (?)
@ 2017-09-18 11:24     ` Jason A. Donenfeld
  -1 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-18 11:24 UTC (permalink / raw)
  To: Stephan Mueller
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	Ard Biesheuvel, Ilhan Gurel, security, stable, Theodore Ts'o

Hello Stephan,

I realize you have your Linux RNG project, which is a very worthwhile
goal that I support you in. I hope you're eventually successful, and I
apologize for not being more available in the last 2.5 months to chime
in on that patchset thread you posted.

However, your message to this thread here is a completely
inappropriate and nonsensical hijacking, which makes me entirely
question your overall judgement.

(David -- please don't let such hijacking derail or delay these
critical vulnerability fixes from landing.)

> This change is a challenge. The use of the kernel crypto API's DRNG has been
> made to allow FIPS 140-2 compliance. Otherwise, the entire key generation
> logic will not be using the right(TM) DRNG. Thus, I would not suggest to
> replace that for a stable tree.

Complete and utter nonsense. This patch has a history (this is already
v6), and it's pretty obvious from prior discussion and conclusion that
the only reason "crypto/rng" was used for this is because the original
author just had no idea what he was doing (thus leading to using ECB
as well). Besides, from what RNG do you think the seed for that was
generated?

> Note, I am currently working on a pluggable DRNG apporach for /dev/random and
> /dev/urandom to be able to get rid of the use of the kernel crypto API's DRNG
> API. It is ready and I will air that solution shortly.

Good to hear you're still working on that project.

> Yet, it needs work to
> be integrated upstream (and approval from Ted Tso).

Good luck with getting approval... While Ted and I have our
differences like any two kernel developers, I really tend agree with
him in his attitude about this FIPS silliness. It's unlikely you're
going to be able to shovel this stuff into random.c, and I think doing
so will undermine your entire LRNG effort.

> An SP800-90A-compliant DRNG must be used in those circumstances.

Again, complete and utter unsubstantiated nonsense.

> Then I would recommend to leave it as is or hear complaints from other users.

What a poor lack of judgement.

I get it -- this FIPS compliance backwardness is something you're keen
about. But don't start hijacking every thread that involves randomness
with a demand to replace calls to get_random_bytes with wrappers in
outdated, flawed, government compliance crypto API key expanders. From
a cryptographic point of view it's a ridiculous demand. And from an
engineering point of view, it's a ridiculous demand too, for if
get_random_bytes already returned FIPS-compliant randomness, you
wouldn't be writing this email here.

Instead, if you want your FIPS garbage in the Linux RNG, take your
battle for that over to random.c. I'll oppose you to the end on it,
but at the very least it will the the appropriate venue for that
discussion. In the meantime, stop hijacking this thread.

Thanks,
Jason

PS: apologies for what's probably perceived as an unfriendly and
overly hostile tone of this email. It's not my intention to alienate
you, but I do feel necessary in these circumstances to write as
directly as possible.

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-18 11:24     ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-18 11:24 UTC (permalink / raw)
  To: Stephan Mueller
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	Ard Biesheuvel, Ilhan Gurel, security, stable, Theodore Ts'o

Hello Stephan,

I realize you have your Linux RNG project, which is a very worthwhile
goal that I support you in. I hope you're eventually successful, and I
apologize for not being more available in the last 2.5 months to chime
in on that patchset thread you posted.

However, your message to this thread here is a completely
inappropriate and nonsensical hijacking, which makes me entirely
question your overall judgement.

(David -- please don't let such hijacking derail or delay these
critical vulnerability fixes from landing.)

> This change is a challenge. The use of the kernel crypto API's DRNG has been
> made to allow FIPS 140-2 compliance. Otherwise, the entire key generation
> logic will not be using the right(TM) DRNG. Thus, I would not suggest to
> replace that for a stable tree.

Complete and utter nonsense. This patch has a history (this is already
v6), and it's pretty obvious from prior discussion and conclusion that
the only reason "crypto/rng" was used for this is because the original
author just had no idea what he was doing (thus leading to using ECB
as well). Besides, from what RNG do you think the seed for that was
generated?

> Note, I am currently working on a pluggable DRNG apporach for /dev/random and
> /dev/urandom to be able to get rid of the use of the kernel crypto API's DRNG
> API. It is ready and I will air that solution shortly.

Good to hear you're still working on that project.

> Yet, it needs work to
> be integrated upstream (and approval from Ted Tso).

Good luck with getting approval... While Ted and I have our
differences like any two kernel developers, I really tend agree with
him in his attitude about this FIPS silliness. It's unlikely you're
going to be able to shovel this stuff into random.c, and I think doing
so will undermine your entire LRNG effort.

> An SP800-90A-compliant DRNG must be used in those circumstances.

Again, complete and utter unsubstantiated nonsense.

> Then I would recommend to leave it as is or hear complaints from other users.

What a poor lack of judgement.

I get it -- this FIPS compliance backwardness is something you're keen
about. But don't start hijacking every thread that involves randomness
with a demand to replace calls to get_random_bytes with wrappers in
outdated, flawed, government compliance crypto API key expanders. From
a cryptographic point of view it's a ridiculous demand. And from an
engineering point of view, it's a ridiculous demand too, for if
get_random_bytes already returned FIPS-compliant randomness, you
wouldn't be writing this email here.

Instead, if you want your FIPS garbage in the Linux RNG, take your
battle for that over to random.c. I'll oppose you to the end on it,
but at the very least it will the the appropriate venue for that
discussion. In the meantime, stop hijacking this thread.

Thanks,
Jason

PS: apologies for what's probably perceived as an unfriendly and
overly hostile tone of this email. It's not my intention to alienate
you, but I do feel necessary in these circumstances to write as
directly as possible.

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-18 11:24     ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-18 11:24 UTC (permalink / raw)
  To: linux-security-module

Hello Stephan,

I realize you have your Linux RNG project, which is a very worthwhile
goal that I support you in. I hope you're eventually successful, and I
apologize for not being more available in the last 2.5 months to chime
in on that patchset thread you posted.

However, your message to this thread here is a completely
inappropriate and nonsensical hijacking, which makes me entirely
question your overall judgement.

(David -- please don't let such hijacking derail or delay these
critical vulnerability fixes from landing.)

> This change is a challenge. The use of the kernel crypto API's DRNG has been
> made to allow FIPS 140-2 compliance. Otherwise, the entire key generation
> logic will not be using the right(TM) DRNG. Thus, I would not suggest to
> replace that for a stable tree.

Complete and utter nonsense. This patch has a history (this is already
v6), and it's pretty obvious from prior discussion and conclusion that
the only reason "crypto/rng" was used for this is because the original
author just had no idea what he was doing (thus leading to using ECB
as well). Besides, from what RNG do you think the seed for that was
generated?

> Note, I am currently working on a pluggable DRNG apporach for /dev/random and
> /dev/urandom to be able to get rid of the use of the kernel crypto API's DRNG
> API. It is ready and I will air that solution shortly.

Good to hear you're still working on that project.

> Yet, it needs work to
> be integrated upstream (and approval from Ted Tso).

Good luck with getting approval... While Ted and I have our
differences like any two kernel developers, I really tend agree with
him in his attitude about this FIPS silliness. It's unlikely you're
going to be able to shovel this stuff into random.c, and I think doing
so will undermine your entire LRNG effort.

> An SP800-90A-compliant DRNG must be used in those circumstances.

Again, complete and utter unsubstantiated nonsense.

> Then I would recommend to leave it as is or hear complaints from other users.

What a poor lack of judgement.

I get it -- this FIPS compliance backwardness is something you're keen
about. But don't start hijacking every thread that involves randomness
with a demand to replace calls to get_random_bytes with wrappers in
outdated, flawed, government compliance crypto API key expanders. From
a cryptographic point of view it's a ridiculous demand. And from an
engineering point of view, it's a ridiculous demand too, for if
get_random_bytes already returned FIPS-compliant randomness, you
wouldn't be writing this email here.

Instead, if you want your FIPS garbage in the Linux RNG, take your
battle for that over to random.c. I'll oppose you to the end on it,
but at the very least it will the the appropriate venue for that
discussion. In the meantime, stop hijacking this thread.

Thanks,
Jason

PS: apologies for what's probably perceived as an unfriendly and
overly hostile tone of this email. It's not my intention to alienate
you, but I do feel necessary in these circumstances to write as
directly as possible.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-18 11:24     ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-18 11:24 UTC (permalink / raw)
  To: Stephan Mueller
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	Ard Biesheuvel, Ilhan Gurel, security, stable, Theodore Ts'o

Hello Stephan,

I realize you have your Linux RNG project, which is a very worthwhile
goal that I support you in. I hope you're eventually successful, and I
apologize for not being more available in the last 2.5 months to chime
in on that patchset thread you posted.

However, your message to this thread here is a completely
inappropriate and nonsensical hijacking, which makes me entirely
question your overall judgement.

(David -- please don't let such hijacking derail or delay these
critical vulnerability fixes from landing.)

> This change is a challenge. The use of the kernel crypto API's DRNG has been
> made to allow FIPS 140-2 compliance. Otherwise, the entire key generation
> logic will not be using the right(TM) DRNG. Thus, I would not suggest to
> replace that for a stable tree.

Complete and utter nonsense. This patch has a history (this is already
v6), and it's pretty obvious from prior discussion and conclusion that
the only reason "crypto/rng" was used for this is because the original
author just had no idea what he was doing (thus leading to using ECB
as well). Besides, from what RNG do you think the seed for that was
generated?

> Note, I am currently working on a pluggable DRNG apporach for /dev/random and
> /dev/urandom to be able to get rid of the use of the kernel crypto API's DRNG
> API. It is ready and I will air that solution shortly.

Good to hear you're still working on that project.

> Yet, it needs work to
> be integrated upstream (and approval from Ted Tso).

Good luck with getting approval... While Ted and I have our
differences like any two kernel developers, I really tend agree with
him in his attitude about this FIPS silliness. It's unlikely you're
going to be able to shovel this stuff into random.c, and I think doing
so will undermine your entire LRNG effort.

> An SP800-90A-compliant DRNG must be used in those circumstances.

Again, complete and utter unsubstantiated nonsense.

> Then I would recommend to leave it as is or hear complaints from other users.

What a poor lack of judgement.

I get it -- this FIPS compliance backwardness is something you're keen
about. But don't start hijacking every thread that involves randomness
with a demand to replace calls to get_random_bytes with wrappers in
outdated, flawed, government compliance crypto API key expanders. From
a cryptographic point of view it's a ridiculous demand. And from an
engineering point of view, it's a ridiculous demand too, for if
get_random_bytes already returned FIPS-compliant randomness, you
wouldn't be writing this email here.

Instead, if you want your FIPS garbage in the Linux RNG, take your
battle for that over to random.c. I'll oppose you to the end on it,
but at the very least it will the the appropriate venue for that
discussion. In the meantime, stop hijacking this thread.

Thanks,
Jason

PS: apologies for what's probably perceived as an unfriendly and
overly hostile tone of this email. It's not my intention to alienate
you, but I do feel necessary in these circumstances to write as
directly as possible.

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v4] security/keys: rewrite all of big_key crypto
  2017-09-18 11:24     ` Jason A. Donenfeld
  (?)
  (?)
@ 2017-09-19 13:39       ` Theodore Ts'o
  -1 siblings, 0 replies; 79+ messages in thread
From: Theodore Ts'o @ 2017-09-19 13:39 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Stephan Mueller, linux-security-module, keyrings,
	kernel-hardening, LKML, David Howells, Eric Biggers, Herbert Xu,
	Kirill Marinushkin, Ard Biesheuvel, Ilhan Gurel, security,
	stable

On Mon, Sep 18, 2017 at 01:24:18PM +0200, Jason A. Donenfeld wrote:
> Good luck with getting approval... While Ted and I have our
> differences like any two kernel developers, I really tend agree with
> him in his attitude about this FIPS silliness. It's unlikely you're
> going to be able to shovel this stuff into random.c, and I think doing
> so will undermine your entire LRNG effort.

Let me add one more reason why FIPS compliance for the kernel is just
***stupid***.  The way FIPS compliance works, you have to pay hundreds
of thousands of dollars to a FIPS certification lab to certify a
specific binary, complete with the exact build environment (compiler,
binutils, etc.) used to build that kernel binary.

The moment you need to make a change --- say, to fix a critical
zero-day security bug --- this breaks the FIPS certification, and you
then have to go back to the FIPS certification lab, and pay another
hundreds of thousands of dollars for another certification.  This will
take weeks/months, and while you are waiting for the results to come
back from the FIPS certification lab, the hackers will be busy
extracting another 143 million credit histories, or another 4.1
million SF-86 Security Clearance Forms from the systems involved.  :-)

You might say that FIPS certification != FIPS compliance.  Sure, but
the only silly people who care about FIPS compliance also need FIPS
certification, for the US Goverment signoff.

Realistically, people who need FIPS certification will need to use
FIPS certified crypto in hardware.  In which case the FIPS certified
RNG, as well as the FIPS certified crypto, will all be in a single
certified lump of hardware, which doesn't have to change when we need
to fix various kernel bugs.

Cheers,

						- Ted

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-19 13:39       ` Theodore Ts'o
  0 siblings, 0 replies; 79+ messages in thread
From: Theodore Ts'o @ 2017-09-19 13:39 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Stephan Mueller, linux-security-module, keyrings,
	kernel-hardening, LKML, David Howells, Eric Biggers, Herbert Xu,
	Kirill Marinushkin, Ard Biesheuvel, Ilhan Gurel, security,
	stable

On Mon, Sep 18, 2017 at 01:24:18PM +0200, Jason A. Donenfeld wrote:
> Good luck with getting approval... While Ted and I have our
> differences like any two kernel developers, I really tend agree with
> him in his attitude about this FIPS silliness. It's unlikely you're
> going to be able to shovel this stuff into random.c, and I think doing
> so will undermine your entire LRNG effort.

Let me add one more reason why FIPS compliance for the kernel is just
***stupid***.  The way FIPS compliance works, you have to pay hundreds
of thousands of dollars to a FIPS certification lab to certify a
specific binary, complete with the exact build environment (compiler,
binutils, etc.) used to build that kernel binary.

The moment you need to make a change --- say, to fix a critical
zero-day security bug --- this breaks the FIPS certification, and you
then have to go back to the FIPS certification lab, and pay another
hundreds of thousands of dollars for another certification.  This will
take weeks/months, and while you are waiting for the results to come
back from the FIPS certification lab, the hackers will be busy
extracting another 143 million credit histories, or another 4.1
million SF-86 Security Clearance Forms from the systems involved.  :-)

You might say that FIPS certification != FIPS compliance.  Sure, but
the only silly people who care about FIPS compliance also need FIPS
certification, for the US Goverment signoff.

Realistically, people who need FIPS certification will need to use
FIPS certified crypto in hardware.  In which case the FIPS certified
RNG, as well as the FIPS certified crypto, will all be in a single
certified lump of hardware, which doesn't have to change when we need
to fix various kernel bugs.

Cheers,

						- Ted

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-19 13:39       ` Theodore Ts'o
  0 siblings, 0 replies; 79+ messages in thread
From: Theodore Ts'o @ 2017-09-19 13:39 UTC (permalink / raw)
  To: linux-security-module

On Mon, Sep 18, 2017 at 01:24:18PM +0200, Jason A. Donenfeld wrote:
> Good luck with getting approval... While Ted and I have our
> differences like any two kernel developers, I really tend agree with
> him in his attitude about this FIPS silliness. It's unlikely you're
> going to be able to shovel this stuff into random.c, and I think doing
> so will undermine your entire LRNG effort.

Let me add one more reason why FIPS compliance for the kernel is just
***stupid***.  The way FIPS compliance works, you have to pay hundreds
of thousands of dollars to a FIPS certification lab to certify a
specific binary, complete with the exact build environment (compiler,
binutils, etc.) used to build that kernel binary.

The moment you need to make a change --- say, to fix a critical
zero-day security bug --- this breaks the FIPS certification, and you
then have to go back to the FIPS certification lab, and pay another
hundreds of thousands of dollars for another certification.  This will
take weeks/months, and while you are waiting for the results to come
back from the FIPS certification lab, the hackers will be busy
extracting another 143 million credit histories, or another 4.1
million SF-86 Security Clearance Forms from the systems involved.  :-)

You might say that FIPS certification != FIPS compliance.  Sure, but
the only silly people who care about FIPS compliance also need FIPS
certification, for the US Goverment signoff.

Realistically, people who need FIPS certification will need to use
FIPS certified crypto in hardware.  In which case the FIPS certified
RNG, as well as the FIPS certified crypto, will all be in a single
certified lump of hardware, which doesn't have to change when we need
to fix various kernel bugs.

Cheers,

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-19 13:39       ` Theodore Ts'o
  0 siblings, 0 replies; 79+ messages in thread
From: Theodore Ts'o @ 2017-09-19 13:39 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Stephan Mueller, linux-security-module, keyrings,
	kernel-hardening, LKML, David Howells, Eric Biggers, Herbert Xu,
	Kirill Marinushkin, Ard Biesheuvel, Ilhan Gurel, security,
	stable

On Mon, Sep 18, 2017 at 01:24:18PM +0200, Jason A. Donenfeld wrote:
> Good luck with getting approval... While Ted and I have our
> differences like any two kernel developers, I really tend agree with
> him in his attitude about this FIPS silliness. It's unlikely you're
> going to be able to shovel this stuff into random.c, and I think doing
> so will undermine your entire LRNG effort.

Let me add one more reason why FIPS compliance for the kernel is just
***stupid***.  The way FIPS compliance works, you have to pay hundreds
of thousands of dollars to a FIPS certification lab to certify a
specific binary, complete with the exact build environment (compiler,
binutils, etc.) used to build that kernel binary.

The moment you need to make a change --- say, to fix a critical
zero-day security bug --- this breaks the FIPS certification, and you
then have to go back to the FIPS certification lab, and pay another
hundreds of thousands of dollars for another certification.  This will
take weeks/months, and while you are waiting for the results to come
back from the FIPS certification lab, the hackers will be busy
extracting another 143 million credit histories, or another 4.1
million SF-86 Security Clearance Forms from the systems involved.  :-)

You might say that FIPS certification != FIPS compliance.  Sure, but
the only silly people who care about FIPS compliance also need FIPS
certification, for the US Goverment signoff.

Realistically, people who need FIPS certification will need to use
FIPS certified crypto in hardware.  In which case the FIPS certified
RNG, as well as the FIPS certified crypto, will all be in a single
certified lump of hardware, which doesn't have to change when we need
to fix various kernel bugs.

Cheers,

						- Ted

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v6] security/keys: rewrite all of big_key crypto
  2017-09-17 11:50       ` Jason A. Donenfeld
  (?)
  (?)
@ 2017-09-19 15:38         ` David Howells
  -1 siblings, 0 replies; 79+ messages in thread
From: David Howells @ 2017-09-19 15:38 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: dhowells, linux-security-module, keyrings, kernel-hardening,
	linux-kernel, ebiggers3, Herbert Xu, Kirill Marinushkin,
	security, stable

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

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-19 15:38         ` David Howells
  0 siblings, 0 replies; 79+ messages in thread
From: David Howells @ 2017-09-19 15:38 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: dhowells, linux-security-module, keyrings, kernel-hardening,
	linux-kernel, ebiggers3, Herbert Xu, Kirill Marinushkin,
	security, stable

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

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-19 15:38         ` David Howells
  0 siblings, 0 replies; 79+ messages in thread
From: David Howells @ 2017-09-19 15:38 UTC (permalink / raw)
  To: linux-security-module

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-19 15:38         ` David Howells
  0 siblings, 0 replies; 79+ messages in thread
From: David Howells @ 2017-09-19 15:38 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: dhowells, linux-security-module, keyrings, kernel-hardening,
	linux-kernel, ebiggers3, Herbert Xu, Kirill Marinushkin,
	security, stable

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

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
  2017-09-19 13:39       ` Theodore Ts'o
  (?)
@ 2017-09-19 19:04         ` Sandy Harris
  -1 siblings, 0 replies; 79+ messages in thread
From: Sandy Harris @ 2017-09-19 19:04 UTC (permalink / raw)
  To: Theodore Ts'o, Jason A. Donenfeld, Stephan Mueller,
	linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	Ard Biesheuvel, Ilhan Gurel, security, stable

On Tue, Sep 19, 2017 at 9:39 AM, Theodore Ts'o <tytso@mit.edu> wrote:

> On Mon, Sep 18, 2017 at 01:24:18PM +0200, Jason A. Donenfeld wrote:
>> Good luck with getting approval... While Ted and I have our
>> differences like any two kernel developers, I really tend agree with
>> him in his attitude about this FIPS silliness. ...
>
> Let me add one more reason why FIPS compliance for the kernel is just
> ***stupid***.  The way FIPS compliance works, you have to pay hundreds
> of thousands of dollars to a FIPS certification lab to certify a
> specific binary, complete with the exact build environment (compiler,
> binutils, etc.) used to build that kernel binary.
>
> The moment you need to make a change --- say, to fix a critical
> zero-day security bug --- this breaks the FIPS certification, ...
>
> You might say that FIPS certification != FIPS compliance.  Sure, but
> the only silly people who care about FIPS compliance also need FIPS
> certification, for the US Goverment signoff.

I do not think it is just the US that matters here. If I understand
Stefan correctly, one of his concerns is German (or EU?) gov't
standards that are somehow related. I'm very hazy on details.

I emphatically agree with Ted on some points here. Making
FIPS certification a goal for kernel development would be
really dumb. Having multiple RNGs available & compile-time
options to select among them also looks silly to me; we just
need one good one.

On the other hand, I do not see why the driver should not
use a FIPS-compliant PRNG where it can. This would make
things easier for anyone who does seek certification. One
of the big distro vendors? A gov't department or contractor
that wants to use Linux? A corporation's sys admin or
security reviewer? Stefan?

I cannot see much downside to this. Is the current PRNG
more efficient? More easily maintained? Is it still the case
that random(4) must use only hashes, not ciphers, to
avoid restrictions under export laws?

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-19 19:04         ` Sandy Harris
  0 siblings, 0 replies; 79+ messages in thread
From: Sandy Harris @ 2017-09-19 19:04 UTC (permalink / raw)
  To: Theodore Ts'o, Jason A. Donenfeld, Stephan Mueller,
	linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	Ard Biesheuvel, Ilhan Gurel, security, stable

On Tue, Sep 19, 2017 at 9:39 AM, Theodore Ts'o <tytso@mit.edu> wrote:

> On Mon, Sep 18, 2017 at 01:24:18PM +0200, Jason A. Donenfeld wrote:
>> Good luck with getting approval... While Ted and I have our
>> differences like any two kernel developers, I really tend agree with
>> him in his attitude about this FIPS silliness. ...
>
> Let me add one more reason why FIPS compliance for the kernel is just
> ***stupid***.  The way FIPS compliance works, you have to pay hundreds
> of thousands of dollars to a FIPS certification lab to certify a
> specific binary, complete with the exact build environment (compiler,
> binutils, etc.) used to build that kernel binary.
>
> The moment you need to make a change --- say, to fix a critical
> zero-day security bug --- this breaks the FIPS certification, ...
>
> You might say that FIPS certification != FIPS compliance.  Sure, but
> the only silly people who care about FIPS compliance also need FIPS
> certification, for the US Goverment signoff.

I do not think it is just the US that matters here. If I understand
Stefan correctly, one of his concerns is German (or EU?) gov't
standards that are somehow related. I'm very hazy on details.

I emphatically agree with Ted on some points here. Making
FIPS certification a goal for kernel development would be
really dumb. Having multiple RNGs available & compile-time
options to select among them also looks silly to me; we just
need one good one.

On the other hand, I do not see why the driver should not
use a FIPS-compliant PRNG where it can. This would make
things easier for anyone who does seek certification. One
of the big distro vendors? A gov't department or contractor
that wants to use Linux? A corporation's sys admin or
security reviewer? Stefan?

I cannot see much downside to this. Is the current PRNG
more efficient? More easily maintained? Is it still the case
that random(4) must use only hashes, not ciphers, to
avoid restrictions under export laws?

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-19 19:04         ` Sandy Harris
  0 siblings, 0 replies; 79+ messages in thread
From: Sandy Harris @ 2017-09-19 19:04 UTC (permalink / raw)
  To: linux-security-module

On Tue, Sep 19, 2017 at 9:39 AM, Theodore Ts'o <tytso@mit.edu> wrote:

> On Mon, Sep 18, 2017 at 01:24:18PM +0200, Jason A. Donenfeld wrote:
>> Good luck with getting approval... While Ted and I have our
>> differences like any two kernel developers, I really tend agree with
>> him in his attitude about this FIPS silliness. ...
>
> Let me add one more reason why FIPS compliance for the kernel is just
> ***stupid***.  The way FIPS compliance works, you have to pay hundreds
> of thousands of dollars to a FIPS certification lab to certify a
> specific binary, complete with the exact build environment (compiler,
> binutils, etc.) used to build that kernel binary.
>
> The moment you need to make a change --- say, to fix a critical
> zero-day security bug --- this breaks the FIPS certification, ...
>
> You might say that FIPS certification != FIPS compliance.  Sure, but
> the only silly people who care about FIPS compliance also need FIPS
> certification, for the US Goverment signoff.

I do not think it is just the US that matters here. If I understand
Stefan correctly, one of his concerns is German (or EU?) gov't
standards that are somehow related. I'm very hazy on details.

I emphatically agree with Ted on some points here. Making
FIPS certification a goal for kernel development would be
really dumb. Having multiple RNGs available & compile-time
options to select among them also looks silly to me; we just
need one good one.

On the other hand, I do not see why the driver should not
use a FIPS-compliant PRNG where it can. This would make
things easier for anyone who does seek certification. One
of the big distro vendors? A gov't department or contractor
that wants to use Linux? A corporation's sys admin or
security reviewer? Stefan?

I cannot see much downside to this. Is the current PRNG
more efficient? More easily maintained? Is it still the case
that random(4) must use only hashes, not ciphers, to
avoid restrictions under export laws?
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
  2017-09-19 19:04         ` Sandy Harris
  (?)
@ 2017-09-19 19:17           ` Jason A. Donenfeld
  -1 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-19 19:17 UTC (permalink / raw)
  To: Sandy Harris
  Cc: Theodore Ts'o, Stephan Mueller, linux-security-module,
	keyrings, kernel-hardening, LKML, David Howells, Eric Biggers,
	Herbert Xu, Kirill Marinushkin, Ard Biesheuvel, Ilhan Gurel,
	security, stable

On Tue, Sep 19, 2017 at 9:04 PM, Sandy Harris <sandyinchina@gmail.com> wrote:
> On the other hand, I do not see why the driver should not

From my perspective, this discussion is over. I'll be carrying out
David's requested patchset changes and submitting it. If you'd like
different cryptography, you'll have to find somebody else to do that
work for you.

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-19 19:17           ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-19 19:17 UTC (permalink / raw)
  To: Sandy Harris
  Cc: Theodore Ts'o, Stephan Mueller, linux-security-module,
	keyrings, kernel-hardening, LKML, David Howells, Eric Biggers,
	Herbert Xu, Kirill Marinushkin, Ard Biesheuvel, Ilhan Gurel,
	security, stable

On Tue, Sep 19, 2017 at 9:04 PM, Sandy Harris <sandyinchina@gmail.com> wrote:
> On the other hand, I do not see why the driver should not

>From my perspective, this discussion is over. I'll be carrying out
David's requested patchset changes and submitting it. If you'd like
different cryptography, you'll have to find somebody else to do that
work for you.

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-19 19:17           ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-19 19:17 UTC (permalink / raw)
  To: linux-security-module

On Tue, Sep 19, 2017 at 9:04 PM, Sandy Harris <sandyinchina@gmail.com> wrote:
> On the other hand, I do not see why the driver should not

>From my perspective, this discussion is over. I'll be carrying out
David's requested patchset changes and submitting it. If you'd like
different cryptography, you'll have to find somebody else to do that
work for you.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
  2017-09-19 19:04         ` Sandy Harris
  (?)
@ 2017-09-20  1:16           ` Theodore Ts'o
  -1 siblings, 0 replies; 79+ messages in thread
From: Theodore Ts'o @ 2017-09-20  1:16 UTC (permalink / raw)
  To: Sandy Harris
  Cc: Jason A. Donenfeld, Stephan Mueller, linux-security-module,
	keyrings, kernel-hardening, LKML, David Howells, Eric Biggers,
	Herbert Xu, Kirill Marinushkin, Ard Biesheuvel, Ilhan Gurel,
	security, stable

On Tue, Sep 19, 2017 at 03:04:29PM -0400, Sandy Harris wrote:
> On the other hand, I do not see why the driver should not
> use a FIPS-compliant PRNG where it can. This would make
> things easier for anyone who does seek certification. One
> of the big distro vendors? A gov't department or contractor
> that wants to use Linux? A corporation's sys admin or
> security reviewer? Stefan?

First, making it easier for a sysadmin to seek certification is
creating an attractive nuisance.  That means that after said company
sinks $100,000+ into getting a certification, they will be hesitant to
take the kernel update to fix that zero-day bug, less it causes them
to lose that certification.

Secondly, I've worked with a defense contractor wanting to use (and
did use) Linux.  Specifically, on the Zumwalt class destroyer, DD-21,
although back when I worked on it was the DD(X) program.  I can assure
you the fact that /dev/random wasn't FIPS certified wasn't a problem
with either Raytheon or the US Navy.  Really.  If you're really
serious about crypto, and you do government work, it will be type 1
ciphers implemented in hardware, courtesy of the NSA.

So really.  You can use Linux without getting FIPS certification.
Lots of copies of Linux are used in the government already, without
FIPS certification.

> I cannot see much downside to this. Is the current PRNG
> more efficient? More easily maintained? Is it still the case
> that random(4) must use only hashes, not ciphers, to
> avoid restrictions under export laws?

Linux is now using a Chacha20 based random number generator, much like
OpenBSD.  It's stream cipher-based CSPRNG, which is much more
efficient than a block cipher or HMAC based DRBG.  Unfortunately, it's
also not one of the types defined in NIST 800-90A rev 1.

       	     	     	     	     	 - Ted





^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-20  1:16           ` Theodore Ts'o
  0 siblings, 0 replies; 79+ messages in thread
From: Theodore Ts'o @ 2017-09-20  1:16 UTC (permalink / raw)
  To: Sandy Harris
  Cc: Jason A. Donenfeld, Stephan Mueller, linux-security-module,
	keyrings, kernel-hardening, LKML, David Howells, Eric Biggers,
	Herbert Xu, Kirill Marinushkin, Ard Biesheuvel, Ilhan Gurel,
	security, stable

On Tue, Sep 19, 2017 at 03:04:29PM -0400, Sandy Harris wrote:
> On the other hand, I do not see why the driver should not
> use a FIPS-compliant PRNG where it can. This would make
> things easier for anyone who does seek certification. One
> of the big distro vendors? A gov't department or contractor
> that wants to use Linux? A corporation's sys admin or
> security reviewer? Stefan?

First, making it easier for a sysadmin to seek certification is
creating an attractive nuisance.  That means that after said company
sinks $100,000+ into getting a certification, they will be hesitant to
take the kernel update to fix that zero-day bug, less it causes them
to lose that certification.

Secondly, I've worked with a defense contractor wanting to use (and
did use) Linux.  Specifically, on the Zumwalt class destroyer, DD-21,
although back when I worked on it was the DD(X) program.  I can assure
you the fact that /dev/random wasn't FIPS certified wasn't a problem
with either Raytheon or the US Navy.  Really.  If you're really
serious about crypto, and you do government work, it will be type 1
ciphers implemented in hardware, courtesy of the NSA.

So really.  You can use Linux without getting FIPS certification.
Lots of copies of Linux are used in the government already, without
FIPS certification.

> I cannot see much downside to this. Is the current PRNG
> more efficient? More easily maintained? Is it still the case
> that random(4) must use only hashes, not ciphers, to
> avoid restrictions under export laws?

Linux is now using a Chacha20 based random number generator, much like
OpenBSD.  It's stream cipher-based CSPRNG, which is much more
efficient than a block cipher or HMAC based DRBG.  Unfortunately, it's
also not one of the types defined in NIST 800-90A rev 1.

       	     	     	     	     	 - Ted

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v4] security/keys: rewrite all of big_key crypto
@ 2017-09-20  1:16           ` Theodore Ts'o
  0 siblings, 0 replies; 79+ messages in thread
From: Theodore Ts'o @ 2017-09-20  1:16 UTC (permalink / raw)
  To: linux-security-module

On Tue, Sep 19, 2017 at 03:04:29PM -0400, Sandy Harris wrote:
> On the other hand, I do not see why the driver should not
> use a FIPS-compliant PRNG where it can. This would make
> things easier for anyone who does seek certification. One
> of the big distro vendors? A gov't department or contractor
> that wants to use Linux? A corporation's sys admin or
> security reviewer? Stefan?

First, making it easier for a sysadmin to seek certification is
creating an attractive nuisance.  That means that after said company
sinks $100,000+ into getting a certification, they will be hesitant to
take the kernel update to fix that zero-day bug, less it causes them
to lose that certification.

Secondly, I've worked with a defense contractor wanting to use (and
did use) Linux.  Specifically, on the Zumwalt class destroyer, DD-21,
although back when I worked on it was the DD(X) program.  I can assure
you the fact that /dev/random wasn't FIPS certified wasn't a problem
with either Raytheon or the US Navy.  Really.  If you're really
serious about crypto, and you do government work, it will be type 1
ciphers implemented in hardware, courtesy of the NSA.

So really.  You can use Linux without getting FIPS certification.
Lots of copies of Linux are used in the government already, without
FIPS certification.

> I cannot see much downside to this. Is the current PRNG
> more efficient? More easily maintained? Is it still the case
> that random(4) must use only hashes, not ciphers, to
> avoid restrictions under export laws?

Linux is now using a Chacha20 based random number generator, much like
OpenBSD.  It's stream cipher-based CSPRNG, which is much more
efficient than a block cipher or HMAC based DRBG.  Unfortunately, it's
also not one of the types defined in NIST 800-90A rev 1.

       	     	     	     	     	 - Ted




--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v6] security/keys: rewrite all of big_key crypto
  2017-09-17 11:52         ` Jason A. Donenfeld
  (?)
  (?)
@ 2017-09-20  5:30           ` Stephan Mueller
  -1 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-20  5:30 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, ebiggers3, Herbert Xu, Kirill Marinushkin, security,
	stable

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

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20  5:30           ` Stephan Mueller
  0 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-20  5:30 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, ebiggers3, Herbert Xu, Kirill Marinushkin, security,
	stable

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

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20  5:30           ` Stephan Mueller
  0 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-20  5:30 UTC (permalink / raw)
  To: linux-security-module

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20  5:30           ` Stephan Mueller
  0 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-20  5:30 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-security-module, keyrings, kernel-hardening, linux-kernel,
	dhowells, ebiggers3, Herbert Xu, Kirill Marinushkin, security,
	stable

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

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v6] security/keys: rewrite all of big_key crypto
  2017-09-20  5:30           ` Stephan Mueller
  (?)
  (?)
@ 2017-09-20 10:52             ` Jason A. Donenfeld
  -1 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-20 10:52 UTC (permalink / raw)
  To: Stephan Mueller
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	security, stable

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?

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 10:52             ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-20 10:52 UTC (permalink / raw)
  To: Stephan Mueller
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	security, stable

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?

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 10:52             ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-20 10:52 UTC (permalink / raw)
  To: linux-security-module

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 10:52             ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-20 10:52 UTC (permalink / raw)
  To: Stephan Mueller
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	security, stable

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?

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v6] security/keys: rewrite all of big_key crypto
  2017-09-20 10:52             ` Jason A. Donenfeld
  (?)
  (?)
@ 2017-09-20 13:45               ` Stephan Mueller
  -1 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-20 13:45 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	security, stable

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

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 13:45               ` Stephan Mueller
  0 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-20 13:45 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	security, stable

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

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 13:45               ` Stephan Mueller
  0 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-20 13:45 UTC (permalink / raw)
  To: linux-security-module

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 13:45               ` Stephan Mueller
  0 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-20 13:45 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	security, stable

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

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v6] security/keys: rewrite all of big_key crypto
  2017-09-20 13:45               ` Stephan Mueller
  (?)
  (?)
@ 2017-09-20 14:01                 ` Jason A. Donenfeld
  -1 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-20 14:01 UTC (permalink / raw)
  To: Stephan Mueller
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	security, stable

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.

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 14:01                 ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-20 14:01 UTC (permalink / raw)
  To: Stephan Mueller
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	security, stable

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.

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 14:01                 ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-20 14:01 UTC (permalink / raw)
  To: linux-security-module

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 14:01                 ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-20 14:01 UTC (permalink / raw)
  To: Stephan Mueller
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	security, stable

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.

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v6] security/keys: rewrite all of big_key crypto
  2017-09-20 14:01                 ` Jason A. Donenfeld
  (?)
  (?)
@ 2017-09-20 14:06                   ` Stephan Mueller
  -1 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-20 14:06 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	security, stable

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

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 14:06                   ` Stephan Mueller
  0 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-20 14:06 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	security, stable

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

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 14:06                   ` Stephan Mueller
  0 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-20 14:06 UTC (permalink / raw)
  To: linux-security-module

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 14:06                   ` Stephan Mueller
  0 siblings, 0 replies; 79+ messages in thread
From: Stephan Mueller @ 2017-09-20 14:06 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	security, stable

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

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v6] security/keys: rewrite all of big_key crypto
  2017-09-20 14:06                   ` Stephan Mueller
  (?)
  (?)
@ 2017-09-20 14:09                     ` Jason A. Donenfeld
  -1 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-20 14:09 UTC (permalink / raw)
  To: Stephan Mueller
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	security, stable

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.

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 14:09                     ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-20 14:09 UTC (permalink / raw)
  To: Stephan Mueller
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	security, stable

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.

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 14:09                     ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-20 14:09 UTC (permalink / raw)
  To: linux-security-module

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 14:09                     ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-20 14:09 UTC (permalink / raw)
  To: Stephan Mueller
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	David Howells, Eric Biggers, Herbert Xu, Kirill Marinushkin,
	security, stable

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.

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v6] security/keys: rewrite all of big_key crypto
  2017-09-19 15:38         ` David Howells
  (?)
  (?)
@ 2017-09-20 14:56           ` Jason A. Donenfeld
  -1 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-20 14:56 UTC (permalink / raw)
  To: David Howells
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	Eric Biggers, Herbert Xu, Kirill Marinushkin, security, stable

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.

^ permalink raw reply	[flat|nested] 79+ messages in thread

* Re: [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 14:56           ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-20 14:56 UTC (permalink / raw)
  To: David Howells
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	Eric Biggers, Herbert Xu, Kirill Marinushkin, security, stable

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.

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 14:56           ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-20 14:56 UTC (permalink / raw)
  To: linux-security-module

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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 79+ messages in thread

* [kernel-hardening] Re: [PATCH v6] security/keys: rewrite all of big_key crypto
@ 2017-09-20 14:56           ` Jason A. Donenfeld
  0 siblings, 0 replies; 79+ messages in thread
From: Jason A. Donenfeld @ 2017-09-20 14:56 UTC (permalink / raw)
  To: David Howells
  Cc: linux-security-module, keyrings, kernel-hardening, LKML,
	Eric Biggers, Herbert Xu, Kirill Marinushkin, security, stable

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.

^ permalink raw reply	[flat|nested] 79+ messages in thread

end of thread, other threads:[~2017-09-20 14:56 UTC | newest]

Thread overview: 79+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-16 13:00 [PATCH v4] security/keys: rewrite all of big_key crypto Jason A. Donenfeld
2017-09-16 13:00 ` [kernel-hardening] " Jason A. Donenfeld
2017-09-16 13:00 ` Jason A. Donenfeld
2017-09-16 13:00 ` Jason A. Donenfeld
2017-09-16 13:05 ` [PATCH v5] " Jason A. Donenfeld
2017-09-16 13:05   ` [kernel-hardening] " Jason A. Donenfeld
2017-09-16 13:05   ` Jason A. Donenfeld
2017-09-16 13:05   ` Jason A. Donenfeld
2017-09-17  6:04   ` Eric Biggers
2017-09-17  6:04     ` [kernel-hardening] " Eric Biggers
2017-09-17  6:04     ` Eric Biggers
2017-09-17  6:04     ` Eric Biggers
2017-09-17 11:50     ` Jason A. Donenfeld
2017-09-17 11:50       ` [kernel-hardening] " Jason A. Donenfeld
2017-09-17 11:50       ` Jason A. Donenfeld
2017-09-17 11:50       ` Jason A. Donenfeld
2017-09-17 11:52       ` [PATCH v6] " Jason A. Donenfeld
2017-09-17 11:52         ` [kernel-hardening] " Jason A. Donenfeld
2017-09-17 11:52         ` Jason A. Donenfeld
2017-09-17 11:52         ` Jason A. Donenfeld
2017-09-20  5:30         ` Stephan Mueller
2017-09-20  5:30           ` [kernel-hardening] " Stephan Mueller
2017-09-20  5:30           ` Stephan Mueller
2017-09-20  5:30           ` Stephan Mueller
2017-09-20 10:52           ` Jason A. Donenfeld
2017-09-20 10:52             ` [kernel-hardening] " Jason A. Donenfeld
2017-09-20 10:52             ` Jason A. Donenfeld
2017-09-20 10:52             ` Jason A. Donenfeld
2017-09-20 13:45             ` Stephan Mueller
2017-09-20 13:45               ` [kernel-hardening] " Stephan Mueller
2017-09-20 13:45               ` Stephan Mueller
2017-09-20 13:45               ` Stephan Mueller
2017-09-20 14:01               ` Jason A. Donenfeld
2017-09-20 14:01                 ` [kernel-hardening] " Jason A. Donenfeld
2017-09-20 14:01                 ` Jason A. Donenfeld
2017-09-20 14:01                 ` Jason A. Donenfeld
2017-09-20 14:06                 ` Stephan Mueller
2017-09-20 14:06                   ` [kernel-hardening] " Stephan Mueller
2017-09-20 14:06                   ` Stephan Mueller
2017-09-20 14:06                   ` Stephan Mueller
2017-09-20 14:09                   ` Jason A. Donenfeld
2017-09-20 14:09                     ` [kernel-hardening] " Jason A. Donenfeld
2017-09-20 14:09                     ` Jason A. Donenfeld
2017-09-20 14:09                     ` Jason A. Donenfeld
2017-09-19 15:38       ` David Howells
2017-09-19 15:38         ` [kernel-hardening] " David Howells
2017-09-19 15:38         ` David Howells
2017-09-19 15:38         ` David Howells
2017-09-20 14:56         ` Jason A. Donenfeld
2017-09-20 14:56           ` [kernel-hardening] " Jason A. Donenfeld
2017-09-20 14:56           ` Jason A. Donenfeld
2017-09-20 14:56           ` Jason A. Donenfeld
2017-09-18  8:49 ` [PATCH v4] " Stephan Mueller
2017-09-18  8:49   ` [kernel-hardening] " Stephan Mueller
2017-09-18  8:49   ` Stephan Mueller
2017-09-18  8:49   ` Stephan Mueller
2017-09-18  9:04   ` [kernel-hardening] " Greg KH
2017-09-18  9:04     ` Greg KH
2017-09-18  9:04     ` Greg KH
2017-09-18  9:12     ` Stephan Mueller
2017-09-18  9:12       ` Stephan Mueller
2017-09-18  9:12       ` Stephan Mueller
2017-09-18 11:24   ` Jason A. Donenfeld
2017-09-18 11:24     ` [kernel-hardening] " Jason A. Donenfeld
2017-09-18 11:24     ` Jason A. Donenfeld
2017-09-18 11:24     ` Jason A. Donenfeld
2017-09-19 13:39     ` Theodore Ts'o
2017-09-19 13:39       ` [kernel-hardening] " Theodore Ts'o
2017-09-19 13:39       ` Theodore Ts'o
2017-09-19 13:39       ` Theodore Ts'o
2017-09-19 19:04       ` [kernel-hardening] " Sandy Harris
2017-09-19 19:04         ` Sandy Harris
2017-09-19 19:04         ` Sandy Harris
2017-09-19 19:17         ` Jason A. Donenfeld
2017-09-19 19:17           ` Jason A. Donenfeld
2017-09-19 19:17           ` Jason A. Donenfeld
2017-09-20  1:16         ` Theodore Ts'o
2017-09-20  1:16           ` Theodore Ts'o
2017-09-20  1:16           ` Theodore Ts'o

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.