Merge branch 'security-disallowed-passwords-14-8' into '14-8-stable-ee'

Disallow login if password matches a fixed list

See merge request gitlab-org/security/gitlab!2358

app/models/user.rb

@@ -880,6 +880,23 @@ class User < ApplicationRecord
     reset_password_sent_at.present? && reset_password_sent_at >= 1.minute.ago
   end
+  # See https://gitlab.com/gitlab-org/security/gitlab/-/issues/638
+  DISALLOWED_PASSWORDS = %w[123qweQWE!@#000000000].freeze
+
+  # Overwrites valid_password? from Devise::Models::DatabaseAuthenticatable
+  # In constant-time, check both that the password isn't on a denylist AND
+  # that the password is the user's password
+  def valid_password?(password)
+    password_allowed = true
+    DISALLOWED_PASSWORDS.each do |disallowed_password|
+      password_allowed = false if Devise.secure_compare(password, disallowed_password)
+    end
+
+    original_result = super
+
+    password_allowed && original_result
+  end
+
   def remember_me!
     super if ::Gitlab::Database.read_write?
   end

spec/factories/users.rb

@@ -27,6 +27,10 @@ FactoryBot.define do
       after(:build) { |user, _| user.block! }
     end
+    trait :disallowed_password do
+      password { User::DISALLOWED_PASSWORDS.first }
+    end
+
     trait :blocked_pending_approval do
       after(:build) { |user, _| user.block_pending_approval! }
     end

spec/features/users/login_spec.rb

@@ -150,6 +150,27 @@ RSpec.describe 'Login', :clean_gitlab_redis_sessions do
     end
   end
+  describe 'with a disallowed password' do
+    let(:user) { create(:user, :disallowed_password) }
+
+    before do
+      expect(authentication_metrics)
+        .to increment(:user_unauthenticated_counter)
+        .and increment(:user_password_invalid_counter)
+    end
+
+    it 'disallows login' do
+      gitlab_sign_in(user, password: user.password)
+
+      expect(page).to have_content('Invalid login or password.')
+    end
+
+    it 'does not update Devise trackable attributes' do
+      expect { gitlab_sign_in(user, password: user.password) }
+        .not_to change { User.ghost.reload.sign_in_count }
+    end
+  end
+
   describe 'with the ghost user' do
     it 'disallows login' do
       expect(authentication_metrics)

spec/models/user_spec.rb

@@ -5623,6 +5623,36 @@ RSpec.describe User do
     end
   end
+  describe '#valid_password?' do
+    subject { user.valid_password?(password) }
+
+    context 'user with password not in disallowed list' do
+      let(:user) { create(:user) }
+      let(:password) { user.password }
+
+      it { is_expected.to be_truthy }
+
+      context 'using a wrong password' do
+        let(:password) { 'WRONG PASSWORD' }
+
+        it { is_expected.to be_falsey }
+      end
+    end
+
+    context 'user with disallowed password' do
+      let(:user) { create(:user, :disallowed_password) }
+      let(:password) { user.password }
+
+      it { is_expected.to be_falsey }
+
+      context 'using a wrong password' do
+        let(:password) { 'WRONG PASSWORD' }
+
+        it { is_expected.to be_falsey }
+      end
+    end
+  end
+
   describe '#password_expired?' do
     let(:user) { build(:user, password_expires_at: password_expires_at) }

spec/support/helpers/login_helpers.rb

@@ -91,11 +91,12 @@ module LoginHelpers
   # user - User instance to login with
   # remember - Whether or not to check "Remember me" (default: false)
   # two_factor_auth - If two-factor authentication is enabled (default: false)
-  def gitlab_sign_in_with(user, remember: false, two_factor_auth: false)
+  # password - password to attempt to login with
+  def gitlab_sign_in_with(user, remember: false, two_factor_auth: false, password: nil)
     visit new_user_session_path
     fill_in "user_login", with: user.email
-    fill_in "user_password", with: "12345678"
+    fill_in "user_password", with: (password || "12345678")
     check 'user_remember_me' if remember
     click_button "Sign in"