security/keys: properly zero out sensitive key material in big_key

Error paths forgot to zero out sensitive material, so this patch changes some kfrees into a kzfrees.

---
 security/keys/big_key.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

index 6acb00f6f22c..507d6fb86a4f 100644
@@ -195,7 +195,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 +211,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 +227,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 +259,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;
 }
 

@@ -328,7 +328,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],

2.14.1

This started out as just replacing the use of crypto/rng with get_random_bytes_wait, so that we wouldn't use bad randomness at boot time. But, upon looking further, it appears that there were even deeper underlying cryptographic problems, and that this seems to have been committed with very little crypto review. So, I rewrote the whole thing, trying to keep to the conventions introduced by the previous author, to fix these cryptographic flaws.

It makes no sense to seed crypto/rng at boot time and then keep using it like this, when in fact there's already get_random_bytes_wait, which can ensure there's enough entropy and be a much more standard way of generating keys. Since this sensitive material is being stored untrusted, using ECB and no authentication is simply not okay at all. I find it surprising and a bit horrifying that this code even made it past basic crypto review, which perhaps points to some larger issues. This patch moves from using AES-ECB to using AES-GCM. Since keys are uniquely generated each time, we can set the nonce to zero. There was also a race condition in which the same key would be reused at the same time in different threads. A mutex fixes this issue now.

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

* Low entropy key generation, allowing an attacker to potentially guess or predict keys.
* Unauthenticated encryption, allowing an attacker to modify the cipher text in particular ways in order to manipulate the plaintext, which is is even more frightening considering the next point.
* Use of ECB mode, allowing an attacker to trivially swap blocks or compare identical plaintext blocks.
* Key re-use.
* Faulty memory zeroing.

---
 security/keys/Kconfig   |   4 +-
 security/keys/big_key.c | 127 ++++++++++++++++++++++--------------------------
 2 files changed, 60 insertions(+), 71 deletions(-)

index a7a23b5541f8..91eafada3164 100644
@@ -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

2.14.1

Not sure I parsed your message correctly, but that comment there was a suggestion from Eric several months ago when I first posted it. The idea is that the key shouldn't be reused with the same nonce, so in case somebody ambitious comes along and tries to add an .update function, they'll be reminded to take this into account. Alternatively they might choose to just generate a new key, which is fine too. Either way, it's a useful warning to have in place.