Skip to content

[SECURITY] upgrade xml-crypto, fix signature xpath #215

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.

Sign up for GitHub

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

Closed
donhcd wants to merge 2 commits into from
Closed

[SECURITY] upgrade xml-crypto, fix signature xpath #215

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f91eb04
[SECURITY] upgrade xml-crypto, fix signature xpath
donhcd Oct 31, 2020
ae61a1a
feat: Scope xpath to root of document
mcab Feb 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions lib/saml2.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,10 +243,21 @@ decrypt_assertion = (dom, private_keys, cb) ->
# This checks the signature of a saml document and returns either array containing the signed data if valid, or null
# if the signature is invalid. Comparing the result against null is NOT sufficient for signature checks as it doesn't
# verify the signature is signing the important content, nor is it preventing the parsing of unsigned content.
check_saml_signature = (xml, certificate) ->
check_saml_signature = (_xml, certificate) ->
# xml-crypto requires that whitespace is normalized as such:
# https://github.com/yaronn/xml-crypto/commit/17f75c538674c0afe29e766b058004ad23bd5136#diff-5dfe38baf287dcf756a17c2dd63483781b53bf4b669e10efdd01e74bcd8e780aL69
xml = _xml.replace(/\r\n?/g, '\n')
doc = (new xmldom.DOMParser()).parseFromString(xml)

signature = xmlcrypto.xpath(doc, "./*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']")
# Find the correct section of the XML doc to check the signature for
maybe_req = xmlcrypto.xpath(doc, "//*[local-name(.)='AuthnRequest']")
maybe_req = maybe_req && maybe_req[0]
maybe_resp = xmlcrypto.xpath(doc, "//*[local-name(.)='Response']")
maybe_resp = maybe_resp && maybe_resp[0]
maybe_assert = xmlcrypto.xpath(doc, "//*[local-name(.)='Assertion']")
maybe_assert = maybe_assert && maybe_assert[0]
to_check = maybe_req || maybe_resp || maybe_assert

@mcab mcab Dec 4, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there ever a case where we might want to check an Assertion first over an AuthnRequest? Or is it strictly this order?

@donhcd donhcd Dec 7, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I'm not entirely sure; I put them in this order just because it happened to let the tests pass - I don't even know what the set of accepted combinations of authreq, response, assertion should be... I think I would need to review the saml spec or something to get to the bottom of this.

@mcab mcab Feb 1, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems possible to validate an improper AuthnRequest if there happens to be a Response nested within it. An actual test case eludes me, but it does seem possible.

To address these multiple checks, I think we have to consider two things: why the initial xpath change was made, and what happens with this new version of xpath.

Why the initial xpath change was made

Previously, we selected all Signatures, and relied on only one <Signature> being present. #82 changed this to be more flexible in cases where a <Response> and <Assertion> could both have a <Signature>.

From how I'm interpreting section 5.3, page 68-69 of the SAML spec:

A SAML assertion may be embedded within another SAML element, such as an enclosing
or a request or response, which may be signed.

We need to check for the presence of a signature on both the request or response, or the enclosed assertion.

When a SAML assertion does not contain a <ds:Signature> element, but is contained in an enclosing SAML element that contains a <ds:Signature> element, and the signature applies to the <Assertion> element and all its children, then the assertion can be considered to inherit the signature from the enclosing element. The resulting interpretation should be equivalent to the case where the assertion itself was signed with the same key and signature options.

The internal <Assertion> does not have a signature, but the parent SAML element does have a signature and the signature applies to the <Assertion> plus its children, then the <Assertion> takes on the signature of the parent SAML element.

We never have a case (defined in spec) when both the signature of the parent element and the internal <Assertion> does have a signature. Do we verify both? Verify the parent element's signature applies to the wrapped <Assertion> and validate only the parent?

Ideally, as we're fixing this signature xpath, we're deliberate on these cases to properly justify which element's <Signature> we should validate. In this PR, it seems like we would do <Assertion> > <Response> > <AuthnRequest>, which doesn't seem right. Changing this PR to validate the <Signature> closest to the root would be in line with what this library has done in the past, but it still might not be right.

What happens with this new version of xpath

I'm initially only testing the cases for sign_authn_request_with_embedded_signature and check_saml_signature.

If we use the version in this PR and specifically scope it to the entire document:

  signature = xmlcrypto.xpath(doc, "./*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']")
  console.log "signatures", signature

we end up failing the check because no signature is properly grabbed:

  saml2
    private helpers
      sign_authn_request_with_embedded_signature
signatures []
        1) correctly embeds the signature
      check_saml_signature
signatures []
        2) accepts signed xml
signatures []
        ✓ rejects xml without a signature
signatures []
        ✓ rejects xml with an invalid signature
signatures []
        3) validates a Response signature when a signature also exists within the Assertion

If we use the current version on master:

  signature = xmlcrypto.xpath(doc, "./*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']")
  console.log "signatures", signature

we can see that those tests pass because the proper signatures are grabbed:

  saml2
    private helpers
      sign_authn_request_with_embedded_signature
signatures [
  Element {
    _nsMap: { '': 'http://www.w3.org/2000/09/xmldsig#' },
    attributes: { '0': [Attr], _ownerElement: [Circular], length: 1 },
    childNodes: { '0': [Element], '1': [Element], length: 2 },
[...]

Ideally, we change it so that the signatures are being grabbed properly.

If we change it to undo #82, we pass all the checks (grab signatures within each document), but fail when we grab signatures in both the <Assertion> and the <Response>.

If we change the xpath to point to the documentElement of the parsed XML, things pass:

  signature = xmlcrypto.xpath(doc.documentElement, "./*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']")
> make test

  saml2
    private helpers
      sign_authn_request_with_embedded_signature
        ✓ correctly embeds the signature
      check_saml_signature
        ✓ accepts signed xml
        ✓ rejects xml without a signature
        ✓ rejects xml with an invalid signature
        ✓ validates a Response signature when a signature also exists within the Assertion (40ms)

and passes all the other checks. It still feels wrong (taking the first direct descendant for Signature) but it does seem to preserve the original semantics. Happy to discuss further if that's the right approach.

signature = xmlcrypto.xpath(to_check, "./*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']")

@mcab mcab Dec 4, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL that you could return matches via this way! I thought to_check would return a boolean, but didn't realize it would select the particular path via xpath.

no-op, just thought it was neat. :)

return null unless signature.length is 1
sig = new xmlcrypto.SignedXml()
sig.keyInfoProvider = getKey: -> format_pem(certificate, 'CERTIFICATE')
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
"async": "^2.5.0",
"debug": "^2.6.0",
"underscore": "^1.8.0",
"xml-crypto": "^0.10.0",
"xml-crypto": "^2.0.0",
"xml-encryption": "^1.2.1",
"xml2js": "^0.4.0",
"xmlbuilder": "~2.2.0",
Expand Down