Skip to content

Commit

Permalink
Add extra validations to prevent Signature wrapping attacks
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
pitbulk committed Jun 16, 2016
1 parent 8453f12 commit a571f52
Showing 1 changed file with 47 additions and 5 deletions.
52 changes: 47 additions & 5 deletions lib/onelogin/ruby-saml/response.rb
Original file line number Diff line number Diff line change
Expand Up @@ -337,8 +337,15 @@ def validate_success_status
# @raise [ValidationError] if soft == false and validation fails
#
def validate_structure
structure_error_msg = "Invalid SAML Response. Not match the saml-schema-protocol-2.0.xsd"
unless valid_saml?(document, soft)
return append_error("Invalid SAML Response. Not match the saml-schema-protocol-2.0.xsd")
return append_error(structure_error_msg)
end

unless decrypted_document.nil?
unless valid_saml?(decrypted_document, soft)
return append_error(structure_error_msg)
end
end

true
Expand Down Expand Up @@ -434,11 +441,44 @@ def validate_signed_elements
{"ds"=>DSIG}
)
signed_elements = []
seis = []
ids = []
signature_nodes.each do |signature_node|
signed_element = signature_node.parent.name
if signed_element != 'Response' && signed_element != 'Assertion'
return append_error("Found an unexpected Signature Element. SAML Response rejected")
end

if signature_node.parent.attributes['ID'].nil?
return append_error("Signed Element must contain ID. SAML Response rejected")
end

id = signature_node.parent.attributes.get_attribute("ID").value
if ids.include?(id)
return append_error("Duplicated ID. SAML Response rejected")
end
ids.push(id)

# Check that reference URI matches the parent ID and no duplicate References or IDs
ref = REXML::XPath.first(signature_node, ".//ds:Reference", {"ds"=>DSIG})
if ref
uri = ref.attributes.get_attribute("URI")
if uri && !uri.value.empty?
sei = uri.value[1..-1]
id = signature_node.parent.attributes.get_attribute("ID").value

unless sei == id
return append_error("Found an invalid Signed Element. SAML Response rejected")
end

if seis.include?(sei)
return append_error("Duplicated Reference URI. SAML Response rejected")
end

seis.push(sei)
end
end

signed_elements << signed_element
end

Expand Down Expand Up @@ -614,8 +654,9 @@ def validate_signature
# otherwise, review if the decrypted assertion contains a signature
sig_elements = REXML::XPath.match(
document,
"/p:Response/ds:Signature]",
{ "p" => PROTOCOL, "ds" => DSIG }
"/p:Response[@ID=$id]/ds:Signature]",
{ "p" => PROTOCOL, "ds" => DSIG },
{ 'id' => document.signed_element_id }
)

use_original = sig_elements.size == 1 || decrypted_document.nil?
Expand All @@ -625,8 +666,9 @@ def validate_signature
if sig_elements.nil? || sig_elements.size == 0
sig_elements = REXML::XPath.match(
doc,
"/p:Response/a:Assertion/ds:Signature",
{"p" => PROTOCOL, "a" => ASSERTION, "ds"=>DSIG}
"/p:Response/a:Assertion[@ID=$id]/ds:Signature",
{"p" => PROTOCOL, "a" => ASSERTION, "ds"=>DSIG},
{ 'id' => doc.signed_element_id }
)
end

Expand Down

0 comments on commit a571f52

Please sign in to comment.