brute force vulnerability

[futuretap]

Sorcery 0.14.0 is vulnerable to brute force attacks even if a login_lock_time_period is configured.

If an attacker continues their login attempts after the initial login_lock_time_period has passed, Sorcery no longer rejects those login attempts until eventually the attacker has guessed the right password.

This is caused by an early return in model/submodules/brute_force_protection.rb:68 which skips the update of the lock_expires_at field. So failed attempts don't update lock_expires_at as long as lock_expires_at contains any date, even if already passed.

I'll provide a fix.

[joshbuker]

I could be mistaken, but this sounds like a misunderstanding of how the brute force protection works.

My understanding of the functionality, is that if an account receives X invalid login attempts within a certain time frame all login attempts will be blocked until the login_lock_time_period expires. Once that time frame has passed, logins will be allowed as usual unless the bad login count is exceeded again. This process can repeat infinitely, and mainly acts as a way to slow down brute force attempts to the point of in-feasibility, while not overly burdening users.

Extending a lockout period if additional bad requests are received during the lockout is not an intended functionality, as far as I'm aware. The hook that returns if it's present on line 68 is intentional, as the lock_expires_at field will be made nil once a lock expires, using the callback on line 121.

[futuretap]

You misunderstood the point. After the initial login_lock_time_period expires, an infinite number of brute force attempts are allowed without ever considering another login_lock_time_period.

This effectively disables the brute force protection after the first login_lock_time_period has passed. It is only enabled back again after a successful login (at which point, the lock_expires_at field is set back to nil).

This is clearly a severe vulnerability.

[joshbuker]

@futuretap

base.sorcery_config.before_authenticate << :prevent_locked_user_login

Inside def prevent_locked_user_login:

Lines 120-123:

if !login_unlocked? && config.login_lock_time_period != 0
  login_unlock! if send(config.lock_expires_at_attribute_name) <= Time.now.in_time_zone
end

return false, :locked unless login_unlocked?

And inside Sorcery's authenticate method:
@sorcery_config.before_authenticate.each do |callback|

From what I can tell, we add a callback that gets called every authentication regardless of if the login is valid, which rejects a login if the account is locked.

Can you please review those spots of the code, and help me understand where the brute force protection is getting disabled after the first lockout period?

Edit: The ordering in the authenticate method will cause an authentication to fail because of an invalid password before it fails of a locked account, but it will still prevent a successful login while locked out. This might cause some bad logging for applications that log the error returned by the authenticate method, but it would not let attackers into a locked account. The error message for a bad login shouldn't be communicated to the end user for obvious security implications, so that shouldn't be a concern either, as far as I can tell.

Edit 2: That would be the case if it still ran the callbacks, but because it returns early the callback to unlock indeed only gets run after a successful login. Issue confirmed, I'll create security releases and deploy today.

[joshbuker]

@futuretap A security patch has been released (v0.15.0), and a related security adversary is currently being processed by Github. Thank you for pointing this out!