Merge "Add hash_equals() fallback and use it"

diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php
index 9cbb9d6..c95c380 100644 (file)
--- a/includes/GlobalFunctions.php
+++ b/includes/GlobalFunctions.php
@@ -104,6 +104,53 @@ if ( !function_exists( 'gzdecode' ) ) {
                return gzinflate( substr( $data, 10, -8 ) );
        }
 }
+
+// hash_equals function only exists in PHP >= 5.6.0
+if ( !function_exists( 'hash_equals' ) ) {
+       /**
+        * Check whether a user-provided string is equal to a fixed-length secret without
+        * revealing bytes of the secret through timing differences.
+        *
+        * This timing guarantee -- that a partial match takes the same time as a complete
+        * mismatch -- is why this function is used in some security-sensitive parts of the code.
+        * For example, it shouldn't be possible to guess an HMAC signature one byte at a time.
+        *
+        * Longer explanation: http://www.emerose.com/timing-attacks-explained
+        *
+        * @codeCoverageIgnore
+        * @param string $known_string Fixed-length secret to compare against
+        * @param string $user_string User-provided string
+        * @return bool True if the strings are the same, false otherwise
+        */
+       function hash_equals( $known_string, $user_string ) {
+               // Strict type checking as in PHP's native implementation
+               if ( !is_string( $known_string ) ) {
+                       trigger_error( 'hash_equals(): Expected known_string to be a string, ' .
+                               gettype( $known_string ) . ' given', E_USER_WARNING );
+
+                       return false;
+               }
+
+               if ( !is_string( $user_string ) ) {
+                       trigger_error( 'hash_equals(): Expected user_string to be a string, ' .
+                               gettype( $user_string ) . ' given', E_USER_WARNING );
+
+                       return false;
+               }
+
+               // Note that we do one thing PHP doesn't: try to avoid leaking information about
+               // relative lengths of $known_string and $user_string, and of multiple $known_strings.
+               // However, lengths may still inevitably leak through, for example, CPU cache misses.
+               $known_string_len = strlen( $known_string );
+               $user_string_len = strlen( $user_string );
+               $result = $known_string_len ^ $user_string_len;
+               for ( $i = 0; $i < $user_string_len; $i++ ) {
+                       $result |= ord( $known_string[$i % $known_string_len] ) ^ ord( $user_string[$i] );
+               }
+
+               return ( $result === 0 );
+       }
+}
 /// @endcond
 
 /**

diff --git a/includes/User.php b/includes/User.php
index 941a405..7b35a29 100644 (file)
--- a/includes/User.php
+++ b/includes/User.php
@@ -1146,7 +1146,7 @@ class User {
                        $token = rtrim( $proposedUser->getToken( false ) ); // correct token
                        // Make comparison in constant time (bug 61346)
                        $passwordCorrect = strlen( $token )
-                               && $this->compareSecrets( $token, $request->getCookie( 'Token' ) );
+                               && hash_equals( $token, $request->getCookie( 'Token' ) );
                        $from = 'cookie';
                } else {
                        // No session or persistent login cookie
@@ -1165,27 +1165,6 @@ class User {
                }
        }
 
-       /**
-        * A comparison of two strings, not vulnerable to timing attacks
-        * @param string $answer The secret string that you are comparing against.
-        * @param string $test Compare this string to the $answer.
-        * @return bool True if the strings are the same, false otherwise
-        */
-       protected function compareSecrets( $answer, $test ) {
-               if ( strlen( $answer ) !== strlen( $test ) ) {
-                       $passwordCorrect = false;
-               } else {
-                       $result = 0;
-                       $answerLength = strlen( $answer );
-                       for ( $i = 0; $i < $answerLength; $i++ ) {
-                               $result |= ord( $answer[$i] ) ^ ord( $test[$i] );
-                       }
-                       $passwordCorrect = ( $result == 0 );
-               }
-
-               return $passwordCorrect;
-       }
-
        /**
         * Load user and user_group data from the database.
         * $this->mId must be set, this is how the user is identified.

diff --git a/includes/specials/SpecialRunJobs.php b/includes/specials/SpecialRunJobs.php
index 63eff36..4c8c8f3 100644 (file)
--- a/includes/specials/SpecialRunJobs.php
+++ b/includes/specials/SpecialRunJobs.php
@@ -64,19 +64,7 @@ class SpecialRunJobs extends UnlistedSpecialPage {
                $cSig = self::getQuerySignature( $squery ); // correct signature
                $rSig = $params['signature']; // provided signature
 
-               // Constant-time signature verification
-               // http://www.emerose.com/timing-attacks-explained
-               // @todo Make a common method for this
-               if ( !is_string( $rSig ) || strlen( $rSig ) !== strlen( $cSig ) ) {
-                       $verified = false;
-               } else {
-                       $result = 0;
-                       $cSigLength = strlen( $cSig );
-                       for ( $i = 0; $i < $cSigLength; $i++ ) {
-                               $result |= ord( $cSig[$i] ) ^ ord( $rSig[$i] );
-                       }
-                       $verified = ( $result == 0 );
-               }
+               $verified = is_string( $rSig ) && hash_equals( $cSig, $rSig );
                if ( !$verified || $params['sigexpiry'] < time() ) {
                        header( "HTTP/1.0 400 Bad Request" );
                        print 'Invalid or stale signature provided';