Validate RelayState for SLO redirects

lib/omniauth/strategies/saml.rb

@@ -1,5 +1,6 @@
 require 'omniauth'
 require 'ruby-saml'
+require 'uri'
 
 module OmniAuth
   module Strategies
@@ -142,20 +143,12 @@ def handle_response(raw_response, opts, settings)
       end
 
       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
-        end
+        return resolve_slo_default_relay_state unless request.params.has_key?("RelayState") && request.params["RelayState"] != ""
+
+        validated_relay_state = validate_relay_state(request.params["RelayState"])
+        return validated_relay_state if validated_relay_state
+
+        resolve_slo_default_relay_state
       end
 
       def handle_logout_response(raw_response, settings)
@@ -279,6 +272,39 @@ def additional_params_for_authn_request
           end
         end
       end
+
+      def resolve_slo_default_relay_state
+        slo_default_relay_state = options.slo_default_relay_state
+        return slo_default_relay_state unless 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
+      end
+
+      def validate_relay_state(relay_state)
+        return nil if relay_state.nil?
+
+        sanitized_relay_state = relay_state.to_s.strip
+        return nil if sanitized_relay_state.empty?
+        return nil if sanitized_relay_state.start_with?('//')
+
+        uri = URI.parse(sanitized_relay_state)
+
+        return nil if uri.scheme || uri.host
+
+        path = uri.path
+        return nil unless path && path.start_with?('/')
+
+        sanitized = String.new(path)
+        sanitized << "?#{uri.query}" if uri.query
+        sanitized << "##{uri.fragment}" if uri.fragment
+        sanitized
+      rescue URI::Error
+        nil
+      end
     end
   end
 end

spec/omniauth/strategies/saml_spec.rb

@@ -280,13 +280,30 @@ def post_xml(xml = :example_response, opts = {})
       before :each do
         post "/auth/saml/slo", {
           SAMLResponse: load_xml(:example_logout_response),
-          RelayState: "https://example.com/",
+          RelayState: "/signed-out",
         }, "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\//
+        expect(last_response.location).to eq("/signed-out")
+      end
+    end
+
+    context "when response relay state is unsafe" do
+      before :each do
+        saml_options[:sp_entity_id] = "https://idp.sso.example.com/metadata/29490"
+        saml_options["slo_default_relay_state"] = "/signed-out"
+
+        post "/auth/saml/slo", {
+          SAMLResponse: load_xml(:example_logout_response),
+          RelayState: "https://attacker.test/",
+        }, "rack.session" => {"saml_transaction_id" => "_3fef1069-d0c6-418a-b68d-6f008a4787e9"}
+      end
+
+      it "should redirect to the default relay state" do
+        expect(last_response).to be_redirect
+        expect(last_response.location).to eq("/signed-out")
       end
     end
 
@@ -296,7 +313,7 @@ def post_xml(xml = :example_response, opts = {})
       let(:params) do
         {
           "SAMLRequest" => load_xml(:example_logout_request),
-          "RelayState" => "https://example.com/",
+          "RelayState" => "/signed-out",
         }
       end
 
@@ -306,7 +323,26 @@ def post_xml(xml = :example_response, opts = {})
         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=%2Fsigned-out/
+        end
+      end
+
+      context "when relay state is unsafe" do
+        let(:params) do
+          {
+            "SAMLRequest" => load_xml(:example_logout_request),
+            "RelayState" => "https://attacker.test",
+          }
+        end
+
+        before do
+          saml_options["slo_default_relay_state"] = "/signed-out"
+          subject
+        end
+
+        it "uses the default relay state" do
+          expect(last_response).to be_redirect
+          expect(last_response.location).to match /RelayState=%2Fsigned-out/
         end
       end
 
@@ -340,30 +376,51 @@ def test_default_relay_state(static_default_relay_state = nil, &block_default_re
       saml_options["slo_default_relay_state"] = static_default_relay_state || block_default_relay_state
       post "/auth/saml/spslo"
 
-      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/
-    end
+        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=%2Fsigned-out/
+      end
 
-    it "should redirect to logout request" do
-      test_default_relay_state("https://example.com/")
-    end
+      it "should redirect to logout request" do
+        test_default_relay_state("/signed-out")
+      end
 
-    it "should redirect to logout request with a block" do
-      test_default_relay_state do
-        "https://example.com/"
+      it "should redirect to logout request with a block" do
+        test_default_relay_state { "/signed-out" }
       end
-    end
 
-    it "should redirect to logout request with a block with a request parameter" do
-      test_default_relay_state do |request|
-        "https://example.com/"
+      it "should redirect to logout request with a block with a request parameter" do
+        test_default_relay_state { |request| "/signed-out" } 
       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"
+      it "falls back to default when relay state is unsafe" do
+        saml_options["slo_default_relay_state"] = "/signed-out"
+        post "/auth/saml/spslo", { "RelayState" => "https://attacker.test" }
+
+        expect(last_response).to be_redirect
+        expect(last_response.location).to match /RelayState=%2Fsigned-out/
+      end
+
+      it "rejects protocol relative relay state" do
+        saml_options["slo_default_relay_state"] = "/signed-out"
+        post "/auth/saml/spslo", { "RelayState" => "//attacker.test" }
+
+        expect(last_response).to be_redirect
+        expect(last_response.location).to match /RelayState=%2Fsigned-out/
+      end
+
+      it "rejects javascript relay state" do
+        saml_options["slo_default_relay_state"] = "/signed-out"
+        post "/auth/saml/spslo", { "RelayState" => "javascript:alert(1)" }
+
+        expect(last_response).to be_redirect
+        expect(last_response.location).to match /RelayState=%2Fsigned-out/
+      end
+
+      it "should give not implemented without an idp_slo_service_url" do
+        saml_options.delete(:idp_slo_service_url)
+        post "/auth/saml/spslo"
 
       expect(last_response.status).to eq 501
       expect(last_response.body).to match /Not Implemented/