Skip to content

Add limitless counting failed login function #184

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/generators/sorcery/templates/initializer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,11 @@
#
# user.login_lock_time_period =

# When true, sorcery continue to count failed login after reached limit.
# Default: `false`
#
# user.limitless_counting_failed_login =

# Unlock token attribute name
# Default: `:unlock_token`
#
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ module InstanceMethods
# Runs as a hook after a failed login.
def update_failed_logins_count!(credentials)
user = user_class.sorcery_adapter.find_by_credentials(credentials)
user.register_failed_login! if user
user.register_failed_login!(credentials[1]) if user
end

# Resets the failed logins counter.
Expand Down
12 changes: 8 additions & 4 deletions lib/sorcery/model/submodules/brute_force_protection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think another config option is needed here. Is there a reason why someone would want to disable this option?

# after reached limit.
:unlock_token_attribute_name, # Unlock token attribute name
:unlock_token_email_method_name, # Mailer method name
:unlock_token_mailer_disabled, # When true, dont send unlock token via email
Expand All @@ -25,6 +27,7 @@ def self.included(base)
:@lock_expires_at_attribute_name => :lock_expires_at,
:@consecutive_login_retries_amount_limit => 50,
:@login_lock_time_period => 60 * 60,
:@limitless_counting_failed_login => false,

:@unlock_token_attribute_name => :unlock_token,
:@unlock_token_email_method_name => :send_unlock_token_email,
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

As mentioned, passing the password to this callback is unnecessary at best, and malicious at worst.

config = sorcery_config
return unless login_unlocked?

sorcery_adapter.increment(config.failed_logins_count_attribute_name)
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)
Copy link
Member

@joshbuker joshbuker Jun 5, 2021

Choose a reason for hiding this comment

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

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:

  1. Brute force a login until it locks
  2. Continue brute force attack, monitoring the response time for each request
  3. Upon a request that takes ~50-100ms less than average, you know you've hit a correct password
  4. 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.


return if login_locked? || send(config.failed_logins_count_attribute_name) < config.consecutive_login_retries_amount_limit

login_lock!
end
Expand Down
61 changes: 50 additions & 11 deletions spec/shared_examples/user_brute_force_protection_shared_examples.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
shared_examples_for 'rails_3_brute_force_protection_model' do
let(:user) { create_new_user }
let(:email) { '[email protected]' }
let(:valid_password) { 'secret' }
let(:invalid_password) { 'foobar' }
let(:user) { create_new_user(username: 'foo_bar', email: email, password: valid_password) }
before(:each) do
User.sorcery_adapter.delete_all
end
Expand Down Expand Up @@ -38,6 +41,12 @@
expect(config.login_lock_time_period).to eq 2.hours
end

it "enables configuration option 'limitless_counting_failed_login'" do
sorcery_model_property_set(:limitless_counting_failed_login, :my_limitless_counting_failed_login)

expect(config.limitless_counting_failed_login).to eq :my_limitless_counting_failed_login
end

describe '#login_locked?' do
it 'is locked' do
user.send("#{config.lock_expires_at_attribute_name}=", Time.now + 5.days)
Expand All @@ -51,12 +60,12 @@
end
end

describe '#register_failed_login!' do
describe '#register_failed_login!(password)' do
it 'locks user when number of retries reached the limit' do
expect(user.lock_expires_at).to be_nil

sorcery_model_property_set(:consecutive_login_retries_amount_limit, 1)
user.register_failed_login!
user.register_failed_login!(invalid_password)
lock_expires_at = User.sorcery_adapter.find_by_id(user.id).lock_expires_at

expect(lock_expires_at).not_to be_nil
Expand All @@ -69,7 +78,7 @@
sorcery_model_property_set(:login_lock_time_period, 0)
sorcery_model_property_set(:unlock_token_mailer, SorceryMailer)

3.times { user.register_failed_login! }
3.times { user.register_failed_login!(invalid_password) }

expect(ActionMailer::Base.deliveries.size).to eq 0
end
Expand All @@ -84,30 +93,60 @@
end

it 'does not automatically send unlock email' do
3.times { user.register_failed_login! }
3.times { user.register_failed_login!(invalid_password) }

expect(ActionMailer::Base.deliveries.size).to eq 1
end

it 'generates unlock token before mail is sent' do
3.times { user.register_failed_login! }
3.times { user.register_failed_login!(invalid_password) }

expect(ActionMailer::Base.deliveries.last.body.to_s.match(user.unlock_token)).not_to be_nil
end
end

context 'limitless_counting_failed_login is true' do
before do
sorcery_model_property_set(:consecutive_login_retries_amount_limit, 1)
sorcery_model_property_set(:limitless_counting_failed_login, true)
2.times { user.register_failed_login!(invalid_password) }
end

it 'increment failed logins count attribute with invalid password after reached limit' do
expect(user.failed_logins_count).to eq 2
end

it 'does not increment failed logins count attribute with valid password after reached limit' do
user.register_failed_login!(valid_password)
expect(user.failed_logins_count).to eq 2
end
end

context 'limitless_counting_failed_login is false' do
before do
sorcery_model_property_set(:consecutive_login_retries_amount_limit, 1)
sorcery_model_property_set(:limitless_counting_failed_login, false)
end

it 'does not increment failed logins count attribute after reached limit' do
user.register_failed_login!(invalid_password)

expect(user.failed_logins_count).to eq 1
end
end
end

context '.authenticate' do
it 'unlocks after lock time period passes' do
sorcery_model_property_set(:consecutive_login_retries_amount_limit, 2)
sorcery_model_property_set(:login_lock_time_period, 0.2)
2.times { user.register_failed_login! }
2.times { user.register_failed_login!(invalid_password) }

lock_expires_at = User.sorcery_adapter.find_by_id(user.id).lock_expires_at
expect(lock_expires_at).not_to be_nil

Timecop.travel(Time.now.in_time_zone + 0.3)
User.authenticate('[email protected]', 'secret')
User.authenticate(email, valid_password)

lock_expires_at = User.sorcery_adapter.find_by_id(user.id).lock_expires_at
expect(lock_expires_at).to be_nil
Expand All @@ -118,12 +157,12 @@
sorcery_model_property_set(:consecutive_login_retries_amount_limit, 2)
sorcery_model_property_set(:login_lock_time_period, 0)

2.times { user.register_failed_login! }
2.times { user.register_failed_login!(invalid_password) }

unlock_date = user.lock_expires_at
Timecop.travel(Time.now.in_time_zone + 1)

user.register_failed_login!
user.register_failed_login!(invalid_password)

expect(user.lock_expires_at.to_s).to eq unlock_date.to_s
Timecop.return
Expand All @@ -135,7 +174,7 @@
sorcery_model_property_set(:consecutive_login_retries_amount_limit, 2)
sorcery_model_property_set(:login_lock_time_period, 0)
sorcery_model_property_set(:unlock_token_mailer, SorceryMailer)
3.times { user.register_failed_login! }
3.times { user.register_failed_login!(invalid_password) }

expect(user.unlock_token).not_to be_nil

Expand Down