[Raring CVE-2013-1792] KEYS: Fix race with concurrent install_user_keyrings()

[Luis Henriques]

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 at 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 at redhat.com>
> Signed-off-by: David Howells <dhowells at redhat.com>
> Signed-off-by: Luis Henriques <luis.henriques at 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 at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team