Merge pull request #627 from visfleet/force-escape-downcasing

Force escape downcasing for Azure SLO

Author: pitbulk

README.md

@@ -66,7 +66,7 @@ However, ruby-saml never enables this dangerous Nokogiri configuration;
 ruby-saml never enables DTDLOAD, and it never disables NONET.
 
 The OneLogin::RubySaml::IdpMetadataParser class does not validate in any way the URL
-that is introduced in order to be parsed. 
+that is introduced in order to be parsed.
 
 Usually the same administrator that handles the Service Provider also sets the URL to
 the IdP, which should be a trusted resource.
@@ -790,7 +790,13 @@ Here is an example that we could add to our previous controller to process a SAM
 # Method to handle IdP initiated logouts
 def idp_logout_request
   settings = Account.get_saml_settings
-  logout_request = OneLogin::RubySaml::SloLogoutrequest.new(params[:SAMLRequest])
+  # ADFS URL-Encodes SAML data as lowercase, and the toolkit by default uses
+  # uppercase. Turn it True for ADFS compatibility on signature verification
+  settings.security[:lowercase_url_encoding] = true
+
+  logout_request = OneLogin::RubySaml::SloLogoutrequest.new(
+    params[:SAMLRequest], settings: settings
+  )
   if !logout_request.is_valid?
     logger.error "IdP initiated LogoutRequest was not valid!"
     return render :inline => logger.error

lib/onelogin/ruby-saml/settings.rb

@@ -282,6 +282,7 @@ def get_binding(value)
           :check_idp_cert_expiration  => false,
           :check_sp_cert_expiration   => false,
           :strict_audience_validation => false,
+          :lowercase_url_encoding     => false
         }.freeze
       }.freeze
     end

lib/onelogin/ruby-saml/slo_logoutrequest.rb

@@ -263,13 +263,13 @@ def validate_signature
         # the exact same URI-encoding as the IDP. (This is not the case if the IDP is ADFS!)
         options[:raw_get_params] ||= {}
         if options[:raw_get_params]['SAMLRequest'].nil? && !options[:get_params]['SAMLRequest'].nil?
-          options[:raw_get_params]['SAMLRequest'] = CGI.escape(options[:get_params]['SAMLRequest'])
+          options[:raw_get_params]['SAMLRequest'] = escape_request_param(options[:get_params]['SAMLRequest'])
         end
         if options[:raw_get_params]['RelayState'].nil? && !options[:get_params]['RelayState'].nil?
-          options[:raw_get_params]['RelayState'] = CGI.escape(options[:get_params]['RelayState'])
+          options[:raw_get_params]['RelayState'] = escape_request_param(options[:get_params]['RelayState'])
         end
         if options[:raw_get_params]['SigAlg'].nil? && !options[:get_params]['SigAlg'].nil?
-          options[:raw_get_params]['SigAlg'] = CGI.escape(options[:get_params]['SigAlg'])
+          options[:raw_get_params]['SigAlg'] = escape_request_param(options[:get_params]['SigAlg'])
         end
 
         # If we only received the raw version of SigAlg,
@@ -336,6 +336,13 @@ def validate_signature
         true
       end
 
+      def escape_request_param(param)
+        CGI.escape(param).tap do |escaped|
+          next unless settings.security[:lowercase_url_encoding]
+
+          escaped.gsub!(/%[A-Fa-f0-9]{2}/) { |match| match.downcase }
+        end
+      end
     end
   end
 end

test/slo_logoutrequest_test.rb

@@ -425,6 +425,52 @@ class RubySamlTest < Minitest::Test
         logout_request_sign_test = OneLogin::RubySaml::SloLogoutrequest.new(params['SAMLRequest'], options)
         assert logout_request_sign_test.send(:validate_signature)
       end
+
+      it "handles Azure AD downcased request encoding" do
+        # Use Logoutrequest only to build the SAMLRequest parameter.
+        settings.security[:signature_method] = XMLSecurity::Document::RSA_SHA256
+        settings.soft = false
+
+        # Creating the query manually to tweak it later instead of using
+        # OneLogin::RubySaml::Utils.build_query
+        request_doc = OneLogin::RubySaml::Logoutrequest.new.create_logout_request_xml_doc(settings)
+        request = Zlib::Deflate.deflate(request_doc.to_s, 9)[2..-5]
+        base64_request = Base64.encode64(request).gsub(/\n/, "")
+        # The original request received from Azure AD comes with downcased
+        # encoded characters, like %2f instead of %2F, and the signature they
+        # send is based on this base64 request.
+        params = {
+          'SAMLRequest' => downcased_escape(base64_request),
+          'SigAlg' => downcased_escape(settings.security[:signature_method]),
+        }
+        # Assemble query string.
+        query = "SAMLRequest=#{params['SAMLRequest']}&SigAlg=#{params['SigAlg']}"
+        # Make normalised signature based on our modified params.
+        sign_algorithm = XMLSecurity::BaseDocument.new.algorithm(
+          settings.security[:signature_method]
+        )
+        signature = settings.get_sp_key.sign(sign_algorithm.new, query)
+        params['Signature'] = downcased_escape(Base64.encode64(signature).gsub(/\n/, ""))
+
+        # Then parameters are usually unescaped, like we manage them in rails
+        params = params.map { |k, v| [k, CGI.unescape(v)] }.to_h
+        # Construct SloLogoutrequest and ask it to validate the signature.
+        # It will fail because the signature is based on the downcased request
+        logout_request_downcased_test = OneLogin::RubySaml::SloLogoutrequest.new(
+          params['SAMLRequest'], get_params: params, settings: settings,
+        )
+        assert_raises(OneLogin::RubySaml::ValidationError, "Invalid Signature on Logout Request") do
+          logout_request_downcased_test.send(:validate_signature)
+        end
+
+        # For this case, the parameters will be forced to be downcased after
+        # being escaped with :lowercase_url_encoding security option
+        settings.security[:lowercase_url_encoding] = true
+        logout_request_force_downcasing_test = OneLogin::RubySaml::SloLogoutrequest.new(
+          params['SAMLRequest'], get_params: params, settings: settings
+        )
+        assert logout_request_force_downcasing_test.send(:validate_signature)
+      end
     end
 
     describe "#validate_signature with multiple idp certs" do

test/test_helper.rb

@@ -346,4 +346,9 @@ def validate_xml!(document, schema)
       end
     end
   end
+
+  # Allows to emulate Azure AD request behavior
+  def downcased_escape(str)
+    CGI.escape(str).gsub(/%[A-Fa-f0-9]{2}/) { |match| match.downcase }
+  end
 end