From e58f9f7a5550be7129e73221f534041cc2d88fed Mon Sep 17 00:00:00 2001 From: Martin Brennan <mjrbrennan@gmail.com> Date: Thu, 4 Feb 2021 09:03:30 +1000 Subject: [PATCH] DEV: Move logic for rate limiting user second factor to one place (#11941) This moves all the rate limiting for user second factor (based on `params[:second_factor_token]` existing) to the one place, which rate limits by IP and also by username if a user is found. --- app/controllers/application_controller.rb | 10 ++++ app/controllers/session_controller.rb | 16 +++--- app/controllers/users_controller.rb | 8 +-- app/controllers/users_email_controller.rb | 2 +- spec/requests/session_controller_spec.rb | 16 +++--- spec/requests/users_controller_spec.rb | 54 +++++++++++++++++++- spec/requests/users_email_controller_spec.rb | 51 ++++++++++++++++++ 7 files changed, 132 insertions(+), 25 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 1f0f7c3c2b2..5ea0184c67e 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -542,6 +542,16 @@ class ApplicationController < ActionController::Base end end + def rate_limit_second_factor!(user) + return if params[:second_factor_token].blank? + + RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 6, 1.minute).performed! + + if user + RateLimiter.new(nil, "second-factor-min-#{user.username}", 6, 1.minute).performed! + end + end + private def locale_from_header diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index a2d2df7b094..1af0ea5f69d 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -3,7 +3,6 @@ class SessionController < ApplicationController before_action :check_local_login_allowed, only: %i(create forgot_password) before_action :rate_limit_login, only: %i(create email_login) - before_action :rate_limit_second_factor_totp, only: %i(create email_login) skip_before_action :redirect_to_login_if_required skip_before_action :preload_json, :check_xhr, only: %i(sso sso_login sso_provider destroy one_time_password) @@ -277,7 +276,10 @@ class SessionController < ApplicationController return invalid_credentials if params[:password].length > User.max_password_length - if user = User.find_by_username_or_email(normalized_login_param) + user = User.find_by_username_or_email(normalized_login_param) + rate_limit_second_factor!(user) + + if user.present? # If their password is correct unless user.confirm_password?(params[:password]) @@ -355,6 +357,8 @@ class SessionController < ApplicationController check_local_login_allowed(user: user, check_login_via_email: true) + rate_limit_second_factor!(user) + if user.present? && !authenticate_second_factor(user) return render(json: @second_factor_failure_payload) end @@ -599,14 +603,6 @@ class SessionController < ApplicationController ).performed! end - def rate_limit_second_factor_totp - return if params[:second_factor_token].blank? - RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed! - if normalized_login_param.present? - RateLimiter.new(nil, "second-factor-min-#{normalized_login_param}", 3, 1.minute).performed! - end - end - def render_sso_error(status:, text:) @sso_error = text render status: status, layout: 'no_ember' diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 651fda83400..91e5b048cfa 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -729,9 +729,7 @@ class UsersController < ApplicationController token = params[:token] password_reset_find_user(token, committing_change: true) - if params[:second_factor_token].present? - RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed! - end + rate_limit_second_factor!(@user) # no point doing anything else if we can't even find # a user from the token @@ -1389,9 +1387,7 @@ class UsersController < ApplicationController totp_data = secure_session["staged-totp-#{current_user.id}"] totp_object = current_user.get_totp_object(totp_data) - [request.remote_ip, current_user.id].each do |key| - RateLimiter.new(nil, "second-factor-min-#{key}", 3, 1.minute).performed! - end + rate_limit_second_factor!(current_user) authenticated = !auth_token.blank? && totp_object.verify( auth_token, diff --git a/app/controllers/users_email_controller.rb b/app/controllers/users_email_controller.rb index 62a505d8ca0..d808c095df2 100644 --- a/app/controllers/users_email_controller.rb +++ b/app/controllers/users_email_controller.rb @@ -77,7 +77,7 @@ class UsersEmailController < ApplicationController redirect_url = path("/u/confirm-new-email/#{params[:token]}") - RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 3, 1.minute).performed! if params[:second_factor_token].present? + rate_limit_second_factor!(@user) if !@error # this is needed becase the form posts this field as JSON and it can be a diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index 18916d2d77f..a2c48bf4d0c 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -1735,11 +1735,12 @@ RSpec.describe SessionController do RateLimiter.enable RateLimiter.clear_all! - 3.times do |x| + 6.times do |x| post "/session.json", params: { login: "#{user.username}#{x}", password: 'myawesomepassword', - second_factor_token: '000000' + second_factor_token: '000000', + second_factor_method: UserSecondFactor.methods[:totp] } expect(response.status).to eq(200) end @@ -1747,7 +1748,8 @@ RSpec.describe SessionController do post "/session.json", params: { login: user.username, password: 'myawesomepassword', - second_factor_token: '000000' + second_factor_token: '000000', + second_factor_method: UserSecondFactor.methods[:totp] } expect(response.status).to eq(429) @@ -1759,11 +1761,12 @@ RSpec.describe SessionController do RateLimiter.enable RateLimiter.clear_all! - 3.times do |x| + 6.times do |x| post "/session.json", params: { login: user.username, password: 'myawesomepassword', - second_factor_token: '000000' + second_factor_token: '000000', + second_factor_method: UserSecondFactor.methods[:totp] }, env: { "REMOTE_ADDR": "1.2.3.#{x}" } expect(response.status).to eq(200) @@ -1773,7 +1776,8 @@ RSpec.describe SessionController do post "/session.json", params: { login: username, password: 'myawesomepassword', - second_factor_token: '000000' + second_factor_token: '000000', + second_factor_method: UserSecondFactor.methods[:totp] }, env: { "REMOTE_ADDR": "1.2.4.#{x}" } expect(response.status).to eq(429) diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index efe34e09174..dd88ff56197 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -323,7 +323,6 @@ describe UsersController do end context "rate limiting" do - before { RateLimiter.clear_all!; RateLimiter.enable } after { RateLimiter.disable } @@ -332,7 +331,7 @@ describe UsersController do token = user.email_tokens.create!(email: user.email).token - 3.times do + 6.times do put "/u/password-reset/#{token}", params: { second_factor_token: 123456, second_factor_method: 1 @@ -349,6 +348,27 @@ describe UsersController do expect(response.status).to eq(429) end + it "rate limits reset passwords by username" do + freeze_time + + token = user.email_tokens.create!(email: user.email).token + + 6.times do |x| + put "/u/password-reset/#{token}", params: { + second_factor_token: 123456, + second_factor_method: 1 + }, env: { "REMOTE_ADDR": "1.2.3.#{x}" } + + expect(response.status).to eq(200) + end + + put "/u/password-reset/#{token}", params: { + second_factor_token: 123456, + second_factor_method: 1 + }, env: { "REMOTE_ADDR": "1.2.3.4" } + + expect(response.status).to eq(429) + end end context '2 factor authentication required' do @@ -3997,6 +4017,36 @@ describe UsersController do expect(user.user_second_factors.count).to eq(1) end + it "rate limits by IP address" do + RateLimiter.enable + RateLimiter.clear_all! + + create_totp + staged_totp_key = read_secure_session["staged-totp-#{user.id}"] + token = ROTP::TOTP.new(staged_totp_key).now + + 7.times do |x| + post "/users/enable_second_factor_totp.json", params: { name: "test", second_factor_token: token } + end + + expect(response.status).to eq(429) + end + + it "rate limits by username" do + RateLimiter.enable + RateLimiter.clear_all! + + create_totp + staged_totp_key = read_secure_session["staged-totp-#{user.id}"] + token = ROTP::TOTP.new(staged_totp_key).now + + 7.times do |x| + post "/users/enable_second_factor_totp.json", params: { name: "test", second_factor_token: token }, env: { "REMOTE_ADDR": "1.2.3.#{x}" } + end + + expect(response.status).to eq(429) + end + context "when an incorrect token is provided" do before do create_totp diff --git a/spec/requests/users_email_controller_spec.rb b/spec/requests/users_email_controller_spec.rb index f2c84d342b6..ffb7b1f00e6 100644 --- a/spec/requests/users_email_controller_spec.rb +++ b/spec/requests/users_email_controller_spec.rb @@ -131,6 +131,57 @@ describe UsersEmailController do user.reload expect(user.email).to eq("new.n.cool@example.com") end + + context "rate limiting" do + before { RateLimiter.clear_all!; RateLimiter.enable } + after { RateLimiter.disable } + + it "rate limits by IP" do + freeze_time + + 6.times do + put "/u/confirm-new-email", params: { + token: "blah", + second_factor_token: "000000", + second_factor_method: UserSecondFactor.methods[:totp] + } + + expect(response.status).to eq(302) + end + + put "/u/confirm-new-email", params: { + token: "blah", + second_factor_token: "000000", + second_factor_method: UserSecondFactor.methods[:totp] + } + + expect(response.status).to eq(429) + end + + it "rate limits by username" do + freeze_time + + 6.times do |x| + user.email_change_requests.last.update(change_state: EmailChangeRequest.states[:complete]) + put "/u/confirm-new-email", params: { + token: user.email_tokens.last.token, + second_factor_token: "000000", + second_factor_method: UserSecondFactor.methods[:totp] + }, env: { "REMOTE_ADDR": "1.2.3.#{x}" } + + expect(response.status).to eq(302) + end + + user.email_change_requests.last.update(change_state: EmailChangeRequest.states[:authorizing_new]) + put "/u/confirm-new-email", params: { + token: user.email_tokens.last.token, + second_factor_token: "000000", + second_factor_method: UserSecondFactor.methods[:totp] + }, env: { "REMOTE_ADDR": "1.2.3.4" } + + expect(response.status).to eq(429) + end + end end context "security key required" do