Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1628046 > unrolled thread

[PATCH 0/5] KEYS: sanitize key payloads

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] KEYS: sanitize key payloads Eric Biggers <ebiggers3@gmail.com> - 2017-04-21 10:40 +0200
    [PATCH 1/5] KEYS: sanitize add_key() and keyctl() key payloads Eric Biggers <ebiggers3@gmail.com> - 2017-04-21 10:40 +0200
    [PATCH 4/5] KEYS: trusted: sanitize all key material Eric Biggers <ebiggers3@gmail.com> - 2017-04-21 10:40 +0200
    [PATCH 2/5] KEYS: user_defined: sanitize key payloads Eric Biggers <ebiggers3@gmail.com> - 2017-04-21 10:40 +0200
      Re: [PATCH 2/5] KEYS: user_defined: sanitize key payloads David Howells <dhowells@redhat.com> - 2017-04-21 16:00 +0200
        Re: [PATCH 2/5] KEYS: user_defined: sanitize key payloads Eric Biggers <ebiggers3@gmail.com> - 2017-04-21 20:40 +0200
          Re: [PATCH 2/5] KEYS: user_defined: sanitize key payloads David Howells <dhowells@redhat.com> - 2017-04-24 16:20 +0200

#1628046 — [PATCH 0/5] KEYS: sanitize key payloads

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

Eric Biggers <ebiggers3@gmail.com> wrote:

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

Add kzfree_rcu()?

David

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web