Problem/Motivation
7.x port of #992540: Nothing clears the "5 failed login attempts" security message when a user resets their own password (8.3.x & 8.4.x)
If a user forgets their password and tries to log in 5 times then they get blocked by flood control. They can now use the password reset functionality per email, but once they log out shortly after that they are still blocked when trying to log in again.
Proposed resolution
Clear the user specific flood events once they used the password recet functionality so that they are able to normally log in again. Do not clear IP address specific flood events because then an attacker with a valid account could clear flood events for victim user accounts.
Remaining tasks
Update and review the patch.
Comments
Comment #2
vijaycs85Straight port of 8.4.x
Comment #4
tatarbjAs drupal7 requires php5.3 the syntax of arrays has to be the old-school one, that's the reason why the patch failed.
I'm posting the fixed patch and sending it to review.
Comment #5
vijaycs85thanks @tatarbj. I was just started work on it. Can you attach a diff file as well please?
Comment #7
tatarbjSure, here you go (nothing else, just that i've mentioned :))
If you want to work on it, i let you, then will review it, fair enough?
Comment #8
vijaycs85Thanks.
sure :) I am going to run the test locally and see how it goes.
Comment #9
vijaycs85Fixed test fail.
Comment #10
mahalingam_cs CreditAttribution: mahalingam_cs at TATA Consultancy Services commentedComment #11
mahalingam_cs CreditAttribution: mahalingam_cs at TATA Consultancy Services commentedApplied patch from #9. Patch applied successfully but found the issue is not yet fixed. When the user gets the message "Sorry, there have been more than 5 failed login attempts for this account. It is temporarily blocked. Try again later or request a new password" and admin re-sets the password in admin panel, user still gets the same error when tried to login with the new password.
Comment #12
vijaycs85@mahalingam_cs I hope you have changed password as admin and tried to login as a user with new password. This scenario is not covered in this issue (as well as the 8.x.x version in #992540: Nothing clears the "5 failed login attempts" security message when a user resets their own password). For this patch, you have to request new password and click reset link in the email. If we decided to cover your test case, it has to be a separate issue, I guess.
Comment #13
vijaycs85@mahalingam_cs, I created separate issue for your scenario at #2881572: User login flood lock doesn't clear when reset password as admin. I am still not sure why it wasn't covered in #992540: Nothing clears the "5 failed login attempts" security message when a user resets their own password. So for it has only 8.4.x patch.
Comment #14
valthebaldWhether changing user password should also reset the flood, is an open question (https://www.drupal.org/node/2881572#comment-12106305).
The patch from #9 is exact backport of #992540: Nothing clears the "5 failed login attempts" security message when a user resets their own password, moving to RTBC
Comment #15
mahalingam_cs CreditAttribution: mahalingam_cs at TATA Consultancy Services commentedApplied patch from #9 and it worked perfect.Comment #14 clarifies my comment form #11, its a open question Whether changing user password should also reset the flood.
Comment #16
tatarbjI've also applied it and works like a charm!
Comment #17
joseph.olstadComment #18
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis looks like it has the same security issue that the Drupal 8 patch had - see https://www.drupal.org/node/992540#comment-12116412
Also, the code comments in the patch are repetitive since they are missing the changes @catch made on commit at https://www.drupal.org/node/992540#comment-12099362
I'm also tagging this for an issue summary update - normally I don't care that much about keeping issue summaries up to date, but this one is really confusing since it actually describes #2881572: User login flood lock doesn't clear when reset password as admin instead :) The bug here is for the case where a user resets their own password.
Comment #19
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #20
klausiUpdated issue summary.
Comment #21
klausiUpdated the patch based on the feedback from David.
Comment #22
tatarbjI've tested the patch on a local, it works well.
But i'd like to change a small detail in the test in order to properly follow how https://api.drupal.org/api/drupal/modules%21user%21user.module/function/... works as it uses the default value of user_failed_login_user_limit variable 5, so i think it would be better if test could check 5 instead of 3, so i've modified the test in a new patch that i'm attaching now with an interdiff.
-- There is no other changes, only test gets more 'proper'.
Cheers,
Balazs.
Comment #23
klausiI think it does not matter if we test with a 3 or 5 attempts setting - it only makes the test run longer. But I also don't really care :)
Looks good otherwise, we are using this patch in production for a week now without problems.
Comment #24
mahalingam_cs CreditAttribution: mahalingam_cs at TATA Consultancy Services commentedApplied patch from #22 and it worked proper.
Comment #25
vijaycs85+1 to RTBC. Great to get this in!
Comment #26
Rob C CreditAttribution: Rob C commentedQuick review / small nits:
> passowrd (typo)
Seems a bit unclear from the comment. Q: Is this order correct?
'There have been more than ' . $user_limit . ' failed login attempts for this account.'
Do we really need to know the number we set previously? (this is the only place where it's used like this in tests once committed i believe)
And:
Let's stick to 5, seems like a good number. (and also used in other tests as a default).
Comment #27
Michael-IDA CreditAttribution: Michael-IDA at Internet Design Alliance commentedOne of my clients just ran into this bug, which took way too long track down and then flush the flood table manually. They were following the given instructions to ‘clear’ the block, so they didn’t call me for several hours, leading to a fairly extreme level of frustration on their part. So...
A few things?
A)
Shouldn't the Title be something like?:
e.g. if an administrator resets the password, the the user specific flood control should be reset as well.
One of their administrators reset the ‘blocked’ administrator’s password, which did NOT clear flood control.
If the website messages say resetting the password clears the block, then any password reset should clear the block, or the messaging needs to be change to identify [see C) though] what exactly does, and does not, clear flood control.
B)
I'd need to dig deeper (and maybe it doesn’t matter for testing?) but should this be hardcoded?
As the Flood control module, https://www.drupal.org/project/flood_control , makes use of a variable for that setting.
See: "Failed login (username) limit" on https://www.drupal.org/files/images/570142-flood-control.png
C)
Probably shouldn’t be addressed here, but:
> 'There have been more than ' . $user_limit . ' failed login attempts for this account.'
Why are we telling anyone, anywhere, how many failed login attempts there were? That just invites bot abuse. Wouldn’t a more security conscious message would be something like:
Best,
Michael
Comment #28
Rob C CreditAttribution: Rob C commentedCould be part of the patch, or a new followup issue. Prefer the latter, because this has been open for some time now and the flood_unblock module provides a simple interface for clearing these already (but implementing this seems logical enough).
That bit is only used in testing.
Comment #29
Michael-IDA CreditAttribution: Michael-IDA at Internet Design Alliance commentedHi Rob,
>> if an administrator resets the password, the the user specific flood control should be reset as well.
> Could be part of the patch, or a new followup issue. Prefer the latter,
I'll admit I'm not familiar with the user system (did a quick search can’t find this though), but ...
Is there a single password reset confined to a master/single api call in D7?
If so
could be moved to there, so that any password reset from anywhere clears flood. Which just makes everyone’s life easier...
If password resets are scattered all over the place (which it somewhat seems like), then I agree, put this patch in as is and I’ll open a separate issue for administrator resets.
Best,
Michael
Comment #30
oadaeh CreditAttribution: oadaeh at Hook 42 commentedI'm attaching a patch that corrects the misspelling of password, and nothing else. I'm also setting it for testing, since it's been several months.
Comment #31
joseph.olstadHas working tests, spelling is fixed, looks good thanks!
RTBC!
Comment #32
joseph.olstadBumping to 7.61. This didn't make it into 7.60
Comment #33
joseph.olstadComment #34
tatarbjComment #35
joseph.olstadComment #36
pandaski CreditAttribution: pandaski at govCMS (Australian Government Department of Finance) commentedFrom our experience, the user can be blocked. For example, the user status can be 0 after 5 failed attempts.
However, the user will try to reset the password.
I think only load the status 1 user is better, and leave the blocked user to a different method.
Comment #37
joseph.olstadThis hasn't made it into D8 yet.
@pandaski , I don't understand your last comment, can you please clarify, which line of code are you referring to? Steps to reproduce? If you have a suggestion for an improvement, can you make a patch and interdiff for it?
Comment #38
MustangGB CreditAttribution: MustangGB commentedComment #39
pandaski CreditAttribution: pandaski at govCMS (Australian Government Department of Finance) commentedWe are using the patch in the latest GovCMS7 distribution
It is good to go, thanks everyone
Comment #40
joseph.olstadThis patch is still clean.
On a side note, yesterday I came accross an issue in D8 where drush uublk didn't actually remove the records from the flood table for the desired user. Maybe a problem with drush.
these types of usability issues should be fixed in D8/D9/D7, not sure why we leave these types of issues for so long.
people need to be able to reset their passwords, flooding also needs to expire or be unfloodable
Comment #41
joseph.olstadPostponed on the D9 issue not being fixed:
#992540: Nothing clears the "5 failed login attempts" security message when a user resets their own password
Comment #42
nagy.balint CreditAttribution: nagy.balint at Webbtik.io commentedSo why is this issue postponed?
Comment #43
MegaChriz CreditAttribution: MegaChriz at WebCoo commentedMy assumption is that it was set as postponed because the issue should be fixed first in newer versions of Drupal. At least, I've seen that as a reason in the past, namely here: #1699378-66: Allow tokens in entity reference views selection arguments. That implies that the policy is that something cannot be fixed in an older version if it hasn't been fixed yet in the latest version.
Maybe that made sense back in 2013, when Views was in the process of being ported to Drupal 8 - and then it could hold up people upgrading from D7 to D8 if the D8 version would lack a feature that the D7 version had? But in this case, I would vote for setting this back to RTBC. Also because it is a bug and not a feature - so I doubt that this particular thing would cause someone upgrade issues if the bug only gets fixed in D7 for now.
Comment #44
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedThe D10 issue is in, so we can continue here to backport that for D7 :)
Patch #30 does not apply anymore, so it needs reroll. We also need to check if the patch from #30 matches what was committed in D10.
Comment #45
joseph.olstadstraight up re-roll of patch 30, no need for interdiff, I made zero change.
it had fuzz, that's it, now this patch resolves the fuzz.
Comment #46
joseph.olstadPatch was rerolled, there was only fuzz, no interdiff needed.
Comment #47
joseph.olstadJustification for RTBC:
Comment #48
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedThanks! I think this looks overally good. I have only two points / nitpicks:
1. Is there a reason why we are increasing the limit from 3 to 5 in this test? It does not seems the same as in D10 patch, so I am curious if there is a reason for this change.
2. We should probably include the full comment from D10 patch (e.g.
// A login attempt after resetting the password should still fail, since the IP-based flood control count is not cleared after a password reset.
).-------------------
Comparing the patch with D10 there is also one other difference, where D10 is clearing also
user.http_login
flood event, but it seems like we do not have such flood event in D7, so it should be OK as it is.Comment #49
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedI have added this issue to the list of potential candidates for the next release. If we can check/fix points from #48, I think this have a fair chance to be committed.
Comment #50
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedI have gone ahead and updated changes in tests to match the D10 version more closely (other than that the patch is the same). I have also reverted the change which increased the number of failed logins from 3 to 5. This was explained in #22, but even so I think we should keep the changes as simple as possible. And given that this change was not in the D10 commit, I suggest to open a follow-up to make such a change in D10 + D7, if needed. Let's focus here only on changes relevant to the issue summary. Thanks!
For the reference, here is the D10 commit: https://git.drupalcode.org/project/drupal/-/commit/a8729aad07ec430e0c217...
Comment #52
joseph.olstadlooks good @poker10,
thanks so much!
We'll take the win!
Comment #53
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedThanks. Adding a tag for a final review.
Comment #54
mcdruidOne thing about this is bothering me slightly.. in the changes to user.test
Perhaps it's just been a long week, but I don't understand the reason for this change to the comment.
It was committed in the parent issue, and looks like it was added here:
https://www.drupal.org/project/drupal/issues/992540#comment-13887545
...but I'm still not entirely sure why.
Comment #55
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedGood catch @mcdruid! Not sure if @joseph.olstad recalls from the D10 issue why this was changed, but I reviewed the
user_login_final_validate()
and the flood control for user is reset in case of successfull login:Therefore the original comment seems correct and as there are no changes to the code around, I think that you are right and we should skip this comment change.
Comment #56
joseph.olstadI saw my name being mentioned.
Here's a new patch with interdiff to address the concern.
Comment #58
mcdruidThanks @joseph.olstad - we were wondering if you remembered the reason for the change to that comment?
On the other hand, sometimes I don't remember what I had for lunch.
We should file a follow-up to restore the original comment in D10/11.
Thanks everybody!
Comment #59
MegaChriz CreditAttribution: MegaChriz at WebCoo commentedThe answer to that is because alexpott says this in his review from #992540-172: Nothing clears the "5 failed login attempts" security message when a user resets their own password, point 3:
I haven't checked the context in which this was said, but the comment change appears to be based on that part of the review.
Comment #60
mcdruidAh, well spotted @MegaChriz
I have a feeling perhaps there was a small typo in that suggested comment and it should have said "we're now going to test.." whereas instead it was mistakenly corrected to say "we're not going to test.." - would that make sense?
Comment #61
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedReading @alexpott's comment again and looking at the context - yes, it looks like a typo (missing letter) and also it looks like that the comment was misplaced. Alex referenced this part of the code:
And the note was saing:
. So I think this new comment was meant to be placed above these two lines, which would make sense. And the original comment should have been untouched, because there is no password reset there.