Skip to content

Commit 9c701f6

Browse files
jeffparsonsjeffparsons
committed
Fix verification of Logoutresponse signatures
Old behaviour is preserved if the correct (raw URI-encoded) parts are not provided. Previous commits fix (and test) the equivalent check for SloLogoutrequest.
1 parent c66b940 commit 9c701f6

File tree

2 files changed

+109
-5
lines changed

2 files changed

+109
-5
lines changed

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

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

214+
# SAML specifies that the signature should be derived from a concatenation
215+
# of URI-encoded values _as sent by the IDP_:
216+
#
217+
# > Further, note that URL-encoding is not canonical; that is, there are multiple legal encodings for a given
218+
# > value. The relying party MUST therefore perform the verification step using the original URL-encoded
219+
# > values it received on the query string. It is not sufficient to re-encode the parameters after they have been
220+
# > processed by software because the resulting encoding may not match the signer's encoding.
221+
#
222+
# <http://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf>
223+
#
224+
# If we don't have the original parts (for backward compatibility) required to correctly verify the signature,
225+
# then fabricate them by re-encoding the parsed URI parameters, and hope that we're lucky enough to use
226+
# the exact same URI-encoding as the IDP. (This is not the case if the IDP is ADFS!)
227+
options[:raw_get_params] ||= {}
228+
if options[:raw_get_params]['SAMLResponse'].nil? && !options[:get_params]['SAMLResponse'].nil?
229+
options[:raw_get_params]['SAMLResponse'] = CGI.escape(options[:get_params]['SAMLResponse'])
230+
end
231+
if options[:raw_get_params]['RelayState'].nil? && !options[:get_params]['RelayState'].nil?
232+
options[:raw_get_params]['RelayState'] = CGI.escape(options[:get_params]['RelayState'])
233+
end
234+
if options[:raw_get_params]['SigAlg'].nil? && !options[:get_params]['SigAlg'].nil?
235+
options[:raw_get_params]['SigAlg'] = CGI.escape(options[:get_params]['SigAlg'])
236+
end
237+
238+
# If we only received the raw version of SigAlg,
239+
# then parse it back into the decoded params hash for convenience.
240+
if options[:get_params]['SigAlg'].nil? && !options[:raw_get_params]['SigAlg'].nil?
241+
options[:get_params]['SigAlg'] = CGI.unescape(options[:raw_get_params]['SigAlg'])
242+
end
243+
214244
idp_cert = settings.get_idp_cert
215245
idp_certs = settings.get_idp_cert_multi
216246

217247
if idp_cert.nil? && (idp_certs.nil? || idp_certs[:signing].empty?)
218248
return options.has_key? :relax_signature_validation
219249
end
220250

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']
251+
query_string = OneLogin::RubySaml::Utils.build_query_from_raw_parts(
252+
:type => 'SAMLResponse',
253+
:raw_data => options[:raw_get_params]['SAMLResponse'],
254+
:raw_relay_state => options[:raw_get_params]['RelayState'],
255+
:raw_sig_alg => options[:raw_get_params]['SigAlg']
226256
)
227257

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

‎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

0 commit comments

Comments
 (0)