This issue has been investigated by the Drupal Security Team and it has been decided to handle this as public security improvement.
After a failed login attempt, the link to the "Reset password" form in the error message includes the user's username or email address in the query string. This can lead to PII disclosure when following that link, e.g. when including advertisements that send the whole page URL as referrer on the target page.
It should at least be configurable whether the email will be included in the generated link or not. A proposed patch will be attached.
Steps to reproduce:
- try to log in to the site with wrong password
- follow the "forgotten your password" link in the error message
- now the user's username or email is disclosed in the page URL.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | image002.jpg | 29.08 KB | rreiss |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !3989
Comments
Comment #1
cussack commentedComment #2
cussack commentedAdded new patch to fix coding style issues.
Comment #3
pfrenssenSecurity issues should not be posted in the public issue queue but directly to the security team. See How to report a security issue. I have made an issue for this in the private security queue: https://security.drupal.org/node/151218.
Comment #4
klausiRepublished this after consideration by the security team
Comment #10
gregglesOne way to solve this with minimal intervention is to hard-code the path to login without a username in it as a translation string.
In settings.php:
Comment #11
rreiss commentedAnother scenario with a similar information disclosure vulnerability (tested only on D7, but being added here as advised by the Drupal security team):
When logged in to a Drupal site (without any special privileges), I can go to “my account”
page, try changing my email to another email that is already in use by another user and
then I am getting the message “The e-mail address xxx@xxx.xxx is already taken”.
That allows an attacker to retrieve users that exists on the site.
Screenshot is attached.
Comment #12
tatarbjI'm planning to bring this issue to the security improvements sprint of DrupalCamp Ruhr this weekend and work on it with sprinters - marking it with the tag of #dcruhr18_SecImproveSprint.
Comment #13
tstoecklerComment #20
quietone commentedI tested on Drupal 10.1.x, minimal install. This is still valid.
Comment #23
bramdriesenBeen thinking about this for a bit. I don't think this should be an opt-in feature like the patch in #2 is doing. I have checked a few other applications and CMS systems (including Wordpress) and none of them (which I tested) pre-fill the username/email field if there is one.
This might break a few tests so those will need to be fixed as well.
Also not sure what the preferred solution for this would be from the core maintainers perspective.
Comment #24
smustgrave commentedLuckily only a failures :)
If I had to vote I would say the opt-in feature makes the most sense.
Though since it's only passing what was inputted in the username field, and not really validating the username, think the default should be to not include the username.
Comment #25
bramdriesenI'm still more fan of removal. Having a checkbox in Core that makes your site less secure seems a tad weird (it would also need a disclaimer then since it has security implications). We would also need to do config changes, provide an upgrade hook etc for this which in my eyes looks a bit overkill as well. It's "just" the password forgot redirect query parameter :-) it's not like we are really breaking or changing functionality. Besides pre-filling that form of course, but most people with browser autocomplete won't even notice this I think.
Comment #26
bramdriesenComment #27
smustgrave commentedSince I’ve never noticed this feature don’t see any issue removing. I don’t think it would be much of a lose
Comment #28
smustgrave commentedAll green removing the feature.
Comment #30
larowlanHiding patches, updated credits
Comment #32
larowlanCommitted 4b0e870 and pushed to 11.x. Thanks!
Because of the minor behaviour change here, decided not to backport this.
Created a change record for the new behaviour