Problem/Motivation

When accessing a one-time login link the referrer header for any third party assets will leak the link and if the user does not click the log in link it will be valid for the next 24 hours.

example STR:

Copied from bugcrowd. Approved by Drupal Security Team for public disclosure.
https://tracker.bugcrowd.com/submissions/6db83d94b58bee90091400154d37119...

You can see this vulnerability by:
1. Go to drupal.org in a logged-out browser
2. Request a password reset link
3. open developer tools in the browser
4. paste the password reset link into the browser
5. examine the request headers on the page assets before clicking "login"

I can reproduce this locally on Drupal 8 by embedding an image from the DA on my local site in a block.

Proposed resolution

When the user accesses a one-time login page, store the tokens in the session and redirect to a a bare URL with no tokens before rendering any HTML

Remaining tasks

After exploring several strategies the current one is this:

  • When a user clicks on a one time login link in an email the first page stores the hash and timestamp in session and redirects to the form with a log in button.
  • The URL does not have the hash and timestamp therefore fixing the referrer issue.
  • The hash and timestamp are also removed from session to prevent disclosure.
  • The form's action URL links back to the one time login link but with /login appended.
  • If everything is valid the user will be logged in.

The optional action parameter is present in Drupal 7 but it was incorrectly removed in the conversion of the form to the routing system.

User interface changes

minor change in one-time login behavior

API changes

small change in behavior of one-time login links

Data model changes

none

CommentFileSizeAuthor
#97 2515050-3-97.patch21.75 KBalexpott
#97 96-97-interdiff.txt1.42 KBalexpott
#96 interdiff.2515050.91.96.txt1.64 KBYesCT
#96 2515050-96.patch22.27 KBYesCT
#91 2515050-2-91.patch22.27 KBalexpott
#91 2515050-2-91-test-only.patch22.27 KBalexpott
#91 89-91-interdiff.txt2.02 KBalexpott
#89 2515050-2-89.patch22.24 KBalexpott
#89 86-89-interdiff.txt1.86 KBalexpott
#86 2515050-2-86.patch21.3 KBalexpott
#86 83-86-interdiff.txt1.88 KBalexpott
#83 2515050-2-83.patch21.9 KBalexpott
#83 81-83-interdiff.txt5.64 KBalexpott
#81 2515050-2-81.patch20.41 KBalexpott
#70 increment.txt2.68 KBpwolanin
#70 2515050-70.patch20.32 KBpwolanin
#68 increment.txt18.18 KBpwolanin
#68 2515050-68.patch20.37 KBpwolanin
#58 2515050-2-58.patch15.96 KBalexpott
#58 55-58-interdiff.txt1.65 KBalexpott
#55 2515050-2-55.patch15.96 KBalexpott
#55 50-55-interdiff.txt1.6 KBalexpott
#50 2515050-2-50.patch15.88 KBalexpott
#50 40-50-interdiff.txt13.51 KBalexpott
#41 2515050-41-D7.patch3.82 KBDavid_Rothstein
#40 2515050-2-40.patch6.47 KBalexpott
#35 2515050-35.patch20.02 KBalexpott
#30 2515050-30.patch16.82 KBalexpott
#30 28-30-interdiff.txt921 bytesalexpott
#28 2515050-28.patch16.81 KBalexpott
#28 26-28-interdiff.txt1.09 KBalexpott
#26 2515050-26.patch16.61 KBalexpott
#26 21-26-interdiff.txt2.22 KBalexpott
#21 2515050-21.patch14.38 KBalexpott
#21 17-21-interdiff.txt4.85 KBalexpott
#18 Screen Shot 2016-04-01 at 14.03.28.png29.27 KBalexpott
#18 Screen Shot 2016-04-01 at 14.03.11.png58.17 KBalexpott
#17 2515050-17.patch12.57 KBalexpott
#17 14-17-interdiff.txt2.59 KBalexpott
#14 2515050-14.patch12.65 KBalexpott
Screen Shot 2015-06-25 at 10.46.23 AM.png417.77 KBpwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin’s picture

Issue tags: +Needs beta evaluation
prasad_gogate’s picture

Looking into this issue

prasad_gogate’s picture

Issue tags: +drupalconasia2016

Issue is replicate.
Steps to replicate the issue.
1. Install Drupal 8 - 8.1.x branch
2. Login with admin user - check if the site works correctly.
3. Logout from the site.
4. go to user/login link
5. Select Reset your password tab.
6. enter the valid email id
7. Copy the user rest link to a new browser tab (while you are still logged into the site with same user)
8. User is again navigated to the site though he is logged in with reset password link.

prasad_gogate’s picture

Issue tags: +Triaged for D8 major current state
prasad_gogate’s picture

sunil-kumar’s picture

Assigned: Unassigned » sunil-kumar
xjm’s picture

(Updating issue credit for the major triage.)

pwolanin’s picture

I'll try to whip up a patch for this soon

mlhess’s picture

Priority: Major » Critical
Issue tags: -Security improvements +Security

This seems like a critical issue. After chatting with some maintainers I am promoting it.

The security team is ok with calling it a harding and it can stay public.

alexpott’s picture

Issue tags: +Triaged D8 critical

Discussed with @catch, @xjm, @effulgentsia and @cottser and we agree that this is a critical security hardening issue.

alexpott’s picture

Issue summary: View changes

So I think the steps to reproduce in #5 are wrong.

I think they are as detailed in the summary:

You can see this vulnerability by:
1. Go to drupal.org in a logged-out browser
2. Request a password reset link
3. open developer tools in the browser
4. paste the password reset link into the browser
5. examine the request headers on the page assets before clicking "login"

The part of the issue summary that says

When accessing a one-time login link from email, the link does not become invalid if the user is already logged in.

Under testing a proved to no longer be an issue. To test locally:

  1. Log in as a user in a browser
  2. Generate a one time login link using Drush drush uli --no-browser
  3. Go to that link in another tab
  4. Go back to the tab which was logged in and refresh - you'll be logged out.

This was fixed in Drupal 8 in #889772: Following a password reset link while logged in leaves users unable to change their password.

The part of the issue summary that said

Note - drupal.org has already been reconfigured to mitigate this problem.

Is also incorrect - I've tested this myself and the referrer header for assets on the one time login link is present and until the user clicks on the log in button the link is valid.

alexpott’s picture

Status: Active » Needs review
Issue tags: -Needs beta evaluation
FileSize
12.65 KB

So I've spent some time thinking about this a playing about with code. After trying to make first visit to the url or the first build of Drupal\user\Form\UserPasswordResetForm expire the link I've worked out that this is not really possible. The conclusion I've come is that the form is actually pointless - we should always redirect from UserController::resetPass(). If we do that we'll never serve a page that has the one time login link in the referrer header. Also user's have one less thing to click.

I think in terms of usability we should actually go further and not take user to the user edit page but a form that only has a field to change the current password as that is always the intention when clicking on these links and the user edit page can get pretty crowded - but we can do that in a followup as this patch does not change where the user actually has to change their password.

alexpott’s picture

Lol so I should have read the issue summary properly...

When the user accesses a one-time login page, store the tokens in the session and redirect to a a bare URL with no tokens before rendering any HTML

That would work as well - but we could take opportunity to allow the user to click less.

bojanz’s picture

The approach outlined in #14 makes sense. The redirect removes an unnecessary step, and fixes the issue.

Minor code comments:

+    // No time out for first time login.
+    if ($user->getLastLoginTime() && $current - $timestamp > $timeout) {

I don't understand this comment. We are checking the timeout in the block below.

  protected function redirectToPasswordReset(UserInterface $user, $timestamp) {
+    drupal_set_message($this->t('You have just used your one-time login link. It is no longer necessary to use this link to log in. Please change your password.'));
+    $this->logger->notice('User %name used one-time login link at time %timestamp.', array('%name' => $user->getUsername(), '%timestamp' => $timestamp));
+    $token = Crypt::randomBytesBase64(55);
+    $_SESSION['pass_reset_' . $user->id()] = $token;
+    return $this->redirect(
+      'entity.user.edit_form',
+      ['user' => $user->id()],
+      [
+        'query' => array('pass-reset-token' => $token),
+        'absolute' => TRUE,
+      ]
+    );
   }

We're adding new code that's using array(), and mixing [] with array(). We should unify that.

alexpott’s picture

Thanks for the review @bojanz

I don't understand this comment.

Me neither. But it is existing code that just got re-indented but of the change to how blocked or non-existing users is handled. But ah... I get it. Let's improve it here so the method becomes easy to maintain.

Fixed mix of array styles in the new method.

alexpott’s picture

So the current approach completely removes this step:

Now when the user uses a valid one time login link they just get the existing message on their user edit page.

The last submitted patch, 14: 2515050-14.patch, failed testing.

dawehner’s picture

Just a quick driveby review

  1. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -89,23 +102,29 @@ public static function create(ContainerInterface $container) {
    +        $user->setLastLoginTime(REQUEST_TIME);
    +        $this->userStorage->updateLastLoginTimestamp($user);
    

    This optimization is just weird, but well, this is our API.

  2. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -114,28 +133,44 @@ public function resetPass($uid, $timestamp, $hash) {
    +    $_SESSION['pass_reset_' . $user->id()] = $token;
    

    Is this really still the right way to manipulate the session. Isn't it meant to be used $request->session?

  3. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -114,28 +133,44 @@ public function resetPass($uid, $timestamp, $hash) {
    +    return $this->redirect(
    ...
    +        'absolute' => TRUE,
    

    It is weird that the caller to $this->redirect() has to specify the absolute URL, is this really needed?

alexpott’s picture

Thanks @dawehner

  1. Yeah this is just copied from user_login_finalize()
  2. Well this is just moved code from Drupal\user\Form\UserPasswordForm - but sure let's just pass around the request.
  3. You are right it is added in the redirect() method

The last submitted patch, 17: 2515050-17.patch, failed testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Yeah this is just copied from user_login_finalize()

Sure, but you agree that its odd.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2515050-21.patch, failed testing.

David_Rothstein’s picture

The confirmation form was added in #24398: password reset is not working in some cases. I'm not sure if those problems are still valid, but I would be wary of just removing it like that in a stable release - especially because this would be a big change to the password reset UI.

What's wrong with the suggestion in the issue summary (store the token in the session and redirect to a URL that replaces the token with a non-token)? I think it would be good to remove the token from the session as soon as the redirect finishes though (and put it in a hidden form field or the form action instead so it can be verified on submit); otherwise there would be a small security regression in the case where the user abandons the password reset process on a public computer.

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.22 KB
16.61 KB

@David_Rothstein the solution actually deals with that - if you are already logged in it just makes the link invalid and deposits you on the account edit page with the correct message - it doesn't check the hash or timestamp or anything. If we'd fixed #24398: password reset is not working in some cases like this then we never would have had #889772: Following a password reset link while logged in leaves users unable to change their password which actually #24398: password reset is not working in some cases introduced.

Also, redirecting to a new URL with new session variables to hold the data is a big change to underlying API. In fact, I think appearing to be logged in straight away is a security enhancement as a the one time login page you don't look logged in but in reality you are just one password less click away from being logged in - so there is no difference. However the only way for the user to prevent a back button being able to reuse this page is to press the button. Again with the current change this is no longer an issue.

Status: Needs review » Needs work

The last submitted patch, 26: 2515050-26.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
16.81 KB

Status: Needs review » Needs work

The last submitted patch, 28: 2515050-28.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
921 bytes
16.82 KB

Ah so KernelTestBase test's requests don't have a session - nice.

David_Rothstein’s picture

@David_Rothstein the solution actually deals with that - if you are already logged in it just makes the link invalid and deposits you on the account edit page with the correct message - it doesn't check the hash or timestamp or anything.

I don't think that deals with the scenario described in #24398: password reset is not working in some cases. My understanding of that issue is that you get "logged in" via the prefetcher, but that's not the same as being logged in via your web browser - the two would not be using the same session. (I assume that's why the issue was reported as "All I see is an access denied message" rather than "I see a message saying I was already logged in".)

Also, redirecting to a new URL with new session variables to hold the data is a big change to underlying API.

I don't see why it would require an API change. The redirect would be to a different URL but the same router path (e.g. the same URL but with the actual token replaced with an invalid token, such as the word "confirm") and thus all the logic for this could basically be internal to UserController::resetPass() / user_pass_reset().

It should be much less of a change than the current patch.

In fact, I think appearing to be logged in straight away is a security enhancement as a the one time login page you don't look logged in but in reality you are just one password less click away from being logged in - so there is no difference. However the only way for the user to prevent a back button being able to reuse this page is to press the button.

Yeah, I agree that the current situation is not ideal in this regard; frankly, in terms of practical effects I think it's a bigger potential problem than the original issue reported here. (Especially because it's not just the back button that's a problem; it's also the browser history which is still typically there even after the browser is closed.)

However I think my suggested solution would solve this too; because of the redirect, the URL with the token in it should not wind up in the browser history. The back button might still be a problem (if it shows you a cached version of the confirm form) but that could probably be addressed by adding a "no-store" HTTP header to the page.

David_Rothstein’s picture

For what it's worth, I'm a little skeptical that this should be classified as a critical bug. It's a security improvement, but to run into this your site basically has to be hotlinking images served from a site that you don't trust (or at least hotlinking images from a site that gets hacked)? In that scenario you'd have a mess on your hands either way. The vulnerability described here does make your problems theoretically worse than they otherwise would be, but still... seems kind of far-fetched.

alexpott’s picture

@David_Rothstein hmmm yep- you're right about the pre-fetch different session issue. I guess we'll have to do the solution in the summary.

Cameron Tod’s picture

Re #32 - wouldn't the referrer also be passed to any other elements on the page - not just images? In which case its conceivable that advertising networks, content aggregators, public CDNs, or other services which incorporate data from many other downstream sources could be seeing these tokens. I feel like that would constitute a critical security risk.

alexpott’s picture

Here's a patch that implements the solution suggested by the summary. No interdiff because it is a new approach.

znerol’s picture

Ah so KernelTestBase test's requests don't have a session - nice.

Sorry for that: #2484991: Add the session to the request in KernelTestBase, BrowserTestBase, and drush

alexpott’s picture

So looking at https://onetimesecret.com (a site whose who business is to make one time links) they also take the user to a page to click a button.

One of the advantages of the automatic login approach is less clicks for users. In Drupal 7 you can add /login to a one-time link to do this. We removed this in #1998198: Convert user_pass_reset to a new-style Form object. Given that issue was about converting the one time link to the new routing system we were wrong to do that there. The reason given was:

Note also that this patch changes the behavior in a subtle way. Previously, you could make a GET request to the URL with /login appended and get logged in.

Now, it actually must run through the whole form submit code path. I think this is a big improvement, since now modules could add additional validate or submit steps if desired.

( #1998198-49: Convert user_pass_reset to a new-style Form object)

I'm not sure I agree with the reason given but I committed the patch so ultimately I'm responsible for the loss of functionality.

Also note that the current approach reverts some of the other supposed benefits of that conversion...

The form builder cannot return redirects, so we need to wrap this in a normal route controller and then render the form within that. Makes for a much cleaner separation of logic anyhow, rather than having all that crap in the form (that code was carried over from ancient Drupal versions).

( #1998198-45: Convert user_pass_reset to a new-style Form object)

Given that the form is now on a different route and is referenced directly in the controller.

Cameron Tod’s picture

Anecdotally, I've had users confused by the one-time login link form behaviour. They were not clear that changing their password was required or useful, and assumed that the one-time login was just that - it didn't imply that they could then login again at any time with their newly updated password.

If I could design an interaction from scratch, I would redirect to/land on a form that only allowed for password change, but not require the user to click an intermediary 'Login' button as they do at the moment - as mentioned in #14.

alexpott’s picture

@Cameron Tod the problem is that landing on a one time use page immediately doesn't work on the internet :( But I agree with the assessment that after clicking on the link the user should be taken to a page whose only purpose is to change the password. OTOH we should make it easier to change this because on use of the drush uli command is to share a user without ever having to know the password according to @moshe.

alexpott’s picture

Whilst writing #37 I realised that actually we can make a smaller change (and completely BC) - which will result in the same effect as the last patch.

No interdiff cause yet another approach.

David_Rothstein’s picture

That seems closer although still a bit more change than I was expecting (new router paths, etc).

Also it looks like it doesn't remove the token from the session until after the form is submitted - based on my comment above I think we'd want to do it earlier than that so we don't introduce a new vector when a user clears their browser history (but not their cookies) on a public computer.

By comparison, attached is the patch I sort of had in mind for Drupal 7 (without the tests). I include the "no-store" header I mentioned above, which is tangentially related to this issue. I tested this manually and verified:
- The token is never in the browser history
- Revisiting an unused password reset URL in the browser history won't let you use it (but clicking again from email does)
- Using the back button to go back to an unused password reset URL won't let you use it either

David_Rothstein’s picture

Re #32 - wouldn't the referrer also be passed to any other elements on the page - not just images? In which case its conceivable that advertising networks, content aggregators, public CDNs, or other services which incorporate data from many other downstream sources could be seeing these tokens.

I guess, but for many of those (e.g. CDNs) you have to trust them anyway if you're going to put your content on there. However I suppose if you're just making an Ajax request to some other site the referrer header gets sent along with that too. Didn't think of that before and I would agree it's a bit worse, since you don't normally have to trust services like that quite as much. (In general, though, the reason it's a 24 hour token in the first place is kind of based on the expectation that it could leak out to places it's not supposed to be...)

Status: Needs review » Needs work

The last submitted patch, 41: 2515050-41-D7.patch, failed testing.

David_Rothstein’s picture

Status: Needs work » Needs review

Probably just needs an adjustment to the test - anyway, back to "needs review".

catch’s picture

Should we also consider making the link valid for a shorter period, say 6 hours? If it expires, you can just request another link anyway. Also should we make that configurable via settings so that sites can vary it up or down themselves?

alexpott’s picture

@David_Rothstein I tried to do the unsetting of the session in the form building or the route controller after we've got the values from the session. But this doesn't work because the when you post the form it will call the route controller and not find the expected session variables.

timmillwood’s picture

Version: 8.0.x-dev » 8.1.x-dev
David_Rothstein’s picture

Should we also consider making the link valid for a shorter period, say 6 hours? If it expires, you can just request another link anyway. Also should we make that configurable via settings so that sites can vary it up or down themselves?

24 hours seems a bit excessive to me also. But changing this should probably be in another issue (if there isn't one already?).

@David_Rothstein I tried to do the unsetting of the session in the form building or the route controller after we've got the values from the session. But this doesn't work because the when you post the form it will call the route controller and not find the expected session variables.

In the Drupal 7 patch this works because $form['#action'] is set explicitly (and has the token in it). So on submit it finds the token in the URL rather than the session. This was helped by the fact that the existing Drupal 7 code already sets $form['#action'], so I didn't even have to do much to get that part to work :)

alexpott’s picture

@David_Rothstein it also works because D7 still has the /login ability but that was erroneously removed by #1998198: Convert user_pass_reset to a new-style Form object :( I've got nothing I can set the form action to - unless I reimplement that logic. Which @moshe would like for drush anyways... ho hum.

alexpott’s picture

Patch attached re-introduces the /login ability and unsets the session parameters after using them. The solution is now extremely similar to @David_Rothstein's in #41. It's much bigger patch because we have to re-implement all the logic.

alexpott’s picture

Assigned: sunil-kumar » Unassigned
alexpott’s picture

Issue summary: View changes
alexpott’s picture

Issue summary: View changes
catch’s picture

Just nits. This looks good to me otherwise.

  1. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -61,19 +73,96 @@ public static function create(ContainerInterface $container) {
    +   *   UID of user requesting reset.
    

    User ID.

  2. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -81,15 +170,72 @@ public static function create(ContainerInterface $container) {
    +   * @param $uid
    +   *   UID of user requesting reset.
    +   * @param $timestamp
    +   *   The current timestamp.
    +   * @param $hash
    +   *   Login link hash.
    +   * @param $timeout
    +   *   Time out, in seconds, until login URL expires.
    

    Missing param types.

alexpott’s picture

Thanks @catch

catch’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

thpoul’s picture

Status: Reviewed & tested by the community » Needs work

Thank you alexpott patch looks great! Here are some more nits.

  1. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -61,19 +73,96 @@ public static function create(ContainerInterface $container) {
    +        $this->logger->notice('User %name used one-time login link at time %timestamp.', array('%name' => $user->getDisplayName(), '%timestamp' => $timestamp));
    

    array() should be []

  2. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -61,19 +73,96 @@ public static function create(ContainerInterface $container) {
    +        // Let the user's password be changed without the current password check.
    

    80 chars rule

  3. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -61,19 +73,96 @@ public static function create(ContainerInterface $container) {
    +          array('user' => $user->id()),
    +          array(
    +            'query' => array('pass-reset-token' => $token),
    +            'absolute' => TRUE,
    +          )
    

    array() should be []

  4. +++ b/core/modules/user/src/Tests/UserPasswordResetTest.php
    @@ -176,6 +185,16 @@ function testUserPasswordReset() {
    +    $edit = array('name' => $this->account->getUsername());
    

    array() should be []

  5. +++ b/core/modules/user/src/Tests/UserPasswordResetTest.php
    @@ -176,6 +185,16 @@ function testUserPasswordReset() {
    +    $this->assertTitle(t('@name | @site', array('@name' => $this->account->getUsername(), '@site' => $this->config('system.site')->get('name'))), 'Logged in using password reset link.');
    

    s/@/%
    array() should be []

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.65 KB
15.96 KB

1 and 3. Whilst there is no standard (yet) as to which array syntax to use I've fixed it here to make the method consistent.
2. Fixed.
4, 5 Here the test is full of array() changing is just not in scope. The @ should not be replaced with % - if it was the test would fail.

Given the nit level of the review in #57 moving back to rtbc. @thpoul for these types of review people often leave the patch at rtbc because then the committer can fix on commit.

mlhess’s picture

This looks good.

dawehner’s picture

This looks good for me. I like that this enables us to use the nicer UX as well.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 58: 2515050-2-58.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
thpoul’s picture

@alexpott thank you for taking the time to explain! I wasn't sure if I did right or wrong. I'm sorry for all the noise.

xjm’s picture

Issue tags: -Triaged for D8 major current state

(Removing the major triage tag since we confirmed it was a critical.)

pwolanin’s picture

Tested locally. Overall I think this is a good approach, but a few very minor questions. In particular, I think the diff could be smaller if we handled all 3 sets of route parameters (slugs) in one route with defaults to make them optional, but that would be re-writing the patch.

  1. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -61,19 +73,97 @@ public static function create(ContainerInterface $container) {
    +        $_SESSION['pass_reset_' . $user->id()] = $token;
    

    This is just moved code, but should it be fixed to use the session object? If not in this, then a trivial follow-up.

  2. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -81,15 +171,72 @@ public static function create(ContainerInterface $container) {
    +      return $this->formBuilder()->getForm('Drupal\user\Form\UserPasswordResetForm', $user, $expiration_date, $timestamp, $hash);
    

    Do we always use class name now instead of form ID? That actually seems a little more fragile.

  3. +++ b/core/modules/user/src/Form/UserPasswordResetForm.php
    @@ -75,20 +47,18 @@ public function buildForm(array $form, FormStateInterface $form_state, AccountIn
    +    $form['#action'] = Url::fromRoute('user.reset', [
    +      'uid' => $user->id(),
    +      'timestamp' => $timestamp,
    +      'hash' => $hash,
    +      'action' => 'login'
    +    ])->toString();
    

    The formatting of this seems a bit odd - is this actually what our code style specifies?

  4. +++ b/core/modules/user/user.routing.yml
    @@ -141,9 +141,21 @@ user.cancel_confirm:
    +  path: '/user/reset/{uid}'
    

    This could probably have all been handled in the same (original) route by using empty defaults for the timestamp and hash slugs.

pwolanin’s picture

There also seems to be a possible bug in the logic - if I go to the login reset page while logged in, the tokens are set in my session. They may get persisted if the redirect is blocked.

It would seem to be better if we never set the values into the session if the user is already authenticated?

pwolanin’s picture

Status: Reviewed & tested by the community » Needs work

Let me try to refactor that - the logic flow right now is also very convoluted, which I think is the sort of thing that leads to security bugs

pwolanin’s picture

Status: Needs work » Needs review
FileSize
20.37 KB
18.18 KB

gah, that took much longer than I expected, but I'm happier with the overall code flow.

Also fixed the $_SESSION usage, though that could be moved to a separate patch if needed.

There is a minor behavior change - previously in some cases you got a redirect + message for an invalid or blocked uid and a 403 in other cases. It's now always the former as the 1st check in the controller.

The controller also always removes the data from the session, so the data can only be present during the one redirect for an anonymous user redirected to the confirm form.

pwolanin’s picture

oops, that was rolled against 8.2.x, but let's see if it applies.

pwolanin’s picture

Minor cleanup. I think we want to log the account name, not the display name, so we know accurately what account logged in.

catch’s picture

#68 looks good, but we still use $_SESSION in dozens of places in core, is it OK to make that change piecemeal? I'd be happier doing that in a spin-off issue rather than as part of fixing the critical.

pwolanin’s picture

@catch - that specific piece of session data is only used in this password reset flow, so I don't think there is any side effect from changing it here.

I don't feel that strongly, but seems a bit odd to be using the session object in one line, and a few lines away to be using $_SESSION for a related task.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/src/Controller/UserController.php
@@ -81,56 +120,143 @@ public static function create(ContainerInterface $container) {
+    if ($redirect = $this->redirectInvalidTimestampHash($reset_link_user, $timestamp, $hash, $timeout)) {
...
+  protected function redirectInvalidTimestampHash(UserInterface $reset_link_user, $timestamp, $hash, $timeout) {
...
+      return FALSE;

I don't think we should have this extra method then. Because it is confusing - returns FALSE to continue - and now it is only called once.

alexpott’s picture

+++ b/core/modules/user/src/AccountForm.php
@@ -124,7 +124,8 @@ public function form(array $form, FormStateInterface $form_state) {
-        $user_pass_reset = isset($_SESSION[$session_key]) && Crypt::hashEquals($_SESSION[$session_key], $token);
+        $session = $this->getRequest()->getSession();
+        $user_pass_reset = Crypt::hashEquals((string) $session->get($session_key), $token);

@@ -387,9 +388,8 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
-    if (isset($_SESSION['pass_reset_' . $user->id()])) {
-      unset($_SESSION['pass_reset_' . $user->id()]);
-    }
+    $session = $this->getRequest()->getSession();
+    $session->remove('pass_reset_' . $user->id());

I changed this too in previous patches but decided it is out of scope and possibly a BC break. IMO this deserves a separate issue to discuss and is not part of solving this bug.

alexpott’s picture

Also I disagree that having the form and the actions on the same route makes things easier. Redirecting to the same route and having it do a multitude of actions is convoluted as there are more paths through the same code. What I like about #58 is that it is clear what does what. And to claim that the logic in #68 is simpler feels really wrong. The same piece of code being used to do the redirects or present the form makes it very difficult to work out what is going on. #66 was fixable without rewriting the entire patch to use the same route for everything.

alexpott’s picture

Also I'm trying to check the claims of #66... What does

if I go to the login reset page while logged in, the tokens are set in my session. They may get persisted if the redirect is blocked.

Actually mean? In either approach if the redirect after the setting the session variables fails, the session will not be cleared. Neither approach actually mitigates that. And how does the redirect get blocked in #58 more than it could be in #70?

catch’s picture

Yeah I'm also not sure this makes things simpler. It removes a method, but all that logic gets added to another method making it longer.

pwolanin’s picture

The one method is a bit longer, but the flow of logic is much more linear.

The current patch better protects against the tokens staying in the session since it always removes them if you land on this route. The earlier patch had flows where they might not be removed.

I don't feel that strongly about 1 vs 2 routes, but David_Rothstein's comment made me think it might be better to keep it in one.

pwolanin’s picture

Discussing w/ alexpott in IRC. One alternative would that would allow for a pretty clear separation to to have 3 distinct routes and not check the validity of the parameters during the 1st two. route 1 - sets data in session + redirects, route 2 presents form w/ action, route 3 checks values and logs in.

I think this could be a good way forward to defining a clear code flow and separation of duties as long as is it no a UX regression if I have to click the login button on the form before getting the error message.

alexpott’s picture

@pwolanin that seems like a good separation of responsibilities... I think the new form and login routes should only be available for logged out users and therefore we only need to change logged-in ness on the first route.

alexpott’s picture

Status: Needs work » Needs review
FileSize
20.41 KB

Okay I've implemented what @pwolanin and I discussed. There's no interdiff as I don;t think reviewing that is particularly helpful as the flow of the code has changed. The patch has three routes responsible for the user password reset process:

  • user.reset - handles the link the user clicks on - just validates the user and redirects to the form
  • user.reset.form - no validation apart from the dependencies of the form - redirects to user.reset.login when the user clicks on the button
  • user.reset.login - validates the hash, timestamp, user and if all the info is correct, logs the user in and redirects to the user edit form

This gives us a really nice separation of concerns and I've managed to remove some deep nesting and elses.

From #78

The current patch better protects against the tokens staying in the session since it always removes them if you land on this route. The earlier patch had flows where they might not be removed.

I still feel this statement is completely unsubstantiated - and given the logic to do with setting the hash and timestamp in session in #70 and this patch is not really any different to #58 I would love to know what this flows are.

catch’s picture

    New approach looks reasonable. The following is nitpicky but it stuck out on a first read-through.

  1. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -61,76 +74,190 @@ public static function create(ContainerInterface $container) {
    +   *
    +   * In order to never disclose a redirect link via a referrer header this
    +   * controller must always return a redirect response.
    +   *
    

    Should this not be 'reset link'?

    Also the comment says this must always return a redirect response, except:

  2. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -61,76 +74,190 @@ public static function create(ContainerInterface $container) {
    +    if ($reset_link_user === NULL || !$reset_link_user->isActive()) {
    +      // Blocked or invalid user ID, so deny access. The parameters will be in the
    +      // watchdog's URL for the administrator to check.
    +      throw new AccessDeniedHttpException();
    +    }
    +
    

    /here it throws an AccessDeniedHttpException - it's not a return, but it's only not a return due to weirdness. This is fine, but technically it exposes the invalid link via referrer header still, so might be worth an explicit comment explaining why that's OK. i.e. "In order to never disclose a valid password reset link via a referrer header." above then "Since the link is not valid, it doesn't matter if we disclose it"

+++ b/core/modules/user/src/Controller/UserController.php
@@ -61,76 +74,190 @@ public static function create(ContainerInterface $container) {
+
+    if (!$hash || !$timestamp || $user === NULL || !$user->isActive()) {
+      throw new AccessDeniedHttpException();
+    }
+

Minor, but we could skip trying to load the user at all if $hash or $timestamp are empty.

alexpott’s picture

Thanks for the review @catch. Looking at 1 & 2 I think there is valid in making UserController::resetPassRedirect() always return a redirect and never do the access denied thing. So reworked the code to do that. Patch also only loads the users if we have a hash and timestamp.

pwolanin’s picture

+++ b/core/modules/user/src/Controller/UserController.php
@@ -61,41 +74,73 @@ public static function create(ContainerInterface $container) {
+   * @deprecated in Drupal 8.x, will be removed before Drupal 9.0.
+   *   Use \Drupal\user\UserController::resetPassRedirect().
+   */
+  public function resetPass($uid, $timestamp, $hash) {

Given that this is replaced by a redirect implementation, why not simply continue to use this method for the user.reset route instead of having 2 methods one of which is unused?

alexpott’s picture

@pwolanin good spot. Do you have any answers for #76?

alexpott’s picture

Made change suggested by @pwolanin in #84 - whilst we lose a tiny bit of self-description on the method name we have less change and that is probably a good trade-off here.

pwolanin’s picture

Re: #76. The flow is quite different now and the session variables set at the very end of the method, so maybe no longer a concern.

You might still remove the session variables again in the final method, though I'm not sure that matters.

+++ b/core/modules/user/user.routing.yml
@@ -140,6 +140,17 @@ user.cancel_confirm:
+  path: '/user/reset/{uid}/{timestamp}/{hash}/login'

For this route, since we have to load the user at the top of the method, why not change the {uid} to {user} and use the paramconverter to get UserInterface $user as the method param?

alexpott’s picture

@pwolanin doing that will result in a different behaviour on the user.reset.login route from the other two routes... they both do access denied if the user does not exist or is blocked. This would change that route to do a 404 if the user does not exist.

alexpott’s picture

Here's tests of the current behaviour.

pwolanin’s picture

Status: Needs review » Needs work

@alexpott - yes, that's true, but it didn't really bother me. Anyhow, not that important, just a thought.

Some more nits, and I think at least one real bug suggesting missing test coverage.

  1. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -104,33 +132,117 @@ public function resetPass($uid, $timestamp, $hash) {
    +   * Returns the user password reset page.
    

    Should be "form" not "page"?

  2. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -104,33 +132,117 @@ public function resetPass($uid, $timestamp, $hash) {
    +  public function getResetPassForm($uid, Request $request) {
    

    Should $request be the 1st param for consistency?

  3. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -104,33 +132,117 @@ public function resetPass($uid, $timestamp, $hash) {
    +    if ($user === NULL && !$user->isActive()) {
    

    Should be || not &&?

    Looks like some missing test coverage?

  4. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -104,33 +132,117 @@ public function resetPass($uid, $timestamp, $hash) {
    +    elseif ($user->isAuthenticated() && ($timestamp >= $user->getLastLoginTime()) && ($timestamp <= $current) && Crypt::hashEquals($hash, user_pass_rehash($user, $timestamp))) {
    

    Side note - I find the naming of isAuthenticated() to be a bit odd.

    Also, this is checking the loaded user account in not uid 0? it should probably instead be part of the line above that's checking the the account is active.

alexpott’s picture

Thanks for the review @pwolanin
1. Fixed
2. Sure
3. Fixed - although oddly I added a test for this in #89 - wonder how that passed - ah I see we were logged in at the time so not testing it properly. Uploading a test only patch that just adds the logout to prove we now have coverage.
4. This is the original logic and I don't think we should be changing it in this issue. If we want to change this then we should open a new issue to do that.

The last submitted patch, 91: 2515050-2-91-test-only.patch, failed testing.

alexpott’s picture

@pwolanin maybe you fancy reviewing the patch? It'd be nice to get this one done - its been at needs review for ages and I think it is ready.

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Ah, yeah - I thought this was done. Looks RTBC, but one minor question:

+++ b/core/modules/user/src/Controller/UserController.php
@@ -104,33 +132,117 @@ public function resetPass($uid, $timestamp, $hash) {
+      return $this->redirect(
+        'entity.user.edit_form',
+        ['user' => $user->id()],
+        [
+          'query' => ['pass-reset-token' => $token],
+          'absolute' => TRUE,
+        ]
+      );

Realize this is just copy/paste but the redirect() method always sets absolute to TRUE, so you don't need to set that option.

We can also clean that up in a follow-up and make sure form state always sets the Url object to be absolute.

YesCT’s picture

YesCT’s picture

Questions (these are not intended to block this at all.)

  1. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -61,41 +74,56 @@ public static function create(ContainerInterface $container) {
    -            array('%other_user' => $account->getUsername(), '%resetting_user' => $reset_link_user->getUsername(), ':logout' => $this->url('user.logout'))), 'warning');
    ...
    +            ['%other_user' => $account->getUsername(), '%resetting_user' => $reset_link_user->getUsername(), ':logout' => $this->url('user.logout')]
    

    noticed in these changed lines (here and other places), some deprecated methods, but looks like the "change" is the array() [] change. So dont need to "fix" the method calls.

  2. +++ b/core/modules/user/src/Form/UserPasswordResetForm.php
    @@ -96,22 +65,8 @@ public function buildForm(array $form, FormStateInterface $form_state, AccountIn
       public function submitForm(array &$form, FormStateInterface $form_state) {
    ...
    +    // This form works by submitting the hash and timestamp to the user.reset
    +    // route with a 'login' action.
       }
    

    so this is an empty method now... and phpstorm didn't know where the parent method was... not sure what the inheritdocs are inheriting and not sure why it is needed if it is the same as the parent, maybe a use will tell it?

Fixed some nits. (I was particularly careful to only change lines already changed in the patch.) Should still be RTBC, but please look at the interdiff to double check.

alexpott’s picture

@YesCT anything that is just an array syntax conversion shouldn't be here... nice spot. Afaics this is the only instance of unnecessary change - all the usages of the short syntax are in the context of large amounts of other changes so the lines are going to be moved anyway and be part of the diff context so I do not think that that matters. And all the new runtime code is using the short syntax anyway (who cares about tests?).

Re the other question we have to have an empty implementation because it is part of \Drupal\Core\Form\FormInterface and therefore it can use the inheritdoc - not sure what is up with your PHPStorm.

catch’s picture

One minor question:

  1. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -104,33 +130,117 @@ public function resetPass($uid, $timestamp, $hash) {
    +    $session->set('pass_reset_hash', $hash);
    

    Why is this using the session from $request.

  2. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -104,33 +130,117 @@ public function resetPass($uid, $timestamp, $hash) {
    +      $_SESSION['pass_reset_' . $user->id()] = $token;
    

    And this using $_SESSION?

Also not related to the patch itself, but I noticed #2787871: Properly deprecate getUserName() and use getAccountName() instead when reviewing at the test coverage.

alexpott’s picture

@catch because we didn't convert $_SESSION['pass_reset_' . $user->id()] = $token; to use the session from the request before 8.0.0 and doing so now might be an API change.

  • catch committed 21d1b47 on 8.3.x
    Issue #2515050 by alexpott, pwolanin, YesCT, David_Rothstein,...

  • catch committed ab361e3 on 8.2.x
    Issue #2515050 by alexpott, pwolanin, YesCT, David_Rothstein,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to all three 8.x branches, thanks!

  • catch committed a9b405d on 8.1.x
    Issue #2515050 by alexpott, pwolanin, YesCT, David_Rothstein,...

Status: Fixed » Closed (fixed)

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

Berdir’s picture

A bit surprised that this got into 8.1. So far we tried really hard to avoid introducing changes that could break something in patch releases.

This is a pretty big change for the user reset form. I have a form alter on this and I'm adding a submit callback to do some additional processing (we have some custom fields to support redirects after first login after registration and also explicit e-mail confirmation so we can let users set a password and still have them set a password on the registration form).

This completely broke because #submit handlers on that form don't do anything anymore now as it submits directly to a different URL which doesn't do form processing. I had to alter that route and replace the controller to make it work again. Noticed this while testing the 8.2 update, which I already found a bit problematic there (would never have noticed this without our luckily extensive test suite) but 8.1?

There is also no change record for this.

David_Rothstein’s picture

Status: Closed (fixed) » Patch (to be ported)

This also still needs backport - there's at least one Drupal 7 patch somewhere above.

catch’s picture

Status: Patch (to be ported) » Closed (fixed)

@David please open a new issue for the 7.x backport, that doesn't take any more time than re-opening a critical 8.x issue.

@berdir yes this should have been 8.2.x-only, didn't apply the patch vs. minor test to it closely enough due to critical priority, but agreed it didn't need to go into 8.1.x.

David_Rothstein’s picture

It takes 10 seconds for me to reopen an issue, but a few minutes to create a new one. That kind of time difference really adds up.

But I did it this time: #2803921: A valid one-time login link may be leaked by the referer header to 3rd parties

Wasn't this issue also supposed to be reopened for a change record?

Berdir’s picture

No so easy to describe, looking for a review of https://www.drupal.org/node/2804081 before I publish it (whoever reviews it and thinks it is OK is of course allowed to publish it too :))

swentel’s picture

Ugh, been bitten by this one too.

@Berdir

I had to alter that route and replace the controller to make it work again

I'm a bit confused here. Do you have access to the submitted values in that controller then ?
I have the same thing as you basically: extra submit handler to set the password directly on the form - any advice would be welcome, don't want to make any security mistakes.

Man, this really is an annoying change :(

Berdir’s picture

There aren't really any relevant submitted values actually.

What this patch basically did is re-add the .../login shortcut known from 7.x and it submits the form against that. The only thing that matters is the form action that it builds in the form, there is no actual form submission processing going on, just the request, which is processed and then you're logged in.

And what I did is replace the controller for *that* request (not the other two involves routes), there you can call the parent or completely replace the logic. Then it also works when you go to /login directly, as a GET request.

roderik’s picture

FYI: as it turns out, besides fixing 'referer header leakage', the patch also introduced the same leakage. On every path where the new user.reset.login throws an AccessDeniedException directly / fails its access check. #3188375: Fix direct-login links (referer header leakage / UX) when already logged in now opened.