Fix 2FA authentication spoofing vulnerability

This commit 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.

2 files changed, 51 insertions, 41 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
diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb
index e7dbc3bdad4..664b23d3214 100644
--- a/spec/controllers/sessions_controller_spec.rb
+++ b/spec/controllers/sessions_controller_spec.rb
@@ -6,7 +6,7 @@ describe SessionsController do
       @request.env['devise.mapping'] = Devise.mappings[:user]
     end
 
-    context 'standard authentications' do
+    context 'when using standard authentications' do
       context 'invalid password' do
         it 'does not authenticate user' do
           post(:create, user: { login: 'invalid', password: 'invalid' })
@@ -16,7 +16,7 @@ describe SessionsController do
         end
       end
 
-      context 'valid password' do
+      context 'when using valid password' do
         let(:user) { create(:user) }
 
         it 'authenticates user correctly' do
@@ -28,7 +28,7 @@ describe SessionsController do
       end
     end
 
-    context 'two-factor authentication' do
+    context 'when using two-factor authentication' do
       let(:user) { create(:user, :two_factor) }
 
       def authenticate_2fa(user_params)
@@ -38,52 +38,59 @@ describe SessionsController do
       ##
       # See #14900 issue
       #
-      context 'authenticating with login and OTP belonging to another user' do
-        let(:another_user) { create(:user, :two_factor) }
+      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)
 
-        context 'OTP 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
+              expect(subject.current_user).to_not eq another_user
+            end
           end
-        end
 
-        context 'OTP invalid for another user' do
-          before do
-            authenticate_2fa(login: another_user.username,
-                             otp_attempt: 'invalid')
-          end
+          context 'when OTP is invalid for another user' do
+            it 'does not authenticate' do
+              authenticate_2fa(login: another_user.username,
+                               otp_attempt: 'invalid')
 
-          it 'does not authenticate' do
-            expect(subject.current_user).to_not eq another_user
+              expect(subject.current_user).to_not eq another_user
+            end
           end
 
-          it 'does not leak information about 2FA enabled' do
-            expect(response).to_not set_flash.now[:alert].to /Invalid two-factor code/
-          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 'authenticating with OTP' do
-          context 'valid OTP' do
-            it 'authenticates correctly' do
-              authenticate_2fa(otp_attempt: user.current_otp)
+            context ' when OTP is invalid' do
+              before { authenticate_2fa(otp_attempt: 'invalid') }
 
-              expect(subject.current_user).to eq user
+              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 'invalid OTP' do
-            before { authenticate_2fa(otp_attempt: 'invalid') }
+          context 'when another user does not have 2FA enabled' do
+            let(:another_user) { create(:user) }
 
-            it 'does not authenticate' do
-              expect(subject.current_user).to_not eq user
-            end
+            it 'does not leak that 2FA is disabled for another user' do
+              authenticate_2fa(login: another_user.username,
+                               otp_attempt: 'invalid')
 
-            it 'warns about invalid OTP code' do
-              expect(response).to set_flash.now[:alert].to /Invalid two-factor code/
+              expect(response).to_not set_flash.now[:alert].to /Invalid login or password/
             end
           end
         end