Problem/Motivation

Context: Wikipedia.

Proposed resolution

Use hash_hmac() instead of hash() for salted hashes.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug, because usage of hash() is not correct.
Issue priority Normal, because it is a security improvement to make getting the hash salt out of a known hash prefix easier.
Prioritized changes The main goal of this issue is security.
Disruption Not disruptive for core/contributed and custom modules/themes because it is internal to DrupalKernel only and both changed had just been introduced.
CommentFileSizeAuthor
#6 2474043-use-hmac-6.diff2.78 KBznerol
#6 interdiff.txt1.02 KBznerol
#4 2474043-use-hmac-4.diff2.78 KBznerol
#4 interdiff.txt1.57 KBznerol
#1 2474043-use-hmac.diff2.35 KBznerol

Comments

znerol’s picture

znerol commented
Status: Active » Needs review
StatusFileSize
new 2474043-use-hmac.diff2.35 KB

I left two instances unchanged:

  1. PhpassHashedPassword.php implements a proven algorithm based a gazillion rounds of hashing. That mechanism is considered to be secure.
  2. update.module / _update_manager_unique_identifier() because that would change how sites are registered on d.o. Also the resulting hash is truncated to 8 characters which helps preventing that the hash_salt can be attacked.

Status: Needs review » Needs work

The last submitted patch, 1: 2474043-use-hmac.diff, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -253,7 +253,7 @@ public static function createFromRequest(Request $request, $class_loader, $envir
-      $prefix = 'drupal.' . hash('sha256', 'drupal.' . Settings::getHashSalt());
+      $prefix = 'drupal.' . hash_hmac('sha256', 'drupal', Settings::getHashSalt());

@@ -431,7 +431,7 @@ public function boot() {
-    FileCacheFactory::setPrefix(hash('sha256', 'FileCache' . Settings::get('hash_salt')));
+    FileCacheFactory::setPrefix(hash_hmac('sha256', 'FileCache', Settings::getHashSalt()));

I think it is worth making a helper method here. To create an apc key with a namespace. Something like:

  protected function getApcKey($namespace) {
    return 'drupal.' . $namespace . hash_hmac('sha256', $namespace, Settings::get('hash_salt'));
  }
znerol’s picture

znerol commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.57 KB
new 2474043-use-hmac-4.diff2.78 KB

Status: Needs review » Needs work

The last submitted patch, 4: 2474043-use-hmac-4.diff, failed testing.

znerol’s picture

znerol commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.02 KB
new 2474043-use-hmac-6.diff2.78 KB
fabianx’s picture

fabianx commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

RTBC, looks great.

I shortly pondered the usage of hash_hmac() but then it looked like it was used for something else and with a prefix security was still okay ...

Thanks for fixing!

damien tournoud’s picture

damien tournoud commented 
+  protected static function getApcKey($cid) {
+    // @todo: This should be using Settings::getHashSalt().
+    // @see https://www.drupal.org/node/2474077
+    return 'drupal.' . $cid . hash_hmac('sha256', $cid, Settings::get('hash_salt'));
+  }

^ Why are we even doing that? There is no security issue here, we could use straight the salt for what I care.

-    return hash('sha256', $this->privateKey->get() . Settings::getHashSalt() . serialize($permissions_by_role));
+    return hash_hmac('sha256', serialize($permissions_by_role), $this->privateKey->get() . Settings::getHashSalt());

^ Using the hash salt sounds wrong here. It's either the private key, or the hash salt, it should really not be both at the same time (I realize we do the same thing in drupal_get_token(), but it is equally wrong).

As the name implied, the hash salt was never meant to be super secret. It was just meant as a way to ensure that having a dump of the database by itself is not sufficient to attack a live site.

fabianx’s picture

fabianx commented
Status: Reviewed & tested by the community » Needs review

#8: Can you help how this should work? What is secure, what is not?

And is there any documentation explaining this in some detail?

pwolanin’s picture

pwolanin commented
Issue tags: +Needs issue summary update

Probably needs a re-roll, and better issue summary

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs review » Postponed (maintainer needs more info)

Patch no longer applies, also this is tagged for an IS update.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: +Bug Smash Initiative

I reviewed this as part of Bug Smash Initiative triaging.

#2474909: Allow Simpletest to use the same APC user cache prefix so that tests can share the classmap and other cache objects made the required changes for APC caching.

But we're still using the same hash() instead of hash_hmac() in the PermissionsHashGenerator.

Question is, is this still worth doing?

@znerol thoughts?

If not, I think this can be closed as a duplicate of #2474909: Allow Simpletest to use the same APC user cache prefix so that tests can share the classmap and other cache objects

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Postponed (maintainer needs more info) » Closed (duplicate)

As part of the Bug Smash Initiative, we are triaging issues that are marked "Postponed (maintainer needs more info)". This issue was marked "Postponed (maintainer needs more info)" two years ago.

Since @larowlan requested feedback in #20 yet did not receive any, I'm marking this issue "Closed (duplicate)" per that comment. Feel free to reopen this issue and provide more information if warranted.

Thanks!

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Closed (duplicate) » Postponed (maintainer needs more info)

Leaving this one open if we can, keen to hear thoughts from znerol

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Postponed (maintainer needs more info) » Closed (duplicate)

Re closing since there hasn't been a follow up in a year.