From 52e0a9b1573ff40367f68d8cbef650542ee70b08 Mon Sep 17 00:00:00 2001 From: Donald Huang Date: Fri, 30 Oct 2020 19:10:10 -0700 Subject: [PATCH 1/3] [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 6ac3993..b27f773 100644 --- a/lib/saml2.coffee +++ b/lib/saml2.coffee @@ -242,10 +242,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 2c2de8e..9c3b3ee 100644 --- a/package.json +++ b/package.json @@ -34,7 +34,7 @@ "async": "^3.2.0", "debug": "^4.3.0", "underscore": "^1.8.0", - "xml-crypto": "^0.10.0", + "xml-crypto": "^2.0.0", "xml-encryption": "^1.2.1", "xml2js": "^0.4.0", "xmlbuilder2": "^2.4.0", From b43eab462b1a5504573d7162a6c0e2a53a83b921 Mon Sep 17 00:00:00 2001 From: Mark Cabanero Date: Mon, 1 Feb 2021 18:19:01 -0800 Subject: [PATCH 2/3] 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, as we've done so in the past. --- lib/saml2.coffee | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/lib/saml2.coffee b/lib/saml2.coffee index b27f773..fcc0733 100644 --- a/lib/saml2.coffee +++ b/lib/saml2.coffee @@ -248,15 +248,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#']") + # xpath failed to capture nodes of direct descendents of the root. + # Call documentElement to explicitly 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') From 4e63121f7bee831e63113fdcca7c942cd0d35be8 Mon Sep 17 00:00:00 2001 From: Andrew Maillet Date: Mon, 13 Nov 2017 10:51:59 -0500 Subject: [PATCH 3/3] feat: accept saml response with xml sig namespace at the root level the xml dsig namespace can be specified at the root of the SAML response instead of on each Signature element. The canonincalized xml for the signature element requires that the namespace declaration be present on the siganture element. --- lib/saml2.coffee | 2 +- ...d_response_twice_signed_dsig_ns_at_top.xml | 39 +++++++++++++++++++ test/saml2.coffee | 4 ++ 3 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 test/data/good_response_twice_signed_dsig_ns_at_top.xml diff --git a/lib/saml2.coffee b/lib/saml2.coffee index fcc0733..2c61d4b 100644 --- a/lib/saml2.coffee +++ b/lib/saml2.coffee @@ -254,7 +254,7 @@ check_saml_signature = (_xml, certificate) -> return null unless signature.length is 1 sig = new xmlcrypto.SignedXml() sig.keyInfoProvider = getKey: -> format_pem(certificate, 'CERTIFICATE') - sig.loadSignature signature[0].toString() + sig.loadSignature signature[0] valid = sig.checkSignature xml if valid return get_signed_data(doc, sig) diff --git a/test/data/good_response_twice_signed_dsig_ns_at_top.xml b/test/data/good_response_twice_signed_dsig_ns_at_top.xml new file mode 100644 index 0000000..ea2591d --- /dev/null +++ b/test/data/good_response_twice_signed_dsig_ns_at_top.xml @@ -0,0 +1,39 @@ + + + + + VmOQiP59NeSBPwrhe5MDQJlNw/E=pPycwjnj6ezRb9vrmEQ0CTBlkRa7inhDCHUj2Z5s6pOuBZq2bdxY1jvplHz5FW6/2SPtsST5Wj6RZMClHGV8rlTBjgA92+EtGJHgaZYemvFTA1n/7SWI9vjP2Doy9JF8AeZlmN9xgZL/wVsKkdv/lk7B6stWfUI/PDRN5JVUYDvoSC2j1pxkdJ6zhFL9XWs9wUNlO2vsP1XsHrQ7ndn4h5K6J9frfc0IX6R7NBu/pDUk2Vx6Xh1RlhuZRD33KN38e58vs1qm1isFcoTgMhdAVjS41yf92b5KFuDp0x4dliV099QUZFtNH+7SH7pJXEkeLQnXpF/73jXLoMUjdn7qNQ== +MIIDGTCCAgGgAwIBAgIJAO8HJfrb3JZeMA0GCSqGSIb3DQEBBQUAMCMxITAfBgNVBAoMGEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDAeFw0xNDAzMTgwMTE3MTdaFw0yNDAzMTcwMTE3MTdaMCMxITAfBgNVBAoMGEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAMFf1kCef6FTPMxQSoThAZGFNmixh8fRDLsUo58pEFwztBRUPWS6s6Ql8mA75aAEdo4+JVyE8QPi5F+fWbnToWkIw7E7YGl6s+EScSMQYHKCLq4mPHPMHtZspFowNp+Vax88SSUo1TKlpVNVIGim8JQ5SRi3p0aD6UAiu9WxQ5s+xHnDwgvQiu3Sa4COl5NQjkC1r2LrhJnJQQiw0hsn1nGgg15jEaDCZa8uPw1EtHv8smoZpjTbwRBVjXtzLskYIRyYLQjvqR+/QAd0XZcav0LdTwQR6obg/CwSgv7qG/WN6t25VIIGQDIUkVMBhLDmCh8QRpTvx1YWumSWW4D2k2kCAwEAAaNQME4wHQYDVR0OBBYEFLpo8Vz1m19xvPmzx+2wf2PaSTIpMB8GA1UdIwQYMBaAFLpo8Vz1m19xvPmzx+2wf2PaSTIpMAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQEFBQADggEBALhwpLS6C+97nWrEICI5yetQjexCJGltMESg1llNYjsbIuJ/S4XbrVzhN4nfNGMSbj8rb/9FT6TSru5QLjJQQmj38pqsWtEhR2vBLclqGqEcJfvPMdn1qAJhJfhrs0KUpsX6xFTnSkNoyGxCP8Wh2C1L0NL5r+x58lkma5vL6ncwWYY+0C3bt1XbBRdeOZHUwuYTIcD+BCNixQiNor7KjO1TzpOb6V3m1SKHu8idDM5fUcKooGbV3WuE7AJrAG5fvt59V9MtMPc2FklVFminfTeYKboEaxZJxuPDbQs2IyJ/0lI8P0Mv4LIKj4+OipQ/fGbZuE7cOioPKKl02dE7eCA= + + + + + This data has no meaning. + http://idp.example.com/metadata.xml + + + nS72XwOKD7SxpBrvb8MFkrmrPJM=cf4xvQneMFGQOsIgG/xrg8dpYbCvBZ5GUZkIMNA7BTT2tBuiP0djR/iS4uHPqnkbYLVnJd2gwh7Mg/7GySAVSSimfsNUk0LxKd59Nmw8z+iVTKzFnK7O6r4ifkPvWpIM28J1fJmqUINXRXBD1JQSY2p/4TPS1DQAUpYf8Yh1R4SWX0Xqiu2XgIrTXVrqh3X76fm4XMFmyL7FNt1wn8qzobqpfDYciNz1ZCfg9NwdFY4AeWSD3HeByJn9ct0CgNyDu5B6ii4CldfEUS6S15IlicuVimVteNXcSbyNN9/EOkBjCk9PtzgqzX5X4T0rF5CU6s2dSUAjCsmIEPWUr38B/w== +MIIDGTCCAgGgAwIBAgIJAO8HJfrb3JZeMA0GCSqGSIb3DQEBBQUAMCMxITAfBgNVBAoMGEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDAeFw0xNDAzMTgwMTE3MTdaFw0yNDAzMTcwMTE3MTdaMCMxITAfBgNVBAoMGEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAMFf1kCef6FTPMxQSoThAZGFNmixh8fRDLsUo58pEFwztBRUPWS6s6Ql8mA75aAEdo4+JVyE8QPi5F+fWbnToWkIw7E7YGl6s+EScSMQYHKCLq4mPHPMHtZspFowNp+Vax88SSUo1TKlpVNVIGim8JQ5SRi3p0aD6UAiu9WxQ5s+xHnDwgvQiu3Sa4COl5NQjkC1r2LrhJnJQQiw0hsn1nGgg15jEaDCZa8uPw1EtHv8smoZpjTbwRBVjXtzLskYIRyYLQjvqR+/QAd0XZcav0LdTwQR6obg/CwSgv7qG/WN6t25VIIGQDIUkVMBhLDmCh8QRpTvx1YWumSWW4D2k2kCAwEAAaNQME4wHQYDVR0OBBYEFLpo8Vz1m19xvPmzx+2wf2PaSTIpMB8GA1UdIwQYMBaAFLpo8Vz1m19xvPmzx+2wf2PaSTIpMAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQEFBQADggEBALhwpLS6C+97nWrEICI5yetQjexCJGltMESg1llNYjsbIuJ/S4XbrVzhN4nfNGMSbj8rb/9FT6TSru5QLjJQQmj38pqsWtEhR2vBLclqGqEcJfvPMdn1qAJhJfhrs0KUpsX6xFTnSkNoyGxCP8Wh2C1L0NL5r+x58lkma5vL6ncwWYY+0C3bt1XbBRdeOZHUwuYTIcD+BCNixQiNor7KjO1TzpOb6V3m1SKHu8idDM5fUcKooGbV3WuE7AJrAG5fvt59V9MtMPc2FklVFminfTeYKboEaxZJxuPDbQs2IyJ/0lI8P0Mv4LIKj4+OipQ/fGbZuE7cOioPKKl02dE7eCA= + + tstudent + + + + + + + https://sp.example.com/metadata.xml + + + + + Test + + + + + urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport + + + + \ No newline at end of file diff --git a/test/saml2.coffee b/test/saml2.coffee index b595339..9480c7e 100644 --- a/test/saml2.coffee +++ b/test/saml2.coffee @@ -187,6 +187,10 @@ describe 'saml2', -> it 'validates a Response signature when a signature also exists within the Assertion', -> assert.notEqual null, saml2.check_saml_signature(get_test_file("good_response_twice_signed.xml"), get_test_file("test.crt")) + it 'validates a Response signature when the dsig namespace is declared at the root level', -> + result = saml2.check_saml_signature(get_test_file("good_response_twice_signed_dsig_ns_at_top.xml"), get_test_file("test.crt")) + assert.notEqual null, result + describe 'check_status_success', => it 'accepts a valid success status', => assert saml2.check_status_success(@good_response_dom), "Did not get 'true' for valid response."