From f91eb04beb9a7ce35c369dd75f76fdfd4c6a028b Mon Sep 17 00:00:00 2001 From: Donald Huang Date: Fri, 30 Oct 2020 19:10:10 -0700 Subject: [PATCH 1/2] [SECURITY] upgrade xml-crypto, fix signature xpath because of https://github.com/advisories/GHSA-c27r-x354-4m68 --- lib/saml2.coffee | 15 +++++++++++++-- package.json | 2 +- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/lib/saml2.coffee b/lib/saml2.coffee index 6fd0b12..1ec8414 100644 --- a/lib/saml2.coffee +++ b/lib/saml2.coffee @@ -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 + signature = xmlcrypto.xpath(to_check, "./*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']") return null unless signature.length is 1 sig = new xmlcrypto.SignedXml() sig.keyInfoProvider = getKey: -> format_pem(certificate, 'CERTIFICATE') diff --git a/package.json b/package.json index 79a0a04..bee6cc4 100644 --- a/package.json +++ b/package.json @@ -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", From ae61a1a995f9f310f2510a9db20b4def119e5c14 Mon Sep 17 00:00:00 2001 From: Mark Cabanero Date: Mon, 1 Feb 2021 18:19:01 -0800 Subject: [PATCH 2/2] feat: Scope xpath to root of document If we put priority on which tags are present for Signature validation (AuthnRequest < Response < Assertion), it's possible for a crafted SAML request to validate the signature for a particular tag, which is invalid for the request. Guard against this by selecting for the signature by starting from the root document element. --- lib/saml2.coffee | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/lib/saml2.coffee b/lib/saml2.coffee index 1ec8414..7bdbb5f 100644 --- a/lib/saml2.coffee +++ b/lib/saml2.coffee @@ -249,15 +249,9 @@ check_saml_signature = (_xml, certificate) -> xml = _xml.replace(/\r\n?/g, '\n') doc = (new xmldom.DOMParser()).parseFromString(xml) - # 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 - signature = xmlcrypto.xpath(to_check, "./*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']") + # Calling xpath failed to capture the direct descendents' nodes. + # Be explicit, and call documentElement to start from the root element of the document. + signature = xmlcrypto.xpath(doc.documentElement, "./*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']") return null unless signature.length is 1 sig = new xmlcrypto.SignedXml() sig.keyInfoProvider = getKey: -> format_pem(certificate, 'CERTIFICATE')