Problem/Motivation

The password reset feature was recently hardened in #2400197: Harden the security where hash values are compared but there is a reproducible regression where a password hash was stored on the user's session, and the form builder seeks to compare that value against the string expected in the GET parameter, even if it's not present. Thus, the hash-checker fails, attempting to compare a string value to NULL:

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).

Proposed resolution

Test against the GET values before proceeding with the password-reset logic.

Remaining tasks

Does a test need to be written for this? As it sits, this is not failing any text presently in core.

User interface changes

None

API changes

None

Data model changes

None

Original report by Arla

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:

    Run `drush uli`, open the returned link and click "Log in"
    You're now on user/1/edit?pass-reset-token={hash}
    Remove the query string (go to user/1/edit)
    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.
CommentFileSizeAuthor
#10 pass-reset-token-2624986-10.patch2.27 KBarla
#10 pass-reset-token-2624986-10-TEST_ONLY.patch1.18 KBarla
#7 interdiff-2624986-4-7.txt877 bytesheykarthikwithu
#7 2624986-7.patch1.09 KBheykarthikwithu
#4 reset-tolken-2624986-4.patch1.1 KBkristofferwiklund
reset-token.patch1.08 KBbradjones1

Comments

bradjones1 created an issue. See original summary.

arla’s picture

arla commented
Issue tags: +Needs tests
fgm’s picture

fgm
Primary language French
Location Paris, France
commented 
+++ b/core/modules/user/src/AccountForm.php
@@ -127,8 +127,8 @@ public function form(array $form, FormStateInterface $form_state) {
+        $user_pass_reset = isset($_SESSION['pass_reset_' . $account->id()]) && Crypt::hashEquals($_SESSION['pass_reset_' . $account->id()], $token);

Nitpicking : while you're updating this, I think it would be more readable avoid repeating the 'pass_reset_' . $account->id() concatenation + function call by using a variable.

kristofferwiklund’s picture

kristofferwiklund commented
StatusFileSize
new reset-tolken-2624986-4.patch1.1 KB

Had similar problem.

Logging in as user 1 with the link from "drush uli". Then go to /user/1/edit gives a big error message.

The above patch solves the problem. I have also update the code with regard of the comment #3.

arla’s picture

arla commented

Looks good to me. Tested manually.

The parentheses around $token = ... are not necessary but perhaps they increase legibility so ±0.

Related question: what should we expect when a user uses the password reset link but then does not immediately change the password? (which is the case at hand.)

bradjones1’s picture

bradjones1
Primary language English
commented
Version: 8.0.x-dev » 8.2.x-dev

Bumping.

heykarthikwithu’s picture

heykarthikwithu
Primary language English
Location Bengaluru 🌍
commented
StatusFileSize
new 2624986-7.patch1.09 KB
new interdiff-2624986-4-7.txt877 bytes

rerolled in 8.2.x with minor change.

arla’s picture

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

Tested manually again, works well.

alexpott’s picture

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

Is it not possible to write an automated test for this? It would seem as though it should be possible.

arla’s picture

arla commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new pass-reset-token-2624986-10-TEST_ONLY.patch1.18 KB
new pass-reset-token-2624986-10.patch2.27 KB

Adding test.

The last submitted patch, 10: pass-reset-token-2624986-10-TEST_ONLY.patch, failed testing.

alexpott’s picture

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

The test and change look good. Nice find.

alexpott’s picture

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

I've been working with the same code extensively in #2515050: A valid one-time login link may be leaked by the referer header to 3rd parties and from the work I've done there it is obvious that this change makes sense.

xjm’s picture

xjm
she/her
Primary language English
commented
Version: 8.2.x-dev » 8.1.x-dev

Since #2400197: Harden the security where hash values are compared was committed to 8.1.x I'm assuming this also applies to that branch? (Currently we are filing issues against the minimum branch based on our allowed changes policy.) At first glance this issue seems beta- and patch-release safe to me.

  • catch committed bfd7cc2 on 8.2.x 
    Issue #2624986 by Arla, heykarthikwithu, bradjones1, kristofferwiklund:...

  • catch committed 7a31523 on 8.1.x 
    Issue #2624986 by Arla, heykarthikwithu, bradjones1, kristofferwiklund:...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Status: Fixed » Closed (fixed)

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