From f39ae8a903f0184e03580676d61f4b74a4228102 Mon Sep 17 00:00:00 2001
From: Robin Ward <robin.ward@gmail.com>
Date: Tue, 2 Feb 2021 18:26:28 -0500
Subject: [PATCH] SECURITY: Rate limit MFA by login if possible (#11938)

This ensures we rate limit on logins where possible, we also normalize logins for the rate limiters centrally.
---
 app/controllers/session_controller.rb    | 22 ++++++++++-----
 spec/requests/session_controller_spec.rb | 34 +++++++++++++++++++++---
 2 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb
index 7b6166f6917..a2d2df7b094 100644
--- a/app/controllers/session_controller.rb
+++ b/app/controllers/session_controller.rb
@@ -277,10 +277,7 @@ class SessionController < ApplicationController
 
     return invalid_credentials if params[:password].length > User.max_password_length
 
-    login = params[:login].strip
-    login = login[1..-1] if login[0] == "@"
-
-    if user = User.find_by_username_or_email(login)
+    if user = User.find_by_username_or_email(normalized_login_param)
 
       # If their password is correct
       unless user.confirm_password?(params[:password])
@@ -408,12 +405,12 @@ class SessionController < ApplicationController
     RateLimiter.new(nil, "forgot-password-hr-#{request.remote_ip}", 6, 1.hour).performed!
     RateLimiter.new(nil, "forgot-password-min-#{request.remote_ip}", 3, 1.minute).performed!
 
-    user = User.find_by_username_or_email(params[:login])
+    user = User.find_by_username_or_email(normalized_login_param)
 
     if user
       RateLimiter.new(nil, "forgot-password-login-day-#{user.username}", 6, 1.day).performed!
     else
-      RateLimiter.new(nil, "forgot-password-login-hour-#{params[:login].to_s[0..100]}", 5, 1.hour).performed!
+      RateLimiter.new(nil, "forgot-password-login-hour-#{normalized_login_param}", 5, 1.hour).performed!
     end
 
     user_presence = user.present? && user.human? && !user.staged
@@ -482,6 +479,16 @@ class SessionController < ApplicationController
 
   protected
 
+  def normalized_login_param
+    login = params[:login].to_s
+    if login.present?
+      login = login[1..-1] if login[0] == "@"
+      User.normalize_username(login.strip)[0..100]
+    else
+      nil
+    end
+  end
+
   def check_local_login_allowed(user: nil, check_login_via_email: false)
     # admin-login can get around enabled SSO/disabled local logins
     return if user&.admin?
@@ -595,6 +602,9 @@ class SessionController < ApplicationController
   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:)
diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb
index dfdff8f6676..18916d2d77f 100644
--- a/spec/requests/session_controller_spec.rb
+++ b/spec/requests/session_controller_spec.rb
@@ -1731,17 +1731,16 @@ RSpec.describe SessionController do
         expect(json["error_type"]).to eq("rate_limit")
       end
 
-      it 'rate limits second factor attempts' do
+      it 'rate limits second factor attempts by IP' do
         RateLimiter.enable
         RateLimiter.clear_all!
 
-        3.times do
+        3.times do |x|
           post "/session.json", params: {
-            login: user.username,
+            login: "#{user.username}#{x}",
             password: 'myawesomepassword',
             second_factor_token: '000000'
           }
-
           expect(response.status).to eq(200)
         end
 
@@ -1755,6 +1754,33 @@ RSpec.describe SessionController do
         json = response.parsed_body
         expect(json["error_type"]).to eq("rate_limit")
       end
+
+      it 'rate limits second factor attempts by login' do
+        RateLimiter.enable
+        RateLimiter.clear_all!
+
+        3.times do |x|
+          post "/session.json", params: {
+            login: user.username,
+            password: 'myawesomepassword',
+            second_factor_token: '000000'
+          }, env: { "REMOTE_ADDR": "1.2.3.#{x}" }
+
+          expect(response.status).to eq(200)
+        end
+
+        [user.username + " ", user.username.capitalize, user.username].each_with_index do |username , x|
+          post "/session.json", params: {
+            login: username,
+            password: 'myawesomepassword',
+            second_factor_token: '000000'
+          }, env: { "REMOTE_ADDR": "1.2.4.#{x}" }
+
+          expect(response.status).to eq(429)
+          json = response.parsed_body
+          expect(json["error_type"]).to eq("rate_limit")
+        end
+      end
     end
   end