Problem/Motivation

#2828724: Username enumeration via one time login route partially fixed this but we missed the following case. If you're logged in and visit a URL like [site_url}/user/reset/[user_id]/1/1 you'll see 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.

Steps to reproduce

Proposed resolution

Change user interface text to not disclose the user name for the resetting user.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#17 3327294-17.patch5.22 KBpoker10
#16 3327294-nr-bot.txt85 bytesneeds-review-queue-bot
#15 interdiff-3327294_14-15.txt422 bytespoker10
#15 3327294-15.patch5.23 KBpoker10
#14 interdiff-3327294_11-14.txt2.59 KBpoker10
#14 3327294-14.patch4.74 KBpoker10
#11 interdiff-3327294_6-11.txt3.17 KBpoker10
#11 3327294-11.patch3.55 KBpoker10
#11 3327294-11_test-only.patch1.13 KBpoker10
#6 3327294-6.patch1.11 KBpoker10

Comments

alexpott created an issue. See original summary.

poker10’s picture

poker10 commented

Could the solution be to keep the string as it is, but only print this when the reset link is actually valid? Because the standard user should not be able to craft a valid link for other user. In case the link is invalid (just like /user/reset/1/1/1), then we can print the message what we already have on a different place:

The one-time login link you clicked is invalid.

What do you think? I do not think it is important to inform the user about the fact, that the link have to be used while logged out, if the link is invalid.

alexpott’s picture

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

I think it is important to inform the user to log out. The logged in as another user must be an excruciating small use-case but I don't see what's gained by having username in this text.

I think the text could be:

Another user (%other_user) is already logged into the site on this computer, but you tried to use a one-time link for another user. Please <a href=":logout">log out</a> and try using the link again.

With no loss of functionality. We could even replace
'%resetting_user' => $reset_link_user->getAccountName(),
with
'%resetting_user' => $reset_link_user->id(),
as the user ID is already in the url...

poker10’s picture

poker10 commented

Yes, for the valid link I agree.

But the question is, if it is important to inform the user even if the link is invalid. Because I think this is the primary case of enumeration - users can try links like /user/reset/1/1/1, /user/reset/2/1/1, etc.. These links are not valid, therefore a message that the link is invalid can possibly be prioritized.

Actually there is no check if the link is valid in this part, see: https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/modules/user/src/Controller/UserController.php#L136 , only a check if the user exists:

if ($reset_link_user = $this->userStorage->load($uid)) {
  ...
}
bbrala’s picture

bbrala
Location Netherlands
commented

Wouldn't the correct fix be to always show the default message "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.". Perhaps make the message consistent and suggest to log out if logged in.

I'd be fine though with at least removing the username for now as alex suggested, and take a step back in another issue to find out how we can get all messaging consistend across Drupal to minimize exposure of information.

I'll work on this later this week. Unless someone else beats me to it, then i'll put in the review.

poker10’s picture

poker10 commented
Status: Active » Needs review
StatusFileSize
new 3327294-6.patch1.11 KB

Ok, this is what I meant by checking if the link is valid. I think this option could be better, as it prefers a message The one-time login link you clicked is invalid. in case the link is not valid.

So the examples:

1. If a logged in user uses an invalid reset link: /user/reset/1/1/1 then:

  • before patch: Another user (XX) is already logged into the site on this computer, but you tried to use a one-time link for user YY. Please log out and try using the link again.
  • after patch: The one-time login link you clicked is invalid.

2. If a logged in user uses a valid reset link: /user/reset/1/1673978256/rCRqJ0X800xD0eKVFhBmMnkdpDgKtKkzYIGav7sBqSg, then the message would be the same (unchanged) before and after patch: Another user (XX) is already logged into the site on this computer, but you tried to use a one-time link for user YY. Please log out and try using the link again.. So the behavior in case of the valid reset link is not changed. I think here we do not have to protect the username anymore, as the logged-in user has access to the valid reset link, so this user is able to login to the second account.

---------

But in case you will still find better to remove the username at all (as @alexpott mentioned), I will be OK with that.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

I think the approach in #6 is fine. Needs a test and I think we should refactor the hash & timestamp validity checking into its own method.

aaronmchale’s picture

aaronmchale
Primary language English
Location Edinburgh, Scotland
commented

Maybe use the word "device" instead of "computer", that seems to be more common these days, in our mobile-first world.

alexpott’s picture

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

@AaronMcHale I think the wording of the message shouldn't be in scope unless we are changing it. @poker10's solution is not adding any new wording. It's tweaking when you see different messages.

aaronmchale’s picture

aaronmchale
Primary language English
Location Edinburgh, Scotland
commented

@alexpott Yep that makes sense, I just assumed that was a change in this issue. If it has not been changed in this issue then yes let's leave it out of scope, thanks.

poker10’s picture

poker10 commented
Status: Needs work » Needs review
Issue tags: +Security improvements
StatusFileSize
new 3327294-11_test-only.patch1.13 KB
new 3327294-11.patch3.55 KB
new interdiff-3327294_6-11.txt3.17 KB

Updating the patch according to the comment #7. Adding also simple test case and test-only patch.

The last submitted patch, 11: 3327294-11_test-only.patch, failed testing. View results

alexpott’s picture

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

I think we need a follow-up to inject the time service into this class.

Also I think we should consider making the following change to \Drupal\user\Controller\UserController::confirmCancel() which shares the same timestamp / hash / user checking logic.

diff --git a/core/modules/user/src/Controller/UserController.php b/core/modules/user/src/Controller/UserController.php
index d634706f3d..47ac09e84f 100644
--- a/core/modules/user/src/Controller/UserController.php
+++ b/core/modules/user/src/Controller/UserController.php
@@ -406,7 +406,7 @@ public function confirmCancel(UserInterface $user, $timestamp = 0, $hashed_pass
     $account_data = $this->userData->get('user', $user->id());
     if (isset($account_data['cancel_method']) && !empty($timestamp) && !empty($hashed_pass)) {
       // Validate expiration and hashed password/login.
-      if ($timestamp <= $current && $current - $timestamp < $timeout && $user->id() && $timestamp >= $user->getLastLoginTime() && hash_equals($hashed_pass, user_pass_rehash($user, $timestamp))) {
+      if ($current - $timestamp < $timeout && $user->id() && $this->validatePathParameters($user, $timestamp, $hashed_pass)) {
         $edit = [
           'user_cancel_notify' => $account_data['cancel_notify'] ?? $this->config('user.settings')->get('notify.status_canceled'),
         ];

I think the consistency is worth it.

Also I think we should consider whether we should add the timeout check to \Drupal\user\Controller\UserController::validatePathParameters() - because there really is no difference between the messages:
'You have tried to use a one-time login link that has expired. Please request a new one using the form below.'
and
'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.'

And then we have less code to maintain and even more consistency.

poker10’s picture

poker10 commented
Status: Needs work » Needs review
StatusFileSize
new 3327294-14.patch4.74 KB
new interdiff-3327294_11-14.txt2.59 KB

Uploading an updated patch - the method confirmCancel() now uses the same validation via validatePathParameters() and the validatePathParameters() now has a new timeout parameter, so that it can be used consistently.

poker10’s picture

poker10 commented
StatusFileSize
new 3327294-15.patch5.23 KB
new interdiff-3327294_14-15.txt422 bytes

Removed one deprecation, so PHPStan was complaining. Let's try again.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3327294-nr-bot.txt85 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

poker10’s picture

poker10 commented
Version: 9.5.x-dev » 11.x-dev
Status: Needs work » Needs review
StatusFileSize
new 3327294-17.patch5.22 KB

Let's add a 11.x patch as well..

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests +Needs Review Queue Initiative

Appear that tests are included in the patch so removing the tag.

#13 mentions a follow up but #14 believe addresses that concern, correct me if I'm wrong.

Looking at the patch nothing stands out so think this is good for committer review.

  • catch committed 05e95eb4 on 10.1.x 
    Issue #3327294 by poker10, alexpott: Username enumeration via one time...

  • catch committed 27aba04e on 11.x 
    Issue #3327294 by poker10, alexpott: Username enumeration via one time...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

We still need a follow-up to inject the time service, it's currently using \Drupal, so it's not using deprecated constants but it's also not injecting.

I double checked that we already have test coverage for the 'you're trying to log in with a valid login link but you're logged in as someone else' which can easily happen during development with masquerade/test accounts/multiple browsers open etc.

However given this fixes username unumeration happy to commit without injection to 11.x and 10.1.x, if we added injection to 10.1.x we'd be deprecating in a patch release, allowed since it's @internal but not necessary.

poker10’s picture

poker10 commented

@catch , is the proposed follow-up already covered by this issue #3112298: Replace REQUEST_TIME in classes with direct container access ? I see the core/modules/user/src/Controller/UserController.php mentioned here. Thanks!

catch’s picture

catch
he/him
Primary language English
commented

@poker10 no because that's about the constants not injection Vs the \Drupal class. As far as that meta's concerned this issue will have fixed it.

quietone’s picture

quietone commented
Issue tags: -Needs followup

I am not sure what Meta is referred to in #23. I looked at the other issue, #3112298: Replace REQUEST_TIME in classes with direct container access and it is injecting the service in classes, including UserController. That issue does have a meta but I don't see it referring to the UserController class, except in the MR.

So, I think that a followup is not needed here because it is being addressed in 3112298. Therefore, I am removing the tag. I trust someone will correct me if I am wrong.

catch’s picture

catch
he/him
Primary language English
commented

Oh the meta is #2902895: [meta][no patch] Replace uses of REQUEST_TIME and time() with time service which is about removing usage of the constants. However the patch committed here doesn't inject the time service, it uses \Drupal so we could have a follow-up for that. But also it's a very minor issue so it could also just wait for someone trying to remove \Drupal calls from classes in general.

Status: Fixed » Closed (fixed)

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