diff options
author | Rémy Coutable <remy@gitlab.com> | 2016-04-07 11:56:44 +0000 |
---|---|---|
committer | Rémy Coutable <remy@gitlab.com> | 2016-04-07 11:56:44 +0000 |
commit | 237324cc17c57d0ac96c413864b67388d278bfb1 (patch) | |
tree | a8768eae3103261659e2889760db00210141249a /app | |
parent | 92897d7683bdf17da9708e065465d55ecd808dff (diff) | |
parent | 33a8dfd04fbd1c0858ead20c020ede07e7b0962a (diff) | |
download | gitlab-ce-237324cc17c57d0ac96c413864b67388d278bfb1.tar.gz |
Merge branch 'fix/2fa-authentication-spoofing' into 'master'
Fix 2FA authentication spoofing
## Summary
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.
## Fix
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.
## Further work
Current 2FA code is a bit tricky, so it probably needs some refactoring.
See merge request !1947
Diffstat (limited to 'app')
-rw-r--r-- | app/controllers/sessions_controller.rb | 15 |
1 files changed, 9 insertions, 6 deletions
diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 65677a3dd3c..c29f4609e93 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -5,7 +5,8 @@ class SessionsController < Devise::SessionsController skip_before_action :check_2fa_requirement, only: [:destroy] prepend_before_action :check_initial_setup, only: [:new] - 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] @@ -56,10 +57,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 @@ -83,11 +84,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 |