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

Add rate limiting to two factor attempts. #510

Merged
merged 17 commits into from
Feb 13, 2023

Conversation

dd32
Copy link
Member

@dd32 dd32 commented Feb 2, 2023

This is intended as an alternative to #482 for #477, as a way of limiting exposure to brute force attacks while at the same time preserving the account password and ability for the end-user to login.

This is a draft, and is intended on offering what I feel is a potentially better UX than forced password reset, which may simply further aggravate a legitimate user who has simply used the wrong tokens.

This is implemented through two user_meta keys that store:

  • Last failed two-factor attempt
  • Number of failed two-factor attempts

Combined with a minimum time delay between 2FA attempts (Set at 5s) and incremental back-off of that delay (ie. 2nd failed attempt is 10s delay, 3rd failure is 20s delay, etc, a maximum of 1hr delay is included) prevents access to the account, but, allows the account owner to still login if they're simply using the wrong 2FA key or other similar issues.

While I set the delay at 5s, this could be as low as 1s, resulting in a time delay of 1s, 2s, 4s, 8s, 16s, 32s, 64s, etc.. While this would allow more initial guesses, it's also enough to prevent brute-force attacks being worthwhile. 5 seconds felt like a small enough yet high enough number to start with here.

The maximum time is set to 1hr, but could also be reduced to something smaller such as 15 minutes without drastically impacting upon security. As even at 15 minutes, 4 guesses per hour is unlikely to succeed.

Filters are included that another plugin could alter the rate limit, use atomic cache operations (such as wp_cache_incr()), or use a service that includes per-IP or request-matching to limit two factor attempts.

login-backoff.mp4

@dd32 dd32 requested a review from iandunn February 2, 2023 07:34
@dd32
Copy link
Member Author

dd32 commented Feb 2, 2023

Filters ... use atomic cache operations (such as wp_cache_incr())

On the topic of atomic operations...

Due to the implementation of this using meta keys, it would be possible for an attacker to throw 100 requests at the 2FA page at once, and potentially only count as one failed login attempt, this is because there's no way (without using a custom DB query) to atomically ensure that the user_meta (or transient, etc) is not being updated from another PHP thread.

This is deemed acceptable, as PHP worker exhaustion would likely occur first, causing the majority of the requests to be processed after the initial database row set, which would cause them to hit the maximum tries block. (Let's assume the first 10 requests would run in parallel, with the further 90 being queued waiting for those initial to process)

Using atomic operations, such as wp_cache_incr() wouldn't necessarily help here, other than causing it to set it to 10 failed tries immediately rather than 10 processes all updating it to 1 failed tries, the two-factor attempts would still likely be processed first.

Copy link
Member

@iandunn iandunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally still prefer the reset approach, or a hybrid. I don't have any big objection to this as a stand-alone, either, though.

Left some minor feedback inline, but nothing blocking.

class-two-factor-core.php Outdated Show resolved Hide resolved
class-two-factor-core.php Show resolved Hide resolved
class-two-factor-core.php Outdated Show resolved Hide resolved
class-two-factor-core.php Outdated Show resolved Hide resolved
class-two-factor-core.php Outdated Show resolved Hide resolved
class-two-factor-core.php Outdated Show resolved Hide resolved
class-two-factor-core.php Outdated Show resolved Hide resolved
@jeffpaul jeffpaul added this to the 0.8.0 milestone Feb 3, 2023
Co-authored-by: Ian Dunn <ian@iandunn.name>
@dd32 dd32 marked this pull request as ready for review February 8, 2023 01:15
@dd32 dd32 merged commit 4c9607b into WordPress:master Feb 13, 2023
@dd32 dd32 added the Bug label Feb 13, 2023
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.

3 participants