From 479dbce0bd9e130225a5cefee8c3a68dd77f11fa Mon Sep 17 00:00:00 2001 From: Brian Wolff Date: Sun, 7 Feb 2016 12:20:42 -0500 Subject: [PATCH] [TEST MODE] Prevent CSRF for anons using tokens based on IP. This is a testing only mode, where failures are logged only. See bug T40417 for details. unit-tests have been removed from this version of the patch. ------ Due to performance concerns, we can't have sessions for anon users. So instead, lets give them tokens based on their IP and a non-unique secret. Anons sharing an IP will still be vulnerable to a CSRF, but there's not much point doing a CSRF in that case since anon actions are all associated with the IP, so any action you do via CSRF, may as well just be done directly. In order not to exclude users with extremely rapidly changing IPs, we use normal tokens if the anon already happens to have a session. Anons with a session can still use the session-less IP based tokens for 1 hour after the session is started. T122056 should be fixed before this is deployed, as this may increase the security implications of that bug. This primary attack this is meant to deal with, would be someone trying to evade an IP-based block using CSRF, possibly using CSRF to make the damage appear to be coming from many IPs. Its important that tokens using this scheme expire in a reasonable amount of time. Otherwise someone could try to "collect" a large number of tokens to use in a targetted CSRF attack later. This sets anon tokens to expire after 4 hours. The 4 hour expiry (And 1 hour session-less compatibility window) have been chosend very arbitrarily. I'm not sure what an appropriate value would be, especially for the session-less compatibility window. This breaks mw.user.tokens client-side api. However breakage should be minimal as the built-in mw.Api library will fetch in background. Bug: T40417 Change-Id: I0aa2dd3bd208336967afebc9c4bee196abb5b9fd --- autoload.php | 2 + includes/api/ApiQueryTokens.php | 12 +- includes/htmlform/HTMLForm.php | 13 +- .../ResourceLoaderUserTokensModule.php | 10 ++ includes/session/Session.php | 43 ++++++- includes/session/StatefulLoggedOutToken.php | 141 +++++++++++++++++++++ includes/session/StatelessLoggedOutToken.php | 115 +++++++++++++++++ includes/specials/SpecialUserlogin.php | 4 +- includes/user/User.php | 6 +- 9 files changed, 322 insertions(+), 24 deletions(-) create mode 100644 includes/session/StatefulLoggedOutToken.php create mode 100644 includes/session/StatelessLoggedOutToken.php diff --git a/autoload.php b/autoload.php index 4d48de0..b3cd6b1 100644 --- a/autoload.php +++ b/autoload.php @@ -802,6 +802,8 @@ $wgAutoloadLocalClasses = array( 'MediaWiki\\Session\\SessionManagerInterface' => __DIR__ . '/includes/session/SessionManagerInterface.php', 'MediaWiki\\Session\\SessionProvider' => __DIR__ . '/includes/session/SessionProvider.php', 'MediaWiki\\Session\\SessionProviderInterface' => __DIR__ . '/includes/session/SessionProviderInterface.php', + 'MediaWiki\\Session\\StatefulLoggedOutToken' => __DIR__ . '/includes/session/StatefulLoggedOutToken.php', + 'MediaWiki\\Session\\StatelessLoggedOutToken' => __DIR__ . '/includes/session/StatelessLoggedOutToken.php', 'MediaWiki\\Session\\Token' => __DIR__ . '/includes/session/Token.php', 'MediaWiki\\Session\\UserInfo' => __DIR__ . '/includes/session/UserInfo.php', 'MediaWiki\\Site\\MediaWikiPageNameNormalizer' => __DIR__ . '/includes/site/MediaWikiPageNameNormalizer.php', diff --git a/includes/api/ApiQueryTokens.php b/includes/api/ApiQueryTokens.php index 3f3464b..49ae42b 100644 --- a/includes/api/ApiQueryTokens.php +++ b/includes/api/ApiQueryTokens.php @@ -71,8 +71,8 @@ class ApiQueryTokens extends ApiQueryBase { 'patrol' => 'patrol', 'rollback' => 'rollback', 'userrights' => 'userrights', - 'login' => array( '', 'login' ), - 'createaccount' => array( '', 'createaccount' ), + 'login' => array( '', 'login', false ), + 'createaccount' => array( '', 'createaccount', false ), ); Hooks::run( 'ApiQueryTokensRegisterTypes', array( &$salts ) ); ksort( $salts ); @@ -86,11 +86,9 @@ class ApiQueryTokens extends ApiQueryBase { * @param User $user * @param MediaWiki\\Session\\Session $session * @param string|array $salt A string will be used as the salt for - * User::getEditTokenObject() to fetch the token, which will give a - * LoggedOutEditToken (always "+\\") for anonymous users. An array will - * be used as parameters to MediaWiki\\Session\\Session::getToken(), which - * will always return a full token even for anonymous users. An array will - * also persist the session. + * User::getEditTokenObject() to fetch the token. + * An array will be used as parameters to MediaWiki\\Session\\Session::getToken(), + * and will also persist the session. * @return MediaWiki\\Session\\Token */ public static function getToken( User $user, MediaWiki\Session\Session $session, $salt ) { diff --git a/includes/htmlform/HTMLForm.php b/includes/htmlform/HTMLForm.php index 2282dc2..ff3c77b 100644 --- a/includes/htmlform/HTMLForm.php +++ b/includes/htmlform/HTMLForm.php @@ -489,13 +489,12 @@ class HTMLForm extends ContextSource { $submit = true; // no session check needed } elseif ( $this->getRequest()->wasPosted() ) { $editToken = $this->getRequest()->getVal( 'wpEditToken' ); - if ( $this->getUser()->isLoggedIn() || $editToken != null ) { - // Session tokens for logged-out users have no security value. - // However, if the user gave one, check it in order to give a nice - // "session expired" error instead of "permission denied" or such. - $submit = $this->getUser()->matchEditToken( $editToken, $this->mTokenSalt ); - } else { - $submit = true; + $submit = $this->getUser()->matchEditToken( $editToken, $this->mTokenSalt ); + if ( !$submit && $this->getUser()->isAnon() ) { + // Anon token match failure could be caused by user's IP + // changes frequently, and there is no session. Thus, enable + // session persistence in case that is the cause of token failure. + $this->getRequest()->getSession()->persist(); } } diff --git a/includes/resourceloader/ResourceLoaderUserTokensModule.php b/includes/resourceloader/ResourceLoaderUserTokensModule.php index 78fec50..e2ef014 100644 --- a/includes/resourceloader/ResourceLoaderUserTokensModule.php +++ b/includes/resourceloader/ResourceLoaderUserTokensModule.php @@ -43,6 +43,16 @@ class ResourceLoaderUserTokensModule extends ResourceLoaderModule { protected function contextUserTokens( ResourceLoaderContext $context ) { $user = $context->getUserObj(); + if ( $user->isAnon() && !$user->getRequest()->getSession()->isPersistent() ) { + // Show old-style tokens for anons without session. + // Don't want to break client side api's well in testing phase. + return array( + 'editToken' => '+\\', + 'patrolToken' => '+\\', + 'watchToken' => '+\\', + 'csrfToken' => '+\\', + ); + } return array( 'editToken' => $user->getEditToken(), 'patrolToken' => $user->getEditToken( 'patrol' ), diff --git a/includes/session/Session.php b/includes/session/Session.php index 4ad69ae..89bfcd2 100644 --- a/includes/session/Session.php +++ b/includes/session/Session.php @@ -322,10 +322,23 @@ final class Session implements \Countable, \Iterator { * * @param string|string[] $salt Token salt * @param string $key Token key + * @param boolean $allowStateless Allow stateless tokens for non-logged in users * @return MediaWiki\\Session\\SessionToken */ - public function getToken( $salt = '', $key = 'default' ) { - $new = false; + public function getToken( $salt = '', $key = 'default', $allowStateless = true ) { + // Secret to use for stateless tokens + $nonUniqueSecret = \RequestContext::getMain()->getConfig()->get( 'SecretKey' ) + . $key; + if ( $allowStateless && !$this->isPersistent() && $this->getUser()->isAnon() ) { + return new StatelessLoggedOutToken( + $this->getRequest()->getIP(), $nonUniqueSecret, (string)$salt + ); + } + + // If someone calls this function multiple times, we + // want to return same new everytime within a request, + // as new is used to determine if cookies are enabled. + static $new = array(); $secrets = $this->get( 'wsTokenSecrets' ); if ( !is_array( $secrets ) ) { $secrets = array(); @@ -336,12 +349,34 @@ final class Session implements \Countable, \Iterator { $secret = \MWCryptRand::generateHex( 32 ); $secrets[$key] = $secret; $this->set( 'wsTokenSecrets', $secrets ); - $new = true; + $new[$key] = true; } if ( is_array( $salt ) ) { $salt = join( '|', $salt ); } - return new Token( $secret, (string)$salt, $new ); + // StatefulLoggedOutToken also accepts stateless tokens for + // compatability, so we only use them if $allowStateless is true. + // if allowStateless is false, then we don't want the compatability, + // so we use normal Token's in that case. + if ( $allowStateless && $this->getUser()->isAnon() ) { + // Logged out persistent tokens need to know + // how long ago the session started to determine if + // stateless tokens should still be accepted. + $ts = $this->get( 'wsAnonTokenStartTS' ); + if ( !$ts ) { + $ts = wfTimestampNow(); + $this->set( 'wsAnonTokenStartTS', $ts ); + } + return new StatefulLoggedOutToken( + $this->getRequest()->getIP(), + $nonUniqueSecret, + $secret, + $ts, + (string)$salt, + isset( $new[$key] ) + ); + } /* else */ + return new Token( $secret, (string)$salt, isset( $new[$key] ) ); } /** diff --git a/includes/session/StatefulLoggedOutToken.php b/includes/session/StatefulLoggedOutToken.php new file mode 100644 index 0000000..617d87f --- /dev/null +++ b/includes/session/StatefulLoggedOutToken.php @@ -0,0 +1,141 @@ +statelessCompatToken = new StatelessLoggedOutToken( + $ip, $statelessSecret, $salt + ); + $this->statefulTimestamp = $statefulTime; + parent::__construct( $statefulSecret, $salt, $new ); + $this->salt = $salt; // temporary for log messages + } + + public function match( $userToken, $maxAge = null ) { + $res = $this->realMatch( $userToken, $maxAge ); + if ( !$res && hash_equals( '+\\', substr( $userToken, -2 ) ) ) { + $log = LoggerFactory::getInstance( 'token' ); + if ( strlen( $userToken ) === 2 ) { + $log->info( + "Old style +\\ '" . $this->salt . "'-token provided for anon", + array( "method" => __METHOD__, "salt" => $this->salt ) + ); + } else { + $log->info( + "Invalid '$this->salt' token provided for anon with session", + array( "method" => __METHOD__, "salt" => $this->salt ) + ); + } + return true; + } + return $res; + } + + public function realMatch( $userToken, $maxAge = null ) { + $normalMatch = parent::match( $userToken, $maxAge ); + if ( $normalMatch ) { + return $normalMatch; + } + + // If the session was just recently created, still allow the user + // to submit old style cookies. + $compatMaxAge = $maxAge ? min( $maxAge, self::MAX_STATELESS_TOKEN_TIME ) + : self::MAX_STATELESS_TOKEN_TIME; + $compatMatch = $this->statelessCompatToken->realMatch( $userToken, $compatMaxAge ); + $compatWindow = \wfTimestamp( TS_UNIX ) - self::MAX_STATELESS_TOKEN_TIME; + $stateAge = \wfTimestamp( TS_UNIX, $this->statefulTimestamp ); + if ( $stateAge > $compatWindow || $this->wasNew() ) { + return $compatMatch; + } elseif ( $compatMatch ) { + LoggerFactory::getInstance( 'token' )->info( + 'Valid stateless anon token rejected for stateful anon because state' + . ' too old (Session started: {age})', + array( + 'method' => __METHOD__, + 'age' => \wfTimestamp( TS_ISO_8601, $stateAge ) + ) + ); + } + return false; + } + + /** + * If this token is new, we don't know if the user supports cookies yet. + * + * Thus still return a stateful cookie for this request. + */ + public function toString() { + if ( $this->wasNew() ) { + return $this->statelessCompatToken->toString(); + } + return parent::toString(); + } +} diff --git a/includes/session/StatelessLoggedOutToken.php b/includes/session/StatelessLoggedOutToken.php new file mode 100644 index 0000000..78daebc --- /dev/null +++ b/includes/session/StatelessLoggedOutToken.php @@ -0,0 +1,115 @@ +getIP() . $wgSecretKey, $salt + * ); + * @since 1.27 + */ +class StatelessLoggedOutToken extends Token { + + const MAX_ANON_TOKEN_AGE = 14400; // 4 hours + private $ip; + private $salt = ''; + + public function __construct( $ip, $secret, $salt ) { + $this->ip = $ip; + $this->salt = $salt; + // Always make the token "new" since not from cookies. + parent::__construct( $ip . $secret, $salt, true ); + } + + public function match( $userToken, $maxAge = null ) { + $res = $this->realMatch( $userToken, $maxAge ); + if ( !$res && hash_equals( '+\\', substr( $userToken, -2 ) ) ) { + $log = LoggerFactory::getInstance( 'token' ); + if ( strlen( $userToken ) === 2 ) { + // Until mw.user.tokens is changed, this will have a + // lot of false positives for stateless tokens, esp from + // stashedit api action. + $log->debug( + "Old style +\\ '$this->salt'-token provided for anon", + array( "method" => __METHOD__, "salt" => $this->salt ) + ); + } else { + $log->info( + "Invalid '$this->salt' token provided for anon w/o session", + array( "method" => __METHOD__, "salt" => $this->salt ) + ); + } + return true; + } + return $res; + } + + public function realMatch( $userToken, $maxAge = null ) { + $maxAge = $maxAge ?: self::MAX_ANON_TOKEN_AGE; + $maxAge = min( $maxAge, self::MAX_ANON_TOKEN_AGE ); + $res = parent::match( $userToken, $maxAge ); + if ( !$res ) { + // @todo In the event of token match failure, it + // might make sense to automatically persist the session. + // On the other hand, it seems kind of wrong for a Token + // object to effect state. + // In any case, edit page will persist the session the + // moment you hit preview or save, and htmlform will + // persist on failure. + $isItExpired = parent::match( $userToken, null ); + if ( $isItExpired ) { + $this->logExpired( $maxAge, $userToken ); + } + } + return $res; + } + + /** + * Log that we rejected a token as expired. + * + * We want to keep track of how often this happens, to ensure + * that most users aren't inconvenienced by the relatively short expiry + * of anon tokens. + * + * @param $maxAge integer Number of seconds token is good for. + * @param $token String Token being checked + */ + private function logExpired( $maxAge, $token ) { + LoggerFactory::getInstance( 'token' )->info( + // putting $maxAge directly in msg so aggregated separately + "Stateless token for {ip} rejected as expired (older than $maxAge s)", + array( + 'maxAge' => $maxAge, + 'ip' => $this->ip, + 'method' => __METHOD__, + 'tokenTimestamp' => $this->getTimestamp( $token ) + ) + ); + } +} diff --git a/includes/specials/SpecialUserlogin.php b/includes/specials/SpecialUserlogin.php index 05e5229..beefc64 100644 --- a/includes/specials/SpecialUserlogin.php +++ b/includes/specials/SpecialUserlogin.php @@ -1573,7 +1573,7 @@ class LoginForm extends SpecialPage { */ public static function getLoginToken() { global $wgRequest; - return $wgRequest->getSession()->getToken( '', 'login' ); + return $wgRequest->getSession()->getToken( '', 'login', false ); } /** @@ -1604,7 +1604,7 @@ class LoginForm extends SpecialPage { */ public static function getCreateaccountToken() { global $wgRequest; - return $wgRequest->getSession()->getToken( '', 'createaccount' ); + return $wgRequest->getSession()->getToken( '', 'createaccount', false ); } /** diff --git a/includes/user/User.php b/includes/user/User.php index dccfd77..0ac5bed 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -21,6 +21,7 @@ */ use MediaWiki\Session\SessionManager; +use MediaWiki\Session\Token; /** * String Some punctuation to prevent editing from broken text-mangling proxies. @@ -4156,13 +4157,10 @@ class User implements IDBAccessObject { * @return MediaWiki\\Session\\Token The new edit token */ public function getEditTokenObject( $salt = '', $request = null ) { - if ( $this->isAnon() ) { - return new LoggedOutEditToken(); - } - if ( !$request ) { $request = $this->getRequest(); } + return $request->getSession()->getToken( $salt ); } -- 2.0.1