From e30d15717db65d24e170cb958c5967bcd150a715 Mon Sep 17 00:00:00 2001 From: Zach Margolis Date: Thu, 25 May 2017 17:25:19 -0400 Subject: [PATCH 1/2] Lock users out temporarily after too many OTPs **Why**: Use the database rather than rack-attack --- .../concerns/two_factor_authenticatable.rb | 17 +++++- .../two_factor_authentication_controller.rb | 11 ++++ app/services/analytics.rb | 1 + app/services/otp_rate_limiter.rb | 42 +++++++++++++ ...25180217_add_otp_rate_limiting_to_users.rb | 6 ++ db/schema.rb | 2 + ...o_factor_authentication_controller_spec.rb | 40 ++++++++++++- spec/features/idv/flow_spec.rb | 2 + .../change_factor_spec.rb | 2 + .../two_factor_authentication/sign_in_spec.rb | 33 ++++++++++ spec/services/otp_rate_limiter_spec.rb | 60 +++++++++++++++++++ 11 files changed, 214 insertions(+), 2 deletions(-) create mode 100644 app/services/otp_rate_limiter.rb create mode 100644 db/migrate/20170525180217_add_otp_rate_limiting_to_users.rb create mode 100644 spec/services/otp_rate_limiter_spec.rb diff --git a/app/controllers/concerns/two_factor_authenticatable.rb b/app/controllers/concerns/two_factor_authenticatable.rb index 837c867cf68..e793e556cdb 100644 --- a/app/controllers/concerns/two_factor_authenticatable.rb +++ b/app/controllers/concerns/two_factor_authenticatable.rb @@ -33,6 +33,16 @@ def handle_second_factor_locked_user(type) ) end + def handle_too_many_otp_sends + analytics.track_event(Analytics::MULTI_FACTOR_AUTH_MAX_SENDS) + decorator = current_user.decorate + sign_out + render( + 'two_factor_authentication/shared/max_login_attempts_reached', + locals: { type: :otp, decorator: decorator } + ) + end + def require_current_password redirect_to user_password_confirm_path end @@ -52,7 +62,12 @@ def reset_attempt_count_if_user_no_longer_locked_out UpdateUser.new( user: current_user, - attributes: { second_factor_attempts_count: 0, second_factor_locked_at: nil } + attributes: { + second_factor_attempts_count: 0, + second_factor_locked_at: nil, + otp_send_count: 0, + otp_last_sent_at: nil, + } ).call end diff --git a/app/controllers/users/two_factor_authentication_controller.rb b/app/controllers/users/two_factor_authentication_controller.rb index 76e13eef3f4..382ccba545c 100644 --- a/app/controllers/users/two_factor_authentication_controller.rb +++ b/app/controllers/users/two_factor_authentication_controller.rb @@ -35,12 +35,19 @@ def reauthn_param end def handle_valid_otp_delivery_preference(method) + if otp_rate_limiter.exceeded_otp_send_limit? + otp_rate_limiter.lock_out_user + + return handle_too_many_otp_sends + end + send_user_otp(method) session[:code_sent] = 'true' redirect_to login_two_factor_path(otp_delivery_preference: method, reauthn: reauthn?) end def send_user_otp(method) + otp_rate_limiter.increment current_user.create_direct_otp job = "#{method.capitalize}OtpSenderJob".constantize @@ -70,5 +77,9 @@ def phone_to_deliver_to user_session[:unconfirmed_phone] end + + def otp_rate_limiter + @_otp_rate_limited ||= OtpRateLimiter.new(current_user) + end end end diff --git a/app/services/analytics.rb b/app/services/analytics.rb index 577e68c2b86..96811367afd 100644 --- a/app/services/analytics.rb +++ b/app/services/analytics.rb @@ -55,6 +55,7 @@ def uuid MULTI_FACTOR_AUTH_ENTER_OTP_VISIT = 'Multi-Factor Authentication: enter OTP visited'.freeze MULTI_FACTOR_AUTH_MAX_ATTEMPTS = 'Multi-Factor Authentication: max attempts reached'.freeze MULTI_FACTOR_AUTH_PHONE_SETUP = 'Multi-Factor Authentication: phone setup'.freeze + MULTI_FACTOR_AUTH_MAX_SENDS = 'Multi-Factor Authentication: max otp sends reached'.freeze OPENID_CONNECT_BEARER_TOKEN = 'OpenID Connect: bearer token authentication'.freeze OPENID_CONNECT_LOGOUT = 'OpenID Connect: logout'.freeze OPENID_CONNECT_REQUEST_AUTHORIZATION = 'OpenID Connect: authorization request'.freeze diff --git a/app/services/otp_rate_limiter.rb b/app/services/otp_rate_limiter.rb new file mode 100644 index 00000000000..da5f07cd76f --- /dev/null +++ b/app/services/otp_rate_limiter.rb @@ -0,0 +1,42 @@ +class OtpRateLimiter + def initialize(current_user) + @current_user = current_user + end + + def exceeded_otp_send_limit? + otp_last_sent_at = current_user.otp_last_sent_at + now = Time.zone.now + + otp_last_sent_at.present? && + otp_last_sent_at + otp_findtime > now && + current_user.otp_send_count >= otp_maxretry_times + end + + def lock_out_user + UpdateUser.new( + user: current_user, + attributes: { + second_factor_locked_at: Time.zone.now, + otp_last_sent_at: nil, + otp_send_count: 0, + } + ).call + end + + def increment + current_user.otp_last_sent_at = Time.zone.now + current_user.otp_send_count += 1 + end + + private + + attr_reader :current_user + + def otp_findtime + Figaro.env.otp_delivery_blocklist_findtime.to_i.minutes + end + + def otp_maxretry_times + Figaro.env.otp_delivery_blocklist_maxretry.to_i + end +end diff --git a/db/migrate/20170525180217_add_otp_rate_limiting_to_users.rb b/db/migrate/20170525180217_add_otp_rate_limiting_to_users.rb new file mode 100644 index 00000000000..29f9e7a492a --- /dev/null +++ b/db/migrate/20170525180217_add_otp_rate_limiting_to_users.rb @@ -0,0 +1,6 @@ +class AddOtpRateLimitingToUsers < ActiveRecord::Migration + def change + add_column :users, :otp_send_count, :integer, default: 0 + add_column :users, :otp_last_sent_at, :timestamp + end +end diff --git a/db/schema.rb b/db/schema.rb index 4895d88e652..61ea52ddf08 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -170,6 +170,8 @@ t.string "attribute_cost" t.text "encrypted_phone" t.integer "otp_delivery_preference", default: 0, null: false + t.integer "otp_send_count", default: 0 + t.datetime "otp_last_sent_at" end add_index "users", ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true, using: :btree diff --git a/spec/controllers/users/two_factor_authentication_controller_spec.rb b/spec/controllers/users/two_factor_authentication_controller_spec.rb index 02214bcae8c..c1db46b6e90 100644 --- a/spec/controllers/users/two_factor_authentication_controller_spec.rb +++ b/spec/controllers/users/two_factor_authentication_controller_spec.rb @@ -111,7 +111,8 @@ def index describe '#send_code' do context 'when selecting SMS OTP delivery' do before do - sign_in_before_2fa + @user = create(:user) + sign_in_before_2fa(@user) @old_otp = subject.current_user.direct_otp allow(SmsOtpSenderJob).to receive(:perform_later) end @@ -147,6 +148,43 @@ def index get :send_code, otp_delivery_selection_form: { otp_delivery_preference: 'sms' } end + + it 'increments the otp_send_count each time' do + expect(@user.reload.otp_send_count).to eq(0) + + get :send_code, otp_delivery_selection_form: { otp_delivery_preference: 'sms' } + + expect(@user.reload.otp_send_count).to eq(1) + + get :send_code, otp_delivery_selection_form: { otp_delivery_preference: 'sms' } + + expect(@user.reload.otp_send_count).to eq(2) + end + + it 'sets otp_last_sent_at each time' do + expect(@user.reload.otp_last_sent_at).to be_nil + + now = Time.zone.now + get :send_code, otp_delivery_selection_form: { otp_delivery_preference: 'sms' } + + expect(@user.reload.otp_last_sent_at.to_i).to eq(now.to_i) + + Timecop.freeze(now + 10) do + get :send_code, otp_delivery_selection_form: { otp_delivery_preference: 'sms' } + end + + expect(@user.reload.otp_last_sent_at.to_i).to eq(now.to_i + 10) + end + + it 'marks the user as locked out after too many attempts' do + expect(@user.second_factor_locked_at).to be_nil + + (Figaro.env.otp_delivery_blocklist_maxretry.to_i + 1).times do + get :send_code, otp_delivery_selection_form: { otp_delivery_preference: 'sms' } + end + + expect(@user.reload.second_factor_locked_at.to_f).to be_within(0.1).of(Time.zone.now.to_f) + end end context 'when selecting voice OTP delivery' do diff --git a/spec/features/idv/flow_spec.rb b/spec/features/idv/flow_spec.rb index 3530bb75ce9..4ebe49ed1dd 100644 --- a/spec/features/idv/flow_spec.rb +++ b/spec/features/idv/flow_spec.rb @@ -384,6 +384,8 @@ end scenario 'continue phone OTP verification after cancel' do + allow(Figaro.env).to receive(:otp_delivery_blocklist_maxretry).and_return('4') + different_phone = '555-555-9876' user = sign_in_live_with_2fa visit verify_session_path diff --git a/spec/features/two_factor_authentication/change_factor_spec.rb b/spec/features/two_factor_authentication/change_factor_spec.rb index 76dbb1a4c8f..49880e3891c 100644 --- a/spec/features/two_factor_authentication/change_factor_spec.rb +++ b/spec/features/two_factor_authentication/change_factor_spec.rb @@ -15,6 +15,8 @@ end scenario 'editing phone number' do + allow(Figaro.env).to receive(:otp_delivery_blocklist_maxretry).and_return('4') + mailer = instance_double(ActionMailer::MessageDelivery, deliver_later: true) allow(UserMailer).to receive(:phone_changed).with(user).and_return(mailer) diff --git a/spec/features/two_factor_authentication/sign_in_spec.rb b/spec/features/two_factor_authentication/sign_in_spec.rb index 80d33721990..861f572a7d2 100644 --- a/spec/features/two_factor_authentication/sign_in_spec.rb +++ b/spec/features/two_factor_authentication/sign_in_spec.rb @@ -143,6 +143,39 @@ def submit_prefilled_otp_code end end + context 'user requests an OTP too many times', js: true do + it 'locks the user out and leaves user on the page during entire lockout period' do + allow(Figaro.env).to receive(:session_check_frequency).and_return('0') + allow(Figaro.env).to receive(:session_check_delay).and_return('0') + five_minute_countdown_regex = /4:5\d/ + + user = create(:user, :signed_up) + sign_in_before_2fa(user) + + Figaro.env.otp_delivery_blocklist_maxretry.to_i.times do + click_link t('links.two_factor_authentication.resend_code.sms') + end + + expect(page).to have_content t('titles.account_locked') + expect(page).to have_content(five_minute_countdown_regex) + + findtime = Figaro.env.otp_delivery_blocklist_findtime.to_i.minutes + + # let lockout period expire + UpdateUser.new( + user: user, + attributes: { + otp_last_sent_at: Time.zone.now - findtime, + } + ).call + + sign_in_before_2fa(user) + click_button t('forms.buttons.submit.default') + + expect(current_path).to eq account_path + end + end + context 'user signs in while locked out' do it 'signs the user out and lets them know they are locked out' do user = create(:user, :signed_up, second_factor_locked_at: Time.zone.now - 1.minute) diff --git a/spec/services/otp_rate_limiter_spec.rb b/spec/services/otp_rate_limiter_spec.rb new file mode 100644 index 00000000000..69e9461613a --- /dev/null +++ b/spec/services/otp_rate_limiter_spec.rb @@ -0,0 +1,60 @@ +require 'rails_helper' + +RSpec.describe OtpRateLimiter do + let(:current_user) { build(:user) } + subject(:otp_rate_limiter) { OtpRateLimiter.new(current_user) } + + describe '#exceeded_otp_send_limit?' do + it 'is false by default' do + expect(otp_rate_limiter.exceeded_otp_send_limit?).to eq(false) + end + + it 'is true after maxretry_times attemps in findtime minutes' do + expect(otp_rate_limiter.exceeded_otp_send_limit?).to eq(false) + + Figaro.env.otp_delivery_blocklist_maxretry.to_i.times do + otp_rate_limiter.increment + end + + expect(otp_rate_limiter.exceeded_otp_send_limit?).to eq(true) + end + end + + describe '#increment' do + it 'sets the otp_last_sent_at' do + expect(current_user.otp_last_sent_at).to be_nil + + now = Time.zone.now + otp_rate_limiter.increment + + expect(current_user.otp_last_sent_at.to_i).to eq(now.to_i) + end + + it 'increments the otp_send_count' do + expect { otp_rate_limiter.increment }. + to change { current_user.otp_send_count }.from(0).to(1) + end + end + + describe '#lock_out_user' do + before do + current_user.otp_last_sent_at = 5.minutes.ago + current_user.otp_send_count = 0 + end + + it 'resets otp_last_sent_at and otp_send_count' do + otp_rate_limiter.lock_out_user + + expect(current_user.otp_last_sent_at).to be_nil + expect(current_user.otp_send_count).to eq(0) + end + + it 'sets the second_factor_locked_at' do + expect(current_user.second_factor_locked_at).to be_nil + + otp_rate_limiter.lock_out_user + + expect(current_user.second_factor_locked_at.to_i).to eq(Time.zone.now.to_i) + end + end +end From af3f5c7a3d8bfe46dc4da8b6a8ba2c058d8188ff Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Mon, 19 Jun 2017 13:53:43 -0400 Subject: [PATCH 2/2] Track OTPs per phone number, use consistent lockout **Why**: - Since we don't enforce uniqueness of phone number, a malicious user could create multiple accounts with the same number and request OTPs from each account. We want to keep track of the request count based on the phone number used and then lock out all users who have that phone number. - To keep things simple, we use the same lockout period for both max OTP attempts and max OTP requests --- .../concerns/two_factor_authenticatable.rb | 10 +- app/controllers/users/sessions_controller.rb | 2 +- .../two_factor_authentication_controller.rb | 4 +- app/decorators/user_decorator.rb | 18 ++- app/models/otp_requests_tracker.rb | 22 +++ app/services/otp_rate_limiter.rb | 55 ++++--- .../shared/max_otp_requests_reached.html.erb | 19 +++ config/application.yml.example | 14 +- config/initializers/figaro.rb | 2 +- config/initializers/rack_attack.rb | 18 --- config/locales/devise/en.yml | 3 + config/locales/devise/es.yml | 1 + ...25180217_add_otp_rate_limiting_to_users.rb | 6 - ...70621202836_create_otp_requests_tracker.rb | 16 ++ db/schema.rb | 17 ++- .../otp_verification_controller_spec.rb | 3 +- .../totp_verification_controller_spec.rb | 3 +- ...o_factor_authentication_controller_spec.rb | 30 +--- spec/decorators/user_decorator_spec.rb | 8 +- .../two_factor_authentication/sign_in_spec.rb | 140 ++++++++++++++++-- spec/services/otp_rate_limiter_spec.rb | 26 ++-- 21 files changed, 295 insertions(+), 122 deletions(-) create mode 100644 app/models/otp_requests_tracker.rb create mode 100644 app/views/two_factor_authentication/shared/max_otp_requests_reached.html.erb delete mode 100644 db/migrate/20170525180217_add_otp_rate_limiting_to_users.rb create mode 100644 db/migrate/20170621202836_create_otp_requests_tracker.rb diff --git a/app/controllers/concerns/two_factor_authenticatable.rb b/app/controllers/concerns/two_factor_authenticatable.rb index e793e556cdb..e728c786c23 100644 --- a/app/controllers/concerns/two_factor_authenticatable.rb +++ b/app/controllers/concerns/two_factor_authenticatable.rb @@ -38,8 +38,8 @@ def handle_too_many_otp_sends decorator = current_user.decorate sign_out render( - 'two_factor_authentication/shared/max_login_attempts_reached', - locals: { type: :otp, decorator: decorator } + 'two_factor_authentication/shared/max_otp_requests_reached', + locals: { decorator: decorator } ) end @@ -58,15 +58,13 @@ def check_already_authenticated end def reset_attempt_count_if_user_no_longer_locked_out - return unless decorated_user.no_longer_blocked_from_entering_2fa_code? + return unless decorated_user.no_longer_locked_out? UpdateUser.new( user: current_user, attributes: { second_factor_attempts_count: 0, second_factor_locked_at: nil, - otp_send_count: 0, - otp_last_sent_at: nil, } ).call end @@ -94,7 +92,7 @@ def handle_invalid_otp(type: 'otp') flash.now[:error] = t("devise.two_factor_authentication.invalid_#{type}") - if decorated_user.blocked_from_entering_2fa_code? + if decorated_user.locked_out? handle_second_factor_locked_user(type) else render_show_after_invalid diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index 543ce17ae9e..ae84123fdb3 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -102,7 +102,7 @@ def user_signed_in_and_not_locked_out?(user) end def user_locked_out?(user) - UserDecorator.new(user).blocked_from_entering_2fa_code? + UserDecorator.new(user).locked_out? end def store_sp_metadata_in_session diff --git a/app/controllers/users/two_factor_authentication_controller.rb b/app/controllers/users/two_factor_authentication_controller.rb index 382ccba545c..90c9e42a9ea 100644 --- a/app/controllers/users/two_factor_authentication_controller.rb +++ b/app/controllers/users/two_factor_authentication_controller.rb @@ -35,6 +35,8 @@ def reauthn_param end def handle_valid_otp_delivery_preference(method) + otp_rate_limiter.reset_count_and_otp_last_sent_at if decorated_user.no_longer_locked_out? + if otp_rate_limiter.exceeded_otp_send_limit? otp_rate_limiter.lock_out_user @@ -79,7 +81,7 @@ def phone_to_deliver_to end def otp_rate_limiter - @_otp_rate_limited ||= OtpRateLimiter.new(current_user) + @_otp_rate_limited ||= OtpRateLimiter.new(phone: phone_to_deliver_to, user: current_user) end end end diff --git a/app/decorators/user_decorator.rb b/app/decorators/user_decorator.rb index 4f3a7bad068..e1731e1d855 100644 --- a/app/decorators/user_decorator.rb +++ b/app/decorators/user_decorator.rb @@ -19,7 +19,7 @@ def lockout_time_remaining_in_words end def lockout_time_remaining - (Devise.direct_otp_valid_for - (Time.zone.now - user.second_factor_locked_at)).to_i + (lockout_period - (Time.zone.now - user.second_factor_locked_at)).to_i end def confirmation_period_expired_error @@ -98,12 +98,12 @@ def qrcode(otp_secret_key) qrcode.as_png(size: 280).to_data_url end - def blocked_from_entering_2fa_code? - user.second_factor_locked_at.present? && !blocked_from_2fa_period_expired? + def locked_out? + user.second_factor_locked_at.present? && !lockout_period_expired? end - def no_longer_blocked_from_entering_2fa_code? - user.second_factor_locked_at.present? && blocked_from_2fa_period_expired? + def no_longer_locked_out? + user.second_factor_locked_at.present? && lockout_period_expired? end def should_acknowledge_personal_key?(session) @@ -136,7 +136,11 @@ def masked_number(number) "***-***-#{number[-4..-1]}" end - def blocked_from_2fa_period_expired? - (Time.zone.now - user.second_factor_locked_at) > Devise.direct_otp_valid_for + def lockout_period + Figaro.env.lockout_period_in_minutes.to_i.minutes + end + + def lockout_period_expired? + (Time.zone.now - user.second_factor_locked_at) > lockout_period end end diff --git a/app/models/otp_requests_tracker.rb b/app/models/otp_requests_tracker.rb new file mode 100644 index 00000000000..206783f1bc6 --- /dev/null +++ b/app/models/otp_requests_tracker.rb @@ -0,0 +1,22 @@ +class OtpRequestsTracker < ActiveRecord::Base + include EncryptableAttribute + + encrypted_attribute_without_setter(name: :phone) + + def self.find_or_create_with_phone(phone) + tries ||= 1 + phone ||= phone.strip + phone_fingerprint ||= Pii::Fingerprinter.fingerprint(phone) + + where(phone_fingerprint: phone_fingerprint). + first_or_create(phone: phone, otp_send_count: 0, otp_last_sent_at: Time.zone.now) + rescue ActiveRecord::RecordNotUnique + retry unless (tries -= 1).zero? + raise + end + + def phone=(phone) + set_encrypted_attribute(name: :phone, value: phone) + self.phone_fingerprint = phone.present? ? encrypted_attributes[:phone].fingerprint : '' + end +end diff --git a/app/services/otp_rate_limiter.rb b/app/services/otp_rate_limiter.rb index da5f07cd76f..1fb7bc29dcf 100644 --- a/app/services/otp_rate_limiter.rb +++ b/app/services/otp_rate_limiter.rb @@ -1,36 +1,55 @@ class OtpRateLimiter - def initialize(current_user) - @current_user = current_user + def initialize(phone:, user:) + @phone = phone + @user = user end def exceeded_otp_send_limit? - otp_last_sent_at = current_user.otp_last_sent_at - now = Time.zone.now + return false if entry_for_current_phone.blank? + + if rate_limit_period_expired? + reset_count_and_otp_last_sent_at + return false + end + + max_requests_reached? + end + + def max_requests_reached? + entry_for_current_phone.otp_send_count >= otp_maxretry_times + end + + def rate_limit_period_expired? + otp_last_sent_at.present? && (otp_last_sent_at + otp_findtime) < Time.zone.now + end - otp_last_sent_at.present? && - otp_last_sent_at + otp_findtime > now && - current_user.otp_send_count >= otp_maxretry_times + def reset_count_and_otp_last_sent_at + entry_for_current_phone.update(otp_last_sent_at: Time.zone.now, otp_send_count: 0) end def lock_out_user - UpdateUser.new( - user: current_user, - attributes: { - second_factor_locked_at: Time.zone.now, - otp_last_sent_at: nil, - otp_send_count: 0, - } - ).call + UpdateUser.new(user: user, attributes: { second_factor_locked_at: Time.zone.now }).call end def increment - current_user.otp_last_sent_at = Time.zone.now - current_user.otp_send_count += 1 + now = Time.zone.now + + entry_for_current_phone.otp_send_count += 1 + entry_for_current_phone.otp_last_sent_at = now + entry_for_current_phone.save! end private - attr_reader :current_user + attr_reader :phone, :user + + def entry_for_current_phone + @entry ||= OtpRequestsTracker.find_or_create_with_phone(phone) + end + + def otp_last_sent_at + entry_for_current_phone.otp_last_sent_at + end def otp_findtime Figaro.env.otp_delivery_blocklist_findtime.to_i.minutes diff --git a/app/views/two_factor_authentication/shared/max_otp_requests_reached.html.erb b/app/views/two_factor_authentication/shared/max_otp_requests_reached.html.erb new file mode 100644 index 00000000000..c5db7d9aabe --- /dev/null +++ b/app/views/two_factor_authentication/shared/max_otp_requests_reached.html.erb @@ -0,0 +1,19 @@ +<% lockout_time_in_words = decorator.lockout_time_remaining_in_words %> +<% lockout_time_remaining = decorator.lockout_time_remaining %> +<% title t('titles.account_locked') %> + +

+ <%= t('titles.account_locked') %> +

+

+<%= t("devise.two_factor_authentication.max_otp_requests_reached") %> +

+

+ <%= t('devise.two_factor_authentication.please_try_again_html', + time_remaining: content_tag(:span, lockout_time_in_words, id: 'countdown')) %> +

+ +<%= nonced_javascript_tag do %> + var test = <%= lockout_time_remaining %> * 1000; + window.LoginGov.countdownTimer(document.getElementById('countdown'), test); +<% end %> diff --git a/config/application.yml.example b/config/application.yml.example index e033efa27e3..3d9d481d90c 100644 --- a/config/application.yml.example +++ b/config/application.yml.example @@ -73,14 +73,14 @@ development: hmac_fingerprinter_key: 'a2c813d4dca919340866ba58063e4072adc459b767a74cf2666d5c1eef3861db26708e7437abde1755eb24f4034386b0fea1850a1cb7e56bff8fae3cc6ade96c' hmac_fingerprinter_key_queue: '["old-key-one", "old-key-two"]' idp_sso_target_url: 'https://upaya-mockidp.18f.gov/saml/auth' + lockout_period_in_minutes: '10' logins_per_ip_limit: '20' logins_per_ip_period: '8' newrelic_license_key: 'xxx' newrelic_browser_key: '' newrelic_browser_app_id: '' - otp_delivery_blocklist_bantime: '30' - otp_delivery_blocklist_findtime: '15' - otp_delivery_blocklist_maxretry: '3' + otp_delivery_blocklist_findtime: '5' + otp_delivery_blocklist_maxretry: '10' otp_valid_for: '5' password_pepper: 'f22d4b2cafac9066fe2f4416f5b7a32c6942d82f7e00740c7594a095fa8de8db17c05314be7b18a5d6dd5683e73eadf6cc95aa633e5ad9a701edb95192a6a105' password_strength_enabled: 'true' @@ -129,14 +129,14 @@ production: hmac_fingerprinter_key: 'generate via `rake secret`' hmac_fingerprinter_key_queue: '["old-key-one", "old-key-two"]' idp_sso_target_url: 'example.com/saml/auth' + lockout_period_in_minutes: '10' logins_per_ip_limit: '20' logins_per_ip_period: '8' newrelic_license_key: 'xxx' newrelic_browser_key: '' newrelic_browser_app_id: '' - otp_delivery_blocklist_bantime: '30' - otp_delivery_blocklist_findtime: '15' - otp_delivery_blocklist_maxretry: '3' + otp_delivery_blocklist_findtime: '5' + otp_delivery_blocklist_maxretry: '10' otp_valid_for: '5' participate_in_dap: 'false' password_pepper: 'generate via `rake secret`' @@ -184,11 +184,11 @@ test: hmac_fingerprinter_key: 'a2c813d4dca919340866ba58063e4072adc459b767a74cf2666d5c1eef3861db26708e7437abde1755eb24f4034386b0fea1850a1cb7e56bff8fae3cc6ade96c' hmac_fingerprinter_key_queue: '["old-key-one", "old-key-two"]' idp_sso_target_url: 'http://identityprovider.example.com/saml/auth' + lockout_period_in_minutes: '5' logins_per_ip_limit: '2' logins_per_ip_period: '60' max_mail_events: '2' newrelic_license_key: 'xxx' - otp_delivery_blocklist_bantime: '1' otp_delivery_blocklist_findtime: '1' otp_delivery_blocklist_maxretry: '2' otp_valid_for: '5' diff --git a/config/initializers/figaro.rb b/config/initializers/figaro.rb index 3404e34e1b9..414d367bcec 100644 --- a/config/initializers/figaro.rb +++ b/config/initializers/figaro.rb @@ -10,12 +10,12 @@ 'equifax_ssh_passphrase', 'hmac_fingerprinter_key', 'idp_sso_target_url', + 'lockout_period_in_minutes', 'logins_per_ip_limit', 'logins_per_ip_period', 'max_mail_events', 'max_mail_events_window_in_days', 'min_password_score', - 'otp_delivery_blocklist_bantime', 'otp_delivery_blocklist_findtime', 'otp_delivery_blocklist_maxretry', 'otp_valid_for', diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index ebb7b9bb3c2..f75a491a419 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -83,24 +83,6 @@ def remote_ip end end - # After maxretry OTP requests in findtime minutes, - # block all requests from that user for bantime minutes. - blocklist('OTP delivery') do |req| - # `filter` returns truthy value if request fails, or if it's to a - # previously banned phone_number so the request is blocked - phone_number = req.env['warden'].user&.phone - - Allow2Ban.filter( - "otp-#{phone_number}", - maxretry: Figaro.env.otp_delivery_blocklist_maxretry.to_i, - findtime: Figaro.env.otp_delivery_blocklist_findtime.to_i.minutes, - bantime: Figaro.env.otp_delivery_blocklist_bantime.to_i.minutes - ) do - # The count for the phone_number is incremented if the return value is truthy - req.get? && req.path == '/otp/send' - end - end - ### Custom Throttle Response ### # By default, Rack::Attack returns an HTTP 429 for throttled responses, diff --git a/config/locales/devise/en.yml b/config/locales/devise/en.yml index 60d8a4c63e6..94019d25c3f 100644 --- a/config/locales/devise/en.yml +++ b/config/locales/devise/en.yml @@ -120,6 +120,9 @@ en: max_otp_login_attempts_reached: > Your account is temporarily locked because you have entered the one-time security code incorrectly too many times. + max_otp_requests_reached: > + Your account is temporarily locked because you have requested a + security code too many times. max_personal_key_login_attempts_reached: > Your account is temporarily locked because you have entered the personal key incorrectly too many times. diff --git a/config/locales/devise/es.yml b/config/locales/devise/es.yml index 6d96b683960..acbd5a27c26 100644 --- a/config/locales/devise/es.yml +++ b/config/locales/devise/es.yml @@ -108,6 +108,7 @@ es: max_otp_login_attempts_reached: Su cuenta ha sido bloqueada temporalmente porque ha ingresado el código de acceso único de forma incorrecta demasiadas veces. + max_otp_requests_reached: NOT TRANSLATED YET max_personal_key_login_attempts_reached: Su cuenta ha sido bloqueada temporalmente porque ha ingresado el código de acceso único de forma incorrecta demasiadas veces. diff --git a/db/migrate/20170525180217_add_otp_rate_limiting_to_users.rb b/db/migrate/20170525180217_add_otp_rate_limiting_to_users.rb deleted file mode 100644 index 29f9e7a492a..00000000000 --- a/db/migrate/20170525180217_add_otp_rate_limiting_to_users.rb +++ /dev/null @@ -1,6 +0,0 @@ -class AddOtpRateLimitingToUsers < ActiveRecord::Migration - def change - add_column :users, :otp_send_count, :integer, default: 0 - add_column :users, :otp_last_sent_at, :timestamp - end -end diff --git a/db/migrate/20170621202836_create_otp_requests_tracker.rb b/db/migrate/20170621202836_create_otp_requests_tracker.rb new file mode 100644 index 00000000000..22c4a3b0d41 --- /dev/null +++ b/db/migrate/20170621202836_create_otp_requests_tracker.rb @@ -0,0 +1,16 @@ +class CreateOtpRequestsTracker < ActiveRecord::Migration + def change + create_table :otp_requests_trackers do |t| + t.text :encrypted_phone + t.timestamp :otp_last_sent_at + t.integer :otp_send_count, default: 0 + t.string :attribute_cost + t.string :phone_fingerprint, default: '', null: false + + t.timestamps + + t.index :phone_fingerprint, unique: true + t.index :updated_at + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 61ea52ddf08..71f4961a843 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170531204549) do +ActiveRecord::Schema.define(version: 20170621202836) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -68,6 +68,19 @@ add_index "identities", ["user_id"], name: "index_identities_on_user_id", using: :btree add_index "identities", ["uuid"], name: "index_identities_on_uuid", unique: true, using: :btree + create_table "otp_requests_trackers", force: :cascade do |t| + t.text "encrypted_phone" + t.datetime "otp_last_sent_at" + t.integer "otp_send_count", default: 0 + t.string "attribute_cost" + t.string "phone_fingerprint", default: "", null: false + t.datetime "created_at" + t.datetime "updated_at" + end + + add_index "otp_requests_trackers", ["phone_fingerprint"], name: "index_otp_requests_trackers_on_phone_fingerprint", unique: true, using: :btree + add_index "otp_requests_trackers", ["updated_at"], name: "index_otp_requests_trackers_on_updated_at", using: :btree + create_table "profiles", force: :cascade do |t| t.integer "user_id", null: false t.boolean "active", default: false, null: false @@ -170,8 +183,6 @@ t.string "attribute_cost" t.text "encrypted_phone" t.integer "otp_delivery_preference", default: 0, null: false - t.integer "otp_send_count", default: 0 - t.datetime "otp_last_sent_at" end add_index "users", ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true, using: :btree diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index 7a1a427e8b1..f7c3b1afcf2 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -158,8 +158,9 @@ context 'when the user lockout period expires' do before do sign_in_before_2fa + lockout_period = Figaro.env.lockout_period_in_minutes.to_i.minutes subject.current_user.update( - second_factor_locked_at: Time.zone.now - Devise.direct_otp_valid_for - 1.second, + second_factor_locked_at: Time.zone.now - lockout_period - 1.second, second_factor_attempts_count: 3 ) end diff --git a/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb index 66152e2f37e..d55bb7d82bf 100644 --- a/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb @@ -87,10 +87,11 @@ context 'when the user lockout period expires' do before do + lockout_period = Figaro.env.lockout_period_in_minutes.to_i.minutes user = create( :user, :signed_up, - second_factor_locked_at: Time.zone.now - Devise.direct_otp_valid_for - 1.second, + second_factor_locked_at: Time.zone.now - lockout_period - 1.second, second_factor_attempts_count: 3 ) sign_in_before_2fa(user) diff --git a/spec/controllers/users/two_factor_authentication_controller_spec.rb b/spec/controllers/users/two_factor_authentication_controller_spec.rb index c1db46b6e90..26b9618fa99 100644 --- a/spec/controllers/users/two_factor_authentication_controller_spec.rb +++ b/spec/controllers/users/two_factor_authentication_controller_spec.rb @@ -111,7 +111,7 @@ def index describe '#send_code' do context 'when selecting SMS OTP delivery' do before do - @user = create(:user) + @user = create(:user, :with_phone) sign_in_before_2fa(@user) @old_otp = subject.current_user.direct_otp allow(SmsOtpSenderJob).to receive(:perform_later) @@ -149,31 +149,15 @@ def index get :send_code, otp_delivery_selection_form: { otp_delivery_preference: 'sms' } end - it 'increments the otp_send_count each time' do - expect(@user.reload.otp_send_count).to eq(0) + it 'calls OtpRateLimiter#exceeded_otp_send_limit? and #increment' do + otp_rate_limiter = instance_double(OtpRateLimiter) + allow(OtpRateLimiter).to receive(:new).with(phone: @user.phone, user: @user). + and_return(otp_rate_limiter) - get :send_code, otp_delivery_selection_form: { otp_delivery_preference: 'sms' } - - expect(@user.reload.otp_send_count).to eq(1) - - get :send_code, otp_delivery_selection_form: { otp_delivery_preference: 'sms' } - - expect(@user.reload.otp_send_count).to eq(2) - end + expect(otp_rate_limiter).to receive(:exceeded_otp_send_limit?) + expect(otp_rate_limiter).to receive(:increment) - it 'sets otp_last_sent_at each time' do - expect(@user.reload.otp_last_sent_at).to be_nil - - now = Time.zone.now get :send_code, otp_delivery_selection_form: { otp_delivery_preference: 'sms' } - - expect(@user.reload.otp_last_sent_at.to_i).to eq(now.to_i) - - Timecop.freeze(now + 10) do - get :send_code, otp_delivery_selection_form: { otp_delivery_preference: 'sms' } - end - - expect(@user.reload.otp_last_sent_at.to_i).to eq(now.to_i + 10) end it 'marks the user as locked out after too many attempts' do diff --git a/spec/decorators/user_decorator_spec.rb b/spec/decorators/user_decorator_spec.rb index 994e203fd49..5cea4336e82 100644 --- a/spec/decorators/user_decorator_spec.rb +++ b/spec/decorators/user_decorator_spec.rb @@ -6,9 +6,9 @@ Timecop.freeze(Time.zone.now) do user = build_stubbed(:user, second_factor_locked_at: Time.zone.now - 180) user_decorator = UserDecorator.new(user) - allow(Devise).to receive(:direct_otp_valid_for).and_return(535) + allow(Figaro.env).to receive(:lockout_period_in_minutes).and_return('8') - expect(user_decorator.lockout_time_remaining).to eq 355 + expect(user_decorator.lockout_time_remaining).to eq 300 end end end @@ -18,10 +18,10 @@ Timecop.freeze(Time.zone.now) do user = build_stubbed(:user, second_factor_locked_at: Time.zone.now - 180) user_decorator = UserDecorator.new(user) - allow(Devise).to receive(:direct_otp_valid_for).and_return(535) + allow(Figaro.env).to receive(:lockout_period_in_minutes).and_return('8') expect(user_decorator.lockout_time_remaining_in_words). - to eq '5 minutes and 55 seconds' + to eq '5 minutes' end end end diff --git a/spec/features/two_factor_authentication/sign_in_spec.rb b/spec/features/two_factor_authentication/sign_in_spec.rb index 861f572a7d2..b3e116f5427 100644 --- a/spec/features/two_factor_authentication/sign_in_spec.rb +++ b/spec/features/two_factor_authentication/sign_in_spec.rb @@ -115,6 +115,7 @@ def submit_prefilled_otp_code it 'locks the user out and leaves user on the page during entire lockout period' do allow(Figaro.env).to receive(:session_check_frequency).and_return('0') allow(Figaro.env).to receive(:session_check_delay).and_return('0') + lockout_period = Figaro.env.lockout_period_in_minutes.to_i.minutes five_minute_countdown_regex = /4:5\d/ user = create(:user, :signed_up) @@ -132,7 +133,7 @@ def submit_prefilled_otp_code UpdateUser.new( user: user, attributes: { - second_factor_locked_at: Time.zone.now - (Devise.direct_otp_valid_for + 1.second), + second_factor_locked_at: Time.zone.now - (lockout_period + 1.second), } ).call @@ -143,10 +144,11 @@ def submit_prefilled_otp_code end end - context 'user requests an OTP too many times', js: true do + context 'user requests an OTP too many times within `findtime` minutes', js: true do it 'locks the user out and leaves user on the page during entire lockout period' do allow(Figaro.env).to receive(:session_check_frequency).and_return('0') allow(Figaro.env).to receive(:session_check_delay).and_return('0') + lockout_period = Figaro.env.lockout_period_in_minutes.to_i.minutes five_minute_countdown_regex = /4:5\d/ user = create(:user, :signed_up) @@ -158,21 +160,139 @@ def submit_prefilled_otp_code expect(page).to have_content t('titles.account_locked') expect(page).to have_content(five_minute_countdown_regex) + expect(page).to have_content t('devise.two_factor_authentication.max_otp_requests_reached') - findtime = Figaro.env.otp_delivery_blocklist_findtime.to_i.minutes + visit root_path + signin(user.email, user.password) + + expect(page).to have_content t('titles.account_locked') + expect(page).to have_content(five_minute_countdown_regex) + expect(page). + to have_content t('devise.two_factor_authentication.max_generic_login_attempts_reached') # let lockout period expire - UpdateUser.new( - user: user, - attributes: { - otp_last_sent_at: Time.zone.now - findtime, - } - ).call + Timecop.travel(lockout_period) do + signin(user.email, user.password) + + expect(current_path).to eq login_two_factor_path(otp_delivery_preference: 'sms') + end + end + end + + context 'findtime period is greater than lockout period' do + it 'does not lock the user' do + allow(Figaro.env).to receive(:otp_delivery_blocklist_findtime).and_return('10') + lockout_period = Figaro.env.lockout_period_in_minutes.to_i.minutes + user = create(:user, :signed_up) sign_in_before_2fa(user) - click_button t('forms.buttons.submit.default') + + Figaro.env.otp_delivery_blocklist_maxretry.to_i.times do + click_link t('links.two_factor_authentication.resend_code.sms') + end + + expect(page).to have_content t('titles.account_locked') + + # let lockout period expire + Timecop.travel(lockout_period) do + signin(user.email, user.password) + + expect(current_path).to eq login_two_factor_path(otp_delivery_preference: 'sms') + end + end + end + + context 'user requests OTP several times but spaced out far apart' do + it 'does not lock the user out' do + max_attempts = Figaro.env.otp_delivery_blocklist_maxretry.to_i + findtime = Figaro.env.otp_delivery_blocklist_findtime.to_i.minutes + user = create(:user, :signed_up) + + sign_in_before_2fa(user) + (max_attempts - 1).times do + click_link t('links.two_factor_authentication.resend_code.sms') + end + click_submit_default expect(current_path).to eq account_path + + phone_fingerprint = Pii::Fingerprinter.fingerprint(user.phone) + rate_limited_phone = OtpRequestsTracker.find_by(phone_fingerprint: phone_fingerprint) + + # let findtime period expire + rate_limited_phone.update(otp_last_sent_at: Time.zone.now - (findtime + 1)) + + visit destroy_user_session_url + sign_in_before_2fa(user) + + expect(rate_limited_phone.reload.otp_send_count).to eq 1 + + click_link t('links.two_factor_authentication.resend_code.sms') + + expect(current_path).to eq login_two_factor_path(otp_delivery_preference: 'sms') + + click_submit_default + + expect(current_path).to eq account_path + end + end + + context '2 users with same phone number request OTP too many times within findtime' do + it 'locks both users out' do + allow(Figaro.env).to receive(:otp_delivery_blocklist_maxretry).and_return('3') + first_user = create(:user, :signed_up, phone: '+1 703-555-1212') + second_user = create(:user, :signed_up, phone: '+1 703-555-1212') + max_attempts = Figaro.env.otp_delivery_blocklist_maxretry.to_i + + sign_in_before_2fa(first_user) + click_link t('links.two_factor_authentication.resend_code.sms') + + expect(current_path).to eq login_two_factor_path(otp_delivery_preference: 'sms') + + visit destroy_user_session_url + + sign_in_before_2fa(second_user) + click_link t('links.two_factor_authentication.resend_code.sms') + phone_fingerprint = Pii::Fingerprinter.fingerprint(first_user.phone) + rate_limited_phone = OtpRequestsTracker.find_by(phone_fingerprint: phone_fingerprint) + + expect(current_path).to eq otp_send_path + expect(rate_limited_phone.otp_send_count).to eq max_attempts + + visit account_path + + expect(current_path).to eq root_path + + visit destroy_user_session_url + + signin(first_user.email, first_user.password) + + expect(page).to have_content t('devise.two_factor_authentication.max_otp_requests_reached') + + visit account_path + expect(current_path).to eq root_path + end + end + + context 'When setting up 2FA for the first time' do + it 'enforces rate limiting only for current phone' do + second_user = create(:user, :signed_up, phone: '+1 202-555-1212') + + sign_in_before_2fa + max_attempts = Figaro.env.otp_delivery_blocklist_maxretry.to_i + + submit_2fa_setup_form_with_valid_phone_and_choose_phone_call_delivery + + max_attempts.times do + click_link t('links.two_factor_authentication.resend_code.voice') + end + + expect(page).to have_content t('titles.account_locked') + + visit root_path + signin(second_user.email, second_user.password) + + expect(current_path).to eq login_two_factor_path(otp_delivery_preference: 'sms') end end diff --git a/spec/services/otp_rate_limiter_spec.rb b/spec/services/otp_rate_limiter_spec.rb index 69e9461613a..bdf838dcac8 100644 --- a/spec/services/otp_rate_limiter_spec.rb +++ b/spec/services/otp_rate_limiter_spec.rb @@ -1,8 +1,10 @@ require 'rails_helper' RSpec.describe OtpRateLimiter do - let(:current_user) { build(:user) } - subject(:otp_rate_limiter) { OtpRateLimiter.new(current_user) } + let(:current_user) { build(:user, :with_phone) } + subject(:otp_rate_limiter) { OtpRateLimiter.new(phone: current_user.phone, user: current_user) } + let(:phone_fingerprint) { Pii::Fingerprinter.fingerprint(current_user.phone) } + let(:rate_limited_phone) { OtpRequestsTracker.find_by(phone_fingerprint: phone_fingerprint) } describe '#exceeded_otp_send_limit?' do it 'is false by default' do @@ -22,31 +24,25 @@ describe '#increment' do it 'sets the otp_last_sent_at' do - expect(current_user.otp_last_sent_at).to be_nil - now = Time.zone.now otp_rate_limiter.increment - expect(current_user.otp_last_sent_at.to_i).to eq(now.to_i) + expect(rate_limited_phone.otp_last_sent_at.to_i).to eq(now.to_i) end it 'increments the otp_send_count' do + otp_rate_limiter.increment + expect { otp_rate_limiter.increment }. - to change { current_user.otp_send_count }.from(0).to(1) + to change { rate_limited_phone.reload.otp_send_count }.from(1).to(2) end end describe '#lock_out_user' do before do - current_user.otp_last_sent_at = 5.minutes.ago - current_user.otp_send_count = 0 - end - - it 'resets otp_last_sent_at and otp_send_count' do - otp_rate_limiter.lock_out_user - - expect(current_user.otp_last_sent_at).to be_nil - expect(current_user.otp_send_count).to eq(0) + otp_rate_limiter.increment + rate_limited_phone.otp_last_sent_at = 5.minutes.ago + rate_limited_phone.otp_send_count = 0 end it 'sets the second_factor_locked_at' do