Commit 9933b52c authored by Grzegorz Bizon's avatar Grzegorz Bizon Committed by Yorick Peterse

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 6842dc66
......@@ -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
......
......@@ -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,11 +38,11 @@ describe SessionsController do
##
# 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 @@ describe SessionsController do
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 @@ describe SessionsController do
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 @@ describe SessionsController do
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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment