Use phutil_hashes_are_identical() when comparing hashes in Phabricator

Summary: See D14025. In all cases where we compare hashes, use strict, constant-time comparisons.

Test Plan: Logged in, logged out, added TOTP, ran Conduit, terminated sessions, submitted forms, changed password. Tweaked CSRF token, got rejected.

Reviewers: chad

Reviewed By: chad

Subscribers: chenxiruanhai

Differential Revision: https://secure.phabricator.com/D14026

src/applications/auth/controller/PhabricatorAuthController.php

@@ -209,7 +209,7 @@ abstract class PhabricatorAuthController extends PhabricatorController {
 
     $actual = $account->getProperty('registrationKey');
     $expect = PhabricatorHash::digest($registration_key);
-    if ($actual !== $expect) {
+    if (!phutil_hashes_are_identical($actual, $expect)) {
       $response = $this->renderError(
         pht(
           'Your browser submitted a different registration key than the one '.

src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php

@@ -21,7 +21,10 @@ final class PhabricatorAuthTerminateSessionController
 
     $sessions = $query->execute();
     foreach ($sessions as $key => $session) {
-      if ($session->getSessionKey() == $current_key) {
+      $is_current = phutil_hashes_are_identical(
+        $session->getSessionKey(),
+        $current_key);
+      if ($is_current) {
         // Don't terminate the current login session.
         unset($sessions[$key]);
       }

src/applications/auth/engine/PhabricatorAuthSessionEngine.php

@@ -296,7 +296,10 @@ final class PhabricatorAuthSessionEngine extends Phobject {
 
     foreach ($sessions as $key => $session) {
       if ($except_session !== null) {
-        if ($except_session == $session->getSessionKey()) {
+        $is_except = phutil_hashes_are_identical(
+          $session->getSessionKey(),
+          $except_session);
+        if ($is_except) {
           continue;
         }
       }

src/applications/auth/factor/PhabricatorTOTPAuthFactor.php

@@ -201,7 +201,7 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     // case the server or client has some clock skew.
     for ($offset = -2; $offset <= 2; $offset++) {
       $real = self::getTOTPCode($key, $now + $offset);
-      if ($real === $code) {
+      if (phutil_hashes_are_identical($real, $code)) {
         return true;
       }
     }

src/applications/auth/provider/PhabricatorAuthProvider.php

@@ -482,7 +482,7 @@ abstract class PhabricatorAuthProvider extends Phobject {
           'problem persists, you may need to clear your cookies.'));
     }
 
-    if ($actual !== $expect) {
+    if (!phutil_hashes_are_identical($actual, $expect)) {
       throw new Exception(
         pht(
           'The authentication provider did not return the correct client '.

src/applications/conduit/controller/PhabricatorConduitAPIController.php

@@ -434,7 +434,8 @@ final class PhabricatorConduitAPIController
       $token = idx($metadata, 'authToken');
       $signature = idx($metadata, 'authSignature');
       $certificate = $user->getConduitCertificate();
-      if (sha1($token.$certificate) !== $signature) {
+      $hash = sha1($token.$certificate);
+      if (!phutil_hashes_are_identical($hash, $signature)) {
         return array(
           'ERR-INVALID-AUTH',
           pht('Authentication is invalid.'),

src/applications/conduit/method/ConduitConnectConduitAPIMethod.php

@@ -142,7 +142,7 @@ final class ConduitConnectConduitAPIMethod extends ConduitAPIMethod {
               $threshold));
       }
       $valid = sha1($token.$user->getConduitCertificate());
-      if ($valid != $signature) {
+      if (!phutil_hashes_are_identical($valid, $signature)) {
         throw new ConduitException('ERR-INVALID-CERTIFICATE');
       }
       $session_key = id(new PhabricatorAuthSessionEngine())->establishSession(

src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php

@@ -13,9 +13,11 @@ final class PhabricatorMetaMTAMailgunReceiveController
     $timestamp = $request->getStr('timestamp');
     $token = $request->getStr('token');
     $sig = $request->getStr('signature');
-    return hash_hmac('sha256', $timestamp.$token, $api_key) == $sig;
+    $hash = hash_hmac('sha256', $timestamp.$token, $api_key);
 
+    return phutil_hashes_are_identical($sig, $hash);
   }
+
   public function processRequest() {
 
     // No CSRF for Mailgun.

src/applications/people/storage/PhabricatorUser.php

@@ -365,19 +365,16 @@ final class PhabricatorUser
   }
 
   public function validateCSRFToken($token) {
-    $salt = null;
+    // We expect a BREACH-mitigating token. See T3684.
-    $version = 'plain';
-
-    // This is a BREACH-mitigating token. See T3684.
     $breach_prefix = self::CSRF_BREACH_PREFIX;
     $breach_prelen = strlen($breach_prefix);
-
+    if (strncmp($token, $breach_prefix, $breach_prelen) !== 0) {
-    if (!strncmp($token, $breach_prefix, $breach_prelen)) {
+      return false;
-      $version = 'breach';
-      $salt = substr($token, $breach_prelen, self::CSRF_SALT_LENGTH);
-      $token = substr($token, $breach_prelen + self::CSRF_SALT_LENGTH);
     }
 
+    $salt = substr($token, $breach_prelen, self::CSRF_SALT_LENGTH);
+    $token = substr($token, $breach_prelen + self::CSRF_SALT_LENGTH);
+
     // When the user posts a form, we check that it contains a valid CSRF token.
     // Tokens cycle each hour (every CSRF_CYLCE_FREQUENCY seconds) and we accept
     // either the current token, the next token (users can submit a "future"
@@ -407,22 +404,11 @@ final class PhabricatorUser
 
     for ($ii = -$csrf_window; $ii <= 1; $ii++) {
       $valid = $this->getRawCSRFToken($ii);
-      switch ($version) {
+
-        // TODO: We can remove this after the BREACH version has been in the
+      $digest = PhabricatorHash::digest($valid, $salt);
-        // wild for a while.
+      $digest = substr($digest, 0, self::CSRF_TOKEN_LENGTH);
-        case 'plain':
+      if (phutil_hashes_are_identical($digest, $token)) {
-          if ($token == $valid) {
+        return true;
-            return true;
-          }
-          break;
-        case 'breach':
-          $digest = PhabricatorHash::digest($valid, $salt);
-          if (substr($digest, 0, self::CSRF_TOKEN_LENGTH) == $token) {
-            return true;
-          }
-          break;
-        default:
-          throw new Exception(pht('Unknown CSRF token format!'));
       }
     }
 

src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php

@@ -50,7 +50,10 @@ final class PhabricatorSessionsSettingsPanel extends PhabricatorSettingsPanel {
     $rows = array();
     $rowc = array();
     foreach ($sessions as $session) {
-      if ($session->getSessionKey() == $current_key) {
+      $is_current = phutil_hashes_are_identical(
+        $session->getSessionKey(),
+        $current_key);
+      if ($is_current) {
         $rowc[] = 'highlighted';
         $button = phutil_tag(
           'a',

src/infrastructure/util/password/PhabricatorPasswordHasher.php

@@ -126,7 +126,7 @@ abstract class PhabricatorPasswordHasher extends Phobject {
     $actual_hash = $this->getPasswordHash($password)->openEnvelope();
     $expect_hash = $hash->openEnvelope();
 
-    return ($actual_hash === $expect_hash);
+    return phutil_hashes_are_identical($actual_hash, $expect_hash);
   }