-
Notifications
You must be signed in to change notification settings - Fork 153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Throttle second factor attempts #477
Comments
One thing to consider is that some hosts like Pantheon have a timeout of 30 seconds for all PHP so it might produce unexpected side-effects and prevent users from logging in. It could also trip some debugging tools that monitor long-running PHP processes.
We could also consider failing all attempts before the timeout is reached. That would work well with a generic notice about "Please try again in X minutes/seconds". |
Er, yeah, that's a much better UX 👍🏻 It'd be nice to maybe do both, so that it ties up the scammer's resources a little bit. Maybe flush the output buffer so the user sees the error, but keep the connection open. That's probably getting too complicated, though. Your idea is much simpler 👍🏻 |
I question if this shouldn't be offloaded into plugins like limit-login-attempts & jetpack rather than being included in this plugin. Do those plugins already work with this perhaps? (I guess it would depend on which hooks they use) |
🤔 I think it makes sense to do it here because the potential vulnerability is introduced by this plugin, and we're in a much better position to be effective than a general purpose plugin. In the context of passwords you can't just block the account itself -- because that'd DDoS the real user -- so you're stuck playing whac-a-mole with IPs etc. In this plugin, we have the advantage of knowing that only the real user should be able to reach the 2nd factor to begin with. So any failed attempt is either a legit user making a typo, or a compromised password. I worry that, in practice, a large percent of users won't have a general purpose plugin installed, and even if they do it might make assumptions that make it fail or allow bypasses in this context. If folks want though, we could drop the throttling feature, and only keep the reset password feature. That's the most essential part IMO. |
This is a DOS waiting to happen. You should never sleep for throttling. Instead, after reaching a threshold, all attempts, whether with a valid code or not are just ignored. |
Yeah, I was hoping it'd be possible to do it in a way that'd avoid that, but I haven't tested that. A DoS would probably still be more likely, just not as much. I ended up not doing any throttling at all in #482 , since that doesn't address the fundamental problem that the attacker has already compromised the password. It just gets reset after 7 failed attempts on the 2nd factor. That prevents brute forcing while also fixing the root problem. |
Would be possible to change the number using a filter? I am thinking on increase it for particular installations. |
Another related question, how often does the fail attempts reset the count? Is it 7 fails in one day? in one month? |
Yeah, you can use the two-factor/class-two-factor-core.php Lines 83 to 94 in 3beacc5
Do you think The attempt counter only gets reset after a successful password is entered, there's no time-based reset. Are there scenarios where that would be beneficial? |
I think 7 is good, but I can imagine scenarios where a really high number could be useful, where this is not that important (they do not have it now so...), but it would be great to have it as a "last resort". So probably a 20 o something like that. I can see myself setting this big number for some projects.
No, if it get reset once they login, it is good enough. :) |
I am checking the code now. |
I'm not sure I understand, are you concerned about the message being flagged as spam? The emails are sent from the default |
@iandunn I just think that that email is not good for sending, but for receiving notifications, but I agree this is a default from WP and nothing wrong with your code. |
Ah, I see; that's a fair point. I think Core's |
Well, I already do that on some installations, but you could use a filter to replace the variable here: two-factor/class-two-factor-core.php Line 1010 in 3beacc5
|
The need for this came up during the security audit, and others seem to agree. @dd32 , do you feel strongly that this and #482 are the wrong approach? If not I'm leaning towards going ahead with the merge. |
I've put together #510 as an alternative to the approach of resetting the password, and instead simply rate limiting the two-factor login attempts, following a similar option as presented in this issue of simply having an incremental backoff. I'm concerned that forcing a user to reset their password after 7 failed tries might be a bit too harsh for some, and it has the potential to lock out a legitimate user entirely from their account if their email is no longer accessible or not accessible at that given point in time. It's not unknown for webservers to have outgoing email issues either. By rate limiting it like that, we're reducing the viability of brute force attacks, while hopefully aggravating as few legitimate logins as possible. I've started with it set at 5s in #510 (Which should allow for an average human to input 2 attempts before seeing the 10s delay) but it could be set even lower to allow for the average user to hit the block after 3-4 attempts, while still being a high enough lockout period to deter brute-force attacks. On some installations (or for some users), force resetting the password might be the correct way to go about it, as could maybe forcing them to use a backup code or emailed code, but none of these solutions fit every installations need.. rate limiting however is complementary to all of those "more severe" heavy handed approaches. |
In hindsight,
Fair point. That situation already exists if they lose their password, etc, though. It's also partially mitigated by Core prompting users to confirm/update it. Resetting would definitely result in more support requests for admins, but I feel like the trade off is worth it.
Very true 👍🏻 #482 could replace the emails with a
That still causes aggravation, though, just a different type. In both PRs the important thing is setting the trigger high enough that legitimate folks only encounter it rarely, if at all.
I'd be leery of having email, since it'd bypass 2FA, but it could be appropriate for some installs/users. |
I think you raised some good points, and IMO #510 is also a good way to approach it. I would definitely rather see us do that than nothing. I'm still a bit stuck on not resetting the password, though. It feels pretty bad to know that there will be situations where we have a high degree of confidence that the user's password is compromised, but will continue to allow it to be used. Given the overlap between the PRs, and various threat models/risk tolerances/etc that users have, maybe this is a case where both approaches should be used, but configurable via filters? In the interest of keeping things moving, though, I'm also fine w/ merging #510 for now and revisiting resets later. |
Hi, I know little about all this, but I want also add, in case it has not been considered on this and if it could affect, that sometimes the problem could be the mail provider. |
TIL, brute-forcing TOTP is much easier than I assumed (yay for checking assumptions!). That applies to other providers as well to varying degrees.
It seems like a simple solution would be to throttle attempts with increasing severity:
... and so on. There could be a filter on that mapping of
attempts -> delay
, to allow customization. Implementing it should be straightforward with ausermeta
counter, and asleep()
insidelogin_form_validate_2fa()
, and some kind of notice to the user.After ~8 attempts, though, it may be safest to assume that the password has been compromised, and to force a reset and email the user.
The text was updated successfully, but these errors were encountered: