diff --git a/src/applications/auth/controller/PhabricatorAuthController.php b/src/applications/auth/controller/PhabricatorAuthController.php --- a/src/applications/auth/controller/PhabricatorAuthController.php +++ b/src/applications/auth/controller/PhabricatorAuthController.php @@ -209,7 +209,7 @@ $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 '. diff --git a/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php b/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php --- a/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php +++ b/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php @@ -21,7 +21,10 @@ $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]); } diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -296,7 +296,10 @@ 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; } } diff --git a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php --- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php @@ -201,7 +201,7 @@ // 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; } } diff --git a/src/applications/auth/provider/PhabricatorAuthProvider.php b/src/applications/auth/provider/PhabricatorAuthProvider.php --- a/src/applications/auth/provider/PhabricatorAuthProvider.php +++ b/src/applications/auth/provider/PhabricatorAuthProvider.php @@ -482,7 +482,7 @@ '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 '. diff --git a/src/applications/conduit/controller/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/PhabricatorConduitAPIController.php --- a/src/applications/conduit/controller/PhabricatorConduitAPIController.php +++ b/src/applications/conduit/controller/PhabricatorConduitAPIController.php @@ -434,7 +434,8 @@ $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.'), diff --git a/src/applications/conduit/method/ConduitConnectConduitAPIMethod.php b/src/applications/conduit/method/ConduitConnectConduitAPIMethod.php --- a/src/applications/conduit/method/ConduitConnectConduitAPIMethod.php +++ b/src/applications/conduit/method/ConduitConnectConduitAPIMethod.php @@ -142,7 +142,7 @@ $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( diff --git a/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php b/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php --- a/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php +++ b/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php @@ -13,9 +13,11 @@ $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. diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -365,19 +365,16 @@ } public function validateCSRFToken($token) { - $salt = null; - $version = 'plain'; - - // This is a BREACH-mitigating token. See T3684. + // We expect a BREACH-mitigating token. See T3684. $breach_prefix = self::CSRF_BREACH_PREFIX; $breach_prelen = strlen($breach_prefix); - - if (!strncmp($token, $breach_prefix, $breach_prelen)) { - $version = 'breach'; - $salt = substr($token, $breach_prelen, self::CSRF_SALT_LENGTH); - $token = substr($token, $breach_prelen + self::CSRF_SALT_LENGTH); + if (strncmp($token, $breach_prefix, $breach_prelen) !== 0) { + return false; } + $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 @@ 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 - // wild for a while. - case 'plain': - if ($token == $valid) { - 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!')); + + $digest = PhabricatorHash::digest($valid, $salt); + $digest = substr($digest, 0, self::CSRF_TOKEN_LENGTH); + if (phutil_hashes_are_identical($digest, $token)) { + return true; } } diff --git a/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php b/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php --- a/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php @@ -50,7 +50,10 @@ $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', diff --git a/src/infrastructure/util/password/PhabricatorPasswordHasher.php b/src/infrastructure/util/password/PhabricatorPasswordHasher.php --- a/src/infrastructure/util/password/PhabricatorPasswordHasher.php +++ b/src/infrastructure/util/password/PhabricatorPasswordHasher.php @@ -126,7 +126,7 @@ $actual_hash = $this->getPasswordHash($password)->openEnvelope(); $expect_hash = $hash->openEnvelope(); - return ($actual_hash === $expect_hash); + return phutil_hashes_are_identical($actual_hash, $expect_hash); }