Skip to content

Commit 6313475

Browse files
pitbulkpitbulk
committed
Improve SAML message signatures using original encoded parameters instead decoded in order to avoid conflicts (URL-encoding is not canonical, reported issues with ADFS)
2 parents 843ade7 + c6256ef commit 6313475

File tree

6 files changed

+264
-10
lines changed

6 files changed

+264
-10
lines changed

‎README.md‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,34 @@
11
# 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)
22

3+
## Updating from 1.5.0 to 1.6.0
4+
5+
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.
6+
7+
The new preferred way to provide _SAMLResponse_, _RelayState_, and _SigAlg_ is via the `options[:raw_get_params]` parameter. For example:
8+
9+
```ruby
10+
# In this example `query_params` is assumed to contain decoded query parameters,
11+
# and `raw_query_params` is assumed to contain encoded query parameters as sent by the IDP.
12+
settings = {
13+
settings.security[:signature_method] = XMLSecurity::Document::RSA_SHA1
14+
settings.soft = false
15+
}
16+
options = {
17+
get_params: {
18+
"Signature" => query_params["Signature"],
19+
},
20+
raw_get_params: {
21+
"SAMLRequest" => raw_query_params["SAMLRequest"],
22+
"SigAlg" => raw_query_params["SigAlg"],
23+
"RelayState" => raw_query_params["RelayState"],
24+
},
25+
}
26+
slo_logout_request = OneLogin::RubySaml::SloLogoutrequest.new(query_params["SAMLRequest"], settings, options)
27+
raise "Invalid Logout Request" unless slo_logout_request.is_valid?
28+
```
29+
30+
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.
31+
332
## Updating from 1.4.2 to 1.4.3
433

534
Version `1.4.3` introduces Recipient validation of SubjectConfirmation elements.

‎lib/onelogin/ruby-saml/logoutresponse.rb‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -211,18 +211,24 @@ def validate_signature
211211
return true unless options.has_key? :get_params
212212
return true unless options[:get_params].has_key? 'Signature'
213213

214+
options[:raw_get_params] = OneLogin::RubySaml::Utils.prepare_raw_get_params(options[:raw_get_params], options[:get_params])
215+
216+
if options[:get_params]['SigAlg'].nil? && !options[:raw_get_params]['SigAlg'].nil?
217+
options[:get_params]['SigAlg'] = CGI.unescape(options[:raw_get_params]['SigAlg'])
218+
end
219+
214220
idp_cert = settings.get_idp_cert
215221
idp_certs = settings.get_idp_cert_multi
216222

217223
if idp_cert.nil? && (idp_certs.nil? || idp_certs[:signing].empty?)
218224
return options.has_key? :relax_signature_validation
219225
end
220226

221-
query_string = OneLogin::RubySaml::Utils.build_query(
222-
:type => 'SAMLResponse',
223-
:data => options[:get_params]['SAMLResponse'],
224-
:relay_state => options[:get_params]['RelayState'],
225-
:sig_alg => options[:get_params]['SigAlg']
227+
query_string = OneLogin::RubySaml::Utils.build_query_from_raw_parts(
228+
:type => 'SAMLResponse',
229+
:raw_data => options[:raw_get_params]['SAMLResponse'],
230+
:raw_relay_state => options[:raw_get_params]['RelayState'],
231+
:raw_sig_alg => options[:raw_get_params]['SigAlg']
226232
)
227233

228234
if idp_certs.nil? || idp_certs[:signing].empty?

‎lib/onelogin/ruby-saml/slo_logoutrequest.rb‎

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -241,18 +241,48 @@ def validate_signature
241241
return true unless options.has_key? :get_params
242242
return true unless options[:get_params].has_key? 'Signature'
243243

244+
# SAML specifies that the signature should be derived from a concatenation
245+
# of URI-encoded values _as sent by the IDP_:
246+
#
247+
# > Further, note that URL-encoding is not canonical; that is, there are multiple legal encodings for a given
248+
# > value. The relying party MUST therefore perform the verification step using the original URL-encoded
249+
# > values it received on the query string. It is not sufficient to re-encode the parameters after they have been
250+
# > processed by software because the resulting encoding may not match the signer's encoding.
251+
#
252+
# <http://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf>
253+
#
254+
# If we don't have the original parts (for backward compatibility) required to correctly verify the signature,
255+
# then fabricate them by re-encoding the parsed URI parameters, and hope that we're lucky enough to use
256+
# the exact same URI-encoding as the IDP. (This is not the case if the IDP is ADFS!)
257+
options[:raw_get_params] ||= {}
258+
if options[:raw_get_params]['SAMLRequest'].nil? && !options[:get_params]['SAMLRequest'].nil?
259+
options[:raw_get_params]['SAMLRequest'] = CGI.escape(options[:get_params]['SAMLRequest'])
260+
end
261+
if options[:raw_get_params]['RelayState'].nil? && !options[:get_params]['RelayState'].nil?
262+
options[:raw_get_params]['RelayState'] = CGI.escape(options[:get_params]['RelayState'])
263+
end
264+
if options[:raw_get_params]['SigAlg'].nil? && !options[:get_params]['SigAlg'].nil?
265+
options[:raw_get_params]['SigAlg'] = CGI.escape(options[:get_params]['SigAlg'])
266+
end
267+
268+
# If we only received the raw version of SigAlg,
269+
# then parse it back into the decoded params hash for convenience.
270+
if options[:get_params]['SigAlg'].nil? && !options[:raw_get_params]['SigAlg'].nil?
271+
options[:get_params]['SigAlg'] = CGI.unescape(options[:raw_get_params]['SigAlg'])
272+
end
273+
244274
idp_cert = settings.get_idp_cert
245275
idp_certs = settings.get_idp_cert_multi
246276

247277
if idp_cert.nil? && (idp_certs.nil? || idp_certs[:signing].empty?)
248278
return options.has_key? :relax_signature_validation
249279
end
250280

251-
query_string = OneLogin::RubySaml::Utils.build_query(
252-
:type => 'SAMLRequest',
253-
:data => options[:get_params]['SAMLRequest'],
254-
:relay_state => options[:get_params]['RelayState'],
255-
:sig_alg => options[:get_params]['SigAlg']
281+
query_string = OneLogin::RubySaml::Utils.build_query_from_raw_parts(
282+
:type => 'SAMLRequest',
283+
:raw_data => options[:raw_get_params]['SAMLRequest'],
284+
:raw_relay_state => options[:raw_get_params]['RelayState'],
285+
:raw_sig_alg => options[:raw_get_params]['SigAlg']
256286
)
257287

258288
if idp_certs.nil? || idp_certs[:signing].empty?

‎lib/onelogin/ruby-saml/utils.rb‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,49 @@ def self.build_query(params)
7575
url_string << "&SigAlg=#{CGI.escape(sig_alg)}"
7676
end
7777

78+
# Reconstruct a canonical query string from raw URI-encoded parts, to be used in verifying a signature
79+
#
80+
# @param params [Hash] Parameters to build the Query String
81+
# @option params [String] :type 'SAMLRequest' or 'SAMLResponse'
82+
# @option params [String] :raw_data URI-encoded, base64 encoded SAMLRequest or SAMLResponse, as sent by IDP
83+
# @option params [String] :raw_relay_state URI-encoded RelayState parameter, as sent by IDP
84+
# @option params [String] :raw_sig_alg URI-encoded SigAlg parameter, as sent by IDP
85+
# @return [String] The Query String
86+
#
87+
def self.build_query_from_raw_parts(params)
88+
type, raw_data, raw_relay_state, raw_sig_alg = [:type, :raw_data, :raw_relay_state, :raw_sig_alg].map { |k| params[k]}
89+
90+
url_string = "#{type}=#{raw_data}"
91+
url_string << "&RelayState=#{raw_relay_state}" if raw_relay_state
92+
url_string << "&SigAlg=#{raw_sig_alg}"
93+
end
94+
95+
# Prepare raw GET parameters (build them from normal parameters
96+
# if not provided).
97+
#
98+
# @param rawparams [Hash] Raw GET Parameters
99+
# @param params [Hash] GET Parameters
100+
# @return [Hash] New raw parameters
101+
#
102+
def self.prepare_raw_get_params(rawparams, params)
103+
rawparams ||= {}
104+
105+
if rawparams['SAMLRequest'].nil? && !params['SAMLRequest'].nil?
106+
rawparams['SAMLRequest'] = CGI.escape(params['SAMLRequest'])
107+
end
108+
if rawparams['SAMLResponse'].nil? && !params['SAMLResponse'].nil?
109+
rawparams['SAMLResponse'] = CGI.escape(params['SAMLResponse'])
110+
end
111+
if rawparams['RelayState'].nil? && !params['RelayState'].nil?
112+
rawparams['RelayState'] = CGI.escape(params['RelayState'])
113+
end
114+
if rawparams['SigAlg'].nil? && !params['SigAlg'].nil?
115+
rawparams['SigAlg'] = CGI.escape(params['SigAlg'])
116+
end
117+
118+
rawparams
119+
end
120+
78121
# Validate the Signature parameter sent on the HTTP-Redirect binding
79122
# @param params [Hash] Parameters to be used in the validation process
80123
# @option params [OpenSSL::X509::Certificate] cert The Identity provider public certtificate

‎test/logoutresponse_test.rb‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,80 @@ class RubySamlTest < Minitest::Test
283283
assert_raises(OneLogin::RubySaml::ValidationError) { logoutresponse.send(:validate_signature) }
284284
assert logoutresponse.errors.include? "Invalid Signature on Logout Response"
285285
end
286+
287+
it "raise when get_params encoding differs from what this library generates" do
288+
settings.security[:signature_method] = XMLSecurity::Document::RSA_SHA1
289+
settings.soft = false
290+
options = {}
291+
options[:get_params] = params
292+
options[:get_params]['RelayState'] = 'http://example.com'
293+
logoutresponse = OneLogin::RubySaml::Logoutresponse.new(params['SAMLResponse'], settings, options)
294+
# Assemble query string.
295+
query = OneLogin::RubySaml::Utils.build_query(
296+
type: 'SAMLResponse',
297+
data: params['SAMLResponse'],
298+
relay_state: params['RelayState'],
299+
sig_alg: params['SigAlg'],
300+
)
301+
# Modify the query string so that it encodes the same values,
302+
# but with different percent-encoding. Sanity-check that they
303+
# really are equialent before moving on.
304+
original_query = query.dup
305+
query.gsub!("example", "ex%61mple")
306+
refute_equal(query, original_query)
307+
assert_equal(CGI.unescape(query), CGI.unescape(original_query))
308+
# Make normalised signature based on our modified params.
309+
sign_algorithm = XMLSecurity::BaseDocument.new.algorithm(settings.security[:signature_method])
310+
signature = settings.get_sp_key.sign(sign_algorithm.new, query)
311+
params['Signature'] = Base64.encode64(signature).gsub(/\n/, "")
312+
# Re-create the Logoutresponse based on these modified parameters,
313+
# and ask it to validate the signature. It will do it incorrectly,
314+
# because it will compute it based on re-encoded query parameters,
315+
# rather than their original encodings.
316+
options[:get_params] = params
317+
logoutresponse = OneLogin::RubySaml::Logoutresponse.new(params['SAMLResponse'], settings, options)
318+
assert_raises(OneLogin::RubySaml::ValidationError, "Invalid Signature on Logout Request") do
319+
logoutresponse.send(:validate_signature)
320+
end
321+
end
322+
323+
it "return true even if raw_get_params encoding differs from what this library generates" do
324+
settings.security[:signature_method] = XMLSecurity::Document::RSA_SHA1
325+
settings.soft = false
326+
options = {}
327+
options[:get_params] = params
328+
options[:get_params]['RelayState'] = 'http://example.com'
329+
logoutresponse = OneLogin::RubySaml::Logoutresponse.new(params['SAMLResponse'], settings, options)
330+
# Assemble query string.
331+
query = OneLogin::RubySaml::Utils.build_query(
332+
type: 'SAMLResponse',
333+
data: params['SAMLResponse'],
334+
relay_state: params['RelayState'],
335+
sig_alg: params['SigAlg'],
336+
)
337+
# Modify the query string so that it encodes the same values,
338+
# but with different percent-encoding. Sanity-check that they
339+
# really are equialent before moving on.
340+
original_query = query.dup
341+
query.gsub!("example", "ex%61mple")
342+
refute_equal(query, original_query)
343+
assert_equal(CGI.unescape(query), CGI.unescape(original_query))
344+
# Make normalised signature based on our modified params.
345+
sign_algorithm = XMLSecurity::BaseDocument.new.algorithm(settings.security[:signature_method])
346+
signature = settings.get_sp_key.sign(sign_algorithm.new, query)
347+
params['Signature'] = Base64.encode64(signature).gsub(/\n/, "")
348+
# Re-create the Logoutresponse based on these modified parameters,
349+
# and ask it to validate the signature. Provide the altered parameter
350+
# in its raw URI-encoded form, so that we don't have to guess the value
351+
# that contributed to the signature.
352+
options[:get_params] = params
353+
options[:get_params].delete("RelayState")
354+
options[:raw_get_params] = {
355+
"RelayState" => "http%3A%2F%2Fex%61mple.com",
356+
}
357+
logoutresponse = OneLogin::RubySaml::Logoutresponse.new(params['SAMLResponse'], settings, options)
358+
assert logoutresponse.send(:validate_signature)
359+
end
286360
end
287361

288362
describe "#validate_signature" do

‎test/slo_logoutrequest_test.rb‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,78 @@ class RubySamlTest < Minitest::Test
330330
logout_request_sign_test.send(:validate_signature)
331331
end
332332
end
333+
334+
it "raise when get_params encoding differs from what this library generates" do
335+
# Use Logoutrequest only to build the SAMLRequest parameter.
336+
settings.security[:signature_method] = XMLSecurity::Document::RSA_SHA1
337+
settings.soft = false
338+
params = OneLogin::RubySaml::Logoutrequest.new.create_params(settings, "RelayState" => "http://example.com")
339+
# Assemble query string.
340+
query = OneLogin::RubySaml::Utils.build_query(
341+
type: 'SAMLRequest',
342+
data: params['SAMLRequest'],
343+
relay_state: params['RelayState'],
344+
sig_alg: params['SigAlg'],
345+
)
346+
# Modify the query string so that it encodes the same values,
347+
# but with different percent-encoding. Sanity-check that they
348+
# really are equialent before moving on.
349+
original_query = query.dup
350+
query.gsub!("example", "ex%61mple")
351+
refute_equal(query, original_query)
352+
assert_equal(CGI.unescape(query), CGI.unescape(original_query))
353+
# Make normalised signature based on our modified params.
354+
sign_algorithm = XMLSecurity::BaseDocument.new.algorithm(settings.security[:signature_method])
355+
signature = settings.get_sp_key.sign(sign_algorithm.new, query)
356+
params['Signature'] = Base64.encode64(signature).gsub(/\n/, "")
357+
# Construct SloLogoutrequest and ask it to validate the signature.
358+
# It will do it incorrectly, because it will compute it based on re-encoded
359+
# query parameters, rather than their original encodings.
360+
options = {}
361+
options[:get_params] = params
362+
options[:settings] = settings
363+
logout_request_sign_test = OneLogin::RubySaml::SloLogoutrequest.new(params['SAMLRequest'], options)
364+
assert_raises(OneLogin::RubySaml::ValidationError, "Invalid Signature on Logout Request") do
365+
logout_request_sign_test.send(:validate_signature)
366+
end
367+
end
368+
369+
it "return true even if raw_get_params encoding differs from what this library generates" do
370+
# Use Logoutrequest only to build the SAMLRequest parameter.
371+
settings.security[:signature_method] = XMLSecurity::Document::RSA_SHA1
372+
settings.soft = false
373+
params = OneLogin::RubySaml::Logoutrequest.new.create_params(settings, "RelayState" => "http://example.com")
374+
# Assemble query string.
375+
query = OneLogin::RubySaml::Utils.build_query(
376+
type: 'SAMLRequest',
377+
data: params['SAMLRequest'],
378+
relay_state: params['RelayState'],
379+
sig_alg: params['SigAlg'],
380+
)
381+
# Modify the query string so that it encodes the same values,
382+
# but with different percent-encoding. Sanity-check that they
383+
# really are equialent before moving on.
384+
original_query = query.dup
385+
query.gsub!("example", "ex%61mple")
386+
refute_equal(query, original_query)
387+
assert_equal(CGI.unescape(query), CGI.unescape(original_query))
388+
# Make normalised signature based on our modified params.
389+
sign_algorithm = XMLSecurity::BaseDocument.new.algorithm(settings.security[:signature_method])
390+
signature = settings.get_sp_key.sign(sign_algorithm.new, query)
391+
params['Signature'] = Base64.encode64(signature).gsub(/\n/, "")
392+
# Construct SloLogoutrequest and ask it to validate the signature.
393+
# Provide the altered parameter in its raw URI-encoded form,
394+
# so that we don't have to guess the value that contributed to the signature.
395+
options = {}
396+
options[:get_params] = params
397+
options[:get_params].delete("RelayState")
398+
options[:raw_get_params] = {
399+
"RelayState" => "http%3A%2F%2Fex%61mple.com",
400+
}
401+
options[:settings] = settings
402+
logout_request_sign_test = OneLogin::RubySaml::SloLogoutrequest.new(params['SAMLRequest'], options)
403+
assert logout_request_sign_test.send(:validate_signature)
404+
end
333405
end
334406

335407
describe "#validate_signature with multiple idp certs" do

0 commit comments

Comments
 (0)