From: Kevin Israel <pleasestand@live.com>
Date: Sun, 13 Apr 2014 17:11:18 +0000 (-0400)
Subject: Add hash_equals() fallback and use it
X-Git-Tag: 1.31.0-rc.0~15542^2
X-Git-Url: http://git.cyclocoop.org/ecrire?a=commitdiff_plain;h=b9e1d5f5c066a26f115eac69e268a98e6591d121;p=lhc%2Fweb%2Fwiklou.git

Add hash_equals() fallback and use it

Two classes (User and SpecialRunJobs) currently contain string
equality checks that purport to be timing-attack resistant.

Reduce code duplication by adding and using a fallback for the
hash_equals() function from PHP 5.6 (currently in beta), in a way
addressing the comment "@todo: make a common method for this".

Change-Id: Iece006ec0216edb3fc5fbef7cc6ec00a6d182775
---

diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php
index 9cbb9d681c..c95c380541 100644
--- 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 941a40509d..7b35a297c0 100644
--- 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 63eff36ca3..4c8c8f3081 100644
--- 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';