Add limitless counting failed login function #184
Conversation
If stopped counting 'failed login' after reached limit, we can't tell the reason of lock either user's action or brute force attack.
| @@ -63,13 +66,14 @@ def define_brute_force_protection_fields | |||
| module InstanceMethods | |||
| # Called by the controller to increment the failed logins counter. | |||
| # Calls 'login_lock!' if login retries limit was reached. | |||
| def register_failed_login! | |||
| def register_failed_login!(password) | |||
There was a problem hiding this comment.
Is the password parameter necessary here? The method simply registers a failed login attempt, its behavior should not depend on the provided password.
Can we just reverse the order of increment call and return unless login_unlocked?. Will that have the desired effect?
There was a problem hiding this comment.
As mentioned, passing the password to this callback is unnecessary at best, and malicious at worst.
| @@ -14,6 +14,8 @@ def self.included(base) | |||
| :consecutive_login_retries_amount_limit, # how many failed logins allowed. | |||
| :login_lock_time_period, # how long the user should be banned. | |||
| # in seconds. 0 for permanent. | |||
| :limitless_counting_failed_login, # When true, continue to count failed login | |||
There was a problem hiding this comment.
I don't think another config option is needed here. Is there a reason why someone would want to disable this option?
|
|
||
| return unless send(config.failed_logins_count_attribute_name) >= config.consecutive_login_retries_amount_limit | ||
| sorcery_adapter.increment(config.failed_logins_count_attribute_name) unless valid_password?(password) |
There was a problem hiding this comment.
I suspect this might open the window to timing attacks, especially considering the trip time to a DB may be significant in certain setups.
Edit: Example of a potential attack:
- Brute force a login until it locks
- Continue brute force attack, monitoring the response time for each request
- Upon a request that takes ~50-100ms less than average, you know you've hit a correct password
- To verify that it wasn't a network hiccup, Try two other known bad passwords, and the suspected valid password a few times, compare the averages. If valid, it should average (whatever the trip time to the DB is) faster.
| @@ -63,13 +66,14 @@ def define_brute_force_protection_fields | |||
| module InstanceMethods | |||
| # Called by the controller to increment the failed logins counter. | |||
| # Calls 'login_lock!' if login retries limit was reached. | |||
| def register_failed_login! | |||
| def register_failed_login!(password) | |||
There was a problem hiding this comment.
As mentioned, passing the password to this callback is unnecessary at best, and malicious at worst.
Hi.
This function is adding limitless counting 'failed login'.
I think, if stopped counting 'failed login' after reached limit, we can't tell the reason of lock either user's action or brute force attack, so I added.
If you don't need, please close this PR
Thanks.