diff options
-rw-r--r-- | app/controllers/omniauth_callbacks_controller.rb | 9 | ||||
-rw-r--r-- | changelogs/unreleased/security-sarcila-verify-saml-request-origin.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/auth/omniauth_identity_linker_base.rb | 5 | ||||
-rw-r--r-- | lib/gitlab/auth/saml/identity_linker.rb | 24 | ||||
-rw-r--r-- | lib/gitlab/auth/saml/origin_validator.rb | 41 | ||||
-rw-r--r-- | lib/omni_auth/strategies/saml.rb | 29 | ||||
-rw-r--r-- | locale/gitlab.pot | 3 | ||||
-rw-r--r-- | spec/controllers/omniauth_callbacks_controller_spec.rb | 58 | ||||
-rw-r--r-- | spec/lib/gitlab/auth/saml/identity_linker_spec.rb | 66 | ||||
-rw-r--r-- | spec/lib/gitlab/auth/saml/origin_validator_spec.rb | 42 | ||||
-rw-r--r-- | spec/lib/omni_auth/strategies/saml_spec.rb | 22 | ||||
-rw-r--r-- | spec/support/omniauth_strategy.rb | 39 |
12 files changed, 303 insertions, 40 deletions
diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index ee2cf47d5cb..755ce3463c4 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -40,6 +40,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController def saml omniauth_flow(Gitlab::Auth::Saml) + rescue Gitlab::Auth::Saml::IdentityLinker::UnverifiedRequest + redirect_unverified_saml_initiation end def omniauth_error @@ -92,8 +94,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController return render_403 unless link_provider_allowed?(oauth['provider']) log_audit_event(current_user, with: oauth['provider']) - - identity_linker ||= auth_module::IdentityLinker.new(current_user, oauth) + identity_linker ||= auth_module::IdentityLinker.new(current_user, oauth, session) link_identity(identity_linker) @@ -194,6 +195,10 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController redirect_to new_user_session_path end + def redirect_unverified_saml_initiation + redirect_to profile_account_path, notice: _('Request to link SAML account must be authorized') + end + def handle_disabled_provider label = Gitlab::Auth::OAuth::Provider.label_for(oauth['provider']) flash[:alert] = _("Signing in using %{label} has been disabled") % { label: label } diff --git a/changelogs/unreleased/security-sarcila-verify-saml-request-origin.yml b/changelogs/unreleased/security-sarcila-verify-saml-request-origin.yml new file mode 100644 index 00000000000..9022bc8a26f --- /dev/null +++ b/changelogs/unreleased/security-sarcila-verify-saml-request-origin.yml @@ -0,0 +1,5 @@ +--- +title: Prevent GitLab accounts takeover if SAML is configured +merge_request: +author: +type: security diff --git a/lib/gitlab/auth/omniauth_identity_linker_base.rb b/lib/gitlab/auth/omniauth_identity_linker_base.rb index c620fc5d6bd..a6c247f31a7 100644 --- a/lib/gitlab/auth/omniauth_identity_linker_base.rb +++ b/lib/gitlab/auth/omniauth_identity_linker_base.rb @@ -3,12 +3,13 @@ module Gitlab module Auth class OmniauthIdentityLinkerBase - attr_reader :current_user, :oauth + attr_reader :current_user, :oauth, :session - def initialize(current_user, oauth) + def initialize(current_user, oauth, session = {}) @current_user = current_user @oauth = oauth @changed = false + @session = session end def link diff --git a/lib/gitlab/auth/saml/identity_linker.rb b/lib/gitlab/auth/saml/identity_linker.rb index ae0d6dded4e..93195c3189f 100644 --- a/lib/gitlab/auth/saml/identity_linker.rb +++ b/lib/gitlab/auth/saml/identity_linker.rb @@ -4,6 +4,30 @@ module Gitlab module Auth module Saml class IdentityLinker < OmniauthIdentityLinkerBase + extend ::Gitlab::Utils::Override + + UnverifiedRequest = Class.new(StandardError) + + override :link + def link + raise_unless_request_is_gitlab_initiated! if unlinked? + + super + end + + protected + + def raise_unless_request_is_gitlab_initiated! + raise UnverifiedRequest unless valid_gitlab_initiated_request? + end + + def valid_gitlab_initiated_request? + OriginValidator.new(session).gitlab_initiated?(saml_response) + end + + def saml_response + oauth.fetch(:extra, {}).fetch(:response_object, {}) + end end end end diff --git a/lib/gitlab/auth/saml/origin_validator.rb b/lib/gitlab/auth/saml/origin_validator.rb new file mode 100644 index 00000000000..4ecc688888f --- /dev/null +++ b/lib/gitlab/auth/saml/origin_validator.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module Gitlab + module Auth + module Saml + class OriginValidator + AUTH_REQUEST_SESSION_KEY = "last_authn_request_id".freeze + + def initialize(session) + @session = session || {} + end + + def store_origin(authn_request) + session[AUTH_REQUEST_SESSION_KEY] = authn_request.uuid + end + + def gitlab_initiated?(saml_response) + return false if identity_provider_initiated?(saml_response) + + matches?(saml_response) + end + + private + + attr_reader :session + + def matches?(saml_response) + saml_response.in_response_to == expected_request_id + end + + def identity_provider_initiated?(saml_response) + saml_response.in_response_to.blank? + end + + def expected_request_id + session[AUTH_REQUEST_SESSION_KEY] + end + end + end + end +end diff --git a/lib/omni_auth/strategies/saml.rb b/lib/omni_auth/strategies/saml.rb new file mode 100644 index 00000000000..ebe062f17e0 --- /dev/null +++ b/lib/omni_auth/strategies/saml.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module OmniAuth + module Strategies + class SAML + extend ::Gitlab::Utils::Override + + # NOTE: This method duplicates code from omniauth-saml + # so that we can access authn_request to store it + # See: https://github.com/omniauth/omniauth-saml/issues/172 + override :request_phase + def request_phase + authn_request = OneLogin::RubySaml::Authrequest.new + + store_authn_request_id(authn_request) + + with_settings do |settings| + redirect(authn_request.create(settings, additional_params_for_authn_request)) + end + end + + private + + def store_authn_request_id(authn_request) + Gitlab::Auth::Saml::OriginValidator.new(session).store_origin(authn_request) + end + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 751fc4c85ce..46b45f5a180 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -13242,6 +13242,9 @@ msgstr "" msgid "Request Access" msgstr "" +msgid "Request to link SAML account must be authorized" +msgstr "" + msgid "Requested %{time_ago}" msgstr "" diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb index 5536b7df64e..9371c434f49 100644 --- a/spec/controllers/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -247,34 +247,70 @@ describe OmniauthCallbacksController, type: :controller do end describe '#saml' do + let(:last_request_id) { 'ONELOGIN_4fee3b046395c4e751011e97f8900b5273d56685' } let(:user) { create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') } let(:mock_saml_response) { File.read('spec/fixtures/authentication/saml_response.xml') } let(:saml_config) { mock_saml_config_with_upstream_two_factor_authn_contexts } + def stub_last_request_id(id) + session['last_authn_request_id'] = id + end + before do + stub_last_request_id(last_request_id) stub_omniauth_saml_config({ enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [saml_config] }) mock_auth_hash_with_saml_xml('saml', +'my-uid', user.email, mock_saml_response) - request.env["devise.mapping"] = Devise.mappings[:user] + request.env['devise.mapping'] = Devise.mappings[:user] request.env['omniauth.auth'] = Rails.application.env_config['omniauth.auth'] - post :saml, params: { SAMLResponse: mock_saml_response } end - context 'when worth two factors' do - let(:mock_saml_response) do - File.read('spec/fixtures/authentication/saml_response.xml') + context 'with GitLab initiated request' do + before do + post :saml, params: { SAMLResponse: mock_saml_response } + end + + context 'when worth two factors' do + let(:mock_saml_response) do + File.read('spec/fixtures/authentication/saml_response.xml') .gsub('urn:oasis:names:tc:SAML:2.0:ac:classes:Password', 'urn:oasis:names:tc:SAML:2.0:ac:classes:SecondFactorIGTOKEN') + end + + it 'expects user to be signed_in' do + expect(request.env['warden']).to be_authenticated + end end - it 'expects user to be signed_in' do - expect(request.env['warden']).to be_authenticated + context 'when not worth two factors' do + it 'expects user to provide second factor' do + expect(response).to render_template('devise/sessions/two_factor') + expect(request.env['warden']).not_to be_authenticated + end end end - context 'when not worth two factors' do - it 'expects user to provide second factor' do - expect(response).to render_template('devise/sessions/two_factor') - expect(request.env['warden']).not_to be_authenticated + context 'with IdP initiated request' do + let(:user) { create(:user) } + let(:last_request_id) { '99999' } + + before do + sign_in user + end + + it 'lets the user know their account isn\'t linked yet' do + post :saml, params: { SAMLResponse: mock_saml_response } + + expect(flash[:notice]).to eq 'Request to link SAML account must be authorized' + end + + it 'redirects to profile account page' do + post :saml, params: { SAMLResponse: mock_saml_response } + + expect(response).to redirect_to(profile_account_path) + end + + it 'doesn\'t link a new identity to the user' do + expect { post :saml, params: { SAMLResponse: mock_saml_response } }.not_to change { user.identities.count } end end end diff --git a/spec/lib/gitlab/auth/saml/identity_linker_spec.rb b/spec/lib/gitlab/auth/saml/identity_linker_spec.rb index 89118ff05ba..7912c8fb4b1 100644 --- a/spec/lib/gitlab/auth/saml/identity_linker_spec.rb +++ b/spec/lib/gitlab/auth/saml/identity_linker_spec.rb @@ -6,45 +6,61 @@ describe Gitlab::Auth::Saml::IdentityLinker do let(:user) { create(:user) } let(:provider) { 'saml' } let(:uid) { user.email } - let(:oauth) { { 'provider' => provider, 'uid' => uid } } + let(:in_response_to) { '12345' } + let(:saml_response) { instance_double(OneLogin::RubySaml::Response, in_response_to: in_response_to) } + let(:session) { { 'last_authn_request_id' => in_response_to } } - subject { described_class.new(user, oauth) } + let(:oauth) do + OmniAuth::AuthHash.new(provider: provider, uid: uid, extra: { response_object: saml_response }) + end - context 'linked identity exists' do - let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid) } + subject { described_class.new(user, oauth, session) } - it "doesn't create new identity" do - expect { subject.link }.not_to change { Identity.count } - end + context 'with valid GitLab initiated request' do + context 'linked identity exists' do + let!(:identity) { user.identities.create!(provider: provider, extern_uid: uid) } - it "sets #changed? to false" do - subject.link + it "doesn't create new identity" do + expect { subject.link }.not_to change { Identity.count } + end - expect(subject).not_to be_changed - end - end + it "sets #changed? to false" do + subject.link - context 'identity needs to be created' do - it 'creates linked identity' do - expect { subject.link }.to change { user.identities.count } + expect(subject).not_to be_changed + end end - it 'sets identity provider' do - subject.link + context 'identity needs to be created' do + it 'creates linked identity' do + expect { subject.link }.to change { user.identities.count } + end - expect(user.identities.last.provider).to eq provider - end + it 'sets identity provider' do + subject.link - it 'sets identity extern_uid' do - subject.link + expect(user.identities.last.provider).to eq provider + end - expect(user.identities.last.extern_uid).to eq uid + it 'sets identity extern_uid' do + subject.link + + expect(user.identities.last.extern_uid).to eq uid + end + + it 'sets #changed? to true' do + subject.link + + expect(subject).to be_changed + end end + end - it 'sets #changed? to true' do - subject.link + context 'with identity provider initiated request' do + let(:session) { { 'last_authn_request_id' => nil } } - expect(subject).to be_changed + it 'attempting to link accounts raises an exception' do + expect { subject.link }.to raise_error(Gitlab::Auth::Saml::IdentityLinker::UnverifiedRequest) end end end diff --git a/spec/lib/gitlab/auth/saml/origin_validator_spec.rb b/spec/lib/gitlab/auth/saml/origin_validator_spec.rb new file mode 100644 index 00000000000..ae120b328ab --- /dev/null +++ b/spec/lib/gitlab/auth/saml/origin_validator_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Auth::Saml::OriginValidator do + let(:session) { instance_double(ActionDispatch::Request::Session) } + + subject { described_class.new(session) } + + describe '#store_origin' do + it 'stores the SAML request ID' do + request_id = double + authn_request = instance_double(OneLogin::RubySaml::Authrequest, uuid: request_id) + + expect(session).to receive(:[]=).with('last_authn_request_id', request_id) + + subject.store_origin(authn_request) + end + end + + describe '#gitlab_initiated?' do + it 'returns false if InResponseTo is not present' do + saml_response = instance_double(OneLogin::RubySaml::Response, in_response_to: nil) + + expect(subject.gitlab_initiated?(saml_response)).to eq(false) + end + + it 'returns false if InResponseTo does not match stored value' do + saml_response = instance_double(OneLogin::RubySaml::Response, in_response_to: "abc") + allow(session).to receive(:[]).with('last_authn_request_id').and_return('123') + + expect(subject.gitlab_initiated?(saml_response)).to eq(false) + end + + it 'returns true if InResponseTo matches stored value' do + saml_response = instance_double(OneLogin::RubySaml::Response, in_response_to: "123") + allow(session).to receive(:[]).with('last_authn_request_id').and_return('123') + + expect(subject.gitlab_initiated?(saml_response)).to eq(true) + end + end +end diff --git a/spec/lib/omni_auth/strategies/saml_spec.rb b/spec/lib/omni_auth/strategies/saml_spec.rb new file mode 100644 index 00000000000..3c59de86d98 --- /dev/null +++ b/spec/lib/omni_auth/strategies/saml_spec.rb @@ -0,0 +1,22 @@ +require 'spec_helper' + +describe OmniAuth::Strategies::SAML, type: :strategy do + let(:idp_sso_target_url) { 'https://login.example.com/idp' } + let(:strategy) { [OmniAuth::Strategies::SAML, { idp_sso_target_url: idp_sso_target_url }] } + + describe 'POST /users/auth/saml' do + it 'redirects to the provider login page' do + post '/users/auth/saml' + + expect(last_response).to redirect_to(/\A#{Regexp.quote(idp_sso_target_url)}/) + end + + it 'stores request ID during request phase' do + request_id = double + allow_any_instance_of(OneLogin::RubySaml::Authrequest).to receive(:uuid).and_return(request_id) + + post '/users/auth/saml' + expect(session['last_authn_request_id']).to eq(request_id) + end + end +end diff --git a/spec/support/omniauth_strategy.rb b/spec/support/omniauth_strategy.rb new file mode 100644 index 00000000000..eefa04bd9dd --- /dev/null +++ b/spec/support/omniauth_strategy.rb @@ -0,0 +1,39 @@ +module StrategyHelpers + include Rack::Test::Methods + include ActionDispatch::Assertions::ResponseAssertions + include Shoulda::Matchers::ActionController + include OmniAuth::Test::StrategyTestCase + + def post(*args) + super.tap do + @response = ActionDispatch::TestResponse.from_response(last_response) + end + end + + def auth_hash + last_request.env['omniauth.auth'] + end + + def self.without_test_mode + original_mode = OmniAuth.config.test_mode + original_on_failure = OmniAuth.config.on_failure + + OmniAuth.config.test_mode = false + OmniAuth.config.on_failure = OmniAuth::FailureEndpoint + + yield + ensure + OmniAuth.config.test_mode = original_mode + OmniAuth.config.on_failure = original_on_failure + end +end + +RSpec.configure do |config| + config.include StrategyHelpers, type: :strategy + + config.around(:all, type: :strategy) do |example| + StrategyHelpers.without_test_mode do + example.run + end + end +end |