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  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

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  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

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

[David Howells]

Eric Biggers  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

[David Howells]

Eric Biggers  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]

Hey David,

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

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

[Eric Biggers]

Hey David,

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

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

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

[Eric Biggers]

From: Eric Biggers 

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 
---
 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 1/5] KEYS: sanitize add_key() and keyctl() key payloads

[Eric Biggers]

From: Eric Biggers 

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 
---
 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