Patchwork KEYS: Fix race with concurrent install_user_keyrings()

login
register
mail settings
Submitter David Howells
Date March 6, 2013, 10:24 p.m.
Message ID <20130306222412.1255.81929.stgit@warthog.procyon.org.uk>
Download mbox | patch
Permalink /patch/2228431/
State New, archived
Headers show

Comments

David Howells - March 6, 2013, 10:24 p.m.
This fixes CVE-2013-1792.

There is a race in install_user_keyrings() that can cause a NULL pointer
dereference when called concurrently for the same user if the uid and
uid-session keyrings are not yet created.  It might be possible for an
unprivileged user to trigger this by calling keyctl() from userspace in
parallel immediately after logging in.

Assume that we have two threads both executing lookup_user_key(), both looking
for KEY_SPEC_USER_SESSION_KEYRING.

	THREAD A			THREAD B
	===============================	===============================
					==>call install_user_keyrings();
	if (!cred->user->session_keyring)
	==>call install_user_keyrings()
					...
					user->uid_keyring = uid_keyring;
	if (user->uid_keyring)
		return 0;
	<==
	key = cred->user->session_keyring [== NULL]
					user->session_keyring = session_keyring;
	atomic_inc(&key->usage); [oops]

At the point thread A dereferences cred->user->session_keyring, thread B hasn't
updated user->session_keyring yet, but thread A assumes it is populated because
install_user_keyrings() returned ok.

The race window is really small but can be exploited if, for example, thread B
is interrupted or preempted after initializing uid_keyring, but before doing
setting session_keyring.

This couldn't be reproduced on a stock kernel.  However, after placing
systemtap probe on 'user->session_keyring = session_keyring;' that introduced
some delay, the kernel could be crashed reliably.

Fix this by checking both pointers before deciding whether to return.
Alternatively, the test could be done away with entirely as it is checked
inside the mutex - but since the mutex is global, that may not be the best way.

Reported-by: Mateusz Guzik <mguzik@redhat.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 security/keys/process_keys.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
James Morris - March 7, 2013, 4:59 a.m.
On Wed, 6 Mar 2013, David Howells wrote:

> Reported-by: Mateusz Guzik <mguzik@redhat.com>
> Signed-off-by: David Howells <dhowells@redhat.com>

Acked-by: James Morris <james.l.morris@oracle.com>
gregkh@linuxfoundation.org - March 11, 2013, 9:02 p.m.
On Thu, Mar 07, 2013 at 03:59:09PM +1100, James Morris wrote:
> On Wed, 6 Mar 2013, David Howells wrote:
> 
> > Reported-by: Mateusz Guzik <mguzik@redhat.com>
> > Signed-off-by: David Howells <dhowells@redhat.com>
> 
> Acked-by: James Morris <james.l.morris@oracle.com>

What happened to this patch?  I don't see it in Linus's tree, James, did
you pick it up?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Andrew Morton - March 11, 2013, 9:10 p.m.
On Mon, 11 Mar 2013 14:02:11 -0700 Greg KH <gregkh@linuxfoundation.org> wrote:

> On Thu, Mar 07, 2013 at 03:59:09PM +1100, James Morris wrote:
> > On Wed, 6 Mar 2013, David Howells wrote:
> > 
> > > Reported-by: Mateusz Guzik <mguzik@redhat.com>
> > > Signed-off-by: David Howells <dhowells@redhat.com>
> > 
> > Acked-by: James Morris <james.l.morris@oracle.com>
> 
> What happened to this patch?  I don't see it in Linus's tree, James, did
> you pick it up?

James has acked it.  I have it queued for processing so it isn't lost. 
It has no cc:stable's in it, but David always forgets that ;)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
gregkh@linuxfoundation.org - March 11, 2013, 9:21 p.m.
On Mon, Mar 11, 2013 at 02:10:32PM -0700, Andrew Morton wrote:
> On Mon, 11 Mar 2013 14:02:11 -0700 Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > On Thu, Mar 07, 2013 at 03:59:09PM +1100, James Morris wrote:
> > > On Wed, 6 Mar 2013, David Howells wrote:
> > > 
> > > > Reported-by: Mateusz Guzik <mguzik@redhat.com>
> > > > Signed-off-by: David Howells <dhowells@redhat.com>
> > > 
> > > Acked-by: James Morris <james.l.morris@oracle.com>
> > 
> > What happened to this patch?  I don't see it in Linus's tree, James, did
> > you pick it up?
> 
> James has acked it.  I have it queued for processing so it isn't lost. 

Ok, I'll keep a lookout for it.

> It has no cc:stable's in it, but David always forgets that ;)

Yeah, I'm used to that :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Linus Torvalds - March 11, 2013, 10:15 p.m.
On Mon, Mar 11, 2013 at 2:10 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 11 Mar 2013 14:02:11 -0700 Greg KH <gregkh@linuxfoundation.org> wrote:
>
>> On Thu, Mar 07, 2013 at 03:59:09PM +1100, James Morris wrote:
>> > On Wed, 6 Mar 2013, David Howells wrote:
>> >
>> > > Reported-by: Mateusz Guzik <mguzik@redhat.com>
>> > > Signed-off-by: David Howells <dhowells@redhat.com>
>> >
>> > Acked-by: James Morris <james.l.morris@oracle.com>
>>
>> What happened to this patch?  I don't see it in Linus's tree, James, did
>> you pick it up?
>
> James has acked it.  I have it queued for processing so it isn't lost.
> It has no cc:stable's in it, but David always forgets that ;)

I usually expect that if the subsystem maintainer is involved, he's
the one to pick it up.

If James' ack was "Linus, please take this directly, I'm busy and have
nothing else pending", then I really would prefer to hear that exact
thing. Otherwise I'm just going to ignore the patch "safe" in the
knowledge that clearly the subsystem maintainer is aware of it.

             Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
James Morris - March 12, 2013, 4:09 a.m.
On Mon, 11 Mar 2013, Greg KH wrote:

> On Thu, Mar 07, 2013 at 03:59:09PM +1100, James Morris wrote:
> > On Wed, 6 Mar 2013, David Howells wrote:
> > 
> > > Reported-by: Mateusz Guzik <mguzik@redhat.com>
> > > Signed-off-by: David Howells <dhowells@redhat.com>
> > 
> > Acked-by: James Morris <james.l.morris@oracle.com>
> 
> What happened to this patch?  I don't see it in Linus's tree, James, did
> you pick it up?

I'll push it to Linus.
David Howells - March 12, 2013, 1:42 p.m.
Andrew Morton <akpm@linux-foundation.org> wrote:

> James has acked it.  I have it queued for processing so it isn't lost. 
> It has no cc:stable's in it, but David always forgets that ;)

Hmmm...  I did try and resend it there on the 7th.  My mail client records
that it did so, but I don't see it in the list archive and I didn't get any
bounce notifications:-/

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch

diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c
index 58dfe08..c5ec083 100644
--- a/security/keys/process_keys.c
+++ b/security/keys/process_keys.c
@@ -57,7 +57,7 @@  int install_user_keyrings(void)
 
 	kenter("%p{%u}", user, uid);
 
-	if (user->uid_keyring) {
+	if (user->uid_keyring && user->session_keyring) {
 		kleave(" = 0 [exist]");
 		return 0;
 	}