Skip to content
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

Core: Reset compromised passwords after 2FA failures #482

Merged
merged 4 commits into from
Feb 24, 2023

Conversation

iandunn
Copy link
Member

@iandunn iandunn commented Oct 18, 2022

Fixes #477
Builds on top of #510

After a reasonable number of 2nd-factor attempts, it's safe to assume that the password been compromised and an attacker is trying to brute force the 2nd factor.

get_user_time_delay() mitigates brute force attempts, but many 2nd factors -- like TOTP and backup codes -- are very weak on their own, so it's not safe to give attackers unlimited attempts.

Since we know that the password is compromised, we have the responsibility to reset it and inform the user. That will guarantee that attackers can't brute force it (unless they compromise the new password).

@pkevan
Copy link
Contributor

pkevan commented Nov 17, 2022

Do we want to consider the attempt counter to be using a transient to benefit those sites running with memcache etc, rather than it always being the DB? Looking at the suggested alternatives metioned that seems like the approach taken for passwords and considering a large site, and the worst case scenario (everything is compromised), then it does open it to resource saturation.

The other thing regarding resetting passwords and emailing the reset link - is this ok? what if the account is compromised via password and the email address is updated? Or am I thinking too far ahead of the scenarios scope?

@iandunn
Copy link
Member Author

iandunn commented Nov 17, 2022

using a transient to benefit those sites running with memcache

That's a good idea 👍🏻

what if the account is compromised via password and the email address is updated?

Wouldn't the attacker be prevented from changing the address until they fully authenticate (including 2FA) ? Logging in via the REST API and XML-RPC are disabled by default.

@pkevan
Copy link
Contributor

pkevan commented Nov 17, 2022

Wouldn't the attacker be prevented from changing the address until they fully authenticate (including 2FA) ? Logging in via the REST API and XML-RPC are disabled by default.

Yes, I was considering those sites who had disabled the functionality for various reasons, like syncing content for search, or applications unable to handle a 2FA workflow .

@iandunn iandunn force-pushed the reset-compromised-pw branch from 3beacc5 to 2d5c53b Compare November 30, 2022 19:16
@iandunn
Copy link
Member Author

iandunn commented Nov 30, 2022

I switched to a transient in 2d5c53b 👍🏻

sites who had disabled the functionality

🤔 If the attacker was using the REST API or XML-RPC, then that wouldn't trigger the reset. They could still trigger it if tried to login via the browser.

If they did update the address, then you're right that this wouldn't be effective. It seems like that's the risk the owner takes when enabling REST/XML, though; the entire plugin is essentially disabled at that point, so I'm not sure the user is any worse off. The user would still get a notification from Core (at their actual address) letting them know that it was changed, though.

Am I missing something, though, or is there something you'd suggest instead?

class-two-factor-core.php Outdated Show resolved Hide resolved
@pkevan
Copy link
Contributor

pkevan commented Dec 1, 2022

Am I missing something, though, or is there something you'd suggest instead?

Probably not 😄 I was just working through some scenarios where this is an edge case (Jetpack content sync) but it seems like the approach is to allow only these specifically, so it depends whether the plugin should handle this (probably not) or it's handled on a per-site basis.

@iandunn
Copy link
Member Author

iandunn commented Feb 3, 2023

@dd32 raised some good points that would improve this PR. There's also overlap with #510 that could be incorporated:

  • Bump the default trigger from 7 to ~30 so that legit users don't have their passwords reset during normal failures
  • Add an admin notice like Add rate limiting to two factor attempts. #510 when a user tries to log in with a reset password, in case they never got the email
  • Add filters that'd allow another plugin to use atomic operations like wp_cache_incr(), or more sophisticated limiting mechanisms that track IP etc.

If a hybrid approach is desirable, though, then this PR could also be refactored to build on top of failure tracking in #510, rather than implementing a separate mechanism.

After a reasonable number of 2nd-factor attempts, it's safe to assume that the password been compromised and an attacker is trying to brute force the 2nd factor.

The existing rate limit mitigates brute force attempts, but many 2nd factors -- like TOTP and backup codes -- are very weak on their own, so it's not safe to give attackers unlimited attempts. Since we know that the password is compromised, we have the responsibility to reset it and inform the user. That will guarantee that attackers can't brute force it (unless they compromise the new password).
@iandunn iandunn force-pushed the reset-compromised-pw branch from b44e7f6 to 45f2040 Compare February 17, 2023 00:51
@iandunn
Copy link
Member Author

iandunn commented Feb 17, 2023

45f2040 refactored this PR to build on top of #510, and implement the improvements mentioned above.

@dd32
Copy link
Member

dd32 commented Feb 17, 2023

Bump the default trigger from 7 to ~30 so that legit users don't have their passwords reset during normal failures

With this change, I'm supportive of this PR approach of resetting passwords, as it's far far less likely to incorrectly affect end-users.

@iandunn iandunn marked this pull request as ready for review February 17, 2023 18:35
@iandunn iandunn requested review from pkevan and dd32 February 17, 2023 18:35
class-two-factor-core.php Outdated Show resolved Hide resolved
@iandunn iandunn merged commit 1833492 into master Feb 24, 2023
@jeffpaul jeffpaul modified the milestones: 0.9.0, 0.8.0 Feb 24, 2023
@jeffpaul jeffpaul deleted the reset-compromised-pw branch March 2, 2023 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Throttle second factor attempts
4 participants