From 702f0d561ce6f90908e2ddd40f183d0007e92217 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 2 Sep 2020 17:14:06 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@13-3-stable-ee --- ...urity-205-dblessing-oauth-token-brute-force.yml | 5 ++ config/initializers/doorkeeper.rb | 2 +- ...28182311_add_o_auth_paths_to_protected_paths.rb | 62 ++++++++++++++++++++++ db/schema_migrations/20200728182311 | 1 + db/structure.sql | 2 +- lib/gitlab/auth.rb | 23 +++++++- spec/lib/gitlab/auth_spec.rb | 57 ++++++++++++++++++++ ...311_add_o_auth_paths_to_protected_paths_spec.rb | 52 ++++++++++++++++++ 8 files changed, 200 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/security-205-dblessing-oauth-token-brute-force.yml create mode 100644 db/migrate/20200728182311_add_o_auth_paths_to_protected_paths.rb create mode 100644 db/schema_migrations/20200728182311 create mode 100644 spec/migrations/20200728182311_add_o_auth_paths_to_protected_paths_spec.rb diff --git a/changelogs/unreleased/security-205-dblessing-oauth-token-brute-force.yml b/changelogs/unreleased/security-205-dblessing-oauth-token-brute-force.yml new file mode 100644 index 00000000000..e53ef7ba09a --- /dev/null +++ b/changelogs/unreleased/security-205-dblessing-oauth-token-brute-force.yml @@ -0,0 +1,5 @@ +--- +title: Protect OAuth endpoints from brute force/password stuffing +merge_request: +author: +type: security 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 -- cgit v1.2.3 From 4c4ca7f8b9e5f3a5fea2a2ff17bdd81a48d837b9 Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Wed, 2 Sep 2020 18:13:05 +0000 Subject: Update GITALY_SERVER_VERSION to 13.3.4 --- GITALY_SERVER_VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 -- cgit v1.2.3 From 6126b054b7df34e2eacc4af33436543b1ab5fe29 Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Wed, 2 Sep 2020 18:13:05 +0000 Subject: Update CHANGELOG.md for 13.3.4 [ci skip] --- CHANGELOG.md | 7 +++++++ .../unreleased/security-205-dblessing-oauth-token-brute-force.yml | 5 ----- 2 files changed, 7 insertions(+), 5 deletions(-) delete mode 100644 changelogs/unreleased/security-205-dblessing-oauth-token-brute-force.yml 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/changelogs/unreleased/security-205-dblessing-oauth-token-brute-force.yml b/changelogs/unreleased/security-205-dblessing-oauth-token-brute-force.yml deleted file mode 100644 index e53ef7ba09a..00000000000 --- a/changelogs/unreleased/security-205-dblessing-oauth-token-brute-force.yml +++ /dev/null @@ -1,5 +0,0 @@ ---- -title: Protect OAuth endpoints from brute force/password stuffing -merge_request: -author: -type: security -- cgit v1.2.3 From aebc07f48e86bb27f3fdf7992f516e0ba73abecf Mon Sep 17 00:00:00 2001 From: GitLab Release Tools Bot Date: Wed, 2 Sep 2020 18:15:07 +0000 Subject: Update VERSION to 13.3.4 --- VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION b/VERSION index 4be057655b4..51d9561af40 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -13.3.3 +13.3.4 -- cgit v1.2.3