diff --git a/README.md b/README.md index b9ad4733e..4bf65bc0f 100644 --- a/README.md +++ b/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. diff --git a/lib/onelogin/ruby-saml/logoutresponse.rb b/lib/onelogin/ruby-saml/logoutresponse.rb index 4fbab8232..1f5c5be02 100644 --- a/lib/onelogin/ruby-saml/logoutresponse.rb +++ b/lib/onelogin/ruby-saml/logoutresponse.rb @@ -211,6 +211,36 @@ 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. + # + # + # + # 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 @@ -218,11 +248,11 @@ def validate_signature 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? diff --git a/lib/onelogin/ruby-saml/slo_logoutrequest.rb b/lib/onelogin/ruby-saml/slo_logoutrequest.rb index c73b32bfe..312fb9cb2 100644 --- a/lib/onelogin/ruby-saml/slo_logoutrequest.rb +++ b/lib/onelogin/ruby-saml/slo_logoutrequest.rb @@ -229,6 +229,36 @@ 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. + # + # + # + # 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 @@ -236,11 +266,11 @@ def validate_signature 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? diff --git a/lib/onelogin/ruby-saml/utils.rb b/lib/onelogin/ruby-saml/utils.rb index fe77d94d4..5184b87f5 100644 --- a/lib/onelogin/ruby-saml/utils.rb +++ b/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 diff --git a/test/logoutresponse_test.rb b/test/logoutresponse_test.rb index d47b53da6..334754f8d 100644 --- a/test/logoutresponse_test.rb +++ b/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 diff --git a/test/slo_logoutrequest_test.rb b/test/slo_logoutrequest_test.rb index 8f09b6bff..9ac3ca8f0 100644 --- a/test/slo_logoutrequest_test.rb +++ b/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