Merge remote-tracking branch 'dev/13-3-stable' into 13-3-stable

10 files changed, 204 insertions, 6 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 89c328b93d3..652f9f8a2a6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,13 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 13.3.4 (2020-09-02)
+
+### Security (1 change)
+
+- Protect OAuth endpoints from brute force/password stuffing.
+
+
 ## 13.3.3 (2020-09-02)
 
 ### Security (23 changes, 1 of them is from the community)
diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION
index 35f1a3e990f..51d9561af40 100644
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
@@ -1 +1 @@
-13.3.3
\ No newline at end of file
+13.3.4
diff --git a/VERSION b/VERSION
index 4be057655b4..51d9561af40 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-13.3.3
+13.3.4
diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb
index 76e29fb6c02..ad0b0c2008f 100644
--- a/config/initializers/doorkeeper.rb
+++ b/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
 
diff --git a/db/migrate/20200728182311_add_o_auth_paths_to_protected_paths.rb b/db/migrate/20200728182311_add_o_auth_paths_to_protected_paths.rb
new file mode 100644
index 00000000000..7a5af0135fa
--- /dev/null
+++ b/db/migrate/20200728182311_add_o_auth_paths_to_protected_paths.rb
@@ -0,0 +1,62 @@
+# 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
diff --git a/db/schema_migrations/20200728182311 b/db/schema_migrations/20200728182311
new file mode 100644
index 00000000000..6bb5a869513
--- /dev/null
+++ b/db/schema_migrations/20200728182311
@@ -0,0 +1 @@
+2aab4599404312ddcc5bc9af11b0a21dfd6aa8aa10d4b4b5086a93ce1ffe77b6
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index 3e3014da914..950f7930a38 100644
--- a/db/structure.sql
+++ b/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,
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 0c6ed6924bf..ece4946383d 100644
--- a/lib/gitlab/auth.rb
+++ b/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
 
@@ -357,6 +369,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
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index 3bd35fb83fd..b6a8ac31074 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -691,12 +691,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)
diff --git a/spec/migrations/20200728182311_add_o_auth_paths_to_protected_paths_spec.rb b/spec/migrations/20200728182311_add_o_auth_paths_to_protected_paths_spec.rb
new file mode 100644
index 00000000000..e12519e15b8
--- /dev/null
+++ b/spec/migrations/20200728182311_add_o_auth_paths_to_protected_paths_spec.rb
@@ -0,0 +1,52 @@
+# 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