[SECURITY] upgrade xml-crypto, fix signature xpath

[donhcd]

because of GHSA-c27r-x354-4m68

a remake of #214

• upgrade xml-crypto
• normalize whitespace in check_saml_signature
• improve the logic to figure out the correct signature to validate in the document - seems like the XPath implementation that xml-crypto was using changed between 0.9.0 (it's unclear which sha 0.10.0 is supposed to correlate to) and 2.0.0

This passes all the tests

[donhcd]

ping @prime-time @mcab can you guys please take a look here, and let me know if there's anything I can do to get this merged? this fixes a security issue, which is pretty high priority for an authentication library. I understand this library is pretty widely used and I think most users would appreciate having this fixed.

[jnardone]

@prime-time @mcab can we get this resolved and a new version issued? please? otherwise will have to fork etc etc.

[mcab]

Hey there! Thanks for taking the time to submit this PR and also bump up version concerns.

Just wanted to follow up on this particular comment before merging:

• improve the logic to figure out the correct signature to validate in the document - seems like the XPath implementation that xml-crypto was using changed between 0.9.0 (it's unclear which sha 0.10.0 is supposed to correlate to) and 2.0.0

We similarly leverage xmlcrypto for get_signed_data. I was curious if we had to do some additional tweaks there, as well.

[mcab]

Two comments:

• to_check's priority, and is it always strictly check AuthnRequest, then Response, then Assertion, in that order.
• Earlier, with xmlcrypto and get_signed_data. I can see that it roughly mirrors SignedXml.prototype.validateReferences; just wanted to sanity check that this doesn't need additional tweaks for it to function.

[mcab]

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

[donhcd]

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]

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.

[mcab]

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. :)

[donhcd]

Thanks for the review @mcab ! I think I need to do a bit more investigation to have concrete answers for some of your concerns, and I've been quite busy the last few days.

If some other interested party is interested in investigating these questions before I can get to them I'd be very grateful for the help; otherwise I will try to circle back on this in the next couple weeks (though I'll try to do so sooner)

[jfaylon]

Hope the fix gets merged and published soon. Thanks :)

[jfaylon]

@donhcd @mcab any update with the fix?

[donhcd]

@jfaylon unfortunately things have been busy and I don't foresee myself having cycles to push this forward any time soon; if you are anxious to get this a fix merged please feel welcome to build off of this and address @mcab's comments

[mcab]

@donhcd, if you're too busy, I believe I can close out the rest with the considerations posted above. I really wish I understood why we need the change (point towards the document's documentElement over the document itself) to have higher confidence, but we could also just bump this by a major version given it's potentially breaking. Externally, we're not changing any of the APIs, but it's also possible that current usage of the library would reveal this limitation.

[donhcd]

@mcab that'd be really great if you could shepherd this to get mainlined, really appreciate your help here 🙏 sorry that I don't have a lot of insight into why this change was so involved

[mcab]

Ack, I thought it would be a straightforward fix, but no!

I was curious if this was truly going to fix things. Formatting the good_response_twice_signed.xml file with the doc.documentElement selector breaks the test.

This is also seen on the current master branch. Why this is the case, I want to make sure. In both cases, the correct signature is being picked up, it's just that the validation check (valid = sig.checkSignature xml) does not pass.

[mcab]

Ah. Seems to stem from how #123 solved this. Going to investigate if reformatting that certain tags were shifted, and that https://github.com/Clever/saml2/pull/123/files#diff-7e648a3b261cbb3a10631af95a7e3f1b324d013ea13f5da3d7874baf5d677a26R190-R193 works.

[mcab]

I have no idea how to git cherry-pick nicely from two remote branches, nor how I was able to push to this branch in the first place. Going to merge #228, since it addresses the concerns brought up here.

Thank you so much @donhcd for submitting a PR for this.

[donhcd]

thanks a ton for getting this over the line @mcab!!