Problem/Motivation

Since we don't support PHP5 #3053363: Remove support for PHP 5 in Drupal 8.8 we no longer need to maintain \Drupal\Component\Utility\Crypt::hashEquals()

Proposed resolution

Deprecate the method for removal in Drupal 9.

Remaining tasks

Create change record
Add @deprecated to docblock
Add @trigger_error() to code
Remove all usages in core
Add legacy test to ensure we don;t break it for the remainder of Drupal 8 lifetime.

User interface changes

None

API changes

\Drupal\Component\Utility\Crypt::hashEquals() is deprecated

Data model changes

None

Release notes snippet

N/a

CommentFileSizeAuthor
#20 interdiff-3053956-18-20.txt2 KBvoleger
#20 3053956-20.patch13.71 KBvoleger
#18 3053956-18.patch13.65 KBalexpott
#18 13-18-interdiff.txt3.62 KBalexpott
#13 interdiff-11-13.txt533 bytesaleevas
#13 3053956-13.patch9.97 KBaleevas
#11 3053956-11.patch9.93 KBalexpott
#11 10-11-interdiff.txt1.89 KBalexpott
#10 Deprecate_Crypt_hashEquals-3053956-10.patch9.01 KBneeravbm
#2 Deprecate_Crypt__hashEquals-3053956-2.patch9.35 KBneeravbm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

neeravbm’s picture

neeravbm CreditAttribution: neeravbm at Red Crackle commented
FileSize
Deprecate_Crypt__hashEquals-3053956-2.patch9.35 KB

I have attached the patch for the items #2, #3 and #4, i.e.

2) Add @deprecated to docblock
3) Add @trigger_error() to code
4) Remove all usages in core

I am new to core commits. Can you explain what do I need to do for #1 and #5?

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Postponed » Needs review

For #1 there is a link in the sidebar. We may as well test this.

neeravbm’s picture

neeravbm CreditAttribution: neeravbm at Red Crackle commented

@cliefen, I didn't understand your comment about link in the sidebar. Can you please elaborate a little more?

cilefen’s picture

cilefen CreditAttribution: cilefen commented

#1 Is to add a change record (aka notice). The link to do so is in the sidebar of this and every issue.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

It's the last of the links shown after the block on the right side.

Add child issue, clone issue, add change notice

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Needs review » Needs work

@neeravbm thanks for the patch. I'm not sure how you generated the patch but unfortunately it can not be applied via using git and so the testbot and me can't test it. Perhaps the advice on https://www.drupal.org/node/707484 might help?

Also I'm sure that with this patch...

+++ core/modules/media/src/Controller/OEmbedIframeController.php	(date 1557694587000)
--- core/modules/user/src/AccountForm.php	(revision 6483dcdefbaf759c335777638741ab45b4828ba8)
+++ core/modules/user/src/AccountForm.php	(date 1557694616000)

+++ core/modules/user/src/Controller/UserController.php	(date 1557694640000)
--- core/modules/toolbar/src/Controller/ToolbarController.php	(revision 6483dcdefbaf759c335777638741ab45b4828ba8)
+++ core/modules/toolbar/src/Controller/ToolbarController.php	(date 1557694606000)

Can remove use Drupal\Component\Utility\Crypt; from these files

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

This should be postponed as hash_equals has only been around since PHP 5.6

neeravbm’s picture

neeravbm CreditAttribution: neeravbm at Red Crackle commented
Status: Needs work » Needs review

@alexpott I generated the patch using PHPStorm. I'll check with the link you mentioned and attempt to modify the patch. Will also remove unused file imports.

@cilefen and @kiamlaluno, thanks for your inputs. Here's the change record I created: https://www.drupal.org/node/3054488. This is my first change record creation so let me know if something needs to be improved.

neeravbm’s picture

neeravbm CreditAttribution: neeravbm at Red Crackle commented
FileSize
Deprecate_Crypt_hashEquals-3053956-10.patch9.01 KB

I have attached the patch creating using "git diff" instead of PHPStorm. I have removed the unused use statements as well.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Issue summary: View changes
FileSize
10-11-interdiff.txt1.89 KB
3053956-11.patch9.93 KB

Add change record and add a test that was interestingly missing - considering we had custom code for PHP5.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
@@ -249,7 +249,7 @@ public function check($password, $hash) {
 
     // Compare using hashEquals() instead of === to mitigate timing attacks.
-    return $computed_hash && Crypt::hashEquals($stored_hash, $computed_hash);
+    return $computed_hash && hash_equals($stored_hash, $computed_hash);

the comment needs to be updated.

aleevas’s picture

aleevas
he/him
Primary language Russian
CreditAttribution: aleevas at Skilld commented
Status: Needs work » Needs review
FileSize
3053956-13.patch9.97 KB
interdiff-11-13.txt533 bytes

Hello!
@berdir I updated the comment.
Hope I understood you right.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Yes, exactly like that.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

It'd be great to get #3057314: Harden hash checking in core in first.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 3053956-13.patch, failed testing. View results

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Issue tags: +Needs reroll

Needs re-roll after #3057314: Harden hash checking in core

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13-18-interdiff.txt3.62 KB
3053956-18.patch13.65 KB

Updated the patch to convert the new instances of Crypt::hashEquals()

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
CreditAttribution: voleger as a volunteer and at GOLEMS GABB, Drupal Ukraine Community for Drupal Ukraine Community, GOLEMS GABB commented
FileSize
3053956-20.patch13.71 KB
interdiff-3053956-18-20.txt2 KB

Fixed CS issues with a deprecation message.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Fixed this on commit:

FILE: ...h/www/8.x/core/lib/Drupal/Core/Password/PhpassHashedPassword.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 5 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Committed 17153a2 and pushed to 8.8.x. Thanks!

  • catch committed 17153a2 on 8.8.x 
    Issue #3053956 by alexpott, neeravbm, aleevas, voleger, Berdir:...

Status: Fixed » Closed (fixed)

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

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented

Published CR.