Created attachment 8435
Proposed patch

attachment blockdisableslogin-vulnerability.patch ignored as obsolete

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.

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.

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

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

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

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:

Confirmed patch fixed on trunk

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

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

	}

	/**

Created attachment 8481
Tims patch ported for 1.16

attachment wgBlockDisablesLogin-rel1_16.patch ignored as obsolete

Created attachment 8482
Tims patch ported for 1.17

Attached:

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.

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

marking fixed 1.16.5 was pushed the other day.

• Bug 31845 has been marked as a duplicate of this bug. ***