r65762 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r65761‎ | r65762 | r65763 >
Date:20:27, 1 May 2010
Author:platonides
Status:ok (Comments)
Tags:
Comment:
Backport r65760.
We may even want to backport it to 1.15.
Modified paths:
  • /branches/REL1_16/phase3/RELEASE-NOTES (modified) (history)
  • /branches/REL1_16/phase3/includes/specials/SpecialUserlogin.php (modified) (history)
  • /branches/REL1_16/phase3/includes/templates/Userlogin.php (modified) (history)

Diff [purge]

Index: branches/REL1_16/phase3/includes/specials/SpecialUserlogin.php
——@@ -72,7 +72,7 @@
7373 $this->mRemember = $request->getCheck( 'wpRemember' );
7474 $this->mLanguage = $request->getText( 'uselang' );
7575 $this->mSkipCookieCheck = $request->getCheck( 'wpSkipCookieCheck' );
76 - $this->mToken = $request->getVal( 'wpLoginToken' );
 76+ $this->mToken = ($this->mType == 'signup' ) ? $request->getVal( 'wpCreateaccountToken' ) : $request->getVal( 'wpLoginToken' );
7777
7878 if ( $wgRedirectOnLogin ) {
7979 $this->mReturnTo = $wgRedirectOnLogin;
——@@ -251,6 +251,25 @@
252252 return false;
253253 }
254254
 255+ # Request forgery checks.
 256+ if ( !self::getCreateaccountToken() ) {
 257+ self::setCreateaccountToken();
 258+ $this->mainLoginForm( wfMsg( 'sessionfailure' ) );
 259+ return false;
 260+ }
 261+
 262+ # The user didn't pass a createaccount token
 263+ if ( !$this->mToken ) {
 264+ $this->mainLoginForm( wfMsg( 'sessionfailure' ) );
 265+ return false;
 266+ }
 267+
 268+ # Validate the createaccount token
 269+ if ( $this->mToken !== self::getCreateaccountToken() ) {
 270+ $this->mainLoginForm( wfMsg( 'sessionfailure' ) );
 271+ return false;
 272+ }
 273+
255274 # Check permissions
256275 if ( !$wgUser->isAllowed( 'createaccount' ) ) {
257276 $this->userNotPrivilegedMessage();
——@@ -263,7 +282,7 @@
264283 $ip = wfGetIP();
265284 if ( $wgUser->isDnsBlacklisted( $ip, true /* check $wgProxyWhitelist */ ) ) {
266285 $this->mainLoginForm( wfMsg( 'sorbs_create_account_reason' ) . ' (' . htmlspecialchars( $ip ) . ')' );
267 - return;
 286+ return false;
268287 }
269288
270289 # Now create a dummy user ($u) and check if it is valid
——@@ -340,6 +359,7 @@
341360 return false;
342361 }
343362
 363+ self::clearCreateaccountToken();
344364 return $this->initUser( $u, false );
345365 }
346366
——@@ -683,20 +703,33 @@
684704 return;
685705 }
686706
687 - # Check against blocked IPs
688 - # fixme -- should we not?
 707+ # Check against blocked IPs so blocked users can't flood admins
 708+ # with password resets
689709 if( $wgUser->isBlocked() ) {
690710 $this->mainLoginForm( wfMsg( 'blocked-mailpassword' ) );
691711 return;
692712 }
693713
694 - // Check for hooks
 714+ # Check for hooks
695715 $error = null;
696716 if ( ! wfRunHooks( 'UserLoginMailPassword', array( $this->mName, &$error ) ) ) {
697717 $this->mainLoginForm( $error );
698718 return;
699719 }
700720
 721+ # If the user doesn't have a login token yet, set one.
 722+ if ( !self::getLoginToken() ) {
 723+ self::setLoginToken();
 724+ $this->mainLoginForm( wfMsg( 'sessionfailure' ) );
 725+ return;
 726+ }
 727+
 728+ # If the user didn't pass a login token, tell them we need one
 729+ if ( !$this->mToken ) {
 730+ $this->mainLoginForm( wfMsg( 'sessionfailure' ) );
 731+ return;
 732+ }
 733+
701734 # Check against the rate limiter
702735 if( $wgUser->pingLimiter( 'mailpassword' ) ) {
703736 $wgOut->rateLimited();
——@@ -717,6 +750,12 @@
718751 return;
719752 }
720753
 754+ # Validate the login token
 755+ if ( $this->mToken !== self::getLoginToken() ) {
 756+ $this->mainLoginForm( wfMsg( 'sessionfailure' ) );
 757+ return;
 758+ }
 759+
721760 # Check against password throttle
722761 if ( $u->isPasswordReminderThrottled() ) {
723762 global $wgPasswordReminderResendTime;
——@@ -732,6 +771,7 @@
733772 $this->mainLoginForm( wfMsg( 'mailerror', $result->getMessage() ) );
734773 } else {
735774 $this->mainLoginForm( wfMsg( 'passwordsent', $u->getName() ), 'success' );
 775+ self::clearLoginToken();
736776 }
737777 }
738778
——@@ -965,11 +1005,18 @@
9661006 $template->set( 'canremember', ( $wgCookieExpiration > 0 ) );
9671007 $template->set( 'remember', $wgUser->getOption( 'rememberpassword' ) or $this->mRemember );
9681008
969 - if ( !self::getLoginToken() ) {
970 - self::setLoginToken();
 1009+ if ( $this->mType == 'signup' ) {
 1010+ if ( !self::getCreateaccountToken() ) {
 1011+ self::setCreateaccountToken();
 1012+ }
 1013+ $template->set( 'token', self::getCreateaccountToken() );
 1014+ } else {
 1015+ if ( !self::getLoginToken() ) {
 1016+ self::setLoginToken();
 1017+ }
 1018+ $template->set( 'token', self::getLoginToken() );
9711019 }
972 - $template->set( 'token', self::getLoginToken() );
973 -
 1020+
9741021 # Prepare language selection links as needed
9751022 if( $wgLoginLanguageSelector ) {
9761023 $template->set( 'languages', $this->makeLanguageSelector() );
——@@ -1034,7 +1081,7 @@
10351082 }
10361083
10371084 /**
1038 - * Generate a new login token and attach it to the current session
 1085+ * Randomly generate a new login token and attach it to the current session
10391086 */
10401087 public static function setLoginToken() {
10411088 global $wgRequest;
——@@ -1046,12 +1093,36 @@
10471094 /**
10481095 * Remove any login token attached to the current session
10491096 */
1050 - public static function clearLoginToken() {
 1097+ public static function clearLoginToken() {
10511098 global $wgRequest;
10521099 $wgRequest->setSessionData( 'wsLoginToken', null );
10531100 }
10541101
10551102 /**
 1103+ * Get the createaccount token from the current session
 1104+ */
 1105+ public static function getCreateaccountToken() {
 1106+ global $wgRequest;
 1107+ return $wgRequest->getSessionData( 'wsCreateaccountToken' );
 1108+ }
 1109+
 1110+ /**
 1111+ * Randomly generate a new createaccount token and attach it to the current session
 1112+ */
 1113+ public static function setCreateaccountToken() {
 1114+ global $wgRequest;
 1115+ $wgRequest->setSessionData( 'wsCreateaccountToken', User::generateToken() );
 1116+ }
 1117+
 1118+ /**
 1119+ * Remove any createaccount token attached to the current session
 1120+ */
 1121+ public static function clearCreateaccountToken() {
 1122+ global $wgRequest;
 1123+ $wgRequest->setSessionData( 'wsCreateaccountToken', null );
 1124+ }
 1125+
 1126+ /**
10561127 * @private
10571128 */
10581129 function cookieRedirectCheck( $type ) {
Index: branches/REL1_16/phase3/includes/templates/Userlogin.php
——@@ -315,6 +315,7 @@
316316 </tr>
317317 </table>
318318 <?php if( @$this->haveData( 'uselang' ) ) { ?><input type="hidden" name="uselang" value="<?php $this->text( 'uselang' ); ?>" /><?php } ?>
 319+<?php if( @$this->haveData( 'token' ) ) { ?><input type="hidden" name="wpCreateaccountToken" value="<?php $this->text( 'token' ); ?>" /><?php } ?>
319320 </form>
320321 </div>
321322 <div id="signupend"><?php $this->msgWiki( 'signupend' ); ?></div>
Index: branches/REL1_16/phase3/RELEASE-NOTES
——@@ -40,6 +40,9 @@
4141 you have the DBA extension for PHP installed, this will improve performance
4242 further.
4343
 44+* (bug 23371) Fixed holes in Special:Userlogin which could lead to abuse of
 45+the system, and disclosure of information from private wikis.
 46+
4447 == Changes since 1.16 beta 2 ==
4548
4649 * Fixed bugs in the [[Special:Userlogin]] and [[Special:Emailuser]] handling of

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r65760Bug 23371: Fix CSRF similar to r64677 covering the other three execute()...platonides20:16, 1 May 2010

Comments

#Comment by Tim Starling (talk | contribs)   05:54, 28 May 2010

Please put RELEASE-NOTES entries in the correct section.

#Comment by Platonides (talk | contribs)   14:47, 28 May 2010

Whoops, sorry. I somehow thought that the next section read "Changes in 1.16 beta 2".

Status & tagging log