Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem validating document with multiple Signature elements #275

Closed
levelboy opened this issue Oct 29, 2015 · 6 comments
Closed

Problem validating document with multiple Signature elements #275

levelboy opened this issue Oct 29, 2015 · 6 comments

Comments

@levelboy
Copy link
Contributor

The lastest release of the gem (1.1.0), fails to validate documents of this form:

<samlp:Response>
    <saml:Issuer></saml:Issuer>
    <ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
        <ds:SignedInfo>
            <ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
            <ds:SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>
            <ds:Reference URI="#_f7379ea04746ee86b0a16f519aea47c83cd0eefad0">
                <ds:Transforms>
                    <ds:Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/>
                    <ds:Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
                </ds:Transforms>
                <ds:DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
                <ds:DigestValue></ds:DigestValue>
            </ds:Reference>
        </ds:SignedInfo>
        <ds:SignatureValue></ds:SignatureValue>
        <ds:KeyInfo>
            <ds:X509Data>
                <ds:X509Certificate></ds:X509Certificate>
            </ds:X509Data>
        </ds:KeyInfo>
    </ds:Signature>
    <samlp:Status>
        <samlp:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Success"/>
    </samlp:Status>
    <saml:Assertion ID="_a945e7e0e04ae098e9603cdcd9360f4a3d558198b1" IssueInstant="2014-12-08T16:22:43Z" Version="2.0" xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
        <saml:Issuer></saml:Issuer>
        <ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
            <ds:SignedInfo>
                <ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
                <ds:SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>
                <ds:Reference URI="#_a945e7e0e04ae098e9603cdcd9360f4a3d558198b1">
                    <ds:Transforms>
                        <ds:Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/>
                        <ds:Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
                    </ds:Transforms>
                    <ds:DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
                    <ds:DigestValue></ds:DigestValue>
                </ds:Reference>
            </ds:SignedInfo>
            <ds:SignatureValue></ds:SignatureValue>
            <ds:KeyInfo>
                <ds:X509Data>
                    <ds:X509Certificate></ds:X509Certificate>
                </ds:X509Data>
            </ds:KeyInfo>
        </ds:Signature>
        <saml:Subject>
            <saml:NameID Format="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified"></saml:NameID>
            <saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer">
                <saml:SubjectConfirmationData InResponseTo="_79f6013431c8237f4de1c57ee7c9f11b8b3c701894" NotOnOrAfter="" Recipient=""/>
            </saml:SubjectConfirmation>
        </saml:Subject>
    </saml:Assertion>
</samlp:Response>

I tracked the problem in the xml_security.rb https://github.com/onelogin/ruby-saml/blob/master/lib/xml_security.rb#L234

Within each signature element, we try to get the Reference node and match the digest_value with the computed hash of the referenced node.
When we get the encoded_digest_value from the Signature element, we use this: https://github.com/onelogin/ruby-saml/blob/master/lib/xml_security.rb#L307, note the //ds:DigestValue. This scopes the call to the whole document and always returns the first DigestValue in the document, regardless of the context element we passed to the call so it always compares the first digest value in the document with the computed hashes of each of the Referenced nodes hence the validation fails. See this answer on SO: http://stackoverflow.com/a/32640500

  1. Is this intended behavior or is it a bug? I can write a PR to fix this.
  2. Why are two XML parsing libraries used? I see both nokogiri and rexml being used here
  3. Have you considered using an external library for the signature verification? (e.g. https://github.com/sinisterchipmunk/xmlsec#nokogiri-xmlsec)

Thanks

@phene
Copy link
Contributor

phene commented Oct 29, 2015

+1 for addressing this -- My team also ran into this issue with the Okta IdP.

@pitbulk
Copy link
Collaborator

pitbulk commented Oct 29, 2015

Maybe this was introduced at this change:
275512a

We always tries to analyze the first Signature. If the signed element (SAMLResponse) has signed childs (Assertion), checking the parent we are sure that the child is ok, so in the XML that you posted, we try to check the signature of the SAMLResponse element.

  1. Please provide a PR and test so others that experience same issue can review.
  2. We plan to refactor all the signature/encrypt methods in a new gem and avoid using 2 XML parsing libraries.
  3. It seems that there is no external library that offer what we required.
    The library that you mentioned, for example, is not completed (has limitations)

@levelboy
Copy link
Contributor Author

@pitbulk Added a PR for this. It has the least amount of change to validate documents, and the modification is according to the fact that a Signature element can only have one Reference node inside.

What's your timeframe for the xml_security refactoring. I would highly like to contribute to that.

@levelboy
Copy link
Contributor Author

btw, if it's not a big hassle, I would greatly appreciate if you can release a new version of the gem (say 1.1.1). I've been waiting for a long time for this. Thank you

@pitbulk
Copy link
Collaborator

pitbulk commented Nov 10, 2015

@phene I released 1.1.1 version. Please try it and let me know if the problem is gone.
@levelboy thanks for the PR, I will release it at Rubygems as soon as we get more feedback on that issue.

@akash-goel
Copy link

I try to add SSO (single sign-on) and use SAML2 protocol.

It works well when we run the application, but when we run the test cases then it gives the digest mismatch and signature validation error.

We have checked the Configuration and configuration is same in both the cases.

Request to share the parameters which are responsible for changing Signature Validation and digest mismatch error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants