From 5fd5b3276f240c8189755db9641e764543b8c0b1 Mon Sep 17 00:00:00 2001 From: Sam Wilson Date: Wed, 4 Jan 2017 11:38:27 +0800 Subject: [PATCH] Validate BlockID cookie before use This change adds a HMAC to the block-cookie to prevent someone spoofing a cookie and so discovering revdeleted users' names. The HMAC is only added if $wgSecretKey is set; if it isn't, the existing plain-ID format is used. A note about this has been added to DefaultSettings.php. Tests are updated and new tests added to demonstrate an inauthentic HMAC, and for when $wgSecretKey is not definied. Bug: T152951 Change-Id: I6a3ef9e91091408c25eaa2d36d58b365d681e8c6 --- includes/Block.php | 52 ++++++++++++++- includes/DefaultSettings.php | 5 +- includes/user/User.php | 63 +++++++++++------- tests/phpunit/includes/user/UserTest.php | 84 +++++++++++++++++++++++- 4 files changed, 175 insertions(+), 29 deletions(-) diff --git a/includes/Block.php b/includes/Block.php index 9d3a2f935c..66b9ff02d5 100644 --- a/includes/Block.php +++ b/includes/Block.php @@ -1467,9 +1467,55 @@ class Block { } // Set the cookie. Reformat the MediaWiki datetime as a Unix timestamp for the cookie. - $cookieValue = $setEmpty ? '' : $this->getId(); - $expiryValue = DateTime::createFromFormat( "YmdHis", $expiryTime ); - $response->setCookie( 'BlockID', $cookieValue, $expiryValue->format( "U" ) ); + $cookieValue = $setEmpty ? '' : $this->getCookieValue(); + $expiryValue = DateTime::createFromFormat( 'YmdHis', $expiryTime )->format( 'U' ); + $cookieOptions = [ 'httpOnly' => false ]; + $response->setCookie( 'BlockID', $cookieValue, $expiryValue, $cookieOptions ); + } + + /** + * Get the BlockID cookie's value for this block. This is usually the block ID concatenated + * with an HMAC in order to avoid spoofing (T152951), but if wgSecretKey is not set will just + * be the block ID. + * @return string The block ID, probably concatenated with "!" and the HMAC. + */ + public function getCookieValue() { + $config = RequestContext::getMain()->getConfig(); + $id = $this->getId(); + $secretKey = $config->get( 'SecretKey' ); + if ( !$secretKey ) { + // If there's no secret key, don't append a HMAC. + return $id; + } + $hmac = MWCryptHash::hmac( $id, $secretKey, false ); + $cookieValue = $id . '!' . $hmac; + return $cookieValue; + } + + /** + * Get the stored ID from the 'BlockID' cookie. The cookie's value is usually a combination of + * the ID and a HMAC (see Block::setCookie), but will sometimes only be the ID. + * @param string $cookieValue The string in which to find the ID. + * @return integer|null The block ID, or null if the HMAC is present and invalid. + */ + public static function getIdFromCookieValue( $cookieValue ) { + // Extract the ID prefix from the cookie value (may be the whole value, if no bang found). + $bangPos = strpos( $cookieValue, '!' ); + $id = ( $bangPos === false ) ? $cookieValue : substr( $cookieValue, 0, $bangPos ); + // Get the site-wide secret key. + $config = RequestContext::getMain()->getConfig(); + $secretKey = $config->get( 'SecretKey' ); + if ( !$secretKey ) { + // If there's no secret key, just use the ID as given. + return $id; + } + $storedHmac = substr( $cookieValue, $bangPos + 1 ); + $calculatedHmac = MWCryptHash::hmac( $id, $secretKey, false ); + if ( $calculatedHmac === $storedHmac ) { + return $id; + } else { + return null; + } } /** diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index a1a4067f56..c4833660d8 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -5989,7 +5989,10 @@ $wgSessionName = false; /** * Whether to set a cookie when a user is autoblocked. Doing so means that a blocked user, even - * after logging out and moving to a new IP address, will still be blocked. + * after logging out and moving to a new IP address, will still be blocked. This cookie will contain + * an authentication code if $wgSecretKey is set, or otherwise will just be the block ID (in + * which case there is a possibility of an attacker discovering the names of revdeleted users, so + * it is best to use this in conjunction with $wgSecretKey being set). */ $wgCookieSetOnAutoblock = false; diff --git a/includes/user/User.php b/includes/user/User.php index 6804df272a..d0a2f9213f 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -1637,29 +1637,9 @@ class User implements IDBAccessObject { // User/IP blocking $block = Block::newFromTarget( $this, $ip, !$bFromSlave ); - // If no block has been found, check for a cookie indicating that the user is blocked. - $blockCookieVal = (int)$this->getRequest()->getCookie( 'BlockID' ); - if ( !$block instanceof Block && $blockCookieVal > 0 ) { - // Load the Block from the ID in the cookie. - $tmpBlock = Block::newFromID( $blockCookieVal ); - if ( $tmpBlock instanceof Block ) { - // Check the validity of the block. - $blockIsValid = $tmpBlock->getType() == Block::TYPE_USER - && !$tmpBlock->isExpired() - && $tmpBlock->isAutoblocking(); - $config = RequestContext::getMain()->getConfig(); - $useBlockCookie = ( $config->get( 'CookieSetOnAutoblock' ) === true ); - if ( $blockIsValid && $useBlockCookie ) { - // Use the block. - $block = $tmpBlock; - $this->blockTrigger = 'cookie-block'; - } else { - // If the block is not valid, clear the block cookie (but don't delete it, - // because it needs to be cleared from LocalStorage as well and an empty string - // value is checked for in the mediawiki.user.blockcookie module). - $tmpBlock->setCookie( $this->getRequest()->response(), true ); - } - } + // Cookie blocking + if ( !$block instanceof Block ) { + $block = $this->getBlockFromCookieValue( $this->getRequest()->getCookie( 'BlockID' ) ); } // Proxy blocking @@ -1738,6 +1718,43 @@ class User implements IDBAccessObject { Hooks::run( 'GetBlockedStatus', [ &$user ] ); } + /** + * Try to load a Block from an ID given in a cookie value. + * @param string|null $blockCookieVal The cookie value to check. + * @return Block|bool The Block object, or false if none could be loaded. + */ + protected function getBlockFromCookieValue( $blockCookieVal ) { + // Make sure there's something to check. The cookie value must start with a number. + if ( strlen( $blockCookieVal ) < 1 || !is_numeric( substr( $blockCookieVal, 0, 1 ) ) ) { + return false; + } + // Load the Block from the ID in the cookie. + $blockCookieId = Block::getIdFromCookieValue( $blockCookieVal ); + if ( $blockCookieId !== null ) { + // An ID was found in the cookie. + $tmpBlock = Block::newFromID( $blockCookieId ); + if ( $tmpBlock instanceof Block ) { + // Check the validity of the block. + $blockIsValid = $tmpBlock->getType() == Block::TYPE_USER + && !$tmpBlock->isExpired() + && $tmpBlock->isAutoblocking(); + $config = RequestContext::getMain()->getConfig(); + $useBlockCookie = ( $config->get( 'CookieSetOnAutoblock' ) === true ); + if ( $blockIsValid && $useBlockCookie ) { + // Use the block. + $this->blockTrigger = 'cookie-block'; + return $tmpBlock; + } else { + // If the block is not valid, clear the block cookie (but don't delete it, + // because it needs to be cleared from LocalStorage as well and an empty string + // value is checked for in the mediawiki.user.blockcookie module). + $tmpBlock->setCookie( $this->getRequest()->response(), true ); + } + } + } + return false; + } + /** * Whether the given IP is in a DNS blacklist. * diff --git a/tests/phpunit/includes/user/UserTest.php b/tests/phpunit/includes/user/UserTest.php index 26e5d89041..deb970820e 100644 --- a/tests/phpunit/includes/user/UserTest.php +++ b/tests/phpunit/includes/user/UserTest.php @@ -599,6 +599,7 @@ class UserTest extends MediaWikiTestCase { $this->setMwGlobals( [ 'wgCookieSetOnAutoblock' => true, 'wgCookiePrefix' => 'wmsitetitle', + 'wgSecretKey' => MWCryptRand::generateHex( 64, true ), ] ); // 1. Log in a test user, and block them. @@ -626,12 +627,13 @@ class UserTest extends MediaWikiTestCase { // Test for the desired cookie name, value, and expiry. $cookies = $request1->response()->getCookies(); $this->assertArrayHasKey( 'wmsitetitleBlockID', $cookies ); - $this->assertEquals( $block->getId(), $cookies['wmsitetitleBlockID']['value'] ); $this->assertEquals( $expiryFiveHours, $cookies['wmsitetitleBlockID']['expire'] ); + $cookieValue = Block::getIdFromCookieValue( $cookies['wmsitetitleBlockID']['value'] ); + $this->assertEquals( $block->getId(), $cookieValue ); // 2. Create a new request, set the cookies, and see if the (anon) user is blocked. $request2 = new FauxRequest(); - $request2->setCookie( 'BlockID', $block->getId() ); + $request2->setCookie( 'BlockID', $block->getCookieValue() ); $user2 = User::newFromSession( $request2 ); $user2->load(); $this->assertNotEquals( $user1->getId(), $user2->getId() ); @@ -669,6 +671,7 @@ class UserTest extends MediaWikiTestCase { $this->setMwGlobals( [ 'wgCookieSetOnAutoblock' => false, 'wgCookiePrefix' => 'wm_no_cookies', + 'wgSecretKey' => MWCryptRand::generateHex( 64, true ), ] ); // 1. Log in a test user, and block them. @@ -705,6 +708,7 @@ class UserTest extends MediaWikiTestCase { $this->setMwGlobals( [ 'wgCookieSetOnAutoblock' => true, 'wgCookiePrefix' => 'wm_infinite_block', + 'wgSecretKey' => MWCryptRand::generateHex( 64, true ), ] ); // 1. Log in a test user, and block them indefinitely. $user1Tmp = $this->getTestUser()->getUser(); @@ -782,4 +786,80 @@ class UserTest extends MediaWikiTestCase { $this->assertNull( $wgUser->getBlock() ); } + /** + * Test that a modified BlockID cookie doesn't actually load the relevant block (T152951). + */ + public function testAutoblockCookieInauthentic() { + // Set up the bits of global configuration that we use. + $this->setMwGlobals( [ + 'wgCookieSetOnAutoblock' => true, + 'wgCookiePrefix' => 'wmsitetitle', + 'wgSecretKey' => MWCryptRand::generateHex( 64, true ), + ] ); + + // 1. Log in a blocked test user. + $user1tmp = $this->getTestUser()->getUser(); + $request1 = new FauxRequest(); + $request1->getSession()->setUser( $user1tmp ); + $block = new Block( [ 'enableAutoblock' => true ] ); + $block->setTarget( $user1tmp ); + $block->insert(); + $user1 = User::newFromSession( $request1 ); + $user1->mBlock = $block; + $user1->load(); + + // 2. Create a new request, set the cookie to an invalid value, and make sure the (anon) + // user not blocked. + $request2 = new FauxRequest(); + $request2->setCookie( 'BlockID', $block->getId() . '!zzzzzzz' ); + $user2 = User::newFromSession( $request2 ); + $user2->load(); + $this->assertTrue( $user2->isAnon() ); + $this->assertFalse( $user2->isLoggedIn() ); + $this->assertFalse( $user2->isBlocked() ); + + // Clean up. + $block->delete(); + } + + /** + * The BlockID cookie is normally verified with a HMAC, but not if wgSecretKey is not set. + * This checks that a non-authenticated cookie still works. + */ + public function testAutoblockCookieNoSecretKey() { + // Set up the bits of global configuration that we use. + $this->setMwGlobals( [ + 'wgCookieSetOnAutoblock' => true, + 'wgCookiePrefix' => 'wmsitetitle', + 'wgSecretKey' => null, + ] ); + + // 1. Log in a blocked test user. + $user1tmp = $this->getTestUser()->getUser(); + $request1 = new FauxRequest(); + $request1->getSession()->setUser( $user1tmp ); + $block = new Block( [ 'enableAutoblock' => true ] ); + $block->setTarget( $user1tmp ); + $block->insert(); + $user1 = User::newFromSession( $request1 ); + $user1->mBlock = $block; + $user1->load(); + $this->assertTrue( $user1->isBlocked() ); + + // 2. Create a new request, set the cookie to just the block ID, and the user should + // still get blocked when they log in again. + $request2 = new FauxRequest(); + $request2->setCookie( 'BlockID', $block->getId() ); + $user2 = User::newFromSession( $request2 ); + $user2->load(); + $this->assertNotEquals( $user1->getId(), $user2->getId() ); + $this->assertNotEquals( $user1->getToken(), $user2->getToken() ); + $this->assertTrue( $user2->isAnon() ); + $this->assertFalse( $user2->isLoggedIn() ); + $this->assertTrue( $user2->isBlocked() ); + $this->assertEquals( true, $user2->getBlock()->isAutoblocking() ); // Non-strict type-check. + + // Clean up. + $block->delete(); + } } -- 2.20.1