Problem/Motivation

Whilst reviewing some contrib code it was noticed that hashes were not being compared or created correctly in several places in core.

In discussion on https://security.drupal.org/node/170071 we agreed this could be a public security hardening issue.

Proposed resolution

Use Crypt::hashEquals() to compare hashes

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#20 3057314-20.patch3.71 KBalexpott
#15 3057314-15.patch3.7 KBalexpott
#15 9-15-interdiff.txt4.32 KBalexpott
#9 3057314-9.patch7.14 KBalexpott
#9 7-9-interdiff.txt611 bytesalexpott
#7 3057314-7.patch6.54 KBalexpott
#7 2-7-interdiff.txt1.93 KBalexpott
#2 3057314-2.patch6.52 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
StatusFileSize
new 3057314-2.patch6.52 KB

alexpott credited chr.fritsch.

alexpott credited larowlan.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Crediting @larowlan for code review on the private security issue.
Crediting @chr.fitsch for private discussions about this issue.

Status: Needs review » Needs work

The last submitted patch, 2: 3057314-2.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 2-7-interdiff.txt1.93 KB
new 3057314-7.patch6.54 KB

Fix the tests.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented 
+++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
@@ -128,7 +128,7 @@ public static function processEntityAutocomplete(array &$element, FormStateInter
+    $selection_settings_key = Crypt::hmacBase64($data, Settings::getHashSalt() . \Drupal::service('private_key')->get());

should we add an empty update hook to clear render caches?

Other than that, looks good to me

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
StatusFileSize
new 7-9-interdiff.txt611 bytes
new 3057314-9.patch7.14 KB

@larowlan sure lets do that.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented

I am really in favor of this.

But I have one concern and I am not sure if this falls under the BC policy. In processEntityAutocomplete we are changing the way how the $selection_settings_key is created.
In the select2 module, I am calling EntityAutocomplete::processEntityAutocomplete($element, new FormState(), $complete_form) and I have implemented my own EntityAutocompleteController. That means the hashes will not match anymore there.
So this change will break select2 and I am not able to make the select2 module 8.7 and 8.8 compatible without implementing my own processEntityAutocomplete method.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented

Oh, I just saw that this issue is targeted for 8.7, so it would break in a patch release.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

> So this change will break select2 and I am not able to make the select2 module 8.7 and 8.8 compatible without implementing my own processEntityAutocomplete method.

You could create both variants and accept either, but yes, I agree that this is a problem.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Hmm.... how about we make this only about the hashEquals changes and then tackle adding the private key in in a different issue. That way we can concentrate on the BC implications.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new 9-15-interdiff.txt4.32 KB
new 3057314-15.patch3.7 KB

Doing #14

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Related issues: +#3058396: Use private key in hash creation in EntityAutocomplete element

I opened #3058396: Use private key in hash creation in EntityAutocomplete element for the correct salting.

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented
Status: Needs review » Reviewed & tested by the community

Thank you @alexpott for quickly changing that issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: Harden hash checking and creation in core » Harden hash checking in core
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Doesn't apply to 8.8.x anymore

error: patch failed: core/modules/image/src/Controller/ImageStyleDownloadController.php:104
error: core/modules/image/src/Controller/ImageStyleDownloadController.php: patch does not apply
make: *** [patch88] Error 1
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
StatusFileSize
new 3057314-20.patch3.71 KB

git handled the merge...

First, rewinding head to replay your work on top of it...
Applying: Do it
Using index info to reconstruct a base tree...
M	core/modules/image/src/Controller/ImageStyleDownloadController.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/image/src/Controller/ImageStyleDownloadController.php
Applying: Fix it
Using index info to reconstruct a base tree...
M	core/modules/image/src/Controller/ImageStyleDownloadController.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/image/src/Controller/ImageStyleDownloadController.php
Applying: Do it
Applying: Do it

Bring on PRs

  • larowlan committed cf939a5 on 8.8.x 
    Issue #3057314 by alexpott, chr.fritsch, larowlan: Harden hash checking...

  • larowlan committed 20ba39f on 8.7.x 
    Issue #3057314 by alexpott, chr.fritsch, larowlan: Harden hash checking...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed cf939a5 and pushed to 8.8.x. Thanks!
c/p to 8.7.x

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.