| From 395268d53ad11dc91d69af008277c9a326e3bec9 Mon Sep 17 00:00:00 2001 |
| From: David Howells <dhowells@redhat.com> |
| Date: Tue, 12 Mar 2013 16:44:31 +1100 |
| Subject: [PATCH] keys: fix race with concurrent install_user_keyrings() |
| |
| commit 0da9dfdd2cd9889201bc6f6f43580c99165cd087 upstream. |
| |
| 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. |
| |
| Signed-off-by: David Howells <dhowells@redhat.com> |
| Reported-by: Mateusz Guzik <mguzik@redhat.com> |
| Signed-off-by: Andrew Morton <akpm@linux-foundation.org> |
| Signed-off-by: James Morris <james.l.morris@oracle.com> |
| Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.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 20a38fed61b1..71c10cec3c18 100644 |
| --- a/security/keys/process_keys.c |
| +++ b/security/keys/process_keys.c |
| @@ -55,7 +55,7 @@ int install_user_keyrings(void) |
| |
| kenter("%p{%u}", user, user->uid); |
| |
| - if (user->uid_keyring) { |
| + if (user->uid_keyring && user->session_keyring) { |
| kleave(" = 0 [exist]"); |
| return 0; |
| } |
| -- |
| 1.8.5.2 |
| |