[D7] Prevent username enumeration via one time login route

[klonos]

This is the respective issue for https://www.drupal.org/project/drupal/issues/2828724 (D9.5) and its D7 counterpart https://www.drupal.org/project/drupal/issues/3326994.

Problem/Motivation

User password reset URLs can be used to enumerate usernames.

For example [site_url}/user/reset/[user_id]/1/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.

...

I am reporting this issue because a customer(bank) pointed out it was a leakage of customer information in their scenario. It is currently possible (v7.51 + v8.19) to get the username of user in the system by requesting a password reset for that UID with a random timestamp and hash.

I reported this issue to the (Drupal) security team already and they concluded the following :

I believe this issue can be fixed in public without a security advisory because of our policy on username disclosure: https://www.drupal.org/node/1004778

In the summary above, I have only kept the bit that is relevant to Backdrop, and have confirmed that it applies to us: Trying to access /user/reset/1/1/1 while logged in with another user account, reveals the username of user1:

[klonos]

I'm thinking that this should be dealt with as part of #4696, with the same setting we are to introduce there. Waiting for feedback on that issue (or here) before I mark this issue here as a duplicate and close it in favor of that other issue.

[klonos]

I am suggesting to change the message in this use case to the following:

You are already logged into this site as [CURRENT USERNAME], but you tried to use a password reset link for another user account. Please logout first, and then try using the link again.

[ghost]

Is this not a duplicate of https://github.com/backdrop/backdrop-issues/issues/5971 (which was fixed and merged last week)?

[klonos]

Yes it is @BWPanda 🤦🏼 ...I did a search of the terms 2828724 and 3326994 in our issue queue, but nothing came up.

I guess that people will find this issue here when they search in the future, and that'll lead them to #5971 😉

Closing...

[herbdool]

I'm reopening this because the D7 backport has a couple useful lines https://git.drupalcode.org/project/drupal/-/commit/bdb7dd43533e99dc1b71bec1598c62ee2ffc4dfa. Such as checking that the timestamp is valid:

if (!empty($reset_link_account) && $timestamp >= $reset_link_account->login && $timestamp <= REQUEST_TIME) {

[herbdool]

If https://github.com/backdrop/backdrop-issues/issues/6420 is merged first we'll have to update this PR to add mail to user_pass_rehash().

[izmeez]

Since #6420 has now been committed does https://github.com/backdrop/backdrop/pull/4674 need update to add mail to user_pass_rehash() ?

[herbdool]

@izmeez this has been updated.

[izmeez]

This is another related issue. While testing the PR tugboat, before logging in, request a new password for a non-existing user works to not identify if the user or email exist. However, request for a known user, e.g. admin returns: Further instructions have been sent to your email address. I was expecting an equally ambiguous reply, "if ..." This is #4696

[izmeez]

There are issues with the PR. I will leave comment there.

[izmeez]

When logged in going to url /user/password the following screen is displayed and the "Reset password" button is active and works.

Is the intention to allow logged in user to request a new password link for themselves but not for another user? How can a logged in user request a new password link for another user? Would that be part of masquerade?

[stpaultim]

I would appreciate a clear description of what this PR is designed to fix.

Is it related to either of these issues? https://github.com/backdrop/backdrop-issues/issues/111 or https://github.com/backdrop/backdrop-issues/issues/4696

[izmeez]

The PR does include the extra checks added to Drupal 7. It would still help to know the overall goal.

[izmeez]

I don't think it has anything to do with either #111 or #4696. The first is closed and the second is a separate issue that is in this ballpark and one I'll push when I can.

[herbdool]

@stpaultim @izmeez Okay, I'm going to describe how to test this PR. And what it does.

It makes sure that if someone goes to [site_url}/user/reset/[user_id]/1/1 that they get the message "The one-time login link you clicked is invalid." (and not "You cannot use a password reset link while logged into the site.")

So that's the first thing you can test.

And the other thing to test is to be logged in as one user (test1), then create a one-time login link for a second user (might need to do this with bee bee uli test2 or blocking/unblocking test2 will send an email to that user with a one-time login link). Then visit that URL while logged in as the first user. It should display:

"You cannot use a password reset link while logged into the site."

Note that this PR looks a bit different than the original Drupal 7 PR since Backdrop had already removed the username from the message. But it still includes the improvement of checking the timestamp in the reset URL to better know if it's a valid one-time link or not.

You can also see that the PR also includes an automated test that does everything that I describe above so I know that it's already passing those tests.

[herbdool]

@stpaultim @izmeez bump. Wondering if either of you can test this. I've put steps in my previous comment.

[argiepiano]

@herbdool I've tested locally and reviewed it. LGTM! @izmeez, your question/comment above is unrelated to the goal of this PR. The functionality of the password reset form has not changed.

[herbdool]

Thanks @argiepiano

[quicksketch]

Thanks @herbdool for the clarification on this PR's purpose. I merged https://github.com/backdrop/backdrop/pull/4674 into 1.x and 1.28.x. Thanks @herbdool, @izmeez, and @argiepiano for working on this small follow-up.