Problem/Motivation
Currently, User X can request a new password an infinite amount of times. This can be confirmed by the logs at admin/reports/dblog.
Update 8/15/13: The issue is still in the Drupal8 download as I have tested.
Proposed resolution
Create flood event and enforce it.
Create/modify tests in order so that this patch can pass. Since the flood protection takes action after a while the user will not be able to login after too many tries, and this the way it should work. The tests should let this pass.
Remaining tasks
Addressing concerns of @David_Rothstein in #73
I was going to say that looked like a good change, but then it occurred to me: Because the flood control is by IP address, we actually have no idea how many times the current user tried to reset their password recently (it could be zero).
Which actually means this: If many users at an organization all share the same IP address, then a malicious user at the same IP address (let's say a disgruntled employee situation) could trigger the flood control deliberately by continually requesting multiple passwords for their own account. Since there were no password reset requests for other accounts, those users will not have any login links in their e-mail to click on, nor will they be able to generate them. So they will be completely locked out of the site (assuming the attacker flooded the login form too), and the only way to get back in is to wait for someone with technical knowledge to go onto the server and fix things.
I have to ask, isn't the above scenario actually more dangerous than the simple "spam" scenario the patch is trying to protect against?
See one opinion on this tradeoff in #127 - tl;dr the benefit outweighs the risk (which is a bit of an edge-case).
User interface changes
TBD
API changes
TBD
Original report by Traverus
Currently, User X can request a new password an infinite amount of times. There should be a flood event created and enforced. I'll include the code that I used to implement my own solution to help get toward a patch for the problem.
As a for instance Bob doesn't like Alice, so he resets Alice's password 1230875109 times. This makes Alice upset because she has an equal amount of e-mails in her inbox.
Thanks!
function xyz_user_pass_form_validate($form, $form_state){
if(!flood_is_allowed('request new password', 1, 86400, $form_state['values']['name'])){
form_set_error('name', 'Reset password limit exceeded. Please contact technical support for further assistance.');
flood_register_event('request new password', 86400, $form_state['values']['name']);
} else {
flood_register_event('request new password', 86400, $form_state['values']['name']);
}
}
hook_form_user_pass_alter($form){
array_unshift($form['#validate'], 'xyz_user_pass_form_validate');
}
Comment | File | Size | Author |
---|---|---|---|
#148 | 1681832-148-interdiff.txt | 1005 bytes | kim.pepper |
#148 | 1681832-148.patch | 18.85 KB | kim.pepper |
#145 | 1681832-145-interdiff.txt | 5.25 KB | kim.pepper |
#145 | 1681832-145.patch | 18.85 KB | kim.pepper |
Comments
Comment #1
k_zoltan CreditAttribution: k_zoltan commentedAttached first patch
Unable to test it now please have a look to.
Also very strange that this isn't done before
Comment #2
k_zoltan CreditAttribution: k_zoltan commentedComment #3
coltraneNice find, I don't think there's a legitimate reason for not having flood control on this.
I assume this issue also exists in D8. We should confirm and fix there as well as D7.
As for the patch, it needs testing and I have this specific feedback:
How about a limit of 20?
Should register the event in the submit handler in case a hook alter implementation on this form sets additional validation checks. Only want to register when the email goes out.
Additionally there's no call to flood_clear_event(), but perhaps there shouldn't be because the goal of this is issue is to protect against abuse of the password reset email functionality.
Comment #4
mihai_brb CreditAttribution: mihai_brb commentedThe patch was changed in order to register the event in the submit handler user_pass_submit function.
Also the password reset limit was lowered to 20 times.
TODO: Check if the problem exists in Drupal8.
Comment #5
mihai_brb CreditAttribution: mihai_brb commentedComment #6
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis limits the password reset per user. This is not desirable. We should limit by IP address instead, by removing the
$name
parameter (here and in theflood_is_allowed()
call).Comment #7
k_zoltan CreditAttribution: k_zoltan commentedChanged limits from per user to per IP, removed the $name variable since not needed any more.
Comment #8
k_zoltan CreditAttribution: k_zoltan commentedComment #9
Rob C CreditAttribution: Rob C commented#7: password_reset_flood_1681832_3.patch queued for re-testing.
Comment #11
YesCT CreditAttribution: YesCT commented#7: password_reset_flood_1681832_3.patch queued for re-testing.
Comment #12
YesCT CreditAttribution: YesCT commentedtagging
oh, and also moving to 8.x. We can backport it to 7.x. Means we'll need a new 8.x patch.
Comment #12.0
grwgreg CreditAttribution: grwgreg commentedUpdating the issue summary
Comment #12.1
grwgreg CreditAttribution: grwgreg commentedUpdating issue
Comment #13
grwgreg CreditAttribution: grwgreg commentedI was able to reproduce this in drupal 8 and updated the issue summary. The flood functions from drupal 7 have been changed in drupal 8 to flood controllers so the code above needs to be updated for drupal 8.
Comment #13.0
grwgreg CreditAttribution: grwgreg commentedIssue updated by grwgreg
Comment #13.1
grwgreg CreditAttribution: grwgreg commented.
Comment #14
grwgreg CreditAttribution: grwgreg commentedSteps to reproduce:
Comment #15
k_zoltan CreditAttribution: k_zoltan commentedI would really help in providing the Drupal 8 patch but after seeing how much things have been changed I'm a bit scared.
Comment #16
k_zoltan CreditAttribution: k_zoltan commentedAlso maybe it would be a great thing to get the patch commited in Drupal7 because of the big difference between the d7 and the d8 versions on how this problem needs to be treated.
Comment #17
CyberschorschI added a flood check for Drupal 8. I am using the same limits as the user login for now which means 50 requests from the same ip is the current limit.
Comment #18
CyberschorschRemoved a whitespace ...
Comment #19
aiypz CreditAttribution: aiypz commentedHI,
This is my first attempt to help out here and my first use of a patch so I've described how I replicated this below (Just to make sure I've got it right).
I tested the patch and can confirm that on the 50+ attempts I get the following message: "Reset password limit exceeded. Please contact technical support for further assistance."
I can also see each reset request in the log file.
I hope this helps. Below is how I replicated it.
Thanks
Stuart
Steps to reproduce:
Install the latest Drupal 8.x release.
Applied patch above by navigating to root of drupal directory and used command: patch -p1 < user_Passwort_reset_flood_1681832_18.patch
Navigate to admin/people with the admin account.
Create a new active user with an email that would be okay with receiving 20+ emails.
Sign out of the admin account.
Navigate to /user/password and fill out the form requesting a new password for the account created in step 3.
Repeat step 5 50+ times.
Sign back in and check admin/reports/dblog for multiple "Password reset instructions mailed to..." messages.
Comment #20
k_zoltan CreditAttribution: k_zoltan commentedI think you did everything right.
The only remark is that after doing what you did
the tickets status should be changed to "reviewed and tested by the community".
But lets see what others with more experience say.
Comment #21
YesCT CreditAttribution: YesCT commented#18: user_Passwort_reset_flood_1681832_18.patch queued for re-testing.
Comment #22
alippai CreditAttribution: alippai commentedAre you sure this is the right order for
register()
andisAllowed()
? If we register not only the requests under the threshold the database is exposed to DDoS attack (unlimited writing of DB without delay or threshold).Comment #23
k_zoltan CreditAttribution: k_zoltan commentedI looked at the current code in the user module, to see how it's done the same thing for the login
in the user.module file line 1339 it checks:
if (!drupal_container()->get('flood')->isAllowed('user.failed_login_ip'
the register comes only after in the user_login_final_validate() function.
So It looks like it should be changed.
Comment #24
k_zoltan CreditAttribution: k_zoltan commentedHere is the same patch as for the #18 just modified the order of the 2 functions
Comment #25
star-szrNeeds a reroll.
Comment #26
phiit CreditAttribution: phiit commentedFirst reroll I've done, hopefully everything is in order!
Comment #27
star-szrDiscussed this with @phiit in IRC, needs another go at the reroll, perhaps applying the patch manually.
Comment #28
k_zoltan CreditAttribution: k_zoltan commentedPlease write down what you discussed, whats the conclusion, what should be done further.
Comment #29
star-szrWe just discussed that these functions have moved around significantly so doing a reroll based on the instructions at http://drupal.org/patch/reroll would likely not be successful. I suggested since it's a small patch to apply the changes to the current codebase manually.
Comment #30
k_zoltan CreditAttribution: k_zoltan commentedI just talked with Gabor Hojtsy at the drupalaton camp and now I understand what you say. I will get the latest drupal 8 from git and try to find the function.
Comment #30.0
k_zoltan CreditAttribution: k_zoltan commentedUpdated issue summary, moved the detail about what to do next from the proposed resolution to remaining tasks. took some words from the original report and used them for the proposed resolution.
Comment #31
k_zoltan CreditAttribution: k_zoltan commentedThe forget password form and its validation has been moved into the
UserPasswordForm
class that is located at the following path:core/modules/user/lib/Drupal/user/UserPasswordForm.php
This is why a reroll didn't falled but the patch is still useless.
Comment #32
k_zoltan CreditAttribution: k_zoltan commentedHere is a patch done with the help of @swentel and @aspilicious
This patch is only a flood protection by IP.
I will do the filtering for the username too.
Comment #33
k_zoltan CreditAttribution: k_zoltan commentedI made a more deeper research and I think the patch is ok as is.
Comment #35
star-szrThanks for working on this @k_zoltan!
Tagging for reroll and there is a bit of trailing whitespace in the patch that should be removed per http://drupal.org/coding-standards#indenting.
Comment #36
dougvann CreditAttribution: dougvann commentedRerolled patch [only my 2nd time submitting a rerolled patch] ;-)
Also curious why we're not using trailing commas. It seems to me that line 77 should end in a comma in preparation for any additional lines that may be added in the future. Yes?
Comment #37
dougvann CreditAttribution: dougvann commentedupdating status...
Comment #39
dougvann CreditAttribution: dougvann commentedCorrected PHP Syntax error
Comment #41
dougvann CreditAttribution: dougvann commentedtrying again
Comment #43
dougvann CreditAttribution: dougvann commented4th time's a charm?
Comment #45
ZenDoodles CreditAttribution: ZenDoodles commentedOne more try...
Comment #47
k_zoltan CreditAttribution: k_zoltan commentedI think we are doing something wrong.
I wondering do the tests work correctly?
Comment #47.0
k_zoltan CreditAttribution: k_zoltan commentedhope to clarify the issue summary by directing attention to the path to solution in #29
Comment #48
CyberschorschRerolled the patch
Comment #49
CyberschorschComment #51
k_zoltan CreditAttribution: k_zoltan commentedGreat job @Cyberschorsch
BUT
These patches will always fail on testing, since the tests are there to prevent that any functionality is broken after the patching.
But this time it should break a feature. The feature of requesting a new password eternally aka. spamming the user with the new password request.
The test needs to be changed in order to get this done.
Comment #52
k_zoltan CreditAttribution: k_zoltan commentedComment #53
eshta CreditAttribution: eshta commentedI'll work on tests for this one today.
Comment #54
eshta CreditAttribution: eshta commentedSo I've gotten it to a point where the tests all pass. There were missing imports. Seems like we should be adding an additional test, however, of this new functionality. I'm working on adding that before I push up the new patch.
Comment #55
eshta CreditAttribution: eshta commentedAdding a new patch for the following:
Comment #56
ZenDoodles CreditAttribution: ZenDoodles commented55: drupal8.user-module.1681832-55.patch queued for re-testing.
Comment #58
ZenDoodles CreditAttribution: ZenDoodles commentedRerolled and created a tests-only patch.
Comment #59
ZenDoodles CreditAttribution: ZenDoodles commentedComment #61
hrbel CreditAttribution: hrbel commentedI will review the change during sprint weekend
Comment #62
hrbel CreditAttribution: hrbel commentedComment #63
eshta CreditAttribution: eshta commentedThanks ZenDoodles for catching this up! I still have a little time if there is more to do :-)
Comment #64
k_zoltan CreditAttribution: k_zoltan commentedUpdated issue summary as best as I understand the situation.
Comment #65
amateescu CreditAttribution: amateescu commentedThis patch looks mostly ok, here's a small nitpicky review :)
Double space before "Please".
Very minor comment here, we must start with a third person singular present tense verb. In this case: "Tests ..." and "Makes ...".
We're missing type hints for these parameters: "string", "bool" and "bool".
Also $flood_trigger should default to FALSE (instead of NULL) in the function signature since it is actually a boolean.
We're converting $flood_trigger to a boolean so we should remove the
isset()
usage here.We generally put an empty line after the last method of a class.
Comment #66
k_zoltan CreditAttribution: k_zoltan commentedFixes for the review above.
Comment #67
amateescu CreditAttribution: amateescu commentedThe patch looks good to me now.
Comment #68
David_Rothstein CreditAttribution: David_Rothstein commentedI won't un-RTBC over a text change, but this definitely looks wrong. Who is "technical support"??? And we shouldn't say "please" in user interface text.
I think this needs to be a more helpful error message that explains to the user how they can actually log in (that's what the flood control message on the user login form does)... In this case, I guess it needs to tell people to check their e-mail and use one of the existing password reset links they find there?
Comment #69
k_zoltan CreditAttribution: k_zoltan commentedHow about the following message "Sorry, there were to many failed password recovery attempts for this account. Check your e-mail and use one of the password reset instructions from there." I will do patch and interdiff later.
Comment #70
k_zoltan CreditAttribution: k_zoltan commentedUpdated patch as promised. Actually the same patch as #66 just changed the error message.
Comment #71
StuartJNCC CreditAttribution: StuartJNCC commented"to many" should be "too many".
This has become rather verbose. How about something like: "You have tried to reset your password too many times. Check your e-mail for instructions on how to proceed."
Comment #72
YesCT CreditAttribution: YesCT commentedNovice for someone to do that change.
https://drupal.org/contributor-tasks/create-patch (that also has links to docs with one of the steps linking to how to do an interdiff)
Comment #73
David_Rothstein CreditAttribution: David_Rothstein commentedI was going to say that looked like a good change, but then it occurred to me: Because the flood control is by IP address, we actually have no idea how many times the current user tried to reset their password recently (it could be zero).
Which actually means this: If many users at an organization all share the same IP address, then a malicious user at the same IP address (let's say a disgruntled employee situation) could trigger the flood control deliberately by continually requesting multiple passwords for their own account. Since there were no password reset requests for other accounts, those users will not have any login links in their e-mail to click on, nor will they be able to generate them. So they will be completely locked out of the site (assuming the attacker flooded the login form too), and the only way to get back in is to wait for someone with technical knowledge to go onto the server and fix things.
I have to ask, isn't the above scenario actually more dangerous than the simple "spam" scenario the patch is trying to protect against?
Comment #74
sunSince the flood protection is an all-or-nothing condition, I'd prefer to use an early-return in this method, instead of indenting the whole function body.
Since the default IP limit is relatively high (50), I wonder whether we should additionally use a per-user limit (which defaults to 5)?
So that a single IP is limited to 50 attempts, but in addition, a single given name or email (if found/valid) is limited to 5?
Or is that overkill?
It looks like this could be a
DrupalUnitTestBase
, which programmatically executes the form instead.In a DUTB test, you can mock a
Request
and pass a custom IP, which in turn allows you to additionally test that the form still works for a different IP.An assertion method should not perform more operations than the assertion(s) itself.
The
drupalGet()
anddrupalPostForm()
should not be part of the assertion method.In addition, a single assertion method should not have a Boolean switch to negate the expectation.
Instead of such arguments, use separate assertion methods instead. The negated ones are typically use "No" in their method names; i.e.:
assertValidPasswordReset()
assertNoValidPasswordReset()
assertPasswordFlood()
assertNoPasswordFlood()
Comment #75
k_zoltan CreditAttribution: k_zoltan commentedI will start working on point 1, 3 and 4.
Point 2 should be discussed.
Updating Issue summary.
Comment #76
k_zoltan CreditAttribution: k_zoltan commentedA few more simple issue summary updates
Comment #77
k_zoltan CreditAttribution: k_zoltan commentedDone point 1 of the comment #74.
The interdiff looks so strange because the of the early return the indentation changed.
Just marked as need review for testing. Actually it needs work.
Comment #78
k_zoltan CreditAttribution: k_zoltan commentedJust marked as need review for testing. Actually it needs work.
Comment #79
k_zoltan CreditAttribution: k_zoltan commentedComment #80
alimac CreditAttribution: alimac commentedMinor tag cleanup - please ignore.
Comment #81
k_zoltan CreditAttribution: k_zoltan commentedWell then lets add this tag too. :)
Comment #84
deepakaryan1988Removing sprint weekend tag!!
As suggested by @YesCT
Comment #85
star-szr:)
Comment #86
YesCT CreditAttribution: YesCT commented@deepakaryan1988 The tag was added January 17, 2015 during the actual global sprint weekend. See https://groups.drupal.org/node/447258
Please leave the tag on issues that were actually worked on during that event.
I think you might have confused a message from May about the sprint weekend tag, that it should not be added to events from May. The tag is for issues worked on during https://groups.drupal.org/node/447258 by people attending sprint weekend sprints.
Comment #89
dpiComment #92
Rob C CreditAttribution: Rob C commentedAdding needs reroll tag to attract some attention + adding some related issues.
Comment #94
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedComment #95
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedRerolling patch #77 :)
Comment #96
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedComment #98
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedComment #100
alexharries CreditAttribution: alexharries commentedI appreciate this is an old task and my post is very much too late to change anything, but perhaps the following approach might be better at preventing people from being locked out of their accounts:
- Each password reset is valid for 48 hours per account;
- We don't send password reset e-mails more-frequently than once every two hours;
- The token used in password reset e-mails in the same 48 hour period is the same.
In a bit more detail...
- When password reset is requested for a username, a token is created and saved in the database which is valid for 48 hours, and an e-mail with a link containing the token is sent to the user's e-mail address (so far, pretty straight-forward). We note the time that the e-mail was sent,
- If another password reset is requested for that username fewer than two hours since the last e-mail was sent, we don't send an e-mail. Instead, we show a message along the lines of, "We have sent you a password reset e-mail within the last two hours. Please check your junk/spam folder in your e-mail client. If you still haven't received your e-mail within another 2 hours/1 hour/30 minutes [don't specify the exact time here; there's no need for that level of detail], please fill out this password reset form again." - this step should not need write to the DB at all (to reduce overhead from write operations) - it can be entirely read-only.
- Cron can be used to clean up the expired tokens.
This approach will greatly reduce the chances of malicious password resets flooding the log tables or sending out many hundreds of e-mails, without allowing a user to become locked out of their account.
Just my $0.0002-worth though, and as mentioned before I appreciate this is way too late to change things now.
Alex
Comment #101
mcdruidUpdated patch with the codesniff fix (switch to short array syntax on one line).
Not sure if there are still other test failures which need to be addressed; we should see in due course.
Comment #103
mcdruidLooks like the constructor of
UserPasswordForm
has changed and I think that was causing multiple test failures.I've also changed another couple of codesniffer fixes (redundant
Use
's).Comment #105
mcdruidCouple of other things needed a tweak; I'll let the interdiff speak for itself.
Also including an interdiff showing changes from patch #98 which is where I started.
Tests pass locally now.
Comment #107
mcdruidThe tests that I was actually running passed locally :)
Looks like another test which is affected by the patch is failing because it's calling a non-existent method. This hopefully fixes that, but looks like we'll then hit failures within this test along the lines of:
...and perhaps some others.
Hopefully this is a little closer than when I started on it today though (interdiff based on that).
Comment #109
mcdruidSetting to NR to update the tests, but this will still fail.
I've corrected a spelling mistake, and hopefully the error due to trying to change an immutable config.
Still a few more failures to work through though.
Comment #111
mcdruidA few small tweaks were required to get tests to pass locally (including one which meant the actual change to the form validation was broken).
All passing locally now; fingers crossed the testbot agrees...
(adding DevDaysLisbon tag as that's where I am working on this :)
Comment #112
mcdruidGreat, so the patch is passing tests again.
However, looking back at #74 and #75 there may be some more work to do,
Having a look at points 2, 3 and 4 from #74.
Comment #113
mcdruidImplemented the suggested refactoring of the test(s) - point 4 from [#74].
(I also lowered the number of resets we're expecting to succeed from 5 to 3 to make the test little lighter - I don't see any reason to do more iterations).
I think point 2 (a per user limit) seems like a decent suggestion.
Point 3 ("It looks like this could be a
DrupalUnitTestBase
") seems like a more significant change - I'm not going to tackle that just yet.Back to NR to check the new test code.
Comment #114
mcdruidImplemented per-user flood control following the same logic as core/modules/user/src/Form/UserLoginForm.php
Changed the wording of the messages slightly to match UserLoginForm more closely too.
Updated the existing tests to use the per-user flood control initially. This seems to be working.
However, this means there's no testing of the IP-based flood control at present.
The (last remaining) point 3 from [#74] was to change the test to be a UnitTest instead, one of the benefits of doing so being that a mocked request could pass custom IPs which would allow us to test the IP-based flood control properly. So that's probably the next todo.
Comment #115
pdenooijer CreditAttribution: pdenooijer as a volunteer commentedMore security is always better, so good work so far! I found some improvements.
Why change
LanguageManagerInterface
toLanguageManager
, seems like this violates the dependency inversion principle.This code could be replaced with a more descriptive function:
with:
and
I guess my naming can be improved as well, but it should :).
Comment #116
mcdruidThanks for the suggestions pdenooijer!
IIRC I had to change
LanguageManager
to get the patch working again, but I can take another look at that.I think the suggestion of moving the user identifier logic into a helper function is a good one, but the code's pretty much copypasta from
\Drupal\user\Form\UserLoginForm::validateAuthentication
so I can see some benefit in staying consistent with that. Perhaps one for a followup issue?In the meantime, I've tweaked the tests so that we do now test both the user- and IP-based flood control. The latter is done a little crudely by submitting multiple reset requests with random user names so that the IP limit is triggered.
Back to NR for testbot, but I will look at LanguageManager again.
Comment #117
pdenooijer CreditAttribution: pdenooijer as a volunteer commentedNo problem! The
LanguageManagerInterface
was used pre patch, so that's why I was wondering why you changed it.I added the following changes to your patch:
configFactory
field, as it is already defined in the extended class.flood
field, as this is defined last.EntityTypeManager
instead of the deprecatedEntityManager
.The helper function idea could indeed be picked up in a follow up. Then the UserLoginForm could also be improved in the same iteration.
Comment #118
pdenooijer CreditAttribution: pdenooijer as a volunteer commentedNo idea why it changed to
needs work
...Comment #120
davidwhthomas CreditAttribution: davidwhthomas commentedNeed to mention, this issue affects programmatic API submissions to
/user/password?_format=json
because there appears to be no flood control in
Drupal\user\Controller\UserAuthenticationController\resetPassword
Comment #122
drummAttached is a re-roll of #117 for 8.8.x, it looks like at least one line of code was fixed by a separate issue upstream already. https://git.drupalcode.org/project/drupal/commit/408c540ccf2#08e2732ea90...
I’m not as-confident in the re-rolling of the tests, which moved files, but still mostly cleanly applied.
Re #120 - to limit the scope of this issue, I think throttling API requests might want to be a separate issue. For my use case, I’d probably just block the API endpoint, and just this issue would help.
Comment #123
drummComment #125
drummThis cleans up the coding standards message, and some deprecation warnings.
Comment #126
drummI split out a D7 backport issue at #3074666: [D7] Password reset form has no flood protection.
Comment #127
mcdruidThe IS still mentions addressing concerns raised by @David_Rothstein in #73:
So a malicious user who can send reset requests from a shared IP address could indeed trigger the IP-based flood control and prevent other users requesting password resets from that same IP address (within the time window). However, (as David mentioned) the users wouldn't be locked out unless the bad actor also triggered IP-based flood control on the login form. At this point, intervention may be required from "someone with technical knowledge".
I'm not actually sure how many people without "technical knowledge" would know that requesting a password reset / one-time login link was a way to circumvent the flood control blocking the login form :)
Also, as time passes the flood events will be cleared, so the problem goes away unless the malicious user maintains the DoS.
I, for one, think that the benefit of implementing flood control for password resets (with a fairly high default limit of 50 for the IP-based restriction) outweighs the potential downside outlined here, especially as this sort of DoS requires the bad actor to be able to send reset requests from the IP they are trying to get blocked.
The other "Remaining task" was implementing user-based flood control in addition to the IP-based. We added that in #114.
Note also that the D7 backport over in #3074666: [D7] Password reset form has no flood protection is RTBC and has been deployed to drupal.org
I'll edit the IS accordingly, and am going to mark this as RTBC.
Comment #128
mcdruidComment #129
pdenooijer CreditAttribution: pdenooijer as a volunteer commentedNice :)!
Comment #130
klausiThe problem with this patch is that it never clears the flood events.
Example:
* User requests new password 5 times because the email gateway used does not deliver fast enough.
* The user is now blocked by flood control from requesting more password reset links
* The old emails finally arrive at the user and they log in, thereby updating their login timestamp in the database. That immediately means that all old password reset URLs in the emails are invalid now.
* The user forgets to set a password and logs out.
* They want to request a new password again, but are still blocked from flood control.
Proposed solution:
* When the password reset link is used then flood_clear_event() should be called with the identifier for the given user. That way the password reset form immediately works again when a user logs in and we avoid this lock-out scenario.
Let me know if you agree, then we should set this back to "needs work" and also cover this with a test case. Related issue: #992540: Nothing clears the "5 failed login attempts" security message when a user resets their own password
Comment #131
catchAgreed with #130. Back to NW for that.
Comment #132
mcdruidOk, interesting point. We actually discussed this briefly over in the D7 backport issue: #3074666-7: [D7] Password reset form has no flood protection.
I think we may need to change the way we're doing the user based-flood control in terms of the identifier we use. At present this is copied from the login form implementation and by default the identifier is made up of both the uid and the IP address. The problem with clearing flood events based on that identifier is that we can't guarantee that the reset request and the login will come from the same IP.
If we want to clear the user-based flood events (and I agree that it'd be a good change to make), the simplest solution is probably to use only the uid as the identifier (which is a configurable option in the existing code, but not the default). That would mean we could derive the identifier reliably when a user logs in with the reset link, and could therefore clear the user-based flood events.
I'm happy to rework the patch along those lines.
Comment #133
pdenooijer CreditAttribution: pdenooijer as a volunteer commentedOther idea would be to keep the same behaviour, but instead change the way it is saved in the database. The uid should be saved for every entry separately beside the identifier, that can be a combination of multiple things (like uid and ip). This way when a user logs in successfully the old entries can easily be cleared.
Comment #134
mcdruidHmm, whilst I think adding the uid as an extra field in what's recorded by flood is a pretty decent idea, it's more obvious (to me) how that would work for user-based events as opposed to the flood events where we only record an IP. I'm thinking, for example of how the events are recorded in \Drupal\Core\Flood\MemoryBackend. I'm sure it's possible but perhaps a bid fiddly. It'd also need a change to the schema used by \Drupal\Core\Flood\DatabaseBackend. I don't know if there are any alternative implementations using e.g. redis or memcache, but if so those would need to be adapted for a change to the interface too.
I'm not actually certain it's worth jumping through those hoops as I don't think it'd be bad to use uid only for the user-based flood events.
What are we really trying to achieve here? Flood control on login prevents (or at least impedes) brute force attempts. The flood control on password resets is arguably more aimed at preventing bad actors being a nuisance by filling users' mail inboxes with hundreds of password reset e-mails. A determined bad actor could circumvent the flood control where the identifier is based on uid + IP by sending reset requests from lots of different IPs (e.g. using tor).
Changing (the default) to a uid-only identifier would better protect users from being flooded with reset e-mails. If we regard it as a downside that doing so makes it easier for a bad actor to lock a user out of being able to request password resets... realistically how much of a problem is that? Unless the bad actor has a way of forcing the user to actually be locked out of the site unless they can request a reset (along the lines of David Rothstein's original concern), I'd argue that the protection adding uid-only user-based flood control outweighs any potential downsides.
Here's a patch which implements uid-only identifiers and clearing of flood events when a password reset is successful. If existing tests pass, we can add more tests to verify that the clearing of the flood events is successful.
Comment #136
mcdruidOops, that's what happens when you try to rush a patch out before you have to jump off a train :)
Let's try that again; interdiff is still against #125 as that's more meaningful.
Comment #137
mcdruidAdded a test which verifies that a successful password reset (login) clears flood events for a user, so they can request another password reset even if they were at the threshold for being blocked before using the last reset URL they requested, per @klausi's suggestion in #130.
If we're happy with this approach, we should backport it to #3074666: [D7] Password reset form has no flood protection.
Comment #138
mcdruid@klausi does this address your concerns (in #130)?
If so, happy for this to go back to RTBC?
Comment #139
mcdruidMoving this back to RTBC as I think we've addressed the concerns in #130.
Remaining question is whether we're happy using only the uid for the user-based flood control.
I'd argue it's okay, and that any site having trouble with it could adjust the thresholds (IP-based vs. user-based) if needs be.
Comment #140
pdenooijer CreditAttribution: pdenooijer at Ordina Digital Services for RTL Nieuws commentedRTBC +1
Comment #141
larowlanCan we do the BC dance here?
e.g. change it to $flood = NULL and if we get passed NULL, fetch it from the singleton and trigger a deprecation error - there are classes in contrib that subclass this - see http://grep.xnddx.ru/node/29192493
And yes, this isn't part of our API, but that doesn't mean we can't be nice about it so that the minor upgrade is a bit smoother for folks running said modules.
😿same here re BC - http://grep.xnddx.ru/node/368235 there are subclasses in contrib
Comment #142
mcdruidThanks!
This patch hopefully adds the BC dance for the new params in the two constructors.
Comment #143
mcdruidBack to RTBC on the basis that I'm pretty sure we've done what @larowlan suggested.
Comment #144
larowlanThanks @mcdruid - we just need to add a deprecation test to ensure the error is triggered.
See e.g.
\Drupal\Tests\Core\Entity\ContentEntityFormTest
for an exampleI'm keeping a close eye on this as its on my wishlist for 8.8
Comment #145
kim.pepperAdded deprecation tests.
Comment #146
andypostHaving 2 legacy tests in this case is fine, RTBC++
Comment #148
kim.pepperFix test fail.
Comment #149
mcdruidGreat! So I think we're back to RTBC hopefully.
Comment #150
larowlanThanks, agree this is ready - we just need a change record.
Setting to NR for that.
Comment #151
kim.pepperAdded a basic change record. Let me know if it needs more info.
Comment #152
mcdruidThanks - CR looks good. I tweaked a couple of words as it mentioned log messages, which we're not actually doing here (they're just errors set on the form).
If/when #2983395: user module's flood controls should do better logging lands we might want to revisit this to see if we want to add logging and 403 responses, but I'd prefer to get the foundations in rather than block this one.
Comment #153
larowlanComment #155
larowlanCommitted dc7b06c and pushed to 8.8.x. Thanks!
Published the change record
Thanks all!