Last modified: 2011-10-23 12:39:50 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.
Created attachment 8435 [details] Proposed patch
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 [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.
(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 [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.
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 [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; >+ > } > > /**
Created attachment 8481 [details] Tims patch ported for 1.16
Created attachment 8482 [details] Tims patch ported for 1.17
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.
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. ***
