Trivial account takeover using forged cookies possible when $wgBlockDisablesLogin = true
Closed, ResolvedPublic

Description

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.


Version: unspecified
Severity: critical

bzimport added a subscriber: Unknown Object (MLST).
bzimport set Reference to bz28639.
Catrope created this task.Via LegacyApr 21 2011, 11:41 AM
Catrope added a comment.Via ConduitApr 21 2011, 11:45 AM

Created attachment 8435
Proposed patch

attachment blockdisableslogin-vulnerability.patch ignored as obsolete

liangent added a comment.Via ConduitApr 21 2011, 11:59 AM

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.

liangent added a comment.Via ConduitApr 21 2011, 12:11 PM

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.

Catrope added a comment.Via ConduitApr 21 2011, 12:42 PM

Created attachment 8436
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.

attachment blockdisableslogin-vulnerability-2.patch ignored as obsolete

Catrope added a comment.Via ConduitApr 21 2011, 12:45 PM

(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.

liangent added a comment.Via ConduitApr 21 2011, 1:09 PM

(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...

tstarling added a comment.Via ConduitMay 2 2011, 1:20 PM

Created attachment 8480
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.

Attached: wgBlockDisablesLogin-tim1.patch

Reedy added a comment.Via ConduitMay 2 2011, 2:46 PM

Confirmed patch fixed on trunk

Migrated patches to 1.16 and 1.17, all seems well. Will attach those patches next

Reedy added a comment.Via ConduitMay 2 2011, 2:47 PM

Comment on attachment 8436
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 ) {

  1. 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;
+

	}

	/**
Reedy added a comment.Via ConduitMay 2 2011, 2:48 PM

Created attachment 8481
Tims patch ported for 1.16

attachment wgBlockDisablesLogin-rel1_16.patch ignored as obsolete

Reedy added a comment.Via ConduitMay 2 2011, 2:55 PM

Created attachment 8482
Tims patch ported for 1.17

Attached: wgBlockDisablesLogin-rel1_17.patch

tstarling added a comment.Via ConduitMay 5 2011, 5:42 AM

The content of attachment 8481 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.

tstarling added a comment.Via ConduitMay 5 2011, 5:43 AM

I deleted the 1.16 patch because it was broken, giving a fatal error on login. The new patch is in r87484.

Peachey88 added a comment.Via ConduitMay 7 2011, 9:15 AM

marking fixed 1.16.5 was pushed the other day.

Rcdeboer added a comment.Via ConduitOct 23 2011, 12:39 PM
  • Bug 31845 has been marked as a duplicate of this bug. ***
csteipp added a project: Security.Via WebMar 26 2015, 8:39 PM

Add Comment