Problem/Motivation

Assume I change email provider or leave a job. I or an attacker requests a password reset link.

I now change the email address on my drupal account, which I've been logged into on my home computer

My former employer or someone with access to my old email account can use the pre-existing one-time login link to gain access to my account (or the whole site if I'm an admin).

Proposed resolution

Invalidate one-time login links if the email address is changed. The easiest fix is to include the current email address as part of the data in user_pass_rehash

Remaining tasks

User interface changes

n/a

API changes

possible small change to user password reset API. In Drupal 8 it looks as though it should take a full account object, not the specific values so we can make improvements later without breaking the API.

Looks like no API change needed in 7 since we are already making a database query where we can load the email string.

Data model changes

None

original report was part of the Drupal 8 bug bounty
https://tracker.bugcrowd.com/submissions/4ab3b7847cdbf5b6d2e946529324105...

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because functionality is broken as to how it "should be". (from a security perspective)
Issue priority Major because it is a valuable security hardening. Not critical because the issue also exists in Drupal 7 core and is not considered a critical security issue there.
Prioritized changes The main goal of this issue is security
Disruption Very minor disruption to any module using the user_pass_rehash() function. In my search of public 8.0.x modules, there are none. The only non-core, 7.x modules I could find that used this function were in the i18n suite so that leads me to believe that this function is rarely used outside of core.
CommentFileSizeAuthor
#33 change_email_invalidate_password_reset-2508627-33-New_Function.patch12.73 KBrvtraveller
#33 change_email_invalidate_password_reset-2508627-33-user_load.patch2.12 KBrvtraveller
#33 change_email_invalidate_password_reset-2508627-33-db_query.patch1.5 KBrvtraveller
#17 interdiff-2508627-14-17.txt1.65 KBrvtraveller
#17 change_email_invalidate_password_reset-2508627-17.patch13.85 KBrvtraveller
#14 interdiff-2508627-12-14.txt820 bytesrvtraveller
#14 change_email_invalidate_password_reset-2508627-14.patch13.85 KBrvtraveller
#12 interdiff-2508627-10-12.txt904 bytesrvtraveller
#12 change_email_invalidate_password_reset-2508627-12.patch13.83 KBrvtraveller
#10 interdiff-2508627-8-10.txt1.18 KBrvtraveller
#10 change_email_invalidate_password_reset-2508627-10.patch13.83 KBrvtraveller
#8 interdiff-2508627-6-8.txt1005 bytesrvtraveller
#8 change_email_invalidate_password_reset-2508627-8.patch13.85 KBrvtraveller
#6 change_email_invalidate_password_reset-2508627-6.patch13.35 KBrvtraveller
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented
Issue summary: View changes
Issue tags: +Needs backport to D7, +Needs backport to D6
pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented
Issue tags: +Security
pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented
Issue tags: +D8 Security Bounty, +D8 Accelerate

A less disruptive variation on this might be just to update the login time when the password is change?

However, that doesn't handle the situation where an admin changes a user's email for them (e.g. becuase their email was hacked, etc)

rvtraveller’s picture

rvtraveller CreditAttribution: rvtraveller as a volunteer commented
Assigned: Unassigned » rvtraveller

I think injecting the user's email address into the hash is the best solution here since it handles both the user changing their email address and an admin changing it on their behalf. Working on a patch.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented

I think changing the Drupal 8 method to to take the loaded account object might be the best course - in 6 and 7 we can just load the data perhaps to avoid further API changes.

rvtraveller’s picture

rvtraveller CreditAttribution: rvtraveller as a volunteer commented
Assigned: rvtraveller » Unassigned
Status: Active » Needs review
FileSize
change_email_invalidate_password_reset-2508627-6.patch13.35 KB

pwolanin and I discussed this in IRC and we both agree that passing the account object to user_pass_rehash is better than just passing in the email address as it provides future possibilities of adding other things to the password reset token. As part of this we also discovered that the phpdoc for user_cancel_url was incorrect as it talked about including certain properties on the account object (we call methods on the object). Patch includes a test to get a password reset token, change the user's email address, and then try and redeem the password reset token (which should properly give an error about being invalid). Let's see what testbot thinks.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented
Status: Needs review » Needs work

Looks like a good start. in user_cancel_url() and user_pass_rehash() the 1st param should be type-hinted on the UserInterface

rvtraveller’s picture

rvtraveller CreditAttribution: rvtraveller as a volunteer commented
Status: Needs work » Needs review
FileSize
change_email_invalidate_password_reset-2508627-8.patch13.85 KB
interdiff-2508627-6-8.txt1005 bytes

Yup, good catch.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented
Status: Needs review » Needs work

user.module already has at the top:

use Drupal\user\UserInterface;

So you don't need to spell out the full thing, just like:

function user_pass_rehash(UserInterface $account, $timestamp) {
rvtraveller’s picture

rvtraveller CreditAttribution: rvtraveller as a volunteer commented
Status: Needs work » Needs review
FileSize
change_email_invalidate_password_reset-2508627-10.patch13.83 KB
interdiff-2508627-8-10.txt1.18 KB

Switched to just be UserInterface.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented

minor:

+    $this->account->setEmail(1 . $this->account->getEmail());

Would be better to concatenate an actual string rather than relying on the cast from int.

rvtraveller’s picture

rvtraveller CreditAttribution: rvtraveller as a volunteer commented
FileSize
change_email_invalidate_password_reset-2508627-12.patch13.83 KB
interdiff-2508627-10-12.txt904 bytes

Converted our email change to be a string "1" instead of an int 1.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented

Thanks. This sting concatenation also looks a little odd in terms of code style:

+  $data = $timestamp .
+    $account->getLastLoginTime() .
+    $account->id() .
+    $account->getEmail();
+  $key = Settings::getHashSalt() .
+    $account->getPassword();

I'd either keep each one on a single line, or build $data up using the .= operator.
https://www.drupal.org/coding-standards#concat

rvtraveller’s picture

rvtraveller CreditAttribution: rvtraveller as a volunteer commented
FileSize
change_email_invalidate_password_reset-2508627-14.patch13.85 KB
interdiff-2508627-12-14.txt820 bytes

Ok. I had originally done it that way for 2 reasons -- it kept the lines under 80 characters and it would make any future changes easier to track through git diffs because we wouldn't have a huge line of concatenations to compare. With that in mind, I've adjusted it to use the .= format.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented

In don't think we have a real hard requirement to stay under 80 except for comments.
https://www.drupal.org/coding-standards#linelength

I'd probably put at least the $key concat on one line, or even inline it in the hmac call. It seems odd to break up just 2 items.

The code comment inside user_pass_rehash can probably be removed also, I don't think it's adding any useful information.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented
Status: Needs review » Needs work

Also, i think the change to the doxygen is wrong, since the login reset URL doesn't include the email address

rvtraveller’s picture

rvtraveller CreditAttribution: rvtraveller as a volunteer commented
Status: Needs work » Needs review
FileSize
change_email_invalidate_password_reset-2508627-17.patch13.85 KB
interdiff-2508627-14-17.txt1.65 KB

In don't think we have a real hard requirement to stay under 80 except for comments.
https://www.drupal.org/coding-standards#linelength

I'd probably put at least the $key concat on one line, or even inline it in the hmac call. It seems odd to break up just 2 items.

In rereading the coding standards, it looks like the goal is to keep things under 80 lines but there are exceptions to every rule and they're more like guidelines in this case. There are certainly plenty of places in Core that exceed the 80 character length. At the same time, I've seen project applications for new modules put back to needs work because an if condition or string concatenation is over 80 characters...

In this case, I can see just putting $key in the hmac call because it is unlikely to change and relatively short. I think putting the $data concatenation on separate lines (as it is in this patch) provides more value than just the 80 character guideline. If we ever change this in the future, it will be very easy to read what got added/removed because you can just look for the

-  $data .= $account->id();
+  $data .= $account->getSomeOtherProperty();

vs 

- $data = $account->getLastLoginTime() . $account->id() . $account->getEmail();
+  $data = $account->getLastLoginTime() . $account->getEmail() . $account->getSomeOtherProperty();
Also, i think the change to the doxygen is wrong, since the login reset URL doesn't include the email address

Updated to reflect what the hash contains and not what the URL contains. I also removed the sentence about retrieving values from the database...I'm guessing it was left over from 7.x

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs beta evaluation

RTBC from my side, but we might need a beta evaluation

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Also a change record.

rvtraveller’s picture

rvtraveller CreditAttribution: rvtraveller as a volunteer commented
Issue summary: View changes
Issue tags: -Needs change record

I took a first stab at creating the change record -- https://www.drupal.org/node/2515902. Feedback welcome as this is my first time writing one and I largely just tried to follow the guide (https://www.drupal.org/contributor-tasks/draft-change-record).

I also started to add a beta evaluation (leaving Needs beta evaluation tag since it isn't complete). @pwolanin -- can you help/add your thoughts to the classification of this as a bug?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Reviewed & tested by the community
Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented
Issue summary: View changes
Issue tags: -Needs beta evaluation

Thanks, tweaked it a little: Beta evaluation looks great!

  • catch committed 9907029 on 8.0.x 
    Issue #2508627 by rvtraveller, pwolanin: Changing email address should...
catch’s picture

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

Committed/pushed to 8.0.x, thanks!

Moving to 7.x for backport.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented
Version: 8.0.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)
rvtraveller’s picture

rvtraveller CreditAttribution: rvtraveller as a volunteer commented
Assigned: Unassigned » rvtraveller
pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented

For 7.x, it seems like there are a couple options.

One might be defining a new API function that core calls directly with an account object and leaving behind the old one as a wrapper that loads the email?

Another would be doing roughly the same as was done for the SA where uid was added and add email as a final, optional, argument and look it up if not supplied.

Probably worth getting feedback from David_Rothstein.

rvtraveller’s picture

rvtraveller CreditAttribution: rvtraveller as a volunteer commented
Assigned: rvtraveller » Unassigned

Unassigning while we wait for feedback from David_Rothstein

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented
Assigned: Unassigned » David_Rothstein
David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Assigned: David_Rothstein » Unassigned

Not sure I really have anything to add here.

In retrospect I guess it would have been nice if we had added the new function in that security release - would have been more future-proof since we were asking everyone to change their code at that point anyway :)

Both of the suggestions in #27 seem backwards compatible. I guess adding a new function that takes the whole account object would be more future-proof now also.

rvtraveller’s picture

rvtraveller CreditAttribution: rvtraveller as a volunteer commented
Assigned: Unassigned » rvtraveller

I think the best option here is to have the function take the whole account object so we can add other pieces of metadata about the user in the future without breaking BC.

Action items as I see it (working on these):

  1. Create a new version of user_pass_rehash() which takes the whole account object (D8 style).
  2. Update core to use this new function.
  3. Modify user_pass_rehash() to load the full user object if the uid is provided or lookup the uid and then load the full user object (to maintain the existing BC). Then call the new user_pass_rehash() with the account object.
  4. Write tests to prove the new user_pass_rehash() works as well as the BC layer (both with and without providing a UID).
David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented

Modify user_pass_rehash() to load the full user object if the uid is provided or lookup the uid and then load the full user object (to maintain the existing BC). Then call the new user_pass_rehash() with the account object.

Maybe it would be slightly better to do it the other way around (have the new function call the old one)? Then we avoid messing with the existing code path as much as possible - if you call the old user_pass_rehash(), a user_load() never needs to get triggered. It would still need to load the mail from the database though.

It probably doesn't matter a whole lot either way.

rvtraveller’s picture

rvtraveller CreditAttribution: rvtraveller as a volunteer commented
Assigned: rvtraveller » Unassigned
Status: Patch (to be ported) » Needs review
FileSize
change_email_invalidate_password_reset-2508627-33-db_query.patch1.5 KB
change_email_invalidate_password_reset-2508627-33-user_load.patch2.12 KB
change_email_invalidate_password_reset-2508627-33-New_Function.patch12.73 KB

Maybe it would be slightly better to do it the other way around (have the new function call the old one)?

I'm not sure I fully follow this. If we just create a new function to call the old function and the old function looks up the email address, what value does the new function have? At this point in the life cycle of D7 I'm not sure we will ever fully remove the old function.

I'm attaching 3 different patches with 3 different approaches to this.

  1. change_email_invalidate_password_reset-2508627-33-db_query.patch -- This patch just adds a db_query to the existing user_pass_rehash() to look up the email address. Advantages of this are that it is a fairly small change and doesn't involve a full user_load() on the user object. However, it is 1 more database query to be run on the request.
  2. change_email_invalidate_password_reset-2508627-33-user_load.patch -- This patch follows the same strategy of the db_query patch (just modify the existing function) but instead of doing a db_query() it does a user_load(). This seems like it would introduce more overhead because of the user_load(), however all of the calls in core to user_pass_rehash() already have a loaded account object and just pull the properties needed for user_pass_rehash() from that object. Therefore, the overhead of a user_load() is minimal because of the static caching.
  3. change_email_invalidate_password_reset-2508627-33-New_Function.patch -- This patch adds a new function, user_password_rehash(), which takes a full account object and pulls the properties it needs from this object. Uses of user_pass_rehash() in core have been updated to use user_password_rehash() where appropriate and user_pass_rehash() has been updated to call user_password_rehash after doing a user_load on the uid. This option seems to give us the most flexibility moving forward because of having the full account object but does result in an additional function call and I don't see us ever being in a situation where the deprecated user_pass_rehash() can be removed.

Of all of these, I like the simplicity of #1 and #2 best. If it were me, I'd probably go with #2 and count on the static caching in user_load() to make the overhead minimal.

  • catch committed 9907029 on 8.1.x 
    Issue #2508627 by rvtraveller, pwolanin: Changing email address should...

  • catch committed 9907029 on 8.3.x 
    Issue #2508627 by rvtraveller, pwolanin: Changing email address should...

  • catch committed 9907029 on 8.3.x 
    Issue #2508627 by rvtraveller, pwolanin: Changing email address should...
izmeez’s picture

izmeez CreditAttribution: izmeez commented

Any thoughts for D7 in terms of the three alternative patches offered in comment #33 ?

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented

#32:

The problem is that if we leave user_pass_rehash() unmodified, then existing contrib code would not have the added security.

What about an option 4):

- Add $mail as new (optional) parameter to user_pass_rehash()
- If mail is not set, load the account object via uid and get the mail from there as 2nd BC layer

Probably for a 7.60 release I would even be fine to add $mail as new required parameter and break BC by putting a notice ...

@David: What do you think?

  • catch committed 9907029 on 8.4.x 
    Issue #2508627 by rvtraveller, pwolanin: Changing email address should...

  • catch committed 9907029 on 8.4.x 
    Issue #2508627 by rvtraveller, pwolanin: Changing email address should...
poker10’s picture

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented
Version: 7.x-dev » 8.0.x-dev
Status: Needs review » Fixed

According to the backport policy I have created a D7 follow-up issue: #3306390: [D7] Changing email address should invalidate one-time login links, so we can finish the backport there. There are multiple options to consider and the discussion can continue there.

Therefore we can close this issue as fixed for D8.

Status: Fixed » Closed (fixed)

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