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 <[email protected]>
Signed-off-by: David Howells <[email protected]>
---
security/keys/process_keys.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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;
}
On Wed, 6 Mar 2013, David Howells wrote:
> Reported-by: Mateusz Guzik <[email protected]>
> Signed-off-by: David Howells <[email protected]>
Acked-by: James Morris <[email protected]>
--
James Morris
<[email protected]>
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 <[email protected]>
> > Signed-off-by: David Howells <[email protected]>
>
> Acked-by: James Morris <[email protected]>
What happened to this patch? I don't see it in Linus's tree, James, did
you pick it up?
thanks,
greg k-h
On Mon, 11 Mar 2013 14:02:11 -0700 Greg KH <[email protected]> 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 <[email protected]>
> > > Signed-off-by: David Howells <[email protected]>
> >
> > Acked-by: James Morris <[email protected]>
>
> 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 ;)
On Mon, Mar 11, 2013 at 02:10:32PM -0700, Andrew Morton wrote:
> On Mon, 11 Mar 2013 14:02:11 -0700 Greg KH <[email protected]> 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 <[email protected]>
> > > > Signed-off-by: David Howells <[email protected]>
> > >
> > > Acked-by: James Morris <[email protected]>
> >
> > 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 :)
On Mon, Mar 11, 2013 at 2:10 PM, Andrew Morton
<[email protected]> wrote:
> On Mon, 11 Mar 2013 14:02:11 -0700 Greg KH <[email protected]> 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 <[email protected]>
>> > > Signed-off-by: David Howells <[email protected]>
>> >
>> > Acked-by: James Morris <[email protected]>
>>
>> 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
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 <[email protected]>
> > > Signed-off-by: David Howells <[email protected]>
> >
> > Acked-by: James Morris <[email protected]>
>
> 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.
--
James Morris
<[email protected]>
Andrew Morton <[email protected]> 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