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

Commit Message

Eric Biggers April 21, 2017, 8:30 a.m. UTC

From: Eric Biggers <[e-mail]>

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 <[e-mail]>
---
 security/keys/keyctl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Eric Biggers April 28, 2017, 5:57 p.m. UTC | #1

Hey David,

On Fri, Apr 21, 2017 at 01:30:33AM -0700, Eric Biggers wrote:
> From: Eric Biggers <[e-mail]>
> 
> 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 <[e-mail]>

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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Howells June 2, 2017, 3:34 p.m. UTC | #2

Eric Biggers <[e-mail]> wrote:

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

Isn't that wrong?  payload can be NULL.

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

Eric Biggers June 2, 2017, 5:24 p.m. UTC | #3

On Fri, Jun 02, 2017 at 04:34:44PM +0100, David Howells wrote:
> Eric Biggers <[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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;