Protect OAuth endpoints from brute force/password stuffing

Prevent brute force/credential spray attacks on the OAuth
token endpoint by incrementing failed attempts. After the
configured Devise `maximum_attempts` the account will be
locked and further attempts will not succeed. This change also
adds the OAuth token path to Rack Attack protected paths.

changelogs/unreleased/security-205-dblessing-oauth-token-brute-force.yml

+---
+title: Protect OAuth endpoints from brute force/password stuffing
+merge_request:
+author:
+type: security

config/initializers/doorkeeper.rb

@@ -17,7 +17,7 @@ Doorkeeper.configure do
   end
 
   resource_owner_from_credentials do |routes|
-    user = Gitlab::Auth.find_with_user_password(params[:username], params[:password])
+    user = Gitlab::Auth.find_with_user_password(params[:username], params[:password], increment_failed_attempts: true)
     user unless user.try(:two_factor_enabled?)
   end
 

db/migrate/20200728182311_add_o_auth_paths_to_protected_paths.rb

+# frozen_string_literal: true
+
+class AddOAuthPathsToProtectedPaths < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  ADD_PROTECTED_PATHS = [
+    '/oauth/authorize',
+    '/oauth/token'
+  ].freeze
+
+  EXISTING_DEFAULT_PROTECTED_PATHS = [
+    '/users/password',
+    '/users/sign_in',
+    '/api/v3/session.json',
+    '/api/v3/session',
+    '/api/v4/session.json',
+    '/api/v4/session',
+    '/users',
+    '/users/confirmation',
+    '/unsubscribes/',
+    '/import/github/personal_access_token',
+    '/admin/session'
+  ].freeze
+
+  NEW_DEFAULT_PROTECTED_PATHS = (EXISTING_DEFAULT_PROTECTED_PATHS + ADD_PROTECTED_PATHS).freeze
+
+  class ApplicationSetting < ActiveRecord::Base
+    self.table_name = 'application_settings'
+  end
+
+  def up
+    change_column_default :application_settings, :protected_paths, NEW_DEFAULT_PROTECTED_PATHS
+
+    ApplicationSetting.reset_column_information
+
+    ApplicationSetting.where.not(protected_paths: nil).each do |application_setting|
+      missing_paths = ADD_PROTECTED_PATHS - application_setting.protected_paths
+
+      next if missing_paths.empty?
+
+      updated_protected_paths = application_setting.protected_paths + missing_paths
+      application_setting.update!(protected_paths: updated_protected_paths)
+    end
+  end
+
+  def down
+    change_column_default :application_settings, :protected_paths, EXISTING_DEFAULT_PROTECTED_PATHS
+
+    ApplicationSetting.reset_column_information
+
+    ApplicationSetting.where.not(protected_paths: nil).each do |application_setting|
+      paths_to_remove = application_setting.protected_paths - EXISTING_DEFAULT_PROTECTED_PATHS
+
+      next if paths_to_remove.empty?
+
+      updated_protected_paths = application_setting.protected_paths - paths_to_remove
+      application_setting.update!(protected_paths: updated_protected_paths)
+    end
+  end
+end

db/schema_migrations/20200728182311

+2aab4599404312ddcc5bc9af11b0a21dfd6aa8aa10d4b4b5086a93ce1ffe77b6
\ No newline at end of file

db/structure.sql

@@ -9174,7 +9174,7 @@ CREATE TABLE public.application_settings (
     throttle_protected_paths_enabled boolean DEFAULT false NOT NULL,
     throttle_protected_paths_requests_per_period integer DEFAULT 10 NOT NULL,
     throttle_protected_paths_period_in_seconds integer DEFAULT 60 NOT NULL,
-    protected_paths character varying(255)[] DEFAULT '{/users/password,/users/sign_in,/api/v3/session.json,/api/v3/session,/api/v4/session.json,/api/v4/session,/users,/users/confirmation,/unsubscribes/,/import/github/personal_access_token,/admin/session}'::character varying[],
+    protected_paths character varying(255)[] DEFAULT '{/users/password,/users/sign_in,/api/v3/session.json,/api/v3/session,/api/v4/session.json,/api/v4/session,/users,/users/confirmation,/unsubscribes/,/import/github/personal_access_token,/admin/session,/oauth/authorize,/oauth/token}'::character varying[],
     throttle_incident_management_notification_enabled boolean DEFAULT false NOT NULL,
     throttle_incident_management_notification_period_in_seconds integer DEFAULT 3600,
     throttle_incident_management_notification_per_period integer DEFAULT 3600,

ee/lib/ee/gitlab/auth.rb

@@ -6,7 +6,7 @@ module EE
       extend ::Gitlab::Utils::Override
 
       override :find_with_user_password
-      def find_with_user_password(login, password)
+      def find_with_user_password(login, password, increment_failed_attempts: false)
         if Devise.omniauth_providers.include?(:kerberos)
           kerberos_user = ::Gitlab::Kerberos::Authentication.login(login, password)
           return kerberos_user if kerberos_user

lib/gitlab/auth.rb

@@ -65,7 +65,15 @@ module Gitlab
         raise Gitlab::Auth::MissingPersonalAccessTokenError
       end
 
-      def find_with_user_password(login, password)
+      # Find and return a user if the provided password is valid for various
+      # authenticators (OAuth, LDAP, Local Database).
+      #
+      # Specify `increment_failed_attempts: true` to increment Devise `failed_attempts`.
+      # CAUTION: Avoid incrementing failed attempts when authentication falls through
+      # different mechanisms, as in `.find_for_git_client`. This may lead to
+      # unwanted access locks when the value provided for `password` was actually
+      # a PAT, deploy token, etc.
+      def find_with_user_password(login, password, increment_failed_attempts: false)
         # Avoid resource intensive checks if login credentials are not provided
         return unless login.present? && password.present?
 
@@ -96,10 +104,14 @@ module Gitlab
           authenticators.compact!
 
           # return found user that was authenticated first for given login credentials
-          authenticators.find do |auth|
+          authenticated_user = authenticators.find do |auth|
             authenticated_user = auth.login(login, password)
             break authenticated_user if authenticated_user
           end
+
+          user_auth_attempt!(user, success: !!authenticated_user) if increment_failed_attempts
+
+          authenticated_user
         end
       end
 
@@ -355,6 +367,13 @@ module Gitlab
       def find_build_by_token(token)
         ::Ci::Build.running.find_by_token(token)
       end
+
+      def user_auth_attempt!(user, success:)
+        return unless user && Gitlab::Database.read_write?
+        return user.unlock_access! if success
+
+        user.increment_failed_attempts!
+      end
     end
   end
 end

spec/lib/gitlab/auth_spec.rb

@@ -682,12 +682,69 @@ RSpec.describe Gitlab::Auth, :use_clean_rails_memory_store_caching do
       expect( gl_auth.find_with_user_password(username, password) ).not_to eql user
     end
 
+    it 'does not find user in locked state' do
+      user.lock_access!
+
+      expect(gl_auth.find_with_user_password(username, password)).not_to eql user
+    end
+
     it "does not find user in ldap_blocked state" do
       user.ldap_block
 
       expect( gl_auth.find_with_user_password(username, password) ).not_to eql user
     end
 
+    context 'with increment_failed_attempts' do
+      wrong_password = 'incorrect_password'
+
+      it 'increments failed_attempts when true and password is incorrect' do
+        expect do
+          gl_auth.find_with_user_password(username, wrong_password, increment_failed_attempts: true)
+          user.reload
+        end.to change(user, :failed_attempts).from(0).to(1)
+      end
+
+      it 'resets failed_attempts when true and password is correct' do
+        user.failed_attempts = 2
+        user.save
+
+        expect do
+          gl_auth.find_with_user_password(username, password, increment_failed_attempts: true)
+          user.reload
+        end.to change(user, :failed_attempts).from(2).to(0)
+      end
+
+      it 'does not increment failed_attempts by default' do
+        expect do
+          gl_auth.find_with_user_password(username, wrong_password)
+          user.reload
+        end.not_to change(user, :failed_attempts)
+      end
+
+      context 'when the database is read only' do
+        before do
+          allow(Gitlab::Database).to receive(:read_only?).and_return(true)
+        end
+
+        it 'does not increment failed_attempts when true and password is incorrect' do
+          expect do
+            gl_auth.find_with_user_password(username, wrong_password, increment_failed_attempts: true)
+            user.reload
+          end.not_to change(user, :failed_attempts)
+        end
+
+        it 'does not reset failed_attempts when true and password is correct' do
+          user.failed_attempts = 2
+          user.save
+
+          expect do
+            gl_auth.find_with_user_password(username, password, increment_failed_attempts: true)
+            user.reload
+          end.not_to change(user, :failed_attempts)
+        end
+      end
+    end
+
     context "with ldap enabled" do
       before do
         allow(Gitlab::Auth::Ldap::Config).to receive(:enabled?).and_return(true)

spec/migrations/20200728182311_add_o_auth_paths_to_protected_paths_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'migrate', '20200728182311_add_o_auth_paths_to_protected_paths.rb')
+
+RSpec.describe AddOAuthPathsToProtectedPaths do
+  subject(:migration) { described_class.new }
+
+  let(:application_settings) { table(:application_settings) }
+  let(:new_paths) do
+    [
+      '/oauth/authorize',
+      '/oauth/token'
+    ]
+  end
+
+  it 'appends new OAuth paths' do
+    application_settings.create!
+
+    protected_paths_before = application_settings.first.protected_paths
+    protected_paths_after = protected_paths_before + new_paths
+
+    expect { migrate! }.to change { application_settings.first.protected_paths }.from(protected_paths_before).to(protected_paths_after)
+  end
+
+  it 'new default includes new paths' do
+    settings_before = application_settings.create!
+
+    expect(settings_before.protected_paths).not_to include(*new_paths)
+
+    migrate!
+
+    application_settings.reset_column_information
+    settings_after = application_settings.create!
+
+    expect(settings_after.protected_paths).to include(*new_paths)
+  end
+
+  it 'does not change the value when the new paths are already included' do
+    application_settings.create!(protected_paths: %w(/users/sign_in /users/password) + new_paths)
+
+    expect { migrate! }.not_to change { application_settings.first.protected_paths }
+  end
+
+  it 'adds one value when the other is already present' do
+    application_settings.create!(protected_paths: %W(/users/sign_in /users/password #{new_paths.first}))
+
+    migrate!
+
+    expect(application_settings.first.protected_paths).to include(new_paths.second)
+  end
+end