diff
Public
Actions

Authored By

	• bzimport
	Nov 22 2014, 2:08 AM

Size

3 KB

Referenced Files

None

Subscribers

None

diff
View Options

	From 0f6ef68e2b9ad2ae8c9e445b2259ac65b8255b36 Mon Sep 17 00:00:00 2001
	From: Brad Jorsch <bjorsch@wikimedia.org>
	Date: Wed, 31 Jul 2013 15:12:15 -0400
	Subject: [PATCH] Fix security hole in CentralAuth

	There are two bugs here that make for one gigantic security hole:
	* CentralAuthHooks::onUserLoadFromSession leaves a valid CentralAuthUser
	object cached on the User object even when the centralauth_Token
	doesn't match.
	* And Special:CentralAutoLogin assumes that CentralAuthUser::getInstance
	doesn't return a valid CentralAuthUser when the User isn't logged in
	(which would be true except for the other bug).

	This patch fixes both bugs, even though fixing either one is enough to
	close the security hole.

	Bug: 52338
	Change-Id: I4dd36300f1835b5ca30854a31cd3788828f6cb68
	---
	CentralAuthHooks.php \| 7 ++++---
	specials/SpecialCentralAutoLogin.php \| 11 ++++++++++-
	2 files changed, 14 insertions(+), 4 deletions(-)

	diff --git a/CentralAuthHooks.php b/CentralAuthHooks.php
	index 02079dc..68050a5 100644
	--- a/CentralAuthHooks.php
	+++ b/CentralAuthHooks.php
	@@ -496,7 +496,6 @@ class CentralAuthHooks {
	return true;
	}
	$userName = $centralUser->getName();
	- $user->setName( $userName );
	$token = $centralUser->getAuthToken();
	} else {
	$prefix = $wgCentralAuthCookiePrefix;
	@@ -534,10 +533,10 @@ class CentralAuthHooks {
	wfDebug( __METHOD__ . ": invalid username\n" );
	return true;
	}
	- $user->setName( $userName );

	// Try the central user
	- $centralUser = CentralAuthUser::getInstance( $user );
	+ // Don't use CentralAuthUser::getInstance, we don't want to cache it on failure.
	+ $centralUser = new CentralAuthUser( $userName );
	if ( !$centralUser->exists() ) {
	wfDebug( __METHOD__ . ": global account doesn't exist\n" );
	return true;
	@@ -566,8 +565,10 @@ class CentralAuthHooks {

	if ( !$localId ) {
	// User does not exist locally, attempt to create it
	+ $user->setName( $userName );
	if ( !self::attemptAddUser( $user ) ) {
	// Can't create user, give up now
	+ $user->setName( false );
	return true;
	}
	} else {
	diff --git a/specials/SpecialCentralAutoLogin.php b/specials/SpecialCentralAutoLogin.php
	index ed500f3..897abd4 100644
	--- a/specials/SpecialCentralAutoLogin.php
	+++ b/specials/SpecialCentralAutoLogin.php
	@@ -93,7 +93,11 @@ class SpecialCentralAutoLogin extends UnlistedSpecialPage {
	}

	CentralAuthUser::setP3P();
	- $centralUser = CentralAuthUser::getInstance( $this->getUser() );
	+ if ( $this->getUser()->isLoggedIn() ) {
	+ $centralUser = CentralAuthUser::getInstance( $this->getUser() );
	+ } else {
	+ $centralUser = null;
	+ }
	$this->do302Redirect( $wikiid, 'createSession', array(
	'gu_id' => $centralUser ? $centralUser->getId() : 0,
	) + $params );
	@@ -140,6 +144,11 @@ class SpecialCentralAutoLogin extends UnlistedSpecialPage {
	return;
	}

	+ if ( !$this->getUser()->isLoggedIn() ) {
	+ $this->doFinalOutput( false, 'Not logged in' );
	+ return;
	+ }
	+
	CentralAuthUser::setP3P();
	// Validate params
	$token = $request->getVal( 'token', '' );
	--
	1.8.3.2

File Metadata

Mime Type: text/x-diff
Storage Engine: blob
Storage Format: Raw Data
Storage Handle: 11293
Default Alt Text: diff (3 KB)

diffPublicActions

diffView Options

File Metadata

Event Timeline

diff
Public
Actions

diff
View Options