[PATCH 0/5] KEYS: sanitize key payloads

[PATCH 0/5] KEYS: sanitize key payloads

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

This patch series introduces more thorough sanitization of keys managed
by the kernel key retention service.  This helps keep sensitive key
material from sticking around in the slab caches after keys are released.

This series covers the syscall interface and several of the common key
types.  It doesn't cover some of the less commonly used key types.  Also,
these changes are of course limited to the keyrings mechanism itself and
don't remove the responsibility for keyrings users to securely handle any
other sensitive data they may copy or generate.  Regardless, there's no
reason not to make the keyrings API follow best practices.

Eric Biggers (5):
  KEYS: sanitize add_key() and keyctl() key payloads
  KEYS: user_defined: sanitize key payloads
  KEYS: encrypted: sanitize all key material
  KEYS: trusted: sanitize all key material
  KEYS: sanitize key structs before freeing

 include/linux/key.h                      |  1 -
 security/keys/encrypted-keys/encrypted.c | 31 +++++++++-----------
 security/keys/gc.c                       |  4 +--
 security/keys/keyctl.c                   |  4 ++-
 security/keys/trusted.c                  | 50 ++++++++++++++------------------
 security/keys/user_defined.c             | 16 +++++++---
 6 files changed, 51 insertions(+), 55 deletions(-)

-- 
2.12.2

[PATCH 1/5] KEYS: sanitize add_key() and keyctl() key payloads

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Before returning from add_key() or one of the keyctl() commands that
takes in a key payload, zero the temporary buffer that was allocated to
hold the key payload copied from userspace.  This may contain sensitive
key material that should not be kept around in the slab caches.

This must not be applied before the patch "KEYS: fix dereferencing NULL
payload with nonzero length".

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/keyctl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 10fcea154c0f..d2852621e358 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -137,6 +137,7 @@ SYSCALL_DEFINE5(add_key, const char __user *, _type,
 
 	key_ref_put(keyring_ref);
  error3:
+	memzero_explicit(payload, plen);
 	kvfree(payload);
  error2:
 	kfree(description);
@@ -347,7 +348,7 @@ long keyctl_update_key(key_serial_t id,
 
 	key_ref_put(key_ref);
 error2:
-	kfree(payload);
+	kzfree(payload);
 error:
 	return ret;
 }
@@ -1098,6 +1099,7 @@ long keyctl_instantiate_key_common(key_serial_t id,
 		keyctl_change_reqkey_auth(NULL);
 
 error2:
+	memzero_explicit(payload, plen);
 	kvfree(payload);
 error:
 	return ret;
-- 
2.12.2

[PATCH 2/5] KEYS: user_defined: sanitize key payloads

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Zero the payloads of user and logon keys before freeing them.  This
prevents sensitive key material from being kept around in the slab
caches after a key is released.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/user_defined.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 26605134f17a..3d8c68eba516 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -86,10 +86,18 @@ EXPORT_SYMBOL_GPL(user_preparse);
  */
 void user_free_preparse(struct key_preparsed_payload *prep)
 {
-	kfree(prep->payload.data[0]);
+	kzfree(prep->payload.data[0]);
 }
 EXPORT_SYMBOL_GPL(user_free_preparse);
 
+static void user_free_payload_rcu(struct rcu_head *head)
+{
+	struct user_key_payload *payload;
+
+	payload = container_of(head, struct user_key_payload, rcu);
+	kzfree(payload);
+}
+
 /*
  * update a user defined key
  * - the key's semaphore is write-locked
@@ -112,7 +120,7 @@ int user_update(struct key *key, struct key_preparsed_payload *prep)
 	prep->payload.data[0] = NULL;
 
 	if (zap)
-		kfree_rcu(zap, rcu);
+		call_rcu(&zap->rcu, user_free_payload_rcu);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(user_update);
@@ -130,7 +138,7 @@ void user_revoke(struct key *key)
 
 	if (upayload) {
 		rcu_assign_keypointer(key, NULL);
-		kfree_rcu(upayload, rcu);
+		call_rcu(&upayload->rcu, user_free_payload_rcu);
 	}
 }
 
@@ -143,7 +151,7 @@ void user_destroy(struct key *key)
 {
 	struct user_key_payload *upayload = key->payload.data[0];
 
-	kfree(upayload);
+	kzfree(upayload);
 }
 
 EXPORT_SYMBOL_GPL(user_destroy);
-- 
2.12.2

[PATCH 3/5] KEYS: encrypted: sanitize all key material

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

For keys of type "encrypted", consistently zero sensitive key material
before freeing it.  This was already being done for the decrypted
payloads of encrypted keys, but not for the master key and the keys
derived from the master key.

Out of an abundance of caution and because it is trivial to do so, also
zero buffers containing the key payload in encrypted form, although
depending on how the encrypted-keys feature is used such information
does not necessarily need to be kept secret.

Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: David Safford <safford@us.ibm.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/encrypted-keys/encrypted.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
index 0010955d7876..1ca895e7e56a 100644
--- a/security/keys/encrypted-keys/encrypted.c
+++ b/security/keys/encrypted-keys/encrypted.c
@@ -397,7 +397,7 @@ static int get_derived_key(u8 *derived_key, enum derived_key_type key_type,
 	memcpy(derived_buf + strlen(derived_buf) + 1, master_key,
 	       master_keylen);
 	ret = calc_hash(derived_key, derived_buf, derived_buf_len);
-	kfree(derived_buf);
+	kzfree(derived_buf);
 	return ret;
 }
 
@@ -533,6 +533,7 @@ static int datablob_hmac_append(struct encrypted_key_payload *epayload,
 	if (!ret)
 		dump_hmac(NULL, digest, HASH_SIZE);
 out:
+	memzero_explicit(derived_key, sizeof(derived_key));
 	return ret;
 }
 
@@ -571,6 +572,7 @@ static int datablob_hmac_verify(struct encrypted_key_payload *epayload,
 		dump_hmac("calc", digest, HASH_SIZE);
 	}
 out:
+	memzero_explicit(derived_key, sizeof(derived_key));
 	return ret;
 }
 
@@ -722,6 +724,7 @@ static int encrypted_key_decrypt(struct encrypted_key_payload *epayload,
 out:
 	up_read(&mkey->sem);
 	key_put(mkey);
+	memzero_explicit(derived_key, sizeof(derived_key));
 	return ret;
 }
 
@@ -828,13 +831,13 @@ static int encrypted_instantiate(struct key *key,
 	ret = encrypted_init(epayload, key->description, format, master_desc,
 			     decrypted_datalen, hex_encoded_iv);
 	if (ret < 0) {
-		kfree(epayload);
+		kzfree(epayload);
 		goto out;
 	}
 
 	rcu_assign_keypointer(key, epayload);
 out:
-	kfree(datablob);
+	kzfree(datablob);
 	return ret;
 }
 
@@ -843,8 +846,7 @@ static void encrypted_rcu_free(struct rcu_head *rcu)
 	struct encrypted_key_payload *epayload;
 
 	epayload = container_of(rcu, struct encrypted_key_payload, rcu);
-	memset(epayload->decrypted_data, 0, epayload->decrypted_datalen);
-	kfree(epayload);
+	kzfree(epayload);
 }
 
 /*
@@ -902,7 +904,7 @@ static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
 	rcu_assign_keypointer(key, new_epayload);
 	call_rcu(&epayload->rcu, encrypted_rcu_free);
 out:
-	kfree(buf);
+	kzfree(buf);
 	return ret;
 }
 
@@ -960,33 +962,26 @@ static long encrypted_read(const struct key *key, char __user *buffer,
 
 	up_read(&mkey->sem);
 	key_put(mkey);
+	memzero_explicit(derived_key, sizeof(derived_key));
 
 	if (copy_to_user(buffer, ascii_buf, asciiblob_len) != 0)
 		ret = -EFAULT;
-	kfree(ascii_buf);
+	kzfree(ascii_buf);
 
 	return asciiblob_len;
 out:
 	up_read(&mkey->sem);
 	key_put(mkey);
+	memzero_explicit(derived_key, sizeof(derived_key));
 	return ret;
 }
 
 /*
- * encrypted_destroy - before freeing the key, clear the decrypted data
- *
- * Before freeing the key, clear the memory containing the decrypted
- * key data.
+ * encrypted_destroy - clear and free the key's payload
  */
 static void encrypted_destroy(struct key *key)
 {
-	struct encrypted_key_payload *epayload = key->payload.data[0];
-
-	if (!epayload)
-		return;
-
-	memzero_explicit(epayload->decrypted_data, epayload->decrypted_datalen);
-	kfree(key->payload.data[0]);
+	kzfree(key->payload.data[0]);
 }
 
 struct key_type key_type_encrypted = {
-- 
2.12.2

[PATCH 4/5] KEYS: trusted: sanitize all key material

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

As the previous patch did for encrypted-keys, zero sensitive any
potentially sensitive data related to the "trusted" key type before it
is freed.  Notably, we were not zeroing the tpm_buf structures in which
the actual key is stored for TPM seal and unseal, nor were we zeroing
the trusted_key_payload in certain error paths.

Cc: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: David Safford <safford@us.ibm.com>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 security/keys/trusted.c | 50 ++++++++++++++++++++++---------------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index 2ae31c5a87de..435e86e13879 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -70,7 +70,7 @@ static int TSS_sha1(const unsigned char *data, unsigned int datalen,
 	}
 
 	ret = crypto_shash_digest(&sdesc->shash, data, datalen, digest);
-	kfree(sdesc);
+	kzfree(sdesc);
 	return ret;
 }
 
@@ -114,7 +114,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
 	if (!ret)
 		ret = crypto_shash_final(&sdesc->shash, digest);
 out:
-	kfree(sdesc);
+	kzfree(sdesc);
 	return ret;
 }
 
@@ -165,7 +165,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
 				  paramdigest, TPM_NONCE_SIZE, h1,
 				  TPM_NONCE_SIZE, h2, 1, &c, 0, 0);
 out:
-	kfree(sdesc);
+	kzfree(sdesc);
 	return ret;
 }
 
@@ -246,7 +246,7 @@ static int TSS_checkhmac1(unsigned char *buffer,
 	if (memcmp(testhmac, authdata, SHA1_DIGEST_SIZE))
 		ret = -EINVAL;
 out:
-	kfree(sdesc);
+	kzfree(sdesc);
 	return ret;
 }
 
@@ -347,7 +347,7 @@ static int TSS_checkhmac2(unsigned char *buffer,
 	if (memcmp(testhmac2, authdata2, SHA1_DIGEST_SIZE))
 		ret = -EINVAL;
 out:
-	kfree(sdesc);
+	kzfree(sdesc);
 	return ret;
 }
 
@@ -564,7 +564,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
 		*bloblen = storedsize;
 	}
 out:
-	kfree(td);
+	kzfree(td);
 	return ret;
 }
 
@@ -678,7 +678,7 @@ static int key_seal(struct trusted_key_payload *p,
 	if (ret < 0)
 		pr_info("trusted_key: srkseal failed (%d)\n", ret);
 
-	kfree(tb);
+	kzfree(tb);
 	return ret;
 }
 
@@ -703,7 +703,7 @@ static int key_unseal(struct trusted_key_payload *p,
 		/* pull migratable flag out of sealed key */
 		p->migratable = p->key[--p->key_len];
 
-	kfree(tb);
+	kzfree(tb);
 	return ret;
 }
 
@@ -1037,12 +1037,12 @@ static int trusted_instantiate(struct key *key,
 	if (!ret && options->pcrlock)
 		ret = pcrlock(options->pcrlock);
 out:
-	kfree(datablob);
-	kfree(options);
+	kzfree(datablob);
+	kzfree(options);
 	if (!ret)
 		rcu_assign_keypointer(key, payload);
 	else
-		kfree(payload);
+		kzfree(payload);
 	return ret;
 }
 
@@ -1051,8 +1051,7 @@ static void trusted_rcu_free(struct rcu_head *rcu)
 	struct trusted_key_payload *p;
 
 	p = container_of(rcu, struct trusted_key_payload, rcu);
-	memset(p->key, 0, p->key_len);
-	kfree(p);
+	kzfree(p);
 }
 
 /*
@@ -1094,13 +1093,13 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
 	ret = datablob_parse(datablob, new_p, new_o);
 	if (ret != Opt_update) {
 		ret = -EINVAL;
-		kfree(new_p);
+		kzfree(new_p);
 		goto out;
 	}
 
 	if (!new_o->keyhandle) {
 		ret = -EINVAL;
-		kfree(new_p);
+		kzfree(new_p);
 		goto out;
 	}
 
@@ -1114,22 +1113,22 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
 	ret = key_seal(new_p, new_o);
 	if (ret < 0) {
 		pr_info("trusted_key: key_seal failed (%d)\n", ret);
-		kfree(new_p);
+		kzfree(new_p);
 		goto out;
 	}
 	if (new_o->pcrlock) {
 		ret = pcrlock(new_o->pcrlock);
 		if (ret < 0) {
 			pr_info("trusted_key: pcrlock failed (%d)\n", ret);
-			kfree(new_p);
+			kzfree(new_p);
 			goto out;
 		}
 	}
 	rcu_assign_keypointer(key, new_p);
 	call_rcu(&p->rcu, trusted_rcu_free);
 out:
-	kfree(datablob);
-	kfree(new_o);
+	kzfree(datablob);
+	kzfree(new_o);
 	return ret;
 }
 
@@ -1158,24 +1157,19 @@ static long trusted_read(const struct key *key, char __user *buffer,
 	for (i = 0; i < p->blob_len; i++)
 		bufp = hex_byte_pack(bufp, p->blob[i]);
 	if ((copy_to_user(buffer, ascii_buf, 2 * p->blob_len)) != 0) {
-		kfree(ascii_buf);
+		kzfree(ascii_buf);
 		return -EFAULT;
 	}
-	kfree(ascii_buf);
+	kzfree(ascii_buf);
 	return 2 * p->blob_len;
 }
 
 /*
- * trusted_destroy - before freeing the key, clear the decrypted data
+ * trusted_destroy - clear and free the key's payload
  */
 static void trusted_destroy(struct key *key)
 {
-	struct trusted_key_payload *p = key->payload.data[0];
-
-	if (!p)
-		return;
-	memset(p->key, 0, p->key_len);
-	kfree(key->payload.data[0]);
+	kzfree(key->payload.data[0]);
 }
 
 struct key_type key_type_trusted = {
-- 
2.12.2

[PATCH 5/5] KEYS: sanitize key structs before freeing

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

While a 'struct key' itself normally does not contain sensitive
information, Documentation/security/keys.txt actually encourages this:

     "Having a payload is not required; and the payload can, in fact,
     just be a value stored in the struct key itself."

In case someone has taken this advice, or will take this advice in the
future, zero the key structure before freeing it.  We might as well, and
as a bonus this could make it a bit more difficult for an adversary to
determine which keys have recently been in use.

This is safe because the key_jar cache does not use a constructor.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 include/linux/key.h | 1 -
 security/keys/gc.c  | 4 +---
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index 0c9b93b0d1f7..78e25aabedaf 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -173,7 +173,6 @@ struct key {
 #ifdef KEY_DEBUGGING
 	unsigned		magic;
 #define KEY_DEBUG_MAGIC		0x18273645u
-#define KEY_DEBUG_MAGIC_X	0xf8e9dacbu
 #endif
 
 	unsigned long		flags;		/* status flags (change with bitops) */
diff --git a/security/keys/gc.c b/security/keys/gc.c
index 15b9ddf510e4..5233c073d982 100644
--- a/security/keys/gc.c
+++ b/security/keys/gc.c
@@ -158,9 +158,7 @@ static noinline void key_gc_unused_keys(struct list_head *keys)
 
 		kfree(key->description);
 
-#ifdef KEY_DEBUGGING
-		key->magic = KEY_DEBUG_MAGIC_X;
-#endif
+		memzero_explicit(key, sizeof(*key));
 		kmem_cache_free(key_jar, key);
 	}
 }
-- 
2.12.2

Re: [PATCH 2/5] KEYS: user_defined: sanitize key payloads

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

> -		kfree_rcu(zap, rcu);
> +		call_rcu(&zap->rcu, user_free_payload_rcu);

Add kzfree_rcu()?

David

Re: [PATCH 3/5] KEYS: encrypted: sanitize all key material

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

> -	memzero_explicit(epayload->decrypted_data, epayload->decrypted_datalen);
> -	kfree(key->payload.data[0]);
> +	kzfree(key->payload.data[0]);

Should kzfree() be using memzero_explicit() rather than memset()?

David

Re: [PATCH 3/5] KEYS: encrypted: sanitize all key material

[Eric Biggers]

On Fri, Apr 21, 2017 at 03:31:08PM +0100, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > -	memzero_explicit(epayload->decrypted_data, epayload->decrypted_datalen);
> > -	kfree(key->payload.data[0]);
> > +	kzfree(key->payload.data[0]);
> 
> Should kzfree() be using memzero_explicit() rather than memset()?
> 
> David

It's not actually needed because it's impossible for the compiler to optimize
away the memset().  memzero_explicit() is only needed on stack data.

The reason I still used memzero_explicit() for heap data in a couple of my
patches, even though it's unnecessary, is just that it makes it clearer that
it's being done for sanitization purposes, as opposed to some random memset.

That's not as much of an issue for kzfree(), since it's explicitly for
sanitization purposes already.

As a separate note, something that might make sense at some point would be to
skip the memset in kzfree() if slab poisoning is enabled.

Eric

Re: [PATCH 2/5] KEYS: user_defined: sanitize key payloads

[Eric Biggers]

On Fri, Apr 21, 2017 at 02:57:17PM +0100, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > -		kfree_rcu(zap, rcu);
> > +		call_rcu(&zap->rcu, user_free_payload_rcu);
> 
> Add kzfree_rcu()?
> 
> David

We could, but it's not trivial because the way kfree_rcu() works is to store the
offset of the rcu_head as the callback function, then have a special case in RCU
reclaim that recognizes "function pointers" with value < 4096 and call kfree()
rather than the function.  To support kzfree_rcu() we'd need to reserve another
4096 bytes of the address space (maybe at the end?), then check for the special
kzfree value on every RCU reclaim.  Or equivalently it could be a flag.  It's
possible, but it may be best to just use a custom callback for now.  Then if it
can be shown later that there are a lot of users who would like a
"kzfree_rcu()", it can be added.

- Eric

Re: [PATCH 2/5] KEYS: user_defined: sanitize key payloads

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

> > Add kzfree_rcu()?
> > 
> > David
> 
> We could, but it's not trivial because the way kfree_rcu() works is to store
> the offset of the rcu_head as the callback function, then have a special
> case in RCU reclaim that recognizes "function pointers" with value < 4096
> and call kfree() rather than the function. ...

Okay, that sounds reasonable.

David

Re: [PATCH 3/5] KEYS: encrypted: sanitize all key material

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

> It's not actually needed because it's impossible for the compiler to optimize
> away the memset().  memzero_explicit() is only needed on stack data.

Okay, also reasonable.

David

Re: [PATCH 0/5] KEYS: sanitize key payloads

[David Howells]

Do you have a git branch I can pull from?

David

Re: [PATCH 0/5] KEYS: sanitize key payloads

[Eric Biggers]

On Thu, Apr 27, 2017 at 04:09:42PM +0100, David Howells wrote:
> Do you have a git branch I can pull from?
> 
> David

No I don't, sorry.

- Eric

Re: [PATCH 1/5] KEYS: sanitize add_key() and keyctl() key payloads

[Eric Biggers]

Hey David,

On Fri, Apr 21, 2017 at 01:30:33AM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> Before returning from add_key() or one of the keyctl() commands that
> takes in a key payload, zero the temporary buffer that was allocated to
> hold the key payload copied from userspace.  This may contain sensitive
> key material that should not be kept around in the slab caches.
> 
> This must not be applied before the patch "KEYS: fix dereferencing NULL
> payload with nonzero length".
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Can you make sure that my other patch "KEYS: fix dereferencing NULL payload with
nonzero length" gets applied along with this one?  Otherwise triggering the NULL
pointer dereference (which really needs to be fixed anyway) becomes even more
trivial.  The only reason I didn't check for NULL before doing the memsets is
that the bug was going to have to be fixed anyway, and the fix backported.

- Eric

Re: [PATCH 1/5] KEYS: sanitize add_key() and keyctl() key payloads

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

>  error2:
> +	memzero_explicit(payload, plen);

Isn't that wrong?  payload can be NULL.

David

Re: [PATCH 1/5] KEYS: sanitize add_key() and keyctl() key payloads

[Eric Biggers]

On Fri, Jun 02, 2017 at 04:34:44PM +0100, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> >  error2:
> > +	memzero_explicit(payload, plen);
> 
> Isn't that wrong?  payload can be NULL.
> 
> David

If you're talking about memset(NULL, ..., 0) being undefined behavior, it's
completely insane but sure, I guess we should add the NULL check to be safe.  It
would also mean there would be no requirement that "KEYS: fix dereferencing NULL
payload with nonzero length" be applied first so the second paragraph of the
commit message would be removed.  I'll send a v2 of just this patch.

Eric