Validate BlockID cookie before use
authorSam Wilson <sam@samwilson.id.au>
Wed, 4 Jan 2017 03:38:27 +0000 (11:38 +0800)
committerSam Wilson <sam@samwilson.id.au>
Fri, 10 Feb 2017 03:35:57 +0000 (11:35 +0800)
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
includes/DefaultSettings.php
includes/user/User.php
tests/phpunit/includes/user/UserTest.php

index 9d3a2f9..66b9ff0 100644 (file)
@@ -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;
+               }
        }
 
        /**
index a1a4067..c483366 100644 (file)
@@ -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;
 
index 6804df2..d0a2f92 100644 (file)
@@ -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.
         *
index 26e5d89..deb9708 100644 (file)
@@ -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();
+       }
 }