Fix verification of SAML message signatures

README.md

@@ -1,5 +1,34 @@
 # Ruby SAML [![Build Status](https://secure.travis-ci.org/onelogin/ruby-saml.svg)](http://travis-ci.org/onelogin/ruby-saml) [![Coverage Status](https://coveralls.io/repos/onelogin/ruby-saml/badge.svg?branch=master%0A)](https://coveralls.io/r/onelogin/ruby-saml?branch=master%0A) [![Gem Version](https://badge.fury.io/rb/ruby-saml.svg)](http://badge.fury.io/rb/ruby-saml)
 
+## Updating from 1.5.0 to 1.6.0
+
+Version `1.6.0` changes the preferred way to construct instances of `Logoutresponse` and `SloLogoutrequest`. Previously the _SAMLResponse_, _RelayState_, and _SigAlg_ parameters of these message types were provided via the constructor's `options[:get_params]` parameter. Unfortunately this can result in incompatibility with other SAML implementations; signatures are specified to be computed based on the _sender's_ URI-encoding of the message, which can differ from that of Ruby SAML. In particular, Ruby SAML's URI-encoding does not match that of Microsoft ADFS, so messages from ADFS can fail signature validation.
+
+The new preferred way to provide _SAMLResponse_, _RelayState_, and _SigAlg_ is via the `options[:raw_get_params]` parameter. For example:
+
+```ruby
+# In this example `query_params` is assumed to contain decoded query parameters,
+# and `raw_query_params` is assumed to contain encoded query parameters as sent by the IDP.
+settings = {
+  settings.security[:signature_method] = XMLSecurity::Document::RSA_SHA1
+  settings.soft = false
+}
+options = {
+  get_params: {
+    "Signature" => query_params["Signature"],
+  },
+  raw_get_params: {
+    "SAMLRequest" => raw_query_params["SAMLRequest"],
+    "SigAlg" => raw_query_params["SigAlg"],
+    "RelayState" => raw_query_params["RelayState"],
+  },
+}
+slo_logout_request = OneLogin::RubySaml::SloLogoutrequest.new(query_params["SAMLRequest"], settings, options)
+raise "Uh oh!" unless slo_logout_request.is_valid?
+```
+
+The old form is still supported for backward compatibility, but all Ruby SAML users should prefer `options[:raw_get_params]` where possible to ensure compatibility with other SAML implementations.
+
 ## Updating from 1.4.2 to 1.4.3
 
 Version `1.4.3` introduces Recipient validation of SubjectConfirmation elements.

lib/onelogin/ruby-saml/logoutresponse.rb

@@ -211,18 +211,48 @@ def validate_signature
         return true unless options.has_key? :get_params
         return true unless options[:get_params].has_key? 'Signature'
 
+        # SAML specifies that the signature should be derived from a concatenation
+        # of URI-encoded values _as sent by the IDP_:
+        #
+        # > Further, note that URL-encoding is not canonical; that is, there are multiple legal encodings for a given
+        # > value. The relying party MUST therefore perform the verification step using the original URL-encoded
+        # > values it received on the query string. It is not sufficient to re-encode the parameters after they have been
+        # > processed by software because the resulting encoding may not match the signer's encoding.
+        #
+        # <http://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf>
+        #
+        # If we don't have the original parts (for backward compatibility) required to correctly verify the signature,
+        # then fabricate them by re-encoding the parsed URI parameters, and hope that we're lucky enough to use
+        # 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]['SAMLResponse'].nil? && !options[:get_params]['SAMLResponse'].nil?
+          options[:raw_get_params]['SAMLResponse'] = CGI.escape(options[:get_params]['SAMLResponse'])
+        end
+        if options[:raw_get_params]['RelayState'].nil? && !options[:get_params]['RelayState'].nil?
+          options[:raw_get_params]['RelayState'] = CGI.escape(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'])
+        end
+
+        # If we only received the raw version of SigAlg,
+        # then parse it back into the decoded params hash for convenience.
+        if options[:get_params]['SigAlg'].nil? && !options[:raw_get_params]['SigAlg'].nil?
+          options[:get_params]['SigAlg'] = CGI.unescape(options[:raw_get_params]['SigAlg'])
+        end
+
         idp_cert = settings.get_idp_cert
         idp_certs = settings.get_idp_cert_multi
 
         if idp_cert.nil? && (idp_certs.nil? || idp_certs[:signing].empty?)
           return options.has_key? :relax_signature_validation
         end
 
-        query_string = OneLogin::RubySaml::Utils.build_query(
-          :type        => 'SAMLResponse',
-          :data        => options[:get_params]['SAMLResponse'],
-          :relay_state => options[:get_params]['RelayState'],
-          :sig_alg     => options[:get_params]['SigAlg']
+        query_string = OneLogin::RubySaml::Utils.build_query_from_raw_parts(
+          :type            => 'SAMLResponse',
+          :raw_data        => options[:raw_get_params]['SAMLResponse'],
+          :raw_relay_state => options[:raw_get_params]['RelayState'],
+          :raw_sig_alg     => options[:raw_get_params]['SigAlg']
         )
 
         if idp_certs.nil? || idp_certs[:signing].empty?

lib/onelogin/ruby-saml/slo_logoutrequest.rb

@@ -229,18 +229,48 @@ def validate_signature
         return true unless options.has_key? :get_params
         return true unless options[:get_params].has_key? 'Signature'
 
+        # SAML specifies that the signature should be derived from a concatenation
+        # of URI-encoded values _as sent by the IDP_:
+        #
+        # > Further, note that URL-encoding is not canonical; that is, there are multiple legal encodings for a given
+        # > value. The relying party MUST therefore perform the verification step using the original URL-encoded
+        # > values it received on the query string. It is not sufficient to re-encode the parameters after they have been
+        # > processed by software because the resulting encoding may not match the signer's encoding.
+        #
+        # <http://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf>
+        #
+        # If we don't have the original parts (for backward compatibility) required to correctly verify the signature,
+        # then fabricate them by re-encoding the parsed URI parameters, and hope that we're lucky enough to use
+        # 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'])
+        end
+        if options[:raw_get_params]['RelayState'].nil? && !options[:get_params]['RelayState'].nil?
+          options[:raw_get_params]['RelayState'] = CGI.escape(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'])
+        end
+
+        # If we only received the raw version of SigAlg,
+        # then parse it back into the decoded params hash for convenience.
+        if options[:get_params]['SigAlg'].nil? && !options[:raw_get_params]['SigAlg'].nil?
+          options[:get_params]['SigAlg'] = CGI.unescape(options[:raw_get_params]['SigAlg'])
+        end
+
         idp_cert = settings.get_idp_cert
         idp_certs = settings.get_idp_cert_multi
 
         if idp_cert.nil? && (idp_certs.nil? || idp_certs[:signing].empty?)
           return options.has_key? :relax_signature_validation
         end
 
-        query_string = OneLogin::RubySaml::Utils.build_query(
-          :type        => 'SAMLRequest',
-          :data        => options[:get_params]['SAMLRequest'],
-          :relay_state => options[:get_params]['RelayState'],
-          :sig_alg     => options[:get_params]['SigAlg']
+        query_string = OneLogin::RubySaml::Utils.build_query_from_raw_parts(
+          :type            => 'SAMLRequest',
+          :raw_data        => options[:raw_get_params]['SAMLRequest'],
+          :raw_relay_state => options[:raw_get_params]['RelayState'],
+          :raw_sig_alg     => options[:raw_get_params]['SigAlg']
         )
 
         if idp_certs.nil? || idp_certs[:signing].empty?

lib/onelogin/ruby-saml/utils.rb

@@ -67,6 +67,24 @@ def self.build_query(params)
         url_string << "&SigAlg=#{CGI.escape(sig_alg)}"
       end
 
+      # Reconstruct a canonical query string from raw URI-encoded parts, to be used in verifying a signature
+      # sent by an IDP.
+      #
+      # @param params [Hash] Parameters to build the Query String
+      # @option params [String] :type 'SAMLRequest' or 'SAMLResponse'
+      # @option params [String] :raw_data URI-encoded, base64 encoded SAMLRequest or SAMLResponse, as sent by IDP
+      # @option params [String] :raw_relay_state URI-encoded RelayState parameter, as sent by IDP
+      # @option params [String] :raw_sig_alg URI-encoded SigAlg parameter, as sent by IDP
+      # @return [String] The Query String
+      #
+      def self.build_query_from_raw_parts(params)
+        type, raw_data, raw_relay_state, raw_sig_alg = [:type, :raw_data, :raw_relay_state, :raw_sig_alg].map { |k| params[k]}
+
+        url_string = "#{type}=#{raw_data}"
+        url_string << "&RelayState=#{raw_relay_state}" if raw_relay_state
+        url_string << "&SigAlg=#{raw_sig_alg}"
+      end
+
       # Validate the Signature parameter sent on the HTTP-Redirect binding
       # @param params [Hash] Parameters to be used in the validation process
       # @option params [OpenSSL::X509::Certificate] cert The Identity provider public certtificate

test/logoutresponse_test.rb

@@ -283,6 +283,80 @@ class RubySamlTest < Minitest::Test
           assert_raises(OneLogin::RubySaml::ValidationError) { logoutresponse.send(:validate_signature) }
           assert logoutresponse.errors.include? "Invalid Signature on Logout Response"
         end
+
+        it "raise when get_params encoding differs from what this library generates" do
+          settings.security[:signature_method] = XMLSecurity::Document::RSA_SHA1
+          settings.soft = false
+          options = {}
+          options[:get_params] = params
+          options[:get_params]['RelayState'] = 'http://example.com'
+          logoutresponse = OneLogin::RubySaml::Logoutresponse.new(params['SAMLResponse'], settings, options)
+          # Assemble query string.
+          query = OneLogin::RubySaml::Utils.build_query(
+            type: 'SAMLResponse',
+            data: params['SAMLResponse'],
+            relay_state: params['RelayState'],
+            sig_alg: params['SigAlg'],
+          )
+          # Modify the query string so that it encodes the same values,
+          # but with different percent-encoding. Sanity-check that they
+          # really are equialent before moving on.
+          original_query = query.dup
+          query.gsub!("example", "ex%61mple")
+          refute_equal(query, original_query)
+          assert_equal(CGI.unescape(query), CGI.unescape(original_query))
+          # 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'] = Base64.encode64(signature).gsub(/\n/, "")
+          # Re-create the Logoutresponse based on these modified parameters,
+          # and ask it to validate the signature. It will do it incorrectly,
+          # because it will compute it based on re-encoded query parameters,
+          # rather than their original encodings.
+          options[:get_params] = params
+          logoutresponse = OneLogin::RubySaml::Logoutresponse.new(params['SAMLResponse'], settings, options)
+          assert_raises(OneLogin::RubySaml::ValidationError, "Invalid Signature on Logout Request") do
+            logoutresponse.send(:validate_signature)
+          end
+        end
+
+        it "return true even if raw_get_params encoding differs from what this library generates" do
+          settings.security[:signature_method] = XMLSecurity::Document::RSA_SHA1
+          settings.soft = false
+          options = {}
+          options[:get_params] = params
+          options[:get_params]['RelayState'] = 'http://example.com'
+          logoutresponse = OneLogin::RubySaml::Logoutresponse.new(params['SAMLResponse'], settings, options)
+          # Assemble query string.
+          query = OneLogin::RubySaml::Utils.build_query(
+            type: 'SAMLResponse',
+            data: params['SAMLResponse'],
+            relay_state: params['RelayState'],
+            sig_alg: params['SigAlg'],
+          )
+          # Modify the query string so that it encodes the same values,
+          # but with different percent-encoding. Sanity-check that they
+          # really are equialent before moving on.
+          original_query = query.dup
+          query.gsub!("example", "ex%61mple")
+          refute_equal(query, original_query)
+          assert_equal(CGI.unescape(query), CGI.unescape(original_query))
+          # 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'] = Base64.encode64(signature).gsub(/\n/, "")
+          # Re-create the Logoutresponse based on these modified parameters,
+          # and ask it to validate the signature. Provide the altered parameter
+          # in its raw URI-encoded form, so that we don't have to guess the value
+          # that contributed to the signature.
+          options[:get_params] = params
+          options[:get_params].delete("RelayState")
+          options[:raw_get_params] = {
+            "RelayState" => "http%3A%2F%2Fex%61mple.com",
+          }
+          logoutresponse = OneLogin::RubySaml::Logoutresponse.new(params['SAMLResponse'], settings, options)
+          assert logoutresponse.send(:validate_signature)
+        end
       end
 
       describe "#validate_signature" do

test/slo_logoutrequest_test.rb

@@ -322,6 +322,78 @@ class RubySamlTest < Minitest::Test
           logout_request_sign_test.send(:validate_signature)
         end
       end
+
+      it "raise when get_params encoding differs from what this library generates" do
+        # Use Logoutrequest only to build the SAMLRequest parameter.
+        settings.security[:signature_method] = XMLSecurity::Document::RSA_SHA1
+        settings.soft = false
+        params = OneLogin::RubySaml::Logoutrequest.new.create_params(settings, "RelayState" => "http://example.com")
+        # Assemble query string.
+        query = OneLogin::RubySaml::Utils.build_query(
+          type: 'SAMLRequest',
+          data: params['SAMLRequest'],
+          relay_state: params['RelayState'],
+          sig_alg: params['SigAlg'],
+        )
+        # Modify the query string so that it encodes the same values,
+        # but with different percent-encoding. Sanity-check that they
+        # really are equialent before moving on.
+        original_query = query.dup
+        query.gsub!("example", "ex%61mple")
+        refute_equal(query, original_query)
+        assert_equal(CGI.unescape(query), CGI.unescape(original_query))
+        # 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'] = Base64.encode64(signature).gsub(/\n/, "")
+        # Construct SloLogoutrequest and ask it to validate the signature.
+        # It will do it incorrectly, because it will compute it based on re-encoded
+        # query parameters, rather than their original encodings.
+        options = {}
+        options[:get_params] = params
+        options[:settings] = settings
+        logout_request_sign_test = OneLogin::RubySaml::SloLogoutrequest.new(params['SAMLRequest'], options)
+        assert_raises(OneLogin::RubySaml::ValidationError, "Invalid Signature on Logout Request") do
+          logout_request_sign_test.send(:validate_signature)
+        end
+      end
+
+      it "return true even if raw_get_params encoding differs from what this library generates" do
+        # Use Logoutrequest only to build the SAMLRequest parameter.
+        settings.security[:signature_method] = XMLSecurity::Document::RSA_SHA1
+        settings.soft = false
+        params = OneLogin::RubySaml::Logoutrequest.new.create_params(settings, "RelayState" => "http://example.com")
+        # Assemble query string.
+        query = OneLogin::RubySaml::Utils.build_query(
+          type: 'SAMLRequest',
+          data: params['SAMLRequest'],
+          relay_state: params['RelayState'],
+          sig_alg: params['SigAlg'],
+        )
+        # Modify the query string so that it encodes the same values,
+        # but with different percent-encoding. Sanity-check that they
+        # really are equialent before moving on.
+        original_query = query.dup
+        query.gsub!("example", "ex%61mple")
+        refute_equal(query, original_query)
+        assert_equal(CGI.unescape(query), CGI.unescape(original_query))
+        # 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'] = Base64.encode64(signature).gsub(/\n/, "")
+        # Construct SloLogoutrequest and ask it to validate the signature.
+        # Provide the altered parameter in its raw URI-encoded form,
+        # so that we don't have to guess the value that contributed to the signature.
+        options = {}
+        options[:get_params] = params
+        options[:get_params].delete("RelayState")
+        options[:raw_get_params] = {
+          "RelayState" => "http%3A%2F%2Fex%61mple.com",
+        }
+        options[:settings] = settings
+        logout_request_sign_test = OneLogin::RubySaml::SloLogoutrequest.new(params['SAMLRequest'], options)
+        assert logout_request_sign_test.send(:validate_signature)
+      end
     end
 
     describe "#validate_signature with multiple idp certs" do