Harden the security where hash values are compared

The same, old comparison check exists in D8's PhpassHashedPassword::check() method. I'm bumping the version. Also, if this really is a security improvement then this seems like less of a feature request and more of a task to me.

How about adding a hash_equals() function to core wrapped in a function_exists() check? That way, we can use this function in all the places as appropriate rather than having both the hash_equals() and the normal checks throughout core.

Also, not sure if it's an option but Symfony has a constant time equality implementation: https://github.com/symfony/security-core/blob/master/Util/StringUtils.ph... which uses hash_equals() if it exists.

Could we use that class without bringing the entire authentication package in?

Added issue summary and adapted the scope of this issue.

@thekevinday: Thanks for reporting this issue. If you are willing to work on it and update your patches, then please assign the issue to you.

+++ b/includes/password.inc
@@ -258,6 +258,13 @@ function user_check_password($password, $account) {
+  if (PHP_MAJOR_VERSION > 4 && PHP_MINOR_VERSION > 5) {
+    return ($hash && hash_equals($stored_hash, $hash));
+  }

Please use function_exists(). If an attacker has access to the source code or is capable of exploiting a persistent remote code execution vulnerability, then a site has much more serious issues than a function being injected here.

+++ b/core/lib/Drupal/Component/Utility/Crypt.php
@@ -126,6 +126,49 @@ public static function hashBase64($data) {
+      if (is_string($known_string)) {

!is_string?

Right.

I'm not sure I fully agree with the proposed resolution here.

clearly using === is essential in any case where the hash might start with a digit.

A constant time comparison is only important in cases where we directly compare user input to a secret like a CSRF token.

for passwords, the inclusion of a salt makes the hashed value of a plain-text password unknown to the attacker, so cases like these I don't see that constant time matters (though I guess it doesn't hurt either).

for passwords, the inclusion of a salt makes the hashed value of a plain-text password unknown to the attacker, so cases like these I don't see that constant time matters (though I guess it doesn't hurt either).

    if (substr($account->getPassword(), 0, 2) == 'U$') {
      // This may be an updated password from user_update_7000(). Such hashes
      // have 'U' added as the first character and need an extra md5() (see the
      // Drupal 7 documentation).
      $stored_hash = substr($account->getPassword(), 1);
      $password = md5($password);
    }

...

    return ($hash && $stored_hash == $hash);

U$ === Unsalted MD5 passwords. If you build a dictionary and send a bunch of guess login attempts, you can actually leak the valid MD5 hash from the timing difference in your failed authentication attempts and use this to optimize your password cracking efforts and intelligently reduce the number of online guesses.

Implementing hash_equals() prevents the leak.

Regarding the patch: I would encourage preventing mbstring.func_overload from causing strlen() to treat the input strings as Unicode and decide to not evaluate every byte.

e.g. https://github.com/symfony/symfony/pull/13984/files

@sarciszewski - re: U$ === Unsalted MD5 passwords.

No, that's not TRUE, rather the unstalted MD5 was then hashed again with a salt and stretched hash.

I'm gonna be honest here: I don't see why we're not just using password_hash() and password_verify() facilitated through password_compat for PHP 5.4.x users.

Re #18: Drupal refuses to run/install if mbstring.func_overload is enabled.
Re #20: see #1845004: Replace custom password hashing library with PHP password_hash()

Re #16:

for passwords, the inclusion of a salt makes the hashed value of a plain-text password unknown to the attacker, so cases like these I don't see that constant time matters (though I guess it doesn't hurt either).

Agreed. I'd prefer to still use a constant-time check here for consistency reasons. Also because this is what's implement inPHP 5.5 and password_compat.

rebuild.php may also have a token comparison that needs to be addressed

re-roll for conflict in core/lib/Drupal/Core/Password/PhpassHashedPassword.php

Add the comparison in rebuild.php

Do we need a test of the compatibility code?

+++ b/core/modules/user/src/AccountForm.php
@@ -129,7 +129,7 @@ public function form(array $form, FormStateInterface $form_state) {
-        $user_pass_reset = $pass_reset = isset($_SESSION['pass_reset_' . $account->id()]) && (\Drupal::request()->query->get('pass-reset-token') == $_SESSION['pass_reset_' . $account->id()]);
+        $user_pass_reset = isset($_SESSION['pass_reset_' . $account->id()]) && \Drupal::request()->query->get('pass-reset-token') === $_SESSION['pass_reset_' . $account->id()];

So this should also use the hashEquals() method?

I don't think we can do a meaningful test case for the compatibility code? You would have to do a timing test which sounds like random test fails to me. And the correctness of the comparison implementation is demonstrated by lots of places that are using it and that are covered with integration tests already.

Yes, the attack surface here is small since you'd have to get a logged-in user's account with an active token in the session to guess it from the URL, but doesn't hurt.

Moving this to a bug since using == instead of === can cause incorrect results.

re-roll for change to user_pass_rehash() in #2508627: Changing email address should invalidate one-time login links

RTBC, but major in my book.

Please do not re-test #32, lets upload a new patch instead.

simplexml_import_dom(): Invalid Nodetype to importsimplexml_import_dom(Object) Drupal\simpletest\WebTestBase->parse() Drupal\simpletest\WebTestBase->drupalPostForm('contact/foo', Array, 'Send message') Drupal\contact\Tests\ContactSitewideTest->submitContact('qDdLl6fA8qLcWgnC', 'tYdnbM2Ps81P0TGjQ6L0nl2WmnpzM8xH@example.com', 'lhJDK5LPLNFx9hjvQ7QFx4iuEOmQskOCIzIK7GVvUOaPHUqWZbhqAl46gsYkv4Vn', 'foo', '=&}vip7P .IEB+hKcVTYQn?yo6J8{F{( "C#m4yH 86MyS1IMW7mMr&Wy? 6BzmD>&AUu|D3#FLa4K3r&t8^+>|@b**Y2$3k& Cpfm*H6/+=]=q@||Jd"?CP*<3B{vV_') Drupal\contact\Tests\ContactSitewideTest->testAutoReply() Drupal\simpletest\TestBase->run(Array) simpletest_script_run_one_test('213', 'Drupal\contact\Tests\ContactStorageTest')	

was one of the failures (just for documentation purposes).

Looks like something broke on the bot - I can't reproduce that failure locally.

Re-posting the patch so people can find the sporadic fail to investigate later.

reroll

The patch won't apply because of #2217985: Replace the custom menu caching strategy in Toolbar with Core's standard caching..

$ git checkout 9024fa61
$ git apply --index 2400197-41.patch
$ git rebase 8.0.x
First, rewinding head to replay your work on top of it...
Applying: 2400197-41.patch
Using index info to reconstruct a base tree...
M	core/modules/toolbar/src/Controller/ToolbarController.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/toolbar/src/Controller/ToolbarController.php
CONFLICT (content): Merge conflict in core/modules/toolbar/src/Controller/ToolbarController.php
Failed to merge in the changes.
Patch failed at 0001 2400197-41.patch

Specifically

<<<<<<< HEAD
  public function checkSubTreeAccess($hash) {
    return AccessResult::allowedIf($this->currentUser()->hasPermission('access toolbar') && $hash == _toolbar_get_subtrees_hash()[0])->cachePerPermissions();
=======
  public function checkSubTreeAccess($hash, $langcode) {
    $expected_hash = _toolbar_get_subtrees_hash($langcode);
    return AccessResult::allowedIf($this->currentUser()->hasPermission('access toolbar') && Crypt::hashEquals($expected_hash, $hash))->cachePerPermissions();
>>>>>>> 2400197-41.patch

I think this is the right merge.

Issue #2292835: Mitigate the risk of remote timing attacks on password authentication was just closed as a duplicate of this one, but its patch included extra crypto sanitizing, using an initialization vector and urandom fallback.

I think it probably makes sense to merge that patch into this one. Here is the current patch on the other issue, for comparison.

Yes, lets incorporate that as its a good hardening related to this change.

Adapted patch on top of previous one.

+++ b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
@@ -248,10 +250,14 @@ public function check($password, $hash) {
+    // Mitigate timing attacks.
+    $nonce = Crypt::randomBytes(32);
+    return ($computed_hash && (hash_hmac($hash_type, $stored_hash, $nonce) === hash_hmac($hash_type, $computed_hash, $nonce)));

Should that not use Crypt::hashEquals?

Not too sure about that. AIUI, the purpose in both cases is to avoid timing attacks. In this approach, comparing strings hashed with a nonce cancels the meaning of any timing difference on the strings, so a simple === is sufficient; while Crypt::hashEquals avoids the timing difference by doing the comparison in near-constant time for a given known string.

So it seems combining both is useless, the level of protection against timing attacks is similar, and Crypt::hashEquals() is probably faster, especially with PHP 5.6 and above. Now, if a security expert could chime in on this...

So it seems combining both is useless

Exactly. I think Crypt::hashEquals is preferable because it fixes the problem with the least amount of effort (and also POLA).

@znerol : is it CNW just to remove this bit about the comparison, or something else ?

If it's the only reason, here is the rerolled patch.

+++ b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
@@ -241,6 +241,7 @@ public function check($password, $hash) {
+

@@ -248,10 +249,13 @@ public function check($password, $hash) {
+

nit - extra new lines, could be fixed on commit.

--

RTBC

About nits: actually, these added new lines in check() have been added per our coding standards, which specify a blank line after a break : https://www.drupal.org/coding-standards#controlstruct

Given the importance of using Crypt::hashEquals() I think we should have a CR telling people about this.

Change record added https://www.drupal.org/node/2545966

Back to RTBC - totally forgot about this one.

I'm trying a re-roll now

simple change in the use statements, patch command was able to apply everything else.

The tags, the issue summary , the title and the patch is not in harmony. If you'd be worried about hash being numeric and == being stupid then you'd simply use === which I believe is the correct fix for this issue Why we are worried about timing attacks and wasting code and time to mitigate when Drupal page never ever take the exact same time to load. Timing attacks work for simpler script but for Drupal? meh. Just use ===.

@chx there are actually at least theoretical attacks here like in the password reset link.

I would agree that a bunch of these are bogus/pointless, especially the PhpassHashedPassword one, but we should at least fix all to be ===

Back to RTBC, tweaked IS a little

If there is a downside to using hash_equals then we could consider using === in the cases where it doesn't help. However, for the sake of security researchers/evaluators reviewing Drupal's code I think it makes sense to use hash_equals wherever possible. It's harder for them to know if a case doesn't truly need hash_equals or not so they are likely to flag === as a problem even when it's not.

Needs another reroll

Reroll b/c #2571647: Create DateFormatterInterface

$ git checkout 458f5bee7bb4f2715530d56068301b5897c363b3
$ git apply --index 2400197-65.patch 
$ git rebase 8.0.x
First, rewinding head to replay your work on top of it...
Applying: 2400197-65.patch
Using index info to reconstruct a base tree...
M	core/modules/user/src/Controller/UserController.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/user/src/Controller/UserController.php

reroll #2533800: Remove all unused use statements from core. don't need credit.

especially when i mess up the reroll.

I discussed this with @xjm and hardening security with no BC break is definitely good "rc target" material, so tagging.

@greggles argument in #69 is persuasive about being consistent even if timing attacks aren't possible.

Committed 6dd4b18 and pushed to 8.0.x. Thanks!

Now I'm getting "User warning: Expected user_string to be a string, NULL given in Drupal\Component\Utility\Crypt::hashEquals() (line 159 of core/lib/Drupal/Component/Utility/Crypt.php)" on user/1/edit.

+++ b/core/modules/user/src/AccountForm.php
@@ -127,7 +128,7 @@ public function form(array $form, FormStateInterface $form_state) {
-        $user_pass_reset = $pass_reset = isset($_SESSION['pass_reset_' . $account->id()]) && (\Drupal::request()->query->get('pass-reset-token') == $_SESSION['pass_reset_' . $account->id()]);
+        $user_pass_reset = isset($_SESSION['pass_reset_' . $account->id()]) && Crypt::hashEquals($_SESSION['pass_reset_' . $account->id()], \Drupal::request()->query->get('pass-reset-token'));

Shouldn't we check that get('pass-reset-token') is not empty, before calling Crypt::hashEquals()?

@Arla what are the steps to reproduce - just going to user/1/edit does not do it for me.

This is marked for Drupal 7 backport.

(Not sure if there's anything left to do for Drupal 8 based on the last couple comments, but currently assuming not.)

I quickly looked into this again. I'm not sure what the determining conditions were at the time when I posted #84, but I just found a way to reproduce:

1. Run `drush uli`, open the returned link and click "Log in"
2. You're now on user/1/edit?pass-reset-token={hash}
3. Remove the query string (go to user/1/edit)
4. Receive warning: Expected user_string to be a string, NULL given in Drupal\Component\Utility\Crypt::hashEquals()

This really seems like an edge case, but to my understanding we should avoid such a warning in any case.

I was able to reproduce the regression in #88 and opened a new ticket, #2624986: Fix regression from #2400197, user edit form expects password reset hash, with a patch against D8.

Sorta follow-up #2822499: CsrfTokenGenerator should use timing attack safe string comparison