Message ID | 1363105926-19617-1-git-send-email-luis.henriques@canonical.com |
---|---|
State | New |
Headers | show |
Please ignore for now this patch. I'll resubmit it in a minute with a SHA1. Cheers, -- Luis On Tue, Mar 12, 2013 at 04:32:06PM +0000, Luis Henriques wrote: > From: David Howells <dhowells@redhat.com> > > CVE-2013-1792 > > BugLink: http://bugs.launchpad.net/bugs/1152788 > > 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> > Signed-off-by: Luis Henriques <luis.henriques@canonical.com> > --- > 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; > } > -- > 1.8.1.2 > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
cherry-picked 0da9dfdd2cd9889201bc6f6f43580c99165cd087
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; }