Commit 702f0d56 authored by GitLab Bot's avatar GitLab Bot

Add latest changes from gitlab-org/security/gitlab@13-3-stable-ee

parent 90432d32
---
title: Protect OAuth endpoints from brute force/password stuffing
merge_request:
author:
type: security
......@@ -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
......
# 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
2aab4599404312ddcc5bc9af11b0a21dfd6aa8aa10d4b4b5086a93ce1ffe77b6
\ No newline at end of file
......@@ -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,
......
......@@ -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
......@@ -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)
......
# 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment