CVE-2022-2166: Improper Restriction of Excessive Authentication Attempts (CWE-307)
value: Improper Restriction of Excessive Authentication Attempts in GitHub repository mastodon/mastodon prior to 4.0.0.
CWE description: The product does not implement sufficient measures to prevent multiple failed authentication attempts within a short time frame, making it more susceptible to brute force attacks.
CVSS v3 base score: 9.8
Published: 2022-11-16T01:15Z
Repository: https://github.com/mastodon/mastodon
Language: Ruby
Files changed: Gemfile, Gemfile.lock, config/initializers/rack_attack.rb, config/routes.rb, spec/config/initializers/rack_attack_spec.rb
Commit date: 2022-11-14 20:26:31 +0100
Commit message: Fix rate limiting for paths with formats (#20675)


Diff: diff --git a/Gemfile b/Gemfile
index 07c300510c..355b7e43f8 100644
--- a/Gemfile
+++ b/Gemfile
@@ -120,10 +120,11 @@ group :test do
   gem 'rails-controller-testing', '~> 1.0'
   gem 'rspec-sidekiq', '~> 3.1'
   gem 'simplecov', '~> 0.21', require: false
   gem 'webmock', '~> 3.18'
   gem 'rspec_junit_formatter', '~> 0.6'
+  gem 'rack-test', '~> 2.0'
 end
 
 group :development do
   gem 'active_record_query_trace', '~> 1.8'
   gem 'annotate', '~> 3.2'
@@ -150,9 +151,7 @@ group :production do
   gem 'lograge', '~> 0.12'
 end
 
 gem 'concurrent-ruby', require: false
 gem 'connection_pool', require: false
-
 gem 'xorcist', '~> 1.1'
-
 gem 'cocoon', '~> 1.2'
diff --git a/Gemfile.lock b/Gemfile.lock
index 28c6e73481..87d07b6312 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -813,10 +813,11 @@ DEPENDENCIES
   puma (~> 5.6)
   pundit (~> 2.2)
   rack (~> 2.2.4)
   rack-attack (~> 6.6)
   rack-cors (~> 1.1)
+  rack-test (~> 2.0)
   rails (~> 6.1.7)
   rails-controller-testing (~> 1.0)
   rails-i18n (~> 6.0)
   rails-settings-cached (~> 0.6)
   rdf-normalize (~> 0.5)
diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb
index 745eb5d3bf..72ef7ba801 100644
--- a/config/initializers/rack_attack.rb
+++ b/config/initializers/rack_attack.rb
@@ -15,10 +15,22 @@ class Rack::Attack
 
     def remote_ip
       @remote_ip ||= (@env["action_dispatch.remote_ip"] || ip).to_s
     end
 
+    def throttleable_remote_ip
+      @throttleable_remote_ip ||= begin
+        ip = IPAddr.new(remote_ip)
+
+        if ip.ipv6?
+          ip.mask(64)
+        else
+          ip
+        end
+      end.to_s
+    end
+
     def authenticated_user_id
       authenticated_token&.resource_owner_id
     end
 
     def unauthenticated?
@@ -27,10 +39,14 @@ class Rack::Attack
 
     def api_request?
       path.start_with?('/api')
     end
 
+    def path_matches?(other_path)
+      /\A#{Regexp.escape(other_path)}(\..*)?\z/ =~ path
+    end
+
     def web_request?
       !api_request?
     end
 
     def paging_request?
@@ -49,75 +65,70 @@ class Rack::Attack
   throttle('throttle_authenticated_api', limit: 300, period: 5.minutes) do |req|
     req.authenticated_user_id if req.api_request?
   end
 
   throttle('throttle_unauthenticated_api', limit: 300, period: 5.minutes) do |req|
-    req.remote_ip if req.api_request? && req.unauthenticated?
+    req.throttleable_remote_ip if req.api_request? && req.unauthenticated?
   end
 
   throttle('throttle_api_media', limit: 30, period: 30.minutes) do |req|
-    req.authenticated_user_id if req.post? && req.path.match?('^/api/v\d+/media')
+    req.authenticated_user_id if req.post? && req.path.match?(/\A\/api\/v\d+\/media\z/i)
   end
 
   throttle('throttle_media_proxy', limit: 30, period: 10.minutes) do |req|
-    req.remote_ip if req.path.start_with?('/media_proxy')
+    req.throttleable_remote_ip if req.path.start_with?('/media_proxy')
   end
 
   throttle('throttle_api_sign_up', limit: 5, period: 30.minutes) do |req|
-    req.remote_ip if req.post? && req.path == '/api/v1/accounts'
+    req.throttleable_remote_ip if req.post? && req.path == '/api/v1/accounts'
   end
 
   throttle('throttle_authenticated_paging', limit: 300, period: 15.minutes) do |req|
     req.authenticated_user_id if req.paging_request?
   end
 
   throttle('throttle_unauthenticated_paging', limit: 300, period: 15.minutes) do |req|
-    req.remote_ip if req.paging_request? && req.unauthenticated?
+    req.throttleable_remote_ip if req.paging_request? && req.unauthenticated?
   end
 
-  API_DELETE_REBLOG_REGEX = /\A\/api\/v1\/statuses\/[\d]+\/unreblog/.freeze
-  API_DELETE_STATUS_REGEX = /\A\/api\/v1\/statuses\/[\d]+/.freeze
+  API_DELETE_REBLOG_REGEX = /\A\/api\/v1\/statuses\/[\d]+\/unreblog\z/.freeze
+  API_DELETE_STATUS_REGEX = /\A\/api\/v1\/statuses\/[\d]+\z/.freeze
 
   throttle('throttle_api_delete', limit: 30, period: 30.minutes) do |req|
     req.authenticated_user_id if (req.post? && req.path.match?(API_DELETE_REBLOG_REGEX)) || (req.delete? && req.path.match?(API_DELETE_STATUS_REGEX))
   end
 
   throttle('throttle_sign_up_attempts/ip', limit: 25, period: 5.minutes) do |req|
-    if req.post? && req.path == '/auth'
-      addr = req.remote_ip
-      addr = IPAddr.new(addr) if addr.is_a?(String)
-      addr = addr.mask(64) if addr.ipv6?
-      addr.to_s
-    end
+    req.throttleable_remote_ip if req.post? && req.path_matches?('/auth')
   end
 
   throttle('throttle_password_resets/ip', limit: 25, period: 5.minutes) do |req|
-    req.remote_ip if req.post? && req.path == '/auth/password'
+    req.throttleable_remote_ip if req.post? && req.path_matches?('/auth/password')
   end
 
   throttle('throttle_password_resets/email', limit: 5, period: 30.minutes) do |req|
-    req.params.dig('user', 'email').presence if req.post? && req.path == '/auth/password'
+    req.params.dig('user', 'email').presence if req.post? && req.path_matches?('/auth/password')
   end
 
   throttle('throttle_email_confirmations/ip', limit: 25, period: 5.minutes) do |req|
-    req.remote_ip if req.post? && %w(/auth/confirmation /api/v1/emails/confirmations).include?(req.path)
+    req.throttleable_remote_ip if req.post? && (req.path_matches?('/auth/confirmation') || req.path == '/api/v1/emails/confirmations')
   end
 
   throttle('throttle_email_confirmations/email', limit: 5, period: 30.minutes) do |req|
-    if req.post? && req.path == '/auth/password'
+    if req.post? && req.path_matches?('/auth/password')
       req.params.dig('user', 'email').presence
     elsif req.post? && req.path == '/api/v1/emails/confirmations'
       req.authenticated_user_id
     end
   end
 
   throttle('throttle_login_attempts/ip', limit: 25, period: 5.minutes) do |req|
-    req.remote_ip if req.post? && req.path == '/auth/sign_in'
+    req.throttleable_remote_ip if req.post? && req.path_matches?('/auth/sign_in')
   end
 
   throttle('throttle_login_attempts/email', limit: 25, period: 1.hour) do |req|
-    req.session[:attempt_user_id] || req.params.dig('user', 'email').presence if req.post? && req.path == '/auth/sign_in'
+    req.session[:attempt_user_id] || req.params.dig('user', 'email').presence if req.post? && req.path_matches?('/auth/sign_in')
   end
 
   self.throttled_responder = lambda do |request|
     now        = Time.now.utc
     match_data = request.env['rack.attack.match_data']
diff --git a/config/routes.rb b/config/routes.rb
index f095ff6551..98aa5a0330 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -71,11 +71,11 @@ Rails.application.routes.draw do
       resource :challenge, only: [:create], controller: :challenges
       get 'sessions/security_key_options', to: 'sessions#webauthn_options'
     end
   end
 
-  devise_for :users, path: 'auth', controllers: {
+  devise_for :users, path: 'auth', format: false, controllers: {
     omniauth_callbacks: 'auth/omniauth_callbacks',
     sessions:           'auth/sessions',
     registrations:      'auth/registrations',
     passwords:          'auth/passwords',
     confirmations:      'auth/confirmations',
@@ -214,11 +214,11 @@ Rails.application.routes.draw do
   end
 
   resource :relationships, only: [:show, :update]
   resource :statuses_cleanup, controller: :statuses_cleanup, only: [:show, :update]
 
-  get '/media_proxy/:id/(*any)', to: 'media_proxy#show', as: :media_proxy
+  get '/media_proxy/:id/(*any)', to: 'media_proxy#show', as: :media_proxy, format: false
 
   resource :authorize_interaction, only: [:show, :create]
   resource :share, only: [:show, :create]
 
   namespace :admin do
@@ -402,11 +402,11 @@ Rails.application.routes.draw do
     end
   end
 
   get '/admin', to: redirect('/admin/dashboard', status: 302)
 
-  namespace :api do
+  namespace :api, format: false do
     # OEmbed
     get '/oembed', to: 'oembed#show', as: :oembed
 
     # JSON / REST API
     namespace :v1 do
diff --git a/spec/config/initializers/rack_attack_spec.rb b/spec/config/initializers/rack_attack_spec.rb
new file mode 100644
index 0000000000..581021cb90
--- /dev/null
+++ b/spec/config/initializers/rack_attack_spec.rb
@@ -0,0 +1,82 @@
+require 'rails_helper'
+
+describe Rack::Attack do
+  include Rack::Test::Methods
+
+  def app
+    Rails.application
+  end
+
+  shared_examples 'throttled endpoint' do
+    context 'when the number of requests is lower than the limit' do
+      it 'does not change the request status' do
+        limit.times do
+          request.call
+          expect(last_response.status).to_not eq(429)
+        end
+      end
+    end
+
+    context 'when the number of requests is higher than the limit' do
+      it 'returns http too many requests' do
+        (limit * 2).times do |i|
+          request.call
+          expect(last_response.status).to eq(429) if i > limit
+        end
+      end
+    end
+  end
+
+  let(:remote_ip) { '1.2.3.5' }
+
+  describe 'throttle excessive sign-up requests by IP address' do
+    context 'through the website' do
+      let(:limit) { 25 }
+      let(:request) { ->() { post path, {}, 'REMOTE_ADDR' => remote_ip } }
+
+      context 'for exact path' do
+        let(:path)  { '/auth' }
+        it_behaves_like 'throttled endpoint'
+      end
+
+      context 'for path with format' do
+        let(:path)  { '/auth.html' }
+        it_behaves_like 'throttled endpoint'
+      end
+    end
+
+    context 'through the API' do
+      let(:limit) { 5 }
+      let(:request) { ->() { post path, {}, 'REMOTE_ADDR' => remote_ip } }
+
+      context 'for exact path' do
+        let(:path)  { '/api/v1/accounts' }
+        it_behaves_like 'throttled endpoint'
+      end
+
+      context 'for path with format' do
+        let(:path)  { '/api/v1/accounts.json' }
+
+        it 'returns http not found' do
+          request.call
+          expect(last_response.status).to eq(404)
+        end
+      end
+    end
+  end
+
+  describe 'throttle excessive sign-in requests by IP address' do
+    let(:limit) { 25 }
+    let(:request) { ->() { post path, {}, 'REMOTE_ADDR' => remote_ip } }
+
+    context 'for exact path' do
+      let(:path)  { '/auth/sign_in' }
+      it_behaves_like 'throttled endpoint'
+    end
+
+    context 'for path with format' do
+      let(:path)  { '/auth/sign_in.html' }
+      it_behaves_like 'throttled endpoint'
+    end
+  end
+end

Vulnerable code: 

    req.remote_ip if req.api_request? && req.unauthenticated?
    req.authenticated_user_id if req.post? && req.path.match?('^/api/v\d+/media')
    req.remote_ip if req.path.start_with?('/media_proxy')
    req.remote_ip if req.post? && req.path == '/api/v1/accounts'
    req.remote_ip if req.paging_request? && req.unauthenticated?
  API_DELETE_REBLOG_REGEX = /\A\/api\/v1\/statuses\/[\d]+\/unreblog/.freeze
  API_DELETE_STATUS_REGEX = /\A\/api\/v1\/statuses\/[\d]+/.freeze
    if req.post? && req.path == '/auth'
      addr = req.remote_ip
      addr = IPAddr.new(addr) if addr.is_a?(String)
      addr = addr.mask(64) if addr.ipv6?
      addr.to_s
    end
    req.remote_ip if req.post? && req.path == '/auth/password'
    req.params.dig('user', 'email').presence if req.post? && req.path == '/auth/password'
    req.remote_ip if req.post? && %w(/auth/confirmation /api/v1/emails/confirmations).include?(req.path)
    if req.post? && req.path == '/auth/password'
    req.remote_ip if req.post? && req.path == '/auth/sign_in'
    req.session[:attempt_user_id] || req.params.dig('user', 'email').presence if req.post? && req.path == '/auth/sign_in'
  devise_for :users, path: 'auth', controllers: {
  get '/media_proxy/:id/(*any)', to: 'media_proxy#show', as: :media_proxy
  namespace :api do
Fixed code:   gem 'rack-test', '~> 2.0'
  rack-test (~> 2.0)
    def throttleable_remote_ip
      @throttleable_remote_ip ||= begin
        ip = IPAddr.new(remote_ip)

        if ip.ipv6?
          ip.mask(64)
        else
          ip
        end
      end.to_s
    end

    def path_matches?(other_path)
      /\A#{Regexp.escape(other_path)}(\..*)?\z/ =~ path
    end

    req.throttleable_remote_ip if req.api_request? && req.unauthenticated?
    req.authenticated_user_id if req.post? && req.path.match?(/\A\/api\/v\d+\/media\z/i)
    req.throttleable_remote_ip if req.path.start_with?('/media_proxy')
    req.throttleable_remote_ip if req.post? && req.path == '/api/v1/accounts'
    req.throttleable_remote_ip if req.paging_request? && req.unauthenticated?
  API_DELETE_REBLOG_REGEX = /\A\/api\/v1\/statuses\/[\d]+\/unreblog\z/.freeze
  API_DELETE_STATUS_REGEX = /\A\/api\/v1\/statuses\/[\d]+\z/.freeze
    req.throttleable_remote_ip if req.post? && req.path_matches?('/auth')
    req.throttleable_remote_ip if req.post? && req.path_matches?('/auth/password')
    req.params.dig('user', 'email').presence if req.post? && req.path_matches?('/auth/password')
    req.throttleable_remote_ip if req.post? && (req.path_matches?('/auth/confirmation') || req.path == '/api/v1/emails/confirmations')
    if req.post? && req.path_matches?('/auth/password')
    req.throttleable_remote_ip if req.post? && req.path_matches?('/auth/sign_in')
    req.session[:attempt_user_id] || req.params.dig('user', 'email').presence if req.post? && req.path_matches?('/auth/sign_in')
  devise_for :users, path: 'auth', format: false, controllers: {
  get '/media_proxy/:id/(*any)', to: 'media_proxy#show', as: :media_proxy, format: false
  namespace :api, format: false do
require 'rails_helper'

describe Rack::Attack do
  include Rack::Test::Methods

  def app
    Rails.application
  end

  shared_examples 'throttled endpoint' do
    context 'when the number of requests is lower than the limit' do
      it 'does not change the request status' do
        limit.times do
          request.call
          expect(last_response.status).to_not eq(429)
        end
      end
    end

    context 'when the number of requests is higher than the limit' do
      it 'returns http too many requests' do
        (limit * 2).times do |i|
          request.call
          expect(last_response.status).to eq(429) if i > limit
        end
      end
    end
  end

  let(:remote_ip) { '1.2.3.5' }

  describe 'throttle excessive sign-up requests by IP address' do
    context 'through the website' do
      let(:limit) { 25 }
      let(:request) { ->() { post path, {}, 'REMOTE_ADDR' => remote_ip } }

      context 'for exact path' do
        let(:path)  { '/auth' }
        it_behaves_like 'throttled endpoint'
      end

      context 'for path with format' do
        let(:path)  { '/auth.html' }
        it_behaves_like 'throttled endpoint'
      end
    end

    context 'through the API' do
      let(:limit) { 5 }
      let(:request) { ->() { post path, {}, 'REMOTE_ADDR' => remote_ip } }

      context 'for exact path' do
        let(:path)  { '/api/v1/accounts' }
        it_behaves_like 'throttled endpoint'
      end

      context 'for path with format' do
        let(:path)  { '/api/v1/accounts.json' }

        it 'returns http not found' do
          request.call
          expect(last_response.status).to eq(404)
        end
      end
    end
  end

  describe 'throttle excessive sign-in requests by IP address' do
    let(:limit) { 25 }
    let(:request) { ->() { post path, {}, 'REMOTE_ADDR' => remote_ip } }

    context 'for exact path' do
      let(:path)  { '/auth/sign_in' }
      it_behaves_like 'throttled endpoint'
    end

    context 'for path with format' do
      let(:path)  { '/auth/sign_in.html' }
      it_behaves_like 'throttled endpoint'
    end
  end
end