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.
| # 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.
| :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?
joshbuker
left a comment
joshbuker
left a comment
There was a problem hiding this comment.
I'll take another look at this later, some significant concerns with this particular implementation however.
| return if login_locked? && !config.limitless_counting_failed_login | ||
|
|
||
| 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.
| # 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.
3 participants
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.