Skip to content

Commit a571f52

Browse files
committed
Add extra validations to prevent Signature wrapping attacks
There was a bug on the toolkit that made it vulnerable to a Signature wrapping attacks in the specific scenario where there was a Signature that referenced at the same time 2 elements (but past the scheme validator process since 1 of the element was inside the encrypted assertion. On this commit we added 3 new validators in order to avoid Signature wrapping attacks: - Extra validations at the validate_signed_elements method to check that the ref URIs and IDs are unique and consistent. - Validate the document (encrypted and decrypted version) against the scheme. - Use at validate_signature method the same logic than in xpath_from_signed_assertion (adding the 'id' => doc.signed_element_id condition).
1 parent 8453f12 commit a571f52

File tree

1 file changed

+47
-5
lines changed

1 file changed

+47
-5
lines changed

lib/onelogin/ruby-saml/response.rb

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -337,8 +337,15 @@ def validate_success_status
337337
# @raise [ValidationError] if soft == false and validation fails
338338
#
339339
def validate_structure
340+
structure_error_msg = "Invalid SAML Response. Not match the saml-schema-protocol-2.0.xsd"
340341
unless valid_saml?(document, soft)
341-
return append_error("Invalid SAML Response. Not match the saml-schema-protocol-2.0.xsd")
342+
return append_error(structure_error_msg)
343+
end
344+
345+
unless decrypted_document.nil?
346+
unless valid_saml?(decrypted_document, soft)
347+
return append_error(structure_error_msg)
348+
end
342349
end
343350

344351
true
@@ -434,11 +441,44 @@ def validate_signed_elements
434441
{"ds"=>DSIG}
435442
)
436443
signed_elements = []
444+
seis = []
445+
ids = []
437446
signature_nodes.each do |signature_node|
438447
signed_element = signature_node.parent.name
439448
if signed_element != 'Response' && signed_element != 'Assertion'
440449
return append_error("Found an unexpected Signature Element. SAML Response rejected")
441450
end
451+
452+
if signature_node.parent.attributes['ID'].nil?
453+
return append_error("Signed Element must contain ID. SAML Response rejected")
454+
end
455+
456+
id = signature_node.parent.attributes.get_attribute("ID").value
457+
if ids.include?(id)
458+
return append_error("Duplicated ID. SAML Response rejected")
459+
end
460+
ids.push(id)
461+
462+
# Check that reference URI matches the parent ID and no duplicate References or IDs
463+
ref = REXML::XPath.first(signature_node, ".//ds:Reference", {"ds"=>DSIG})
464+
if ref
465+
uri = ref.attributes.get_attribute("URI")
466+
if uri && !uri.value.empty?
467+
sei = uri.value[1..-1]
468+
id = signature_node.parent.attributes.get_attribute("ID").value
469+
470+
unless sei == id
471+
return append_error("Found an invalid Signed Element. SAML Response rejected")
472+
end
473+
474+
if seis.include?(sei)
475+
return append_error("Duplicated Reference URI. SAML Response rejected")
476+
end
477+
478+
seis.push(sei)
479+
end
480+
end
481+
442482
signed_elements << signed_element
443483
end
444484

@@ -614,8 +654,9 @@ def validate_signature
614654
# otherwise, review if the decrypted assertion contains a signature
615655
sig_elements = REXML::XPath.match(
616656
document,
617-
"/p:Response/ds:Signature]",
618-
{ "p" => PROTOCOL, "ds" => DSIG }
657+
"/p:Response[@ID=$id]/ds:Signature]",
658+
{ "p" => PROTOCOL, "ds" => DSIG },
659+
{ 'id' => document.signed_element_id }
619660
)
620661

621662
use_original = sig_elements.size == 1 || decrypted_document.nil?
@@ -625,8 +666,9 @@ def validate_signature
625666
if sig_elements.nil? || sig_elements.size == 0
626667
sig_elements = REXML::XPath.match(
627668
doc,
628-
"/p:Response/a:Assertion/ds:Signature",
629-
{"p" => PROTOCOL, "a" => ASSERTION, "ds"=>DSIG}
669+
"/p:Response/a:Assertion[@ID=$id]/ds:Signature",
670+
{"p" => PROTOCOL, "a" => ASSERTION, "ds"=>DSIG},
671+
{ 'id' => doc.signed_element_id }
630672
)
631673
end
632674

0 commit comments

Comments
 (0)