Add rate limiting and code invalidation to prevent brute-force email code logins (#224)

* Add rate limiting and code invalidation to prevent brute-force email code logins

* Use new Login::Attempt model

* Switch enums to string columns

Author: polypixeldev

app/controllers/logins_controller.rb

@@ -118,7 +118,7 @@ def complete
     # Clear the flash - this prevents the error message showing up after an unsuccessful -> successful login
     flash.clear
 
-    service = ProcessLoginService.new(login: @login)
+    service = ProcessLoginService.new(login: @login, ip_address: request.remote_ip, user_agent: request.user_agent)
 
     case params[:method]
     when "webauthn"
@@ -128,6 +128,7 @@ def complete
       )
 
       unless ok
+        record_failed_completion
         redirect_to(auth_users_path, flash: { error: service.errors.full_messages.to_sentence })
         return
       end
@@ -138,6 +139,7 @@ def complete
       )
 
       unless ok
+        record_failed_completion
         flash.now[:error] = service.errors.full_messages.to_sentence
         render(:sms, status: :unprocessable_content)
         return
@@ -149,6 +151,7 @@ def complete
       )
 
       unless ok
+        record_failed_completion
         flash.now[:error] = service.errors.full_messages.to_sentence
         render(:email, status: :unprocessable_content)
         return
@@ -157,13 +160,15 @@ def complete
       ok = service.process_totp(code: params[:code])
 
       unless ok
+        record_failed_completion
         redirect_to(totp_login_path(@login), flash: { error: "Invalid TOTP code, please try again." })
         return
       end
     when "backup_code"
       ok = service.process_backup_code(code: params[:backup_code])
 
       unless ok
+        record_failed_completion
         redirect_to(backup_code_login_path(@login), flash: { error: service.errors.full_messages.to_sentence })
         return
       end
@@ -296,6 +301,11 @@ def fingerprint_info
     }
   end
 
+  # Counts towards the per-IP ban in config/initializers/rack_attack.rb.
+  def record_failed_completion
+    Rack::Attack::Fail2Ban.filter(Rack::Attack.login_complete_fail2ban_key(request.remote_ip), Rack::Attack::LOGIN_COMPLETE_FAIL2BAN) { true }
+  end
+
   def valid_browser_token?
     return true if Rails.env.test?
     return true unless @login.browser_token

app/controllers/sudo_mode_handler.rb

@@ -58,7 +58,7 @@ def call
       return false
     end
 
-    service = ProcessLoginService.new(login:)
+    service = ProcessLoginService.new(login:, ip_address: request.remote_ip, user_agent: request.user_agent)
 
     ok =
       case sudo_params.submit_method

app/jobs/login/attempt/clear_old_attempts_job.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+class Login
+  class Attempt
+    class ClearOldAttemptsJob < ApplicationJob
+      queue_as :low
+
+      def perform
+        Login::Attempt
+          .where.not(ip_address: nil).or(Login::Attempt.where.not(user_agent: nil))
+          .where("created_at < ?", 18.months.ago)
+          .find_each(&:clear_ip_metadata!)
+      end
+
+    end
+
+  end
+
+end

app/models/login.rb

@@ -18,9 +18,10 @@
 #
 # Indexes
 #
-#  index_logins_on_referral_link_id  (referral_link_id)
-#  index_logins_on_user_id           (user_id)
-#  index_logins_on_user_session_id   (user_session_id)
+#  index_logins_on_referral_link_id        (referral_link_id)
+#  index_logins_on_user_id                 (user_id)
+#  index_logins_on_user_id_and_created_at  (user_id,created_at)
+#  index_logins_on_user_session_id         (user_session_id)
 #
 class Login < ApplicationRecord
   include AASM
@@ -30,6 +31,7 @@ class Login < ApplicationRecord
 
   belongs_to :user
   belongs_to :user_session, class_name: "User::Session", optional: true
+  has_many :attempts, class_name: "Login::Attempt", inverse_of: :login
 
   scope(:initial, -> { where(is_reauthentication: false) })
   scope(:reauthentication, -> { where(is_reauthentication: true) })

app/models/login/attempt.rb

@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+# == Schema Information
+#
+# Table name: login_attempts
+#
+#  id         :bigint           not null, primary key
+#  factor     :string           not null
+#  ip_address :inet
+#  status     :string           default("pending"), not null
+#  user_agent :text
+#  created_at :datetime         not null
+#  updated_at :datetime         not null
+#  login_id   :bigint           not null
+#
+# Indexes
+#
+#  index_login_attempts_on_created_at  (created_at)
+#  index_login_attempts_on_login_id    (login_id)
+#
+# Foreign Keys
+#
+#  fk_rails_...  (login_id => logins.id)
+#
+class Login
+  class Attempt < ApplicationRecord
+    belongs_to :login
+    has_one :user, through: :login
+
+    enum :factor, { email: "email" }
+    enum :status, { pending: "pending", failed: "failed", succeeded: "succeeded", blocked: "blocked" }
+
+    scope :counted, -> { where(status: [:pending, :failed]) }
+
+    def clear_ip_metadata!
+      update!(ip_address: nil, user_agent: nil)
+    end
+
+  end
+
+end

app/services/login_code_service/request.rb

@@ -52,6 +52,10 @@ def send_login_code_by_email(user)
         return { error: "You're requesting too many login codes. Please try again later.", method: :email }
       end
 
+      # Only the newest code works. Otherwise every resend adds another valid
+      # code, and each guess has that many more chances of matching one.
+      user.login_codes.active.update_all(used_at: Time.current)
+
       login_code = user.login_codes.create(
         ip_address: @ip_address,
         user_agent: @user_agent

app/services/process_login_service.rb

@@ -3,12 +3,23 @@
 class ProcessLoginService
   extend ActiveModel::Naming
 
+  # Email codes are six digits, so without a cap on wrong guesses they can be
+  # brute-forced. Attempts are counted per Login and per user; the per-user cap
+  # is what bounds an attacker who keeps starting new Logins.
+  MAX_EMAIL_CODE_ATTEMPTS_PER_LOGIN = 5
+  MAX_EMAIL_CODE_ATTEMPTS_PER_USER = 10
+  EMAIL_CODE_ATTEMPTS_WINDOW = 1.hour
+
   attr_reader(:errors, :login, :user)
 
   # @param login [Login]
-  def initialize(login:)
+  # @param ip_address [String, nil] recorded on each Login::Attempt
+  # @param user_agent [String, nil] recorded on each Login::Attempt
+  def initialize(login:, ip_address: nil, user_agent: nil)
     @login = login
     @user = login.user
+    @ip_address = ip_address
+    @user_agent = user_agent
     @errors = ActiveModel::Errors.new(self)
   end
 
@@ -57,22 +68,21 @@ def process_webauthn(raw_credential:, challenge:)
   # @return [Boolean]
   #   Whether the operation succeeded. If `false` check `errors` for details.
   def process_login_code(code:, sms:)
+    # Twilio Verify limits attempts on SMS codes itself; email codes are ours.
+    return process_email_login_code(code:) unless sms
+
     begin
       UserService::ExchangeLoginCodeForUser.new(
         user_id: user.id,
         login_code: code,
-        sms:,
+        sms: true,
       ).run
     rescue Errors::InvalidLoginCode
       errors.add(:base, "Invalid login code")
       return false
     end
 
-    if sms
-      login.update!(authenticated_with_sms: true)
-    else
-      login.update!(authenticated_with_email: true)
-    end
+    login.update!(authenticated_with_sms: true)
 
     true
   end
@@ -115,4 +125,53 @@ def process_backup_code(code:)
     true
   end
 
+  private
+
+  def process_email_login_code(code:)
+    # Record the attempt before counting, so concurrent guesses see each other.
+    # This must commit on its own, outside any wrapping transaction.
+    attempt = login.attempts.create!(factor: :email, ip_address: @ip_address, user_agent: @user_agent)
+
+    user_attempts = Login::Attempt
+                    .email
+                    .counted
+                    .where(created_at: EMAIL_CODE_ATTEMPTS_WINDOW.ago..)
+                    .joins(:login)
+                    # Attempts can only be made on an active Login, so older
+                    # Logins can't have any in the window. Bounding them keeps
+                    # this to a few rows of `index_logins_on_user_id_and_created_at`.
+                    .where(logins: { user_id: user.id, created_at: (EMAIL_CODE_ATTEMPTS_WINDOW + Login::EXPIRATION).ago.. })
+                    .count
+
+    if user_attempts > MAX_EMAIL_CODE_ATTEMPTS_PER_USER
+      attempt.blocked!
+      errors.add(:base, "Too many incorrect login codes. Please try again later.")
+      return false
+    end
+
+    if login.attempts.email.counted.count > MAX_EMAIL_CODE_ATTEMPTS_PER_LOGIN
+      attempt.blocked!
+      errors.add(:base, "Too many incorrect login codes. Please start again and request a new code.")
+      return false
+    end
+
+    begin
+      UserService::ExchangeLoginCodeForUser.new(
+        user_id: user.id,
+        login_code: code,
+        sms: false,
+      ).run
+    rescue Errors::InvalidLoginCode
+      attempt.failed!
+      errors.add(:base, "Invalid login code")
+      return false
+    end
+
+    # A successful sign-in shouldn't eat into the user's budget for the hour.
+    attempt.succeeded!
+    login.update!(authenticated_with_email: true)
+
+    true
+  end
+
 end

config/initializers/forwarded_headers.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+# Requests reach Puma through the Hetzner load balancer and then Caddy. Both set
+# X-Forwarded-*; neither sets, or strips, the RFC 7239 `Forwarded:` header.
+# Rack reads `Forwarded:` first by default, and when it's present ignores
+# X-Forwarded-For entirely, so a client sending `Forwarded: for=<any ip>` would
+# pick its own `request.ip`, and its own `request.remote_ip` too, since
+# ActionDispatch::RemoteIp builds on Rack's `forwarded_for`. That breaks every
+# IP-keyed safelist and throttle in config/initializers/rack_attack.rb. Only
+# trust the headers our proxies set.
+Rack::Request.forwarded_priority = [:x_forwarded]

config/initializers/rack_attack.rb

@@ -1,6 +1,15 @@
 # frozen_string_literal: true
 
 class Rack::Attack
+  # Every rule below keys on `req.ip`, so it has to be an address the client
+  # can't choose, which ActionDispatch::RemoteIp works out
+  class Request < ::Rack::Request
+    def ip
+      (env["action_dispatch.remote_ip"] || env["REMOTE_ADDR"]).to_s
+    end
+
+  end
+
   ### Configure Cache ###
 
   # If you don't want to use Rails.cache (Rack::Attack's default), then
@@ -155,6 +164,38 @@ class Rack::Attack
     end
   end
 
+  # POST /logins/:id/complete is where codes get guessed. The app caps wrong
+  # email codes per Login and per user (see ProcessLoginService); these are a
+  # cheaper layer in front of that. Legitimate users post here once per factor,
+  # plus the odd typo.
+  LOGIN_COMPLETE_PATH = /\A\/logins\/(?<hashid>[^\/]+)\/complete\z/
+
+  throttle("logins/complete/ip", limit: 20, period: 1.minute) do |req|
+    req.ip if req.post? && LOGIN_COMPLETE_PATH.match?(req.path)
+  end
+
+  throttle("logins/complete/login", limit: 10, period: 5.minutes) do |req|
+    if req.post? && (m = req.path.match(LOGIN_COMPLETE_PATH))
+      m[:hashid]
+    end
+  end
+
+  # Only the app knows whether an attempt failed, so LoginsController#complete
+  # records failures with `Fail2Ban.filter` (LOGIN_COMPLETE_FAIL2BAN) and this
+  # rule turns away an IP once it's banned. Spraying guesses across many
+  # accounts gets past the per-user cap, but not this.
+  LOGIN_COMPLETE_FAIL2BAN = { maxretry: 30, findtime: 15.minutes, bantime: 1.hour }.freeze
+
+  def self.login_complete_fail2ban_key(ip)
+    "logins/complete/ip:#{ip}"
+  end
+
+  blocklist("fail2ban failed login completions") do |req|
+    req.post? &&
+      LOGIN_COMPLETE_PATH.match?(req.path) &&
+      Rack::Attack::Fail2Ban.banned?(login_complete_fail2ban_key(req.ip))
+  end
+
   # Throttle POST requests to SMS verification by IP address
   throttle("sms_verify/ip", limit: 5, period: 8.hours) do |req|
     if req.path == "/users/start_sms_auth_verification" && req.post?
@@ -247,4 +288,6 @@ class Rack::Attack
 
 end
 
-Rack::Attack.enabled = Rails.env.production? # only enable in production
+# Only enable in production, unless RACK_ATTACK_ENABLED=true is set to try the
+# rules out locally (also run `bin/rails dev:cache`, or nothing gets counted).
+Rack::Attack.enabled = Rails.env.production? || ENV["RACK_ATTACK_ENABLED"] == "true"

config/schedule.yml

@@ -181,6 +181,11 @@ clear_old_user_sessions:
   class: "User::Session::ClearOldUserSessionsJob"
   queue: low
 
+clear_old_login_attempts:
+  cron: "10 3 * * *" # every day at 3:10 AM
+  class: "Login::Attempt::ClearOldAttemptsJob"
+  queue: low
+
 delete_anonymous_user_sessions:
   cron: "20 * * * *" # run every 1 hour
   class: "User::Session::DeleteAnonymousSessionsJob"