Last modified: 2011-10-23 12:39:50 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 28639 - Trivial account takeover using forged cookies possible when $wgBlockDisablesLogin = true
Trivial account takeover using forged cookies possible when $wgBlockDisablesL...
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
User login and signup (Other open bugs)
unspecified
All All
: Normal critical (vote)
: ---
Assigned To: Nobody - You can work on this!
: patch, patch-reviewed
: 31845 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-21 11:41 UTC by Roan Kattouw
Modified: 2011-10-23 12:39 UTC (History)
8 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Proposed patch (1.56 KB, patch)
2011-04-21 11:45 UTC, Roan Kattouw
Details
Improved patch (1.76 KB, patch)
2011-04-21 12:42 UTC, Roan Kattouw
Details
Patch that avoids putting strange things in $wgUser altogether (2.03 KB, patch)
2011-05-02 13:20 UTC, Tim Starling
Details
Tims patch ported for 1.16 (deleted)
2011-05-02 14:48 UTC, Sam Reed (reedy)
Details
Tims patch ported for 1.17 (2.01 KB, patch)
2011-05-02 14:55 UTC, Sam Reed (reedy)
Details

Description Roan Kattouw 2011-04-21 11:41:20 UTC
Liangent reported the following vulnerability:
* Set $wgBlockDisablesLogin = true; in LocalSettings.php
* Make sure you're logged out in MediaWiki
* Set the wikiUserID and wikiUserName cookies to match those of a sysop
* Visit a sysop-only special page such as Special:BlockIP
* The special page will let you do whatever you want, because you have all the rights of the user whose cookies you forged, despite being an anon according to the user tools section

The following things happen in User::loadFromSession():
* $this->mId is set to the user ID from the forged cookie (line 900)
* isBlocked() is called (line 907), which calls getBlockedStatus() which calls isAllowed( 'ipblock-exempt' ) (line 1116), which calls getRights(), which fills the $this->mRights cache with the rights of the targeted user
* When the auth token mismatches, loadDefault() is called, but it doesn't clear the $this->mRights cache

The soon-to-be-attached patch fixes this by moving the auth token check up to before the blocked status check. An alternative fix would be to call clearInstanceCache( 'defaults' ) instead of loadDefault(), but I think this makes more sense.
Comment 1 Roan Kattouw 2011-04-21 11:45:14 UTC
Created attachment 8435 [details]
Proposed patch
Comment 2 Liangent 2011-04-21 11:59:50 UTC
It seems wikiUserName is unnecessary because the check before ->isBlocked() does not include user name check.

This does not happen only when someone has the ability to forge cookies. When a sysop is logging in without "Remember my password" checked, only session and/or token cookies are set as session cookies (cleared when the browser is closed). So after he/she closed the browser and thought he/she has logged out and left, the second person using this browser can have the sysop's user id cookie, causing this bug to appear.
Comment 3 Liangent 2011-04-21 12:11:48 UTC
The patch fixed the initial bug but didn't fix another related one which exists before.

If I'm a sysop on a $wgBlockDisablesLogin = true; site but I'm blocked by someone else, and I'm logging in with the correct password, it will still run into ->isBlocked() then finally ->getRights(), having ->mRights filled. Just as above, ->loadDefault() is called later after knowing I'm blocked, but I'll get all sysop rights.
Comment 4 Roan Kattouw 2011-04-21 12:42:00 UTC
Created attachment 8436 [details]
Improved patch

(In reply to comment #3)
> The patch fixed the initial bug but didn't fix another related one which exists
> before.
> 
> If I'm a sysop on a $wgBlockDisablesLogin = true; site but I'm blocked by
> someone else, and I'm logging in with the correct password, it will still run
> into ->isBlocked() then finally ->getRights(), having ->mRights filled. Just as
> above, ->loadDefault() is called later after knowing I'm blocked, but I'll get
> all sysop rights.

Good catch. Attached updated patch that calls clearInstanceCache( 'defaults' ) instead of loadDefaults() in that code path.
Comment 5 Roan Kattouw 2011-04-21 12:45:12 UTC
(In reply to comment #2)
> This does not happen only when someone has the ability to forge cookies. When a
> sysop is logging in without "Remember my password" checked, only session and/or
> token cookies are set as session cookies (cleared when the browser is closed).
> So after he/she closed the browser and thought he/she has logged out and left,
> the second person using this browser can have the sysop's user id cookie,
> causing this bug to appear.
Ouch. I had not realized this bug could be exploited by accident like that.
Comment 6 Liangent 2011-04-21 13:09:05 UTC
(In reply to comment #5)
> Ouch. I had not realized this bug could be exploited by accident like that.

That was how I found this bug...
Comment 7 Tim Starling 2011-05-02 13:20:40 UTC
Created attachment 8480 [details]
Patch that avoids putting strange things in $wgUser altogether

Roan's patch seems unconvincing to me. $wgUser is meant to be a representation of what the current user's rights are, the idea of having it set up to be an unauthenticated admin for any period of time makes me uneasy.

I made a patch which creates a separate user object for the user proposed by the cookie or session. Then it copies the relevant data into $wgUser once all checks are complete. It's attached. It should be harder for future developers to screw up. I don't really trust developers to read comments, even when they are written in capitals.
Comment 8 Sam Reed (reedy) 2011-05-02 14:46:41 UTC
Confirmed patch fixed on trunk

Migrated patches to 1.16 and 1.17, all seems well. Will attach those patches next
Comment 9 Sam Reed (reedy) 2011-05-02 14:47:08 UTC
Comment on attachment 8436 [details]
Improved patch

>Index: includes/User.php
>===================================================================
>--- includes/User.php	(revision 86616)
>+++ includes/User.php	(working copy)
>@@ -903,13 +903,9 @@
> 			return false;
> 		}
> 
>-		global $wgBlockDisablesLogin;
>-		if( $wgBlockDisablesLogin && $this->isBlocked() ) {
>-			# User blocked and we've disabled blocked user logins
>-			$this->loadDefaults();
>-			return false;
>-		}
>-
>+		// Check that the token matches. This has to be done
>+		// IMMEDIATELY after loadFromId(), otherwise unauthenticated
>+		// credentials might be used. See bug 28639
> 		if ( $wgRequest->getSessionData( 'wsToken' ) !== null ) {
> 			$passwordCorrect = $this->mToken == $wgRequest->getSessionData( 'wsToken' );
> 			$from = 'session';
>@@ -921,17 +917,31 @@
> 			$this->loadDefaults();
> 			return false;
> 		}
>-
>-		if ( ( $sName == $this->mName ) && $passwordCorrect ) {
>-			$wgRequest->setSessionData( 'wsToken', $this->mToken );
>-			wfDebug( "User: logged in from $from\n" );
>-			return true;
>-		} else {
>+		if ( !$passwordCorrect || $sName != $this->mName ) {
> 			# Invalid credentials
> 			wfDebug( "User: can't log in from $from, invalid credentials\n" );
> 			$this->loadDefaults();
> 			return false;
> 		}
>+		
>+		// Credentials are valid
>+		
>+		global $wgBlockDisablesLogin;
>+		if( $wgBlockDisablesLogin && $this->isBlocked() ) {
>+			# User blocked and we've disabled blocked user logins
>+			# We called isBlocked(), which caches the user's rights
>+			# so loadDefaults() is not good enough. We have to call
>+			# clearInstanceCache() to clear the rights cache.
>+			# See bug 28639
>+			$this->clearInstanceCache( 'defaults' );
>+			return false;
>+		}
>+		
>+		
>+		$wgRequest->setSessionData( 'wsToken', $this->mToken );
>+		wfDebug( "User: logged in from $from\n" );
>+		return true;
>+		
> 	}
> 
> 	/**
Comment 10 Sam Reed (reedy) 2011-05-02 14:48:23 UTC
Created attachment 8481 [details]
Tims patch ported for 1.16
Comment 11 Sam Reed (reedy) 2011-05-02 14:55:15 UTC
Created attachment 8482 [details]
Tims patch ported for 1.17
Comment 12 Tim Starling 2011-05-05 05:42:00 UTC
The content of attachment 8481 [details] has been deleted by
    Tim Starling <tstarling@wikimedia.org>
without providing any reason.

The token used to delete this attachment was generated at 2011-05-05 05:41:47 UTC.
Comment 13 Tim Starling 2011-05-05 05:43:03 UTC
I deleted the 1.16 patch because it was broken, giving a fatal error on login. The new patch is in r87484.
Comment 14 p858snake 2011-05-07 09:15:28 UTC
marking fixed 1.16.5 was pushed the other day.
Comment 15 Remco de Boer 2011-10-23 12:39:50 UTC
*** Bug 31845 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links