Merge branch 'security-sarcila-verify-saml-request-origin-12-3' into '12-3-stable'

Check that SAML identity linking validates the origin of the request

See merge request gitlab/gitlabhq!3396

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
@@ -84,8 +86,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)
 
@@ -178,6 +179,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 }

changelogs/unreleased/security-sarcila-verify-saml-request-origin.yml

+---
+title: Prevent GitLab accounts takeover if SAML is configured
+merge_request:
+author:
+type: security

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

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

lib/gitlab/auth/saml/origin_validator.rb

+# 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

lib/omni_auth/strategies/saml.rb

+# 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

locale/gitlab.pot

@@ -13029,6 +13029,9 @@ msgstr ""
 msgid "Request Access"
 msgstr ""
 
+msgid "Request to link SAML account must be authorized"
+msgstr ""
+
 msgid "Requested %{time_ago}"
 msgstr ""
 

spec/controllers/omniauth_callbacks_controller_spec.rb

@@ -219,34 +219,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

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

spec/lib/gitlab/auth/saml/origin_validator_spec.rb

+# 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

spec/lib/omni_auth/strategies/saml_spec.rb

+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

spec/support/omniauth_strategy.rb

+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