From b1e4006b440185c7e8304c03946bb0155b0edc83 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Tue, 26 Aug 2014 14:12:32 -0400 Subject: [PATCH] Allow for time-limited tokens Tokens returned from User::getEditToken will now include an encoded timestamp (and therefore will be up to 16 bytes longer). User::matchEditToken and User::matchEditTokenNoSuffix are gaining a "maxage" parameter: when specified, only tokens with an encoded timestamp within $maxage seconds will be accepted. Change-Id: Ic4cf148cc1fa9d0561aac0bd2d68a09c7851896d --- RELEASE-NOTES-1.25 | 6 ++ includes/User.php | 75 ++++++++++++++++------ tests/phpunit/includes/api/ApiTestCase.php | 6 +- 3 files changed, 65 insertions(+), 22 deletions(-) diff --git a/RELEASE-NOTES-1.25 b/RELEASE-NOTES-1.25 index 21800223cc..ce2cbd75e6 100644 --- a/RELEASE-NOTES-1.25 +++ b/RELEASE-NOTES-1.25 @@ -12,6 +12,10 @@ production. * $wgPageShowWatchingUsers was removed. * $wgLocalVirtualHosts has been added to replace $wgConf->localVHosts. * $wgAntiLockFlags was removed. +* Edit tokens returned from User::getEditToken may change on every call. Token + validity must be checked by passing the user-supplied token to + User::matchEditToken rather than by testing for equality with a + newly-generated token. === New features in 1.25 === * (bug 58139) ResourceLoaderFileModule now supports language fallback @@ -33,6 +37,8 @@ production. - Usage instructions: https://www.mediawiki.org/wiki/Help:Page_status_indicators - Adjusting custom skins to support indicators: https://www.mediawiki.org/wiki/Manual:Skinning#Page_status_indicators +* Edit tokens may now be time-limited: passing a maximum age to + User::matchEditToken will reject any older tokens. === Bug fixes in 1.25 === * (bug 71003) No additional code will be generated to try to load CSS-embedded diff --git a/includes/User.php b/includes/User.php index e6425f879d..3661b4f68a 100644 --- a/includes/User.php +++ b/includes/User.php @@ -3877,22 +3877,15 @@ class User implements IDBAccessObject { } /** - * Initialize (if necessary) and return a session token value - * which can be used in edit forms to show that the user's - * login credentials aren't being hijacked with a foreign form - * submission. - * - * @since 1.19 + * Internal implementation for self::getEditToken() and + * self::matchEditToken(). * - * @param string|array $salt Array of Strings Optional function-specific data for hashing - * @param WebRequest|null $request WebRequest object to use or null to use $wgRequest - * @return string The new edit token + * @param string|array $salt + * @param WebRequest $request + * @param string|int $timestamp + * @return string */ - public function getEditToken( $salt = '', $request = null ) { - if ( $request == null ) { - $request = $this->getRequest(); - } - + private function getEditTokenAtTimestamp( $salt, $request, $timestamp ) { if ( $this->isAnon() ) { return self::EDIT_TOKEN_SUFFIX; } else { @@ -3904,10 +3897,30 @@ class User implements IDBAccessObject { if ( is_array( $salt ) ) { $salt = implode( '|', $salt ); } - return md5( $token . $salt ) . self::EDIT_TOKEN_SUFFIX; + return hash_hmac( 'md5', $timestamp . $salt, $token, false ) . + dechex( $timestamp ) . + self::EDIT_TOKEN_SUFFIX; } } + /** + * Initialize (if necessary) and return a session token value + * which can be used in edit forms to show that the user's + * login credentials aren't being hijacked with a foreign form + * submission. + * + * @since 1.19 + * + * @param string|array $salt Array of Strings Optional function-specific data for hashing + * @param WebRequest|null $request WebRequest object to use or null to use $wgRequest + * @return string The new edit token + */ + public function getEditToken( $salt = '', $request = null ) { + return $this->getEditTokenAtTimestamp( + $salt, $request ?: $this->getRequest(), wfTimestamp() + ); + } + /** * Generate a looking random token for various uses. * @@ -3928,15 +3941,34 @@ class User implements IDBAccessObject { * @param string $val Input value to compare * @param string $salt Optional function-specific data for hashing * @param WebRequest|null $request Object to use or null to use $wgRequest + * @param int $maxage Fail tokens older than this, in seconds * @return bool Whether the token matches */ - public function matchEditToken( $val, $salt = '', $request = null ) { - $sessionToken = $this->getEditToken( $salt, $request ); + public function matchEditToken( $val, $salt = '', $request = null, $maxage = null ) { + if ( $this->isAnon() ) { + return $val === self::EDIT_TOKEN_SUFFIX; + } + + $suffixLen = strlen( self::EDIT_TOKEN_SUFFIX ); + if ( strlen( $val ) <= 32 + $suffixLen ) { + return false; + } + + $timestamp = hexdec( substr( $val, 32, -$suffixLen ) ); + if ( $maxage !== null && $timestamp < wfTimestamp() - $maxage ) { + // Expired token + return false; + } + + $sessionToken = $this->getEditTokenAtTimestamp( + $salt, $request ?: $this->getRequest(), $timestamp + ); + if ( $val != $sessionToken ) { wfDebug( "User::matchEditToken: broken session data\n" ); } - return $val == $sessionToken; + return hash_equals( $sessionToken, $val ); } /** @@ -3946,11 +3978,12 @@ class User implements IDBAccessObject { * @param string $val Input value to compare * @param string $salt Optional function-specific data for hashing * @param WebRequest|null $request Object to use or null to use $wgRequest + * @param int $maxage Fail tokens older than this, in seconds * @return bool Whether the token matches */ - public function matchEditTokenNoSuffix( $val, $salt = '', $request = null ) { - $sessionToken = $this->getEditToken( $salt, $request ); - return substr( $sessionToken, 0, 32 ) == substr( $val, 0, 32 ); + public function matchEditTokenNoSuffix( $val, $salt = '', $request = null, $maxage = null ) { + $val = substr( $val, 0, strspn( $val, '0123456789abcdef' ) ) . self::EDIT_TOKEN_SUFFIX; + return $this->matchEditToken( $val, $salt, $request, $maxage ); } /** diff --git a/tests/phpunit/includes/api/ApiTestCase.php b/tests/phpunit/includes/api/ApiTestCase.php index cd141947b5..06acbaa1a7 100644 --- a/tests/phpunit/includes/api/ApiTestCase.php +++ b/tests/phpunit/includes/api/ApiTestCase.php @@ -134,10 +134,14 @@ abstract class ApiTestCase extends MediaWikiLangTestCase { } if ( isset( $session['wsToken'] ) && $session['wsToken'] ) { + // @todo Why does this directly mess with the session? Fix that. // add edit token to fake session $session['wsEditToken'] = $session['wsToken']; // add token to request parameters - $params['token'] = md5( $session['wsToken'] ) . User::EDIT_TOKEN_SUFFIX; + $timestamp = wfTimestamp(); + $params['token'] = hash_hmac( 'md5', $timestamp, $session['wsToken'] ) . + dechex( $timestamp ) . + User::EDIT_TOKEN_SUFFIX; return $this->doApiRequest( $params, $session, false, $user ); } else { -- 2.20.1