Welcome to mirror list, hosted at ThFree Co, Russian Federation.

gitlab.com/gitlab-org/gitlab-foss.git - Unnamed repository; edit this file 'description' to name the repository.
summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRémy Coutable <remy@gitlab.com>2016-04-07 14:56:44 +0300
committerRémy Coutable <remy@rymai.me>2016-04-07 18:33:47 +0300
commit4a9f5ef93152cce6309d6e7cbe84bc7006e220cd (patch)
tree170047bce4ca3c7453b53d42237981ce40ca94f5
parent2b060c64e7e44e7ee65c15eaea2992bb3e78b73b (diff)
Merge branch 'fix/2fa-authentication-spoofing' into 'master'
Fix 2FA authentication spoofing This is security fix for vulnerability described at https://gitlab.com/gitlab-org/gitlab-ce/issues/14900. Attacker was able to bypass password authentication of users that have 2FA enabled, and consequently sign is as a different user, without knowing his password, if he managed to guess 2FA One Time Password for that user. It was also possible to enumerate users and check if they have 2FA enabled, because GitLab responded with different error for each case. This MR attempts to change default user search scope if `otp_user_id` session variable has been set. If it is present, it means that user has 2FA enabled, and has already been verified with login and password. In this case we should look for user with `otp_user_id` first, before picking it up by `login`. Both, 2FA authentication spoofing and 2FA discovery have been covered by specs. Current 2FA code is a bit tricky, so it probably needs some refactoring. Signed-off-by: Rémy Coutable <remy@rymai.me>
-rw-r--r--CHANGELOG3
-rw-r--r--app/controllers/sessions_controller.rb15
-rw-r--r--spec/controllers/sessions_controller_spec.rb101
3 files changed, 113 insertions, 6 deletions
diff --git a/CHANGELOG b/CHANGELOG
index 3bbaf66e37e..a67d246b494 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,5 +1,8 @@
Please view this file on the master branch, on stable branches it's out of date.
+v 8.5.10
+ - Fix a 2FA authentication spoofing vulnerability.
+
v 8.5.9
- Don't attempt to fetch any tags from a forked repo (Stan Hu). !3504
diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
index 44eb58e418b..27637090d7f 100644
--- a/app/controllers/sessions_controller.rb
+++ b/app/controllers/sessions_controller.rb
@@ -4,7 +4,8 @@ class SessionsController < Devise::SessionsController
skip_before_action :check_2fa_requirement, only: [:destroy]
- prepend_before_action :authenticate_with_two_factor, only: [:create]
+ prepend_before_action :authenticate_with_two_factor,
+ if: :two_factor_enabled?, only: [:create]
prepend_before_action :store_redirect_path, only: [:new]
before_action :auto_sign_in_with_provider, only: [:new]
before_action :load_recaptcha
@@ -38,10 +39,10 @@ class SessionsController < Devise::SessionsController
end
def find_user
- if user_params[:login]
- User.by_login(user_params[:login])
- elsif user_params[:otp_attempt] && session[:otp_user_id]
+ if session[:otp_user_id]
User.find(session[:otp_user_id])
+ elsif user_params[:login]
+ User.by_login(user_params[:login])
end
end
@@ -65,11 +66,13 @@ class SessionsController < Devise::SessionsController
end
end
+ def two_factor_enabled?
+ find_user.try(:two_factor_enabled?)
+ end
+
def authenticate_with_two_factor
user = self.resource = find_user
- return unless user && user.two_factor_enabled?
-
if user_params[:otp_attempt].present? && session[:otp_user_id]
if valid_otp_attempt?(user)
# Remove any lingering user data from login
diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb
new file mode 100644
index 00000000000..83cc8ec6d26
--- /dev/null
+++ b/spec/controllers/sessions_controller_spec.rb
@@ -0,0 +1,101 @@
+require 'spec_helper'
+
+describe SessionsController do
+ describe '#create' do
+ before do
+ @request.env['devise.mapping'] = Devise.mappings[:user]
+ end
+
+ context 'when using standard authentications' do
+ context 'invalid password' do
+ it 'does not authenticate user' do
+ post(:create, user: { login: 'invalid', password: 'invalid' })
+
+ expect(response)
+ .to set_flash.now[:alert].to /Invalid login or password/
+ end
+ end
+
+ context 'when using valid password' do
+ let(:user) { create(:user) }
+
+ it 'authenticates user correctly' do
+ post(:create, user: { login: user.username, password: user.password })
+
+ expect(response).to set_flash.to /Signed in successfully/
+ expect(subject.current_user). to eq user
+ end
+ end
+ end
+
+ context 'when using two-factor authentication' do
+ let(:user) { create(:user, :two_factor) }
+
+ def authenticate_2fa(user_params)
+ post(:create, { user: user_params }, { otp_user_id: user.id })
+ end
+
+ ##
+ # See #14900 issue
+ #
+ context 'when authenticating with login and OTP of another user' do
+ context 'when another user has 2FA enabled' do
+ let(:another_user) { create(:user, :two_factor) }
+
+ context 'when OTP is valid for another user' do
+ it 'does not authenticate' do
+ authenticate_2fa(login: another_user.username,
+ otp_attempt: another_user.current_otp)
+
+ expect(subject.current_user).to_not eq another_user
+ end
+ end
+
+ context 'when OTP is invalid for another user' do
+ it 'does not authenticate' do
+ authenticate_2fa(login: another_user.username,
+ otp_attempt: 'invalid')
+
+ expect(subject.current_user).to_not eq another_user
+ end
+ end
+
+ context 'when authenticating with OTP' do
+ context 'when OTP is valid' do
+ it 'authenticates correctly' do
+ authenticate_2fa(otp_attempt: user.current_otp)
+
+ expect(subject.current_user).to eq user
+ end
+ end
+
+ context 'when OTP is invalid' do
+ before { authenticate_2fa(otp_attempt: 'invalid') }
+
+ it 'does not authenticate' do
+ expect(subject.current_user).to_not eq user
+ end
+
+ it 'warns about invalid OTP code' do
+ expect(response).to set_flash.now[:alert]
+ .to /Invalid two-factor code/
+ end
+ end
+ end
+
+ context 'when another user does not have 2FA enabled' do
+ let(:another_user) { create(:user) }
+
+ it 'does not leak that 2FA is disabled for another user' do
+ authenticate_2fa(login: another_user.username,
+ otp_attempt: 'invalid')
+
+ expect(response).to set_flash.now[:alert]
+ .to /Invalid two-factor code/
+ end
+ end
+ end
+ end
+ end
+ end
+end