Username enumeration via one time login route

Note that for Drupal 8 this happens regardless of whether you are logged in or logged out, whereas for Drupal 7 it only happens if you're logged in:

1. For Drupal 7 or Drupal 8, if you're logged in and visit a URL like the above it happens via this message:

   Another user (%other_user) is already logged into the site on this computer, but you tried to use a one-time link for user %resetting_user.

2. For Drupal 8 only, if you're logged out and visit a URL like the above, you get a "Reset password" form with the following content (which includes the username):

   This is a one-time login for %user_name and will expire on %expiration_date.

   Click on this button to log in to the site and change your password.

   When you submit the form it tells you then that the link is invalid. But it never should have shown the form in the first place.

The fix in both cases should be to prioritize the "link is invalid" message over anything else, so it always shows when the link is invalid.

Side note: The overall issue is exacerbated a bit by the fact that the raw username is displayed here (not the display name that contrib modules which hide the username would use) but that part is covered by #2801645: Use \Drupal\user\UserInterface::getDisplayName() on user pages instead of the plain User::getUsername() and related issues.

There is a patch on #2863468: User password reset form exposes usernames even when hash is invalid that addresses the case of anonymous users, but not authenticated users.

Here is a patch, based on the work by @cburschka and @pwolanin in #2863468: User password reset form exposes usernames even when hash is invalid, that should address both scenarios.

I'm seeing some errors when running tests locally though - will try to sort those out.

The patch breaks the scenario where a logged in user clicks a valid one-time login link for another user.

Another attempt, which seems to work, as far as I can understand the various scenarios.

However, 4 tests are failing in UserPasswordResetTest, and 2 in testResetImpersonation, but I'm not sure why :(

Fixed all fails and had to make some code changes to match with issue summary.

Just testing this, it seems to work in the majority however if you replace the user ID with a non-existing user ID (or anything else) then you get an error.

i.e. changing:
http://local.dev/user/reset/1/1234567890/ABCXYZ....
to
http://local.dev/user/reset/9999999/1234567890/ABCXYZ....
or
http://local.dev/user/reset/abc/1234567890/ABCXYZ....

Causes:

Call to a member function getLastLoginTime() on null in Drupal\user\Controller\UserController->getResetPassForm() (line 185 of /var/www/drupalvm/web/core/modules/user/src/Controller/UserController.php)

The patch has removed a check for $user not being NULL & the user being active in getResetPassForm().

I've added this back in, and the user gets an Access Denied message now, as per before this patch.

Does this need a test?

The test was already there but it was modified along with the check removed so I'm adding a patch with the roll back for test.

This is now working fine, thanks!

I take that back, sorry!

With this patch, appending /login to any reset URL will log you in automatically for the specified ID, so for example:

Changing
http://local.dev/user/reset/2/1234567890/ABCXYZ
to
http://local.dev/user/reset/1/1234567890/ABCXYZ/login

Will successfully log you in as that user.

I'll try and look into this today.

Here's the patch roled against 8.6.x. I replaced all of the drupal_set_messages iwth Drupal::messenger...

The patch in #25 fails against 8.8.x and needs a re-roll.

patch is re-rolled on 8.8.x-dev

Maybe it will solve the problem of test fails & also added an interdiff for patches.

On 8.8.1 #31 was failing to apply for me, rerolled to be compatible with 8.9.x and added some changes to tests.

Fails testing of this scenario described in the description:

Drupal 8, if you're logged in and visit a URL like the above it happens via this message:
Another user (%other_user) is already logged into the site on this computer, but you tried to use a one-time link for user %resetting_user.

The message still appears and displays the username, even with patch 34 applied.

Steps to reproduce:
1. Install core and apply patch 34
2. Login as user 1
3. Create a new user account, get their user id, we'll assume it's uid 2 for this next step
4. Go to /user/reset/2/1/1

Actual behaviour:
* Message shows username of user 2, eg: Another user (test2) is already logged into the site on this computer, but you tried to use a one-time link for user admin. Please log out and try using the link again.
* https://www.dropbox.com/s/be4bgjr8urh1pld/Screenshot%202020-04-07%2014.4...

Expected behaviour:
* Message should not reveal the username of the other user.

I have edited the message but if it requires any other fixes please let me know.

forgot the test changes...

Thanks for the patch. It works for me, I get redirected to the password reset form with the message that the link is expired when I go www.example.com/user/reset/1/1/1. When I use a actual link the user get's logged in.

I've re rolled to 8.9.1

Fixing test cases.

Hi @hash6. Didnt notice you assigned this to yourself. I worked on it and uploaded the patch. it seems u just assigned the patch few seconds back. Sorry for that.

Hi,
I'm also trying to deal with this issue, which I consider to be a vulnerability in my setup.
Thank you for the work already done.

I am attaching a new patch that removes some unnecessary checks in UserController::getResetPassForm. It also moves the timeout check above the more expensive validPassResetHash check.

Patch #45 is working fine.
This can be moved to RTBC if no further code changes are there

I took a stab at a patch for D7. I moved the checks for validating the hash, timestamp, ... to the top of the function and altered the message when an other user is already logged in like they did in the patch for D8/9.

Reroll of patch #47 against 9.2.x

Seems that have inherited some spacing issues, retry

I tried applying patch #42 to Drupal 8.9.9 and it failed.

That being said, would it be adequate from a security standpoint to simply add a new TWIG template to the theme and override the form.html.twig file with say, form--user--pass--reset.html.twig?

the 'children' variable within that TWIG returns a string of:

This is a one-time login for jsmith and will expire on January 01, 1970.

I was thinking of getting 'children' rewritten with

This is a one-time login for j***h

and displaying the username string with a mask.

Added a reroll of #52 with username mask suggestion of #53.

Added a patch will fix CS issue of patch #54.

Onwards and upwards in the MR.

Re-Roll of Patch 55 for Drupal 9.2

Re-submitting for testing

I don't understand why we are trying to mask the usernames. It's only partialy solving the issue.

If the link is valid then we can show the real username. That's not a problem.
If the link is not valid then we shouldn't show the form at all (that's the problem) but do something else like show a 403 for example or show a page that says "the one time login link you used is invalid".

There is no point in showing the form with a masked username if the link used is not valid.
Am I missing something?

Rerolled #63

Fixing failed tests of patch #68.

I think the changes in #70 are not quite right. The problem is that messages should be the same if you use the form - or use the one time link that bypasses the form and logs you in directly. The changes in #70 make it so that the link that logs you in directly does not provide the same messages as it currently does about expired links and that feels wrong.

Patched attached moves the redirect on error to it's own function from \Drupal\user\Controller\UserController::resetPassLogin(). This allows that function and \Drupal\user\Controller\UserController::resetPass() do exactly the same thing regardless if a user goes to /user/reset/1/HASH/TIME or /user/reset/1/HASH/TIME/login.

My patch also fixes a bug in \Drupal\Tests\user\Functional\UserLoginHttpTest caused by not refreshing the container. The test on HEAD assumes that after pressing the log in button on the reset pass form you are logged in but actually because of the stale container that don't actually get correct login links. With this change this log in links no redirect and have an error message whereas before you'd have to press the log in button to see the message.

Here's test evidence that the automatic login route and the go via a form and log button route now work the same.

I've ran through the code. Also tested manually with an reset link on a deactivated user and a normal user. I get a 403 with an deactivated user. And a redirect regarding an invalid link with another user.

1. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -149,13 +150,19 @@ public function resetPass(Request $request, $uid, $timestamp, $hash) {
   +    $redirect = $this->determineErrorRedirect($reset_link_user, $timestamp, $hash);
   +    if ($redirect === NULL) {
   +      $session = $request->getSession();
   +      $session->set('pass_reset_hash', $hash);
   +      $session->set('pass_reset_timeout', $timestamp);
   +      $redirect = $this->redirect(
   +        'user.reset.form',
   +        ['uid' => $uid]
   +      );
   +    }
   +    return $redirect;
   

   It is great this is refactored to use a method! Way cleaner to have a single method to determine the error.

   I would invert this if though and either:

   // The supplied information is invalid, redirect to reset password form.
   if ($redirect !== NULL) {
     return $redirect;
   }
   

   or

   // The supplied information is invalid, redirect to reset password form.
   if ($redirect instanceof RedirectResponse) {
     return $redirect;
   }
   

   This would be more readable mentally in my opinion.

2. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -222,26 +229,11 @@ public function getResetPassForm(Request $request, $uid) {
   +    $redirect = $this->determineErrorRedirect($user, $timestamp, $hash);
   +    if ($redirect === NULL) {
   

   This is the same pattern, i really like to have an easy condition and then return early :)

3. +++ b/core/modules/user/src/Controller/UserController.php
   @@ -260,6 +252,46 @@ public function resetPassLogin($uid, $timestamp, $hash, Request $request) {
   +  protected function determineErrorRedirect(?UserInterface $user, int $timestamp, string $hash): ?RedirectResponse {
   +    $current = REQUEST_TIME;
   +    // Verify that the user exists and is active.
   +    if ($user === NULL || !$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();
   +    }
   +
   +    // Time out, in seconds, until login URL expires.
   +    $timeout = $this->config('user.settings')->get('password_reset_timeout');
   +    // No time out for first time login.
   +    if ($user->getLastLoginTime() && $current - $timestamp > $timeout) {
   +      $this->messenger()->addError($this->t('You have tried to use a one-time login link that has expired. Please request a new one using the form below.'));
   +      return $this->redirect('user.pass');
   +    }
   +    elseif ($user->isAuthenticated() && ($timestamp >= $user->getLastLoginTime()) && ($timestamp <= $current) && hash_equals($hash, user_pass_rehash($user, $timestamp))) {
   +      // The information provided is valid.
   +      return NULL;
   +    }
   

   My first look at this had me questioning this method name. I mean, only one of the 3 outcomes is a redirect. As the docblock mentions, it validates user, hash and timestamp.

   Not sure how this would be better though. A thought was, perhaps it should only throw exceptions if something is wrong, but that kinda complicates the implementation as a whole.

   Basically, it does what is advertised, it just feels like a weird method with those 3 totally different outcomes.

4. +++ b/core/modules/user/tests/src/Functional/UserLoginHttpTest.php
   @@ -106,6 +106,8 @@ public function testLogin() {
   +    $this->rebuildAll();
   
   @@ -243,6 +245,7 @@ public function testPasswordReset() {
   +    $this->rebuildAll();
   

   Tested the tests without this, because i was skeptical. But seems they fail otherwise.

5. +++ b/core/modules/user/tests/src/Functional/UserPasswordResetTest.php
   @@ -151,7 +151,8 @@ public function testUserPasswordReset() {
   +    $this->assertSession()->pageTextContains('You have tried to use a one-time login link that has either been used or is no longer valid. Please request a new one using the form below.');
   +    $this->drupalGet($resetURL . '/login');
   
   @@ -177,7 +178,8 @@ public function testUserPasswordReset() {
   +    $this->assertSession()->pageTextContains('You have tried to use a one-time login link that has expired. Please request a new one using the form below.');
   +    $this->drupalGet("user/reset/$_uid/$bogus_timestamp/" . user_pass_rehash($this->account, $bogus_timestamp) . '/login');
   
   @@ -186,6 +188,8 @@ public function testUserPasswordReset() {
   +    $this->drupalGet("user/reset/" . $blocked_account->id() . "/$timestamp/" . user_pass_rehash($blocked_account, $timestamp) . '/login');
   +    $this->assertSession()->statusCodeEquals(403);
   
   @@ -203,7 +207,8 @@ public function testUserPasswordReset() {
   +    $this->assertSession()->pageTextContains('You have tried to use a one-time login link that has either been used or is no longer valid. Please request a new one using the form below.');
   +    $this->drupalGet($old_email_reset_link . '/login');
   
   @@ -563,7 +568,11 @@ public function testResetImpersonation() {
   +    $this->assertSession()->pageTextContains('You have tried to use a one-time login link that has either been used or is no longer valid. Please request a new one using the form below.');
   +    $this->drupalGet($attack_reset_url . '/login');
   

   Love the extra coverage for the login links <3

Great progress, thanks @alexpott, this seems like it is quite close.

Re #75.1 / 2 sure - fixed in the patch attached
Re #75.3 I added more docs. I think the 3 outcomes - exception / redirect / null is okay - and part of making things consistent.
Re #75.4 Yeah the test atm is not currently testing what it thinks it is. If you install a module in a test you need to ensure the container attached to the test object is updated. This is why in Browser tests I think we should not have the container on the test object. This is an old argument.
Re #75.5 that's to ensure we don't lose test coverage as that's what we were testing prior to this change.

Thanks for the changes and the explaination.

I did consider the 403 for a non existant user also. Since that does expose the fact a uid exists, but I concluded that this is fine. Since the most important thing to solve is the exposure of usernames.

This looks great! I'm all for getting this in. I'll probably generate a patch for 9.4.x later also, since I think this is an improtant improvement in privacy that i'd roll out to every site we run.

ANyways, RTBC, thanks!

+++ b/core/modules/user/src/Controller/UserController.php
@@ -222,11 +229,54 @@ public function getResetPassForm(Request $request, $uid) {
+   *
...
+   *   Returns a redirect if the information is incorrect. It redirects to
+   *   'user.pass' route with a message for the user.

Looks like this should be

@return \Symfony\Component\HttpFoundation\RedirectResponse|NULL ?

Indeed... thanks for the review @catch. Patch attached fixes the docs... the return typehint was already correct.

Had a last look through and couldn't find anything to complain about, the code being moved is more or less identical it's just changing execution order so the redirect happens in more cases.

Double checked there's no actual string change here and there isn't making this eligible for 10.0 and 9.5 too, so committed/cherry-picked to the respective branches.

Did my best with issue credits but this is a long issue with a lot of people on it, so might not be perfect.

That is awesome catch. Thanks! We just got a bit more prinlvacy friendly! :)

I have created a D7 backport issue. We probably want to backport this security/privacy improvement also there. See: #3326994: [D7] Username enumeration via one time login route

While working on the D7 backport, I have noticed that only one usecase for D9/10 was fixed. See #2 or #36.

1. For Drupal 7 or Drupal 8, if you're logged in and visit a URL like the above it happens via this message:

Another user (%other_user) is already logged into the site on this computer, but you tried to use a one-time link for user %resetting_user.

2. For Drupal 8 only, if you're logged out and visit a URL like the above, you get a "Reset password" form with the following content (which includes the username):

This is a one-time login for %user_name and will expire on %expiration_date.

I see that the usecase 2 is fixed now - when logged out, you are correctly redirected. But the username seems to be still revealed if the user is logged in and use that URL with specific pattern (usecase 1) - tested on 10.1.0-dev. Was there a decision to fix only the part when the user is logged out or it was missed?

Thanks!

I am setting this back to NW for the question raised in #86, which is asking if Case 1 has been fixed. I skimmed the comments and didn't see an answer.

@poker10 nice catch - I think that that means we should add a follow-up issue to address that case as it will require a string change. Once the follow-up exists I think this issue should be marked as fixed.

I created #3327294: Username enumeration via one time login route when logged in as another user to account for #86.

@alexpott, thanks.

I forgot to remove the needs follow up tag.