Commit 00da609c authored by Grzegorz Bizon's avatar Grzegorz Bizon 💡
Browse files

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.
parent 301f4074

app/controllers/sessions_controller.rb

+9 −6
Changes for app/controllers/sessions_controller.rb: 9 added lines, 6 removed lines.
Original line number Diff line number Diff line
@@ -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 @@ def user_params 
  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 @@ def store_redirect_path 
    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

spec/controllers/sessions_controller_spec.rb

+25 −18
Changes for spec/controllers/sessions_controller_spec.rb: 25 added lines, 18 removed lines.
Original line number Diff line number Diff line
@@ -6,7 +6,7 @@ 
      @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 @@ 
        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 @@ 
      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,11 +38,11 @@ def authenticate_2fa(user_params) 
      ##

      # See #14900 issue

      #

      context 'authenticating with login and OTP belonging to another user' do

      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 'OTP valid for another user' do

          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)
@@ -51,23 +51,17 @@ def authenticate_2fa(user_params) 
            end

          end



        context 'OTP invalid for another user' do

          before do

          context 'when OTP is invalid for another user' do

            it 'does not authenticate' do

              authenticate_2fa(login: another_user.username,

                               otp_attempt: 'invalid')

          end



          it 'does not authenticate' do

              expect(subject.current_user).to_not eq another_user

            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 'authenticating with OTP' do

          context 'valid OTP' do

          context 'when authenticating with OTP' do

            context 'when OTP is valid' do

              it 'authenticates correctly' do

                authenticate_2fa(otp_attempt: user.current_otp)
@@ -75,7 +69,7 @@ def authenticate_2fa(user_params) 
              end

            end



          context 'invalid OTP' do

            context ' when OTP is invalid' do

              before { authenticate_2fa(otp_attempt: 'invalid') }



              it 'does not authenticate' do
@@ -83,7 +77,20 @@ def authenticate_2fa(user_params) 
              end



              it 'warns about invalid OTP code' do

              expect(response).to set_flash.now[:alert].to /Invalid two-factor code/

                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_not set_flash.now[:alert].to /Invalid login or password/

            end

          end

        end