Merge pull request #245 from gerardo-navarro/gerardo-navarro-implement-slo-relay-state-validator

Refine SLO RelayState validation

Author: fh1ch

README.md

@@ -101,10 +101,19 @@ Note that when [integrating with Devise](#devise-integration), the URL path will
 * `:slo_default_relay_state` - The value to use as default `RelayState` for single log outs. The
   value can be a string, or a `Proc` (or other object responding to `call`). The `request`
   instance will be passed to this callable if it has an arity of 1. If the value is a string,
-  the string will be returned, when the `RelayState` is called. Optional.
+  the string will be returned, when the `RelayState` is called.
+  The value is assumed to be safe and is not validated by `:slo_relay_state_validator`.
+  Optional.
 
 * `:slo_enabled` - Enables or disables Single Logout (SLO). Set to `false` to disable SLO. Defaults to `true`. Optional.
 
+* `:slo_relay_state_validator` - A callable used to validate any `RelayState` before performing the redirect
+  in Single Logout flows. The callable receives the RelayState value and the current Rack request.
+  If unset, the default validator is used. The default validator allows only relative paths beginning
+  with `/` and rejects absolute URLs, invalid URIs, protocol-relative URLs, and other schemes.
+  If the given `RelayState` is considered invalid then the `slo_default_relay_state` value is used for the SLO redirect.
+  Optional.
+
 * `:idp_sso_service_url_runtime_params` - A dynamic mapping of request params that exist
   during the request phase of OmniAuth that should to be sent to the IdP after a specific
   mapping. So for example, a param `original_request_param` with value `original_param_value`,

lib/omniauth/strategies/saml.rb

@@ -1,5 +1,6 @@
 require 'omniauth'
 require 'ruby-saml'
+require 'uri'
 
 module OmniAuth
   module Strategies
@@ -27,8 +28,26 @@ def self.inherited(subclass)
         first_name: ["first_name", "firstname", "firstName"],
         last_name: ["last_name", "lastname", "lastName"]
       }
+      DEFAULT_SLO_RELAY_STATE_VALIDATOR = lambda do |relay_state, _request|
+        return true if relay_state.nil? || relay_state == ""
+
+        return false if relay_state.start_with?("//")
+
+        begin
+          uri = URI.parse(relay_state)
+        rescue URI::Error
+          return false
+        end
+
+        return false unless uri.relative?
+
+        path = uri.path
+        path && path.start_with?("/")
+      end
+
       option :slo_default_relay_state
       option :slo_enabled, true
+      option :slo_relay_state_validator, DEFAULT_SLO_RELAY_STATE_VALIDATOR
       option :uid_attribute
       option :idp_slo_session_destroy, proc { |_env, session| session.clear }
 
@@ -148,19 +167,34 @@ def handle_response(raw_response, opts, settings)
 
       def slo_relay_state
         if request.params.has_key?("RelayState") && request.params["RelayState"] != ""
-          request.params["RelayState"]
-        else
-          slo_default_relay_state = options.slo_default_relay_state
-          if slo_default_relay_state.respond_to?(:call)
-            if slo_default_relay_state.arity == 1
-              slo_default_relay_state.call(request)
-            else
-              slo_default_relay_state.call
-            end
-          else
-            slo_default_relay_state
-          end
+          relay_state = request.params["RelayState"]
+
+          return relay_state if valid_slo_relay_state?(relay_state)
         end
+
+        default_slo_relay_state
+      end
+
+      def valid_slo_relay_state?(relay_state)
+        validator = options.slo_relay_state_validator
+
+        return !!call_slo_relay_state_validator(validator, relay_state) if validator.respond_to?(:call)
+
+        !!validator
+      end
+
+      def call_slo_relay_state_validator(validator, relay_state)
+        return validator.call if validator.arity.zero?
+        return validator.call(relay_state) if validator.arity == 1
+        validator.call(relay_state, request)
+      end
+
+      def default_slo_relay_state
+        slo_default_relay_state = options.slo_default_relay_state
+
+        return slo_default_relay_state unless slo_default_relay_state.respond_to?(:call)
+        return slo_default_relay_state.call if slo_default_relay_state.arity.zero?
+        slo_default_relay_state.call(request)
       end
 
       def handle_logout_response(raw_response, settings)

spec/omniauth/strategies/saml_spec.rb

@@ -30,6 +30,55 @@
   end
   let(:strategy) { [OmniAuth::Strategies::SAML, saml_options] }
 
+  shared_examples 'validating RelayState param' do
+    context 'when slo_relay_state_validator is not defined and default' do
+      [
+        ['/signed-out', '//attacker.test',                            '%2Fsigned-out'],
+        ['/signed-out', 'javascript:alert(1)',                        '%2Fsigned-out'],
+        ['/signed-out', 'https://example.com/logout',                 '%2Fsigned-out'],
+        ['/signed-out', 'https://example.com/logout?param=1&two=two', '%2Fsigned-out'],
+        ['/signed-out', '/',                                          '%2F'],
+        ['',            '//attacker.test',                            ''],
+        ['',            '/team/logout',                               '%2Fteam%2Flogout'],
+      ].each do |slo_default_relay_state, relay_state_param, expected_relay_state|
+        context "when slo_default_relay_state: #{slo_default_relay_state.inspect}, relay_state_param: #{relay_state_param.inspect}" do
+          let(:saml_options) { super().merge(slo_default_relay_state: slo_default_relay_state) }
+          let(:params) { super().merge('RelayState' => relay_state_param) }
+
+          it { is_expected.to be_redirect.and have_attributes(location: a_string_including("RelayState=#{expected_relay_state}")) }
+        end
+      end
+    end
+
+    context 'when slo_relay_state_validator is overridden' do
+      [
+        ['/signed-out', proc { |state| state.start_with?('https://trusted.example.com') }, 'https://trusted.example.com/logout', 'https%3A%2F%2Ftrusted.example.com%2Flogout'],
+        ['/signed-out', proc { |state| state.start_with?('https://trusted.example.com') }, 'https://attacker.test/logout',       '%2Fsigned-out'],
+        ['/signed-out', proc { |state| state.start_with?('https://trusted.example.com') }, '/safe/path',                         '%2Fsigned-out'],
+        ['/signed-out', proc { |state, req| state == req.params['RelayState'] },           '/team/logout',                       '%2Fteam%2Flogout'],
+        ['/signed-out', nil,                                                               '//attacker.test',                    '%2Fsigned-out'],
+        ['/signed-out', false,                                                             '//attacker.test',                    '%2Fsigned-out'],
+        ['/signed-out', proc { |_| false },                                                '//attacker.test',                    '%2Fsigned-out'],
+        ['/signed-out', proc { |_| true },                                                 'javascript:alert(1)',                'javascript%3Aalert%281%29'],
+        [nil,           true,                                                              'https://example.com/logout',         'https%3A%2F%2Fexample.com%2Flogout'],
+        [nil,           true,                                                              'javascript:alert(1)',                'javascript%3Aalert%281%29'],
+        [nil,           true,                                                              '/',                                  '%2F'],
+      ].each do |slo_default_relay_state, slo_relay_state_validator, relay_state_param, expected_relay_state|
+        context "when slo_default_relay_state: #{slo_default_relay_state.inspect}, slo_relay_state_validator: #{slo_relay_state_validator.inspect}, relay_state_param: #{relay_state_param.inspect}" do
+          let(:saml_options) do
+            super().merge(
+              slo_default_relay_state: slo_default_relay_state,
+              slo_relay_state_validator: slo_relay_state_validator,
+            )
+          end
+          let(:params) { super().merge('RelayState' => relay_state_param) }
+
+          it { is_expected.to be_redirect.and have_attributes(location: a_string_including("RelayState=#{expected_relay_state}")) }
+        end
+      end
+    end
+  end
+
   describe 'POST /auth/saml' do
     context 'without idp runtime params present' do
       before do
@@ -274,37 +323,128 @@
     end
 
     context "when response is a logout response" do
-      before :each do
-        post "/auth/saml/slo", {
-          SAMLResponse: load_xml(:example_logout_response),
-          RelayState: "https://example.com/",
-        }, "rack.session" => {"saml_transaction_id" => "_3fef1069-d0c6-418a-b68d-6f008a4787e9"}
+      let(:opts) do
+        { "rack.session" => { "saml_transaction_id" => "_3fef1069-d0c6-418a-b68d-6f008a4787e9" } }
       end
 
-      it "should redirect to relaystate" do
-        expect(last_response).to be_redirect
-        expect(last_response.location).to match /https:\/\/example.com\//
+      let(:params) { { SAMLResponse: load_xml(:example_logout_response) } }
+
+      subject(:post_slo_response) { post "/auth/saml/slo", params, opts }
+
+      context "when relay state is relative" do
+        let(:params) { super().merge(RelayState: "/signed-out") }
+
+        it "redirects to the relaystate" do
+          post_slo_response
+
+          expect(last_response).to be_redirect
+          expect(last_response.location).to eq "/signed-out"
+        end
+      end
+
+      context "when relay state is an absolute https URL" do
+        let(:params) { super().merge(RelayState: "https://example.com/") }
+
+        it "redirects without a location header" do
+          post_slo_response
+
+          expect(last_response).to be_redirect
+          expect(last_response.headers.fetch("Location")).to be_nil
+        end
+      end
+
+      context 'when slo_default_relay_state is present' do
+        let(:saml_options) { super().merge(slo_default_relay_state: '/signed-out') }
+
+        context "when response relay state is valid" do
+          let(:params) { super().merge(RelayState: "/safe/logout") }
+
+          it { is_expected.to be_redirect.and have_attributes(location: '/safe/logout') }
+        end
+
+        context "when response relay state is invalid" do
+          let(:params) { super().merge(RelayState: "javascript:alert(1)") }
+
+          it { is_expected.to be_redirect.and have_attributes(location: '/signed-out') }
+        end
+      end
+
+      context 'when slo_default_relay_state is blank' do
+        let(:saml_options) { super().merge(slo_default_relay_state: nil) }
+
+        context "when response relay state is valid" do
+          let(:params) { super().merge(RelayState: "/safe/logout") }
+
+          it { is_expected.to be_redirect.and have_attributes(location: '/safe/logout') }
+        end
+
+        context "when response relay state is invalid" do
+          let(:params) { super().merge(RelayState: "javascript:alert(1)") }
+
+          it { is_expected.to be_redirect.and have_attributes(location: nil) }
+        end
       end
     end
 
     context "when request is a logout request" do
       subject { post "/auth/saml/slo", params, "rack.session" => { "saml_uid" => "[email protected]" } }
 
+      let(:relay_state) { "https://example.com/" }
+
       let(:params) do
         {
           "SAMLRequest" => load_xml(:example_logout_request),
-          "RelayState" => "https://example.com/",
+          "RelayState" => relay_state,
         }
       end
 
       context "when logout request is valid" do
+        let(:relay_state) { "/logout" }
+
         before { subject }
 
         it "should redirect to logout response" do
           expect(last_response).to be_redirect
           expect(last_response.location).to match /https:\/\/idp.sso.example.com\/signoff\/29490/
-          expect(last_response.location).to match /RelayState=https%3A%2F%2Fexample.com%2F/
+          expect(last_response.location).to match /RelayState=%2Flogout/
+        end
+      end
+
+      it_behaves_like 'validating RelayState param'
+
+      context 'when slo_default_relay_state is blank' do
+        let(:saml_options) { super().merge(slo_default_relay_state: nil) }
+
+        context "when request relay state is invalid" do
+          let(:params) do
+            {
+              "SAMLRequest" => load_xml(:example_logout_request),
+              "RelayState" => "javascript:alert(1)",
+            }
+          end
+
+          it "redirects without including a RelayState parameter" do
+            subject
+
+            expect(last_response).to be_redirect
+            expect(last_response.location).to match %r{https://idp\.sso\.example\.com/signoff/29490}
+            expect(last_response.location).not_to match(/RelayState=/)
+          end
+        end
+      end
+
+      context "with a custom relay state validator" do
+        let(:saml_options) do
+          super().merge(
+            slo_relay_state_validator: proc do |relay_state, rack_request|
+              expect(rack_request).to respond_to(:params)
+              relay_state == "custom-state"
+            end,
+          )
         end
+        let(:params) { super().merge("RelayState" => "custom-state") }
+
+        it { is_expected.to be_redirect.and have_attributes(location: a_string_matching(/RelayState=custom-state/)) }
       end
 
       context "when request is an invalid logout request" do
@@ -345,6 +485,9 @@
   end
 
   describe 'POST /auth/saml/spslo' do
+    let(:params) { {} }
+    subject { post "/auth/saml/spslo", params }
+
     def test_default_relay_state(static_default_relay_state = nil, &block_default_relay_state)
       saml_options["slo_default_relay_state"] = static_default_relay_state || block_default_relay_state
       post "/auth/saml/spslo"
@@ -370,6 +513,21 @@ def test_default_relay_state(static_default_relay_state = nil, &block_default_re
       end
     end
 
+    it_behaves_like 'validating RelayState param'
+
+    context 'when slo_default_relay_state is blank' do
+      let(:saml_options) { super().merge(slo_default_relay_state: nil) }
+      let(:params) { { RelayState: "//example.com" } }
+
+      it "redirects without including a RelayState parameter" do
+        subject
+
+        expect(last_response).to be_redirect
+        expect(last_response.location).to match %r{https://idp\.sso\.example\.com/signoff/29490}
+        expect(last_response.location).not_to match(/RelayState=/)
+      end
+    end
+
     it "should give not implemented without an idp_slo_service_url" do
       saml_options.delete(:idp_slo_service_url)
       post "/auth/saml/spslo"