Commit 6da54f2e authored by Rémy Coutable's avatar Rémy Coutable Committed by Robert Speicher

Merge branch 'fix/2fa-authentication-spoofing' into 'master'

Fix 2FA authentication spoofing
Signed-off-by: default avatarRémy Coutable <remy@rymai.me>
parent 1419bb1b
Please view this file on the master branch, on stable branches it's out of date. Please view this file on the master branch, on stable branches it's out of date.
v 8.3.7
- Fix a 2FA authentication spoofing vulnerability.
v 8.3.6 v 8.3.6
- Don't attempt to fetch any tags from a forked repo (Stan Hu). - Don't attempt to fetch any tags from a forked repo (Stan Hu).
......
...@@ -2,7 +2,8 @@ class SessionsController < Devise::SessionsController ...@@ -2,7 +2,8 @@ class SessionsController < Devise::SessionsController
include AuthenticatesWithTwoFactor include AuthenticatesWithTwoFactor
include Recaptcha::ClientHelper include Recaptcha::ClientHelper
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] prepend_before_action :store_redirect_path, only: [:new]
before_action :auto_sign_in_with_provider, only: [:new] before_action :auto_sign_in_with_provider, only: [:new]
before_action :load_recaptcha before_action :load_recaptcha
...@@ -36,10 +37,10 @@ class SessionsController < Devise::SessionsController ...@@ -36,10 +37,10 @@ class SessionsController < Devise::SessionsController
end end
def find_user def find_user
if user_params[:login] if session[:otp_user_id]
User.by_login(user_params[:login])
elsif user_params[:otp_attempt] && session[:otp_user_id]
User.find(session[:otp_user_id]) User.find(session[:otp_user_id])
elsif user_params[:login]
User.by_login(user_params[:login])
end end
end end
...@@ -63,11 +64,13 @@ class SessionsController < Devise::SessionsController ...@@ -63,11 +64,13 @@ class SessionsController < Devise::SessionsController
end end
end end
def two_factor_enabled?
find_user.try(:two_factor_enabled?)
end
def authenticate_with_two_factor def authenticate_with_two_factor
user = self.resource = find_user user = self.resource = find_user
return unless user && user.two_factor_enabled?
if user_params[:otp_attempt].present? && session[:otp_user_id] if user_params[:otp_attempt].present? && session[:otp_user_id]
if valid_otp_attempt?(user) if valid_otp_attempt?(user)
# Remove any lingering user data from login # Remove any lingering user data from login
......
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
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