Merge branch 'security-59549-add-capcha-for-failed-logins-12-0' into '12-0-stable'

Require a captcha after unique failed logins from the same IP

See merge request gitlab/gitlabhq!3296

app/controllers/sessions_controller.rb

@@ -21,13 +21,18 @@ class SessionsController < Devise::SessionsController
   prepend_before_action :ensure_password_authentication_enabled!, if: :password_based_login?, only: [:create]
   before_action :auto_sign_in_with_provider, only: [:new]
+  before_action :store_unauthenticated_sessions, only: [:new]
+  before_action :save_failed_login, if: :action_new_and_failed_login?
   before_action :load_recaptcha
-  after_action :log_failed_login, only: [:new], if: :failed_login?
+  after_action :log_failed_login, if: :action_new_and_failed_login?
+
+  helper_method :captcha_enabled?, :captcha_on_login_required?
   helper_method :captcha_enabled?
   CAPTCHA_HEADER = 'X-GitLab-Show-Login-Captcha'.freeze
+  MAX_FAILED_LOGIN_ATTEMPTS = 5
   def new
     set_minimum_password_length
@@ -71,10 +76,14 @@ class SessionsController < Devise::SessionsController
     request.headers[CAPTCHA_HEADER] && Gitlab::Recaptcha.enabled?
   end
+  def captcha_on_login_required?
+    Gitlab::Recaptcha.enabled_on_login? && unverified_anonymous_user?
+  end
+
   # From https://github.com/plataformatec/devise/wiki/How-To:-Use-Recaptcha-with-Devise#devisepasswordscontroller
   def check_captcha
     return unless user_params[:password].present?
-    return unless captcha_enabled?
+    return unless captcha_enabled? || captcha_on_login_required?
     return unless Gitlab::Recaptcha.load_configurations!
     if verify_recaptcha
@@ -116,10 +125,28 @@ class SessionsController < Devise::SessionsController
     Gitlab::AppLogger.info("Failed Login: username=#{user_params[:login]} ip=#{request.remote_ip}")
   end
+  def action_new_and_failed_login?
+    action_name == 'new' && failed_login?
+  end
+
+  def save_failed_login
+    session[:failed_login_attempts] ||= 0
+    session[:failed_login_attempts] += 1
+  end
+
   def failed_login?
     (options = request.env["warden.options"]) && options[:action] == "unauthenticated"
   end
+  # storing sessions per IP lets us check if there are associated multiple
+  # anonymous sessions with one IP and prevent situations when there are
+  # multiple attempts of logging in
+  def store_unauthenticated_sessions
+    return if current_user
+
+    Gitlab::AnonymousSession.new(request.remote_ip, session_id: request.session.id).store_session_id_per_ip
+  end
+
   # Handle an "initial setup" state, where there's only one user, it's an admin,
   # and they require a password change.
   # rubocop: disable CodeReuse/ActiveRecord
@@ -230,6 +257,18 @@ class SessionsController < Devise::SessionsController
     @ldap_servers ||= Gitlab::Auth::LDAP::Config.available_servers
   end
+  def unverified_anonymous_user?
+    exceeded_failed_login_attempts? || exceeded_anonymous_sessions?
+  end
+
+  def exceeded_failed_login_attempts?
+    session.fetch(:failed_login_attempts, 0) > MAX_FAILED_LOGIN_ATTEMPTS
+  end
+
+  def exceeded_anonymous_sessions?
+    Gitlab::AnonymousSession.new(request.remote_ip).stored_sessions >= MAX_FAILED_LOGIN_ATTEMPTS
+  end
+
   def authentication_method
     if user_params[:otp_attempt]
       "two-factor"

app/helpers/application_settings_helper.rb

@@ -232,6 +232,7 @@ module ApplicationSettingsHelper
       :recaptcha_enabled,
       :recaptcha_private_key,
       :recaptcha_site_key,
+      :login_recaptcha_protection_enabled,
       :receive_max_input_size,
       :repository_checks_enabled,
       :repository_storages,

app/models/application_setting.rb

@@ -76,11 +76,11 @@ class ApplicationSetting < ApplicationRecord
   validates :recaptcha_site_key,
             presence: true,
-            if: :recaptcha_enabled
+            if: :recaptcha_or_login_protection_enabled
   validates :recaptcha_private_key,
             presence: true,
-            if: :recaptcha_enabled
+            if: :recaptcha_or_login_protection_enabled
   validates :sentry_dsn,
             presence: true,
@@ -300,4 +300,8 @@ class ApplicationSetting < ApplicationRecord
     # We already have an ApplicationSetting record, so just return it.
     current_without_cache
   end
+
+  def recaptcha_or_login_protection_enabled
+    recaptcha_enabled || login_recaptcha_protection_enabled
+  end
 end

app/models/application_setting_implementation.rb

@@ -67,6 +67,7 @@ module ApplicationSettingImplementation
         project_export_enabled: true,
         protected_ci_variables: false,
         recaptcha_enabled: false,
+        login_recaptcha_protection_enabled: false,
         repository_checks_enabled: true,
         repository_storages: ['default'],
         require_two_factor_authentication: false,

app/views/admin/application_settings/_spam.html.haml

@@ -8,7 +8,13 @@
         = f.label :recaptcha_enabled, class: 'form-check-label' do
           Enable reCAPTCHA
         %span.form-text.text-muted#recaptcha_help_block Helps prevent bots from creating accounts
-
+    .form-group
+      .form-check
+        = f.check_box :login_recaptcha_protection_enabled, class: 'form-check-input'
+        = f.label :login_recaptcha_protection_enabled, class: 'form-check-label' do
+          Enable reCAPTCHA for login
+        %span.form-text.text-muted#recaptcha_help_block
+          = _('Helps prevent bots from brute-force attacks.')
     .form-group
       = f.label :recaptcha_site_key, 'reCAPTCHA Site Key', class: 'label-bold'
       = f.text_field :recaptcha_site_key, class: 'form-control'
@@ -18,6 +24,7 @@
     .form-group
       = f.label :recaptcha_private_key, 'reCAPTCHA Private Key', class: 'label-bold'
+    .form-group
       = f.text_field :recaptcha_private_key, class: 'form-control'
     .form-group

app/views/admin/application_settings/reporting.html.haml

@@ -9,7 +9,9 @@
     %button.btn.btn-default.js-settings-toggle{ type: 'button' }
       = expanded_by_default? ? _('Collapse') : _('Expand')
     %p
-      = _('Enable reCAPTCHA or Akismet and set IP limits.')
+      - recaptcha_v2_link_url = 'https://developers.google.com/recaptcha/docs/versions'
+      - recaptcha_v2_link_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: recaptcha_v2_link_url }
+      = _('Enable reCAPTCHA or Akismet and set IP limits. For reCAPTCHA, we currently only support %{recaptcha_v2_link_start}v2%{recaptcha_v2_link_end}').html_safe % { recaptcha_v2_link_start: recaptcha_v2_link_start, recaptcha_v2_link_end: '</a>'.html_safe }
   .settings-content
     = render 'spam'

app/views/devise/sessions/_new_base.html.haml

@@ -13,7 +13,7 @@
       .float-right.forgot-password
         = link_to "Forgot your password?", new_password_path(:user)
     %div
-    - if captcha_enabled?
+    - if captcha_enabled? || captcha_on_login_required?
       = recaptcha_tags
   .submit-container.move-submit-down

changelogs/unreleased/security-59549-add-capcha-for-failed-logins.yml

+---
+title: Add :login_recaptcha_protection_enabled setting to prevent bots from brute-force attacks.
+merge_request:
+author:
+type: security

config/initializers/warden.rb

@@ -19,6 +19,7 @@ Rails.application.configure do |config|
   Warden::Manager.after_authentication(scope: :user) do |user, auth, opts|
     ActiveSession.cleanup(user)
+    Gitlab::AnonymousSession.new(auth.request.remote_ip, session_id: auth.request.session.id).cleanup_session_per_ip_entries
   end
   Warden::Manager.after_set_user(scope: :user, only: :fetch) do |user, auth, opts|

db/migrate/20190719122333_add_login_recaptcha_protection_enabled_to_application_settings.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddLoginRecaptchaProtectionEnabledToApplicationSettings < ActiveRecord::Migration[5.1]
+  DOWNTIME = false
+
+  def change
+    add_column :application_settings, :login_recaptcha_protection_enabled, :boolean, default: false, null: false
+  end
+end

db/schema.rb

@@ -233,6 +233,7 @@ ActiveRecord::Schema.define(version: 20190816151221) do
     t.text "asset_proxy_whitelist"
     t.text "encrypted_asset_proxy_secret_key"
     t.string "encrypted_asset_proxy_secret_key_iv"
+    t.boolean "login_recaptcha_protection_enabled", default: false, null: false
     t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id", using: :btree
     t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id", using: :btree
     t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id", using: :btree

lib/api/settings.rb

@@ -110,6 +110,11 @@ module API
         requires :recaptcha_site_key, type: String, desc: 'Generate site key at http://www.google.com/recaptcha'
         requires :recaptcha_private_key, type: String, desc: 'Generate private key at http://www.google.com/recaptcha'
       end
+      optional :login_recaptcha_protection_enabled, type: Boolean, desc: 'Helps prevent brute-force attacks'
+      given login_recaptcha_protection_enabled: ->(val) { val } do
+        requires :recaptcha_site_key, type: String, desc: 'Generate site key at http://www.google.com/recaptcha'
+        requires :recaptcha_private_key, type: String, desc: 'Generate private key at http://www.google.com/recaptcha'
+      end
       optional :repository_checks_enabled, type: Boolean, desc: "GitLab will periodically run 'git fsck' in all project and wiki repositories to look for silent disk corruption issues."
       optional :repository_storages, type: Array[String], desc: 'Storage paths for new projects'
       optional :require_two_factor_authentication, type: Boolean, desc: 'Require all users to set up Two-factor authentication'

lib/gitlab/anonymous_session.rb

+# frozen_string_literal: true
+
+module Gitlab
+  class AnonymousSession
+    def initialize(remote_ip, session_id: nil)
+      @remote_ip = remote_ip
+      @session_id = session_id
+    end
+
+    def store_session_id_per_ip
+      Gitlab::Redis::SharedState.with do |redis|
+        redis.pipelined do
+          redis.sadd(session_lookup_name, session_id)
+          redis.expire(session_lookup_name, 24.hours)
+        end
+      end
+    end
+
+    def stored_sessions
+      Gitlab::Redis::SharedState.with do |redis|
+        redis.scard(session_lookup_name)
+      end
+    end
+
+    def cleanup_session_per_ip_entries
+      Gitlab::Redis::SharedState.with do |redis|
+        redis.srem(session_lookup_name, session_id)
+      end
+    end
+
+    private
+
+    attr_reader :remote_ip, :session_id
+
+    def session_lookup_name
+      @session_lookup_name ||= "#{Gitlab::Redis::SharedState::IP_SESSIONS_LOOKUP_NAMESPACE}:#{remote_ip}"
+    end
+  end
+end

lib/gitlab/recaptcha.rb

@@ -3,7 +3,7 @@
 module Gitlab
   module Recaptcha
     def self.load_configurations!
-      if Gitlab::CurrentSettings.recaptcha_enabled
+      if Gitlab::CurrentSettings.recaptcha_enabled || enabled_on_login?
         ::Recaptcha.configure do |config|
           config.site_key = Gitlab::CurrentSettings.recaptcha_site_key
           config.secret_key = Gitlab::CurrentSettings.recaptcha_private_key
@@ -16,5 +16,9 @@ module Gitlab
     def self.enabled?
       Gitlab::CurrentSettings.recaptcha_enabled
     end
+
+    def self.enabled_on_login?
+      Gitlab::CurrentSettings.login_recaptcha_protection_enabled
+    end
   end
 end

lib/gitlab/redis/shared_state.rb

@@ -9,6 +9,7 @@ module Gitlab
       SESSION_NAMESPACE = 'session:gitlab'.freeze
       USER_SESSIONS_NAMESPACE = 'session:user:gitlab'.freeze
       USER_SESSIONS_LOOKUP_NAMESPACE = 'session:lookup:user:gitlab'.freeze
+      IP_SESSIONS_LOOKUP_NAMESPACE = 'session:lookup:ip:gitlab'.freeze
       DEFAULT_REDIS_SHARED_STATE_URL = 'redis://localhost:6382'.freeze
       REDIS_SHARED_STATE_CONFIG_ENV_VAR_NAME = 'GITLAB_REDIS_SHARED_STATE_CONFIG_FILE'.freeze

locale/gitlab.pot

@@ -3796,7 +3796,7 @@ msgstr ""
 msgid "Enable or disable version check and usage ping."
 msgstr ""
-msgid "Enable reCAPTCHA or Akismet and set IP limits."
+msgid "Enable reCAPTCHA or Akismet and set IP limits. For reCAPTCHA, we currently only support %{recaptcha_v2_link_start}v2%{recaptcha_v2_link_end}"
 msgstr ""
 msgid "Enable shared Runners"
@@ -4983,6 +4983,9 @@ msgstr ""
 msgid "Help page text and support page url."
 msgstr ""
+msgid "Helps prevent bots from brute-force attacks."
+msgstr ""
+
 msgid "Hide archived projects"
 msgstr ""

spec/controllers/sessions_controller_spec.rb

@@ -100,16 +100,8 @@ describe SessionsController do
         end
       end
-      context 'when reCAPTCHA is enabled' do
-        let(:user) { create(:user) }
-        let(:user_params) { { login: user.username, password: user.password } }
-
-        before do
-          stub_application_setting(recaptcha_enabled: true)
-          request.headers[described_class::CAPTCHA_HEADER] = 1
-        end
-
-        it 'displays an error when the reCAPTCHA is not solved' do
+      context 'with reCAPTCHA' do
+        def unsuccesful_login(user_params, sesion_params: {})
           # Without this, `verify_recaptcha` arbitrarily returns true in test env
           Recaptcha.configuration.skip_verify_env.delete('test')
           counter = double(:counter)
@@ -119,14 +111,10 @@ describe SessionsController do
                                       .with(:failed_login_captcha_total, anything)
                                       .and_return(counter)
-          post(:create, params: { user: user_params })
-
-          expect(response).to render_template(:new)
-          expect(flash[:alert]).to include 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.'
-          expect(subject.current_user).to be_nil
+          post(:create, params: { user: user_params }, session: sesion_params)
         end
-        it 'successfully logs in a user when reCAPTCHA is solved' do
+        def succesful_login(user_params, sesion_params: {})
           # Avoid test ordering issue and ensure `verify_recaptcha` returns true
           Recaptcha.configuration.skip_verify_env << 'test'
           counter = double(:counter)
@@ -137,9 +125,80 @@ describe SessionsController do
                                       .and_return(counter)
           expect(Gitlab::Metrics).to receive(:counter).and_call_original
-          post(:create, params: { user: user_params })
-          expect(subject.current_user).to eq user
+          post(:create, params: { user: user_params }, session: sesion_params)
+        end
+        context 'when reCAPTCHA is enabled' do
+          let(:user) { create(:user) }
+          let(:user_params) { { login: user.username, password: user.password } }
+
+          before do
+            stub_application_setting(recaptcha_enabled: true)
+            request.headers[described_class::CAPTCHA_HEADER] = 1
+          end
+
+          it 'displays an error when the reCAPTCHA is not solved' do
+            # Without this, `verify_recaptcha` arbitrarily returns true in test env
+
+            unsuccesful_login(user_params)
+
+            expect(response).to render_template(:new)
+            expect(flash[:alert]).to include 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.'
+            expect(subject.current_user).to be_nil
+          end
+
+          it 'successfully logs in a user when reCAPTCHA is solved' do
+            succesful_login(user_params)
+
+            expect(subject.current_user).to eq user
+          end
+        end
+
+        context 'when reCAPTCHA login protection is enabled' do
+          let(:user) { create(:user) }
+          let(:user_params) { { login: user.username, password: user.password } }
+
+          before do
+            stub_application_setting(login_recaptcha_protection_enabled: true)
+          end
+
+          context 'when user tried to login 5 times' do
+            it 'displays an error when the reCAPTCHA is not solved' do
+              unsuccesful_login(user_params, sesion_params: { failed_login_attempts: 6 })
+
+              expect(response).to render_template(:new)
+              expect(flash[:alert]).to include 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.'
+              expect(subject.current_user).to be_nil
+            end
+
+            it 'successfully logs in a user when reCAPTCHA is solved' do
+              succesful_login(user_params, sesion_params: { failed_login_attempts: 6 })
+
+              expect(subject.current_user).to eq user
+            end
+          end
+
+          context 'when there are more than 5 anonymous session with the same IP' do
+            before do
+              allow(Gitlab::AnonymousSession).to receive_message_chain(:new, :stored_sessions).and_return(6)
+            end
+
+            it 'displays an error when the reCAPTCHA is not solved' do
+              unsuccesful_login(user_params)
+
+              expect(response).to render_template(:new)
+              expect(flash[:alert]).to include 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.'
+              expect(subject.current_user).to be_nil
+            end
+
+            it 'successfully logs in a user when reCAPTCHA is solved' do
+              expect(Gitlab::AnonymousSession).to receive_message_chain(:new, :cleanup_session_per_ip_entries)
+
+              succesful_login(user_params)
+
+              expect(subject.current_user).to eq user
+            end
+          end
         end
       end
     end
@@ -348,4 +407,17 @@ describe SessionsController do
       expect(controller.stored_location_for(:redirect)).to eq(search_path)
     end
   end
+
+  context 'when login fails' do
+    before do
+      set_devise_mapping(context: @request)
+      @request.env["warden.options"] = { action:  'unauthenticated' }
+    end
+
+    it 'does increment failed login counts for session' do
+      get(:new, params: { user: { login: 'failed' } })
+
+      expect(session[:failed_login_attempts]).to eq(1)
+    end
+  end
 end

spec/features/admin/admin_settings_spec.rb

@@ -261,6 +261,7 @@ describe 'Admin updates settings' do
       page.within('.as-spam') do
         check 'Enable reCAPTCHA'
+        check 'Enable reCAPTCHA for login'
         fill_in 'reCAPTCHA Site Key', with: 'key'
         fill_in 'reCAPTCHA Private Key', with: 'key'
         fill_in 'IPs per user', with: 15
@@ -268,7 +269,9 @@ describe 'Admin updates settings' do
       end
       expect(page).to have_content "Application settings saved successfully"
+
       expect(Gitlab::CurrentSettings.recaptcha_enabled).to be true
+      expect(Gitlab::CurrentSettings.login_recaptcha_protection_enabled).to be true
       expect(Gitlab::CurrentSettings.unique_ips_limit_per_user).to eq(15)
     end
   end

spec/lib/gitlab/anonymous_session_spec.rb

+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe Gitlab::AnonymousSession, :clean_gitlab_redis_shared_state do
+  let(:default_session_id) { '6919a6f1bb119dd7396fadc38fd18d0d' }
+  let(:additional_session_id) { '7919a6f1bb119dd7396fadc38fd18d0d' }
+
+  subject { new_anonymous_session }
+
+  def new_anonymous_session(session_id = default_session_id)
+    described_class.new('127.0.0.1', session_id: session_id)
+  end
+
+  describe '#store_session_id_per_ip' do
+    it 'adds session id to proper key' do
+      subject.store_session_id_per_ip
+
+      Gitlab::Redis::SharedState.with do |redis|
+        expect(redis.smembers("session:lookup:ip:gitlab:127.0.0.1")).to eq [default_session_id]
+      end
+    end
+
+    it 'adds expiration time to key' do
+      Timecop.freeze do
+        subject.store_session_id_per_ip
+
+        Gitlab::Redis::SharedState.with do |redis|
+          expect(redis.ttl("session:lookup:ip:gitlab:127.0.0.1")).to eq(24.hours.to_i)
+        end
+      end
+    end
+
+    it 'adds id only once' do
+      subject.store_session_id_per_ip
+      subject.store_session_id_per_ip
+
+      Gitlab::Redis::SharedState.with do |redis|
+        expect(redis.smembers("session:lookup:ip:gitlab:127.0.0.1")).to eq [default_session_id]
+      end
+    end
+
+    context 'when there is already one session' do
+      it 'adds session id to proper key' do
+        subject.store_session_id_per_ip
+        new_anonymous_session(additional_session_id).store_session_id_per_ip
+
+        Gitlab::Redis::SharedState.with do |redis|
+          expect(redis.smembers("session:lookup:ip:gitlab:127.0.0.1")).to contain_exactly(default_session_id, additional_session_id)
+        end
+      end
+    end
+  end
+
+  describe '#stored_sessions' do
+    it 'returns all anonymous sessions per ip' do
+      Gitlab::Redis::SharedState.with do |redis|
+        redis.sadd("session:lookup:ip:gitlab:127.0.0.1", default_session_id)
+        redis.sadd("session:lookup:ip:gitlab:127.0.0.1", additional_session_id)
+      end
+
+      expect(subject.stored_sessions).to eq(2)
+    end
+  end
+
+  it 'removes obsolete lookup through ip entries' do
+    Gitlab::Redis::SharedState.with do |redis|
+      redis.sadd("session:lookup:ip:gitlab:127.0.0.1", default_session_id)
+      redis.sadd("session:lookup:ip:gitlab:127.0.0.1", additional_session_id)
+    end
+
+    subject.cleanup_session_per_ip_entries
+
+    Gitlab::Redis::SharedState.with do |redis|
+      expect(redis.smembers("session:lookup:ip:gitlab:127.0.0.1")).to eq [additional_session_id]
+    end
+  end
+end

spec/views/devise/shared/_signin_box.html.haml_spec.rb

@@ -7,6 +7,7 @@ describe 'devise/shared/_signin_box' do
       assign(:ldap_servers, [])
       allow(view).to receive(:current_application_settings).and_return(Gitlab::CurrentSettings.current_application_settings)
       allow(view).to receive(:captcha_enabled?).and_return(false)
+      allow(view).to receive(:captcha_on_login_required?).and_return(false)
     end
     it 'is shown when Crowd is enabled' do