From 679d8b666bd9d4b29476d156eeceafd2f4d2d9ef Mon Sep 17 00:00:00 2001 From: pollardb Date: Mon, 18 Mar 2019 16:34:05 +0000 Subject: [PATCH 1/7] Handle enveloped transformations with exclusive canonicalization better --- lib/signed-xml.js | 107 ++++++++++++++++++++++++---------------------- 1 file changed, 56 insertions(+), 51 deletions(-) diff --git a/lib/signed-xml.js b/lib/signed-xml.js index bd14cfc6..64faf07f 100644 --- a/lib/signed-xml.js +++ b/lib/signed-xml.js @@ -361,26 +361,52 @@ SignedXml.prototype.validateSignatureValue = function(doc) { var signedInfo = utils.findChilds(this.signatureNode, "SignedInfo") if (signedInfo.length==0) throw new Error("could not find SignedInfo element in the message") - /** - * When canonicalization algorithm is non-exclusive, search for ancestor namespaces - * before validating signature. - */ - var ancestorNamespaces = []; if(this.canonicalizationAlgorithm === "http://www.w3.org/TR/2001/REC-xml-c14n-20010315" || this.canonicalizationAlgorithm === "http://www.w3.org/TR/2001/REC-xml-c14n-20010315#WithComments") { if(!doc || typeof(doc) !== "object"){ throw new Error("When canonicalization method is non-exclusive, whole xml dom must be provided as an argument"); } - - ancestorNamespaces = findAncestorNs(doc, "//*[local-name()='SignedInfo']"); + } + + /** + * Get the InclusiveNamespaces PrefixList + */ + var inclusiveNamespacesPrefixList + var CanonicalizationMethod = utils.findChilds(signedInfo[0], "CanonicalizationMethod") + if (CanonicalizationMethod.length!=0) { + var inclusiveNamespaces = utils.findChilds(CanonicalizationMethod[0], "InclusiveNamespaces") + if (inclusiveNamespaces.length!=0) { + inclusiveNamespacesPrefixList = inclusiveNamespaces[0].getAttribute('PrefixList').split(" "); + } + } + + /** + * Search for ancestor namespaces before validating signature. + */ + var ancestorNamespaces = []; + ancestorNamespaces = findAncestorNs(doc, "//*[local-name()='SignedInfo']"); + + /** + * If you have a PrefixList then use it and the ancestors to add the necessary namespaces + */ + if (inclusiveNamespacesPrefixList) { + inclusiveNamespacesPrefixList.forEach(function(inclusiveNamespacesPrefix) { + ancestorNamespaces.forEach(function (ancestorNamespace) { + if (inclusiveNamespacesPrefix == ancestorNamespace.prefix) { + signedInfo[0].setAttributeNS('http://www.w3.org/2000/xmlns/', 'xmlns:' + ancestorNamespace.prefix, ancestorNamespace.namespaceURI); + } + }) + }) } var c14nOptions = { - ancestorNamespaces: ancestorNamespaces + ancestorNamespaces: ancestorNamespaces, + inclusiveNamespacesPrefixList: inclusiveNamespacesPrefixList }; var signedInfoCanon = this.getCanonXml([this.canonicalizationAlgorithm], signedInfo[0], c14nOptions) var signer = this.findSignatureAlgorithm(this.signatureAlgorithm) + var res = signer.verifySignature(signedInfoCanon, this.signingKey, this.signatureValue) if (!res) this.validationErrors.push("invalid signature: the signature value " + this.signatureValue + " is incorrect") @@ -449,59 +475,35 @@ SignedXml.prototype.validateReferences = function(doc) { } /** - * When canonicalization algorithm is non-exclusive, search for ancestor namespaces - * before validating references. + * Search for ancestor namespaces before validating references. */ if(Array.isArray(ref.transforms)){ - var hasNonExcC14nTransform = false; - for(var t in ref.transforms){ - if(!ref.transforms.hasOwnProperty(t)) continue; - - if(ref.transforms[t] === "http://www.w3.org/TR/2001/REC-xml-c14n-20010315" - || ref.transforms[t] === "http://www.w3.org/TR/2001/REC-xml-c14n-20010315#WithComments") - { - hasNonExcC14nTransform = true; - break; - } - } - - if(hasNonExcC14nTransform){ - ref.ancestorNamespaces = findAncestorNs(doc, elemXpath); - } + + ref.ancestorNamespaces = findAncestorNs(doc, elemXpath); } var c14nOptions = { inclusiveNamespacesPrefixList: ref.inclusiveNamespacesPrefixList, ancestorNamespaces: ref.ancestorNamespaces }; + + if (ref.inclusiveNamespacesPrefixList) { + var prefixList = ref.inclusiveNamespacesPrefixList instanceof Array ? ref.inclusiveNamespacesPrefixList : ref.inclusiveNamespacesPrefixList.split(' '); + prefixList.forEach(function (prefix) { + if (ref.ancestorNamespaces) { + ref.ancestorNamespaces.forEach(function (ancestorNamespace) { + if (prefix == ancestorNamespace.prefix) { + elem[0].setAttributeNS('http://www.w3.org/2000/xmlns/', 'xmlns:' + prefix, ancestorNamespace.namespaceURI); + } + }) + } + }) + } + var canonXml = this.getCanonXml(ref.transforms, elem[0], c14nOptions); var hash = this.findHashAlgorithm(ref.digestAlgorithm) var digest = hash.getHash(canonXml) - - if (!validateDigestValue(digest, ref.digestValue)) { - if (ref.inclusiveNamespacesPrefixList) { - // fallback: apply InclusiveNamespaces workaround (https://github.com/yaronn/xml-crypto/issues/72) - var prefixList = ref.inclusiveNamespacesPrefixList instanceof Array ? ref.inclusiveNamespacesPrefixList : ref.inclusiveNamespacesPrefixList.split(' '); - var supported_definitions = { - 'xs': 'http://www.w3.org/2001/XMLSchema', - 'xsi': 'http://www.w3.org/2001/XMLSchema-instance', - 'saml': 'urn:oasis:names:tc:SAML:2.0:assertion' - } - - prefixList.forEach(function (prefix) { - if (supported_definitions[prefix]) { - elem[0].setAttributeNS('http://www.w3.org/2000/xmlns/', 'xmlns:' + prefix, supported_definitions[prefix]); - } - }); - - canonXml = this.getCanonXml(ref.transforms, elem[0], { inclusiveNamespacesPrefixList: ref.inclusiveNamespacesPrefixList }); - digest = hash.getHash(canonXml); - if (digest === ref.digestValue) { - return true; - } - } - } if (!validateDigestValue(digest, ref.digestValue)) { this.validationErrors.push("invalid signature: for uri " + ref.uri + @@ -612,9 +614,12 @@ SignedXml.prototype.loadReference = function(ref) { transforms.push(utils.findAttr(trans, "Algorithm").value) } - var inclusiveNamespaces = xpath.select("//*[local-name(.)='InclusiveNamespaces']", transformsNode); + var inclusiveNamespaces = utils.findChilds(trans, "InclusiveNamespaces") if (inclusiveNamespaces.length > 0) { - inclusiveNamespacesPrefixList = inclusiveNamespaces[0].getAttribute('PrefixList'); + //Should really only be one prefix list, but maybe there's some circumstances where more than one to lets handle it + for (var i = 0; i Date: Mon, 18 Mar 2019 16:36:06 +0000 Subject: [PATCH 2/7] Add unit test for enveloped transformation with inclusivenamespaces --- test/signature-unit-tests.js | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/test/signature-unit-tests.js b/test/signature-unit-tests.js index 0a7dc343..4221fada 100644 --- a/test/signature-unit-tests.js +++ b/test/signature-unit-tests.js @@ -474,19 +474,20 @@ module.exports = { }, "correctly loads signature": function(test) { - passLoadSignature(test, "./test/static/valid_signature.xml"); - passLoadSignature(test, "./test/static/valid_signature.xml", true); - passLoadSignature(test, "./test/static/valid_signature_with_root_level_sig_namespace.xml"); + passLoadSignature(test, "./test/static/valid_signature.xml") + passLoadSignature(test, "./test/static/valid_signature.xml", true) + passLoadSignature(test, "./test/static/valid_signature_with_root_level_sig_namespace.xml") test.done() }, "verify valid signature": function(test) { passValidSignature(test, "./test/static/valid_signature.xml") - passValidSignature(test, "./test/static/valid_signature_with_lowercase_id_attribute.xml"); + passValidSignature(test, "./test/static/valid_signature_with_lowercase_id_attribute.xml") passValidSignature(test, "./test/static/valid_signature wsu.xml", "wssecurity") passValidSignature(test, "./test/static/valid_signature_with_reference_keyInfo.xml") passValidSignature(test, "./test/static/valid_signature_with_whitespace_in_digestvalue.xml") passValidSignature(test, "./test/static/valid_signature_utf8.xml") + passValidSignature(test, "./test/static/valid_signature_with_unused_prefixes.xml") test.done() }, @@ -639,8 +640,8 @@ function failInvalidSignature(test, file, mode) { function verifySignature(xml, mode) { var doc = new dom().parseFromString(xml) - var node = select("/*/*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", doc)[0] - + var node = select("//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", doc)[0] + var sig = new SignedXml(mode) sig.keyInfoProvider = new FileKeyInfo("./test/static/client_public.pem") sig.loadSignature(node) From ddb650755a64ab9cdfe99bb4f5a75ed2c14c3d2c Mon Sep 17 00:00:00 2001 From: pollardb Date: Mon, 18 Mar 2019 16:37:16 +0000 Subject: [PATCH 3/7] Add test case static file for SOAP-UI style message with transforms with inclusive namespaces --- test/static/valid_signature_with_unused_prefixes.xml | 1 + 1 file changed, 1 insertion(+) create mode 100644 test/static/valid_signature_with_unused_prefixes.xml diff --git a/test/static/valid_signature_with_unused_prefixes.xml b/test/static/valid_signature_with_unused_prefixes.xml new file mode 100644 index 00000000..d27a93fc --- /dev/null +++ b/test/static/valid_signature_with_unused_prefixes.xml @@ -0,0 +1 @@ +GAEFzMhRbXD8lZeTd5GwSqkB73Q=c2bqLRa0Q+/P7+vuXZHO8/iiac8=95WoCzf9J63UgdLCj/05PYaJIAw=pa04vRHbAdvfr6Re5wgx2qFQepE=YYNZUGNHN8uRGzMqPVC2n5XODP8=GvVowT6jH/4fExGYfFzDbzhSwTs=F8Lm5aUouA5FtxQ1krQ3unE9NFh0QSl+QscEyTcK3FrIpWCB195Z7cbmYa9RsiYMdr2wTHHmou/+wrjpk9pZFJq+b0tpHKCpfj6B302Rexb5f+cDpUjBB/NGb11qaUiM65keVIWzmYnHC0iCxsCaG3lwHMELNr7GxNun1U7LzzI=MIIBxDCCAW6gAwIBAgIQxUSXFzWJYYtOZnmmuOMKkjANBgkqhkiG9w0BAQQFADAWMRQwEgYDVQQDEwtSb290IEFnZW5jeTAeFw0wMzA3MDgxODQ3NTlaFw0zOTEyMzEyMzU5NTlaMB8xHTAbBgNVBAMTFFdTRTJRdWlja1N0YXJ0Q2xpZW50MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQC+L6aB9x928noY4+0QBsXnxkQE4quJl7c3PUPdVu7k9A02hRG481XIfWhrDY5i7OEB7KGW7qFJotLLeMec/UkKUwCgv3VvJrs2nE9xO3SSWIdNzADukYh+Cxt+FUU6tUkDeqg7dqwivOXhuOTRyOI3HqbWTbumaLdc8jufz2LhaQIDAQABo0swSTBHBgNVHQEEQDA+gBAS5AktBh0dTwCNYSHcFmRjoRgwFjEUMBIGA1UEAxMLUm9vdCBBZ2VuY3mCEAY3bACqAGSKEc+41KpcNfQwDQYJKoZIhvcNAQEEBQADQQAfIbnMPVYkNNfX1tG1F+qfLhHwJdfDUZuPyRPucWF5qkh6sSdWVBY5sT/txBnVJGziyO8DPYdu2fPMER8ajJfl2019-03-18T16:27:47.984Z2019-03-19T04:27:47.984Zhttp://www.example.com/testActionclientuuid:c519e597-0570-4d35-92e5-0df733a17cc1testtestMessage \ No newline at end of file From 08e4eff3d5a2e61c93d62f46faa80038bb6f23c2 Mon Sep 17 00:00:00 2001 From: pollardb Date: Thu, 21 Mar 2019 15:32:05 +0000 Subject: [PATCH 4/7] Refactor inclusive namespaces to happen in canonicalization code --- lib/exclusive-canonicalization.js | 30 ++++++++++++++++++++++ lib/signed-xml.js | 42 +------------------------------ 2 files changed, 31 insertions(+), 41 deletions(-) diff --git a/lib/exclusive-canonicalization.js b/lib/exclusive-canonicalization.js index 04fb3496..239fb364 100644 --- a/lib/exclusive-canonicalization.js +++ b/lib/exclusive-canonicalization.js @@ -199,6 +199,36 @@ ExclusiveCanonicalization.prototype.process = function(node, options) { var defaultNsForPrefix = options.defaultNsForPrefix || {}; if (!(inclusiveNamespacesPrefixList instanceof Array)) { inclusiveNamespacesPrefixList = inclusiveNamespacesPrefixList.split(' '); } + var ancestorNamespaces = options.ancestorNamespaces || []; + + /** + * Get the InclusiveNamespaces PrefixList + */ + var inclusiveNamespacesPrefixList = options.inclusiveNamespacesPrefixList || []; + var CanonicalizationMethod = utils.findChilds(node, "CanonicalizationMethod") + if (CanonicalizationMethod.length!=0) { + var inclusiveNamespaces = utils.findChilds(CanonicalizationMethod[0], "InclusiveNamespaces") + if (inclusiveNamespaces.length!=0) { + inclusiveNamespacesPrefixList = inclusiveNamespaces[0].getAttribute('PrefixList').split(" "); + } + } + + /** + * If you have a PrefixList then use it and the ancestors to add the necessary namespaces + */ + if (inclusiveNamespacesPrefixList) { + var prefixList = inclusiveNamespacesPrefixList instanceof Array ? inclusiveNamespacesPrefixList : inclusiveNamespacesPrefixList.split(' '); + prefixList.forEach(function (prefix) { + if (ancestorNamespaces) { + ancestorNamespaces.forEach(function (ancestorNamespace) { + if (prefix == ancestorNamespace.prefix) { + node.setAttributeNS('http://www.w3.org/2000/xmlns/', 'xmlns:' + prefix, ancestorNamespace.namespaceURI); + } + }) + } + }) + } + var res = this.processInner(node, [], defaultNs, defaultNsForPrefix, inclusiveNamespacesPrefixList); return res; }; diff --git a/lib/signed-xml.js b/lib/signed-xml.js index 64faf07f..ac348f8d 100644 --- a/lib/signed-xml.js +++ b/lib/signed-xml.js @@ -368,18 +368,6 @@ SignedXml.prototype.validateSignatureValue = function(doc) { throw new Error("When canonicalization method is non-exclusive, whole xml dom must be provided as an argument"); } } - - /** - * Get the InclusiveNamespaces PrefixList - */ - var inclusiveNamespacesPrefixList - var CanonicalizationMethod = utils.findChilds(signedInfo[0], "CanonicalizationMethod") - if (CanonicalizationMethod.length!=0) { - var inclusiveNamespaces = utils.findChilds(CanonicalizationMethod[0], "InclusiveNamespaces") - if (inclusiveNamespaces.length!=0) { - inclusiveNamespacesPrefixList = inclusiveNamespaces[0].getAttribute('PrefixList').split(" "); - } - } /** * Search for ancestor namespaces before validating signature. @@ -387,26 +375,11 @@ SignedXml.prototype.validateSignatureValue = function(doc) { var ancestorNamespaces = []; ancestorNamespaces = findAncestorNs(doc, "//*[local-name()='SignedInfo']"); - /** - * If you have a PrefixList then use it and the ancestors to add the necessary namespaces - */ - if (inclusiveNamespacesPrefixList) { - inclusiveNamespacesPrefixList.forEach(function(inclusiveNamespacesPrefix) { - ancestorNamespaces.forEach(function (ancestorNamespace) { - if (inclusiveNamespacesPrefix == ancestorNamespace.prefix) { - signedInfo[0].setAttributeNS('http://www.w3.org/2000/xmlns/', 'xmlns:' + ancestorNamespace.prefix, ancestorNamespace.namespaceURI); - } - }) - }) - } - var c14nOptions = { - ancestorNamespaces: ancestorNamespaces, - inclusiveNamespacesPrefixList: inclusiveNamespacesPrefixList + ancestorNamespaces: ancestorNamespaces }; var signedInfoCanon = this.getCanonXml([this.canonicalizationAlgorithm], signedInfo[0], c14nOptions) var signer = this.findSignatureAlgorithm(this.signatureAlgorithm) - var res = signer.verifySignature(signedInfoCanon, this.signingKey, this.signatureValue) if (!res) this.validationErrors.push("invalid signature: the signature value " + this.signatureValue + " is incorrect") @@ -486,19 +459,6 @@ SignedXml.prototype.validateReferences = function(doc) { inclusiveNamespacesPrefixList: ref.inclusiveNamespacesPrefixList, ancestorNamespaces: ref.ancestorNamespaces }; - - if (ref.inclusiveNamespacesPrefixList) { - var prefixList = ref.inclusiveNamespacesPrefixList instanceof Array ? ref.inclusiveNamespacesPrefixList : ref.inclusiveNamespacesPrefixList.split(' '); - prefixList.forEach(function (prefix) { - if (ref.ancestorNamespaces) { - ref.ancestorNamespaces.forEach(function (ancestorNamespace) { - if (prefix == ancestorNamespace.prefix) { - elem[0].setAttributeNS('http://www.w3.org/2000/xmlns/', 'xmlns:' + prefix, ancestorNamespace.namespaceURI); - } - }) - } - }) - } var canonXml = this.getCanonXml(ref.transforms, elem[0], c14nOptions); From 76a43f585fd1a4d7abf580bb26c627cf0c7db4f7 Mon Sep 17 00:00:00 2001 From: pollardb Date: Thu, 21 Mar 2019 17:39:40 +0000 Subject: [PATCH 5/7] Only use the CanonicalizationMethod/InclusiveNameSpace if one was not explicitly provided --- lib/exclusive-canonicalization.js | 43 ++++++++++++++++--------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/lib/exclusive-canonicalization.js b/lib/exclusive-canonicalization.js index 239fb364..7566908b 100644 --- a/lib/exclusive-canonicalization.js +++ b/lib/exclusive-canonicalization.js @@ -202,32 +202,33 @@ ExclusiveCanonicalization.prototype.process = function(node, options) { var ancestorNamespaces = options.ancestorNamespaces || []; /** - * Get the InclusiveNamespaces PrefixList - */ - var inclusiveNamespacesPrefixList = options.inclusiveNamespacesPrefixList || []; - var CanonicalizationMethod = utils.findChilds(node, "CanonicalizationMethod") - if (CanonicalizationMethod.length!=0) { - var inclusiveNamespaces = utils.findChilds(CanonicalizationMethod[0], "InclusiveNamespaces") - if (inclusiveNamespaces.length!=0) { - inclusiveNamespacesPrefixList = inclusiveNamespaces[0].getAttribute('PrefixList').split(" "); - } + * If the inclusiveNamespacesPrefixList has not been explicitly provided then look it up in CanonicalizationMethod/InclusiveNamespaces + */ + if (inclusiveNamespacesPrefixList.length == 0) { + var CanonicalizationMethod = utils.findChilds(node, "CanonicalizationMethod") + if (CanonicalizationMethod.length != 0) { + var inclusiveNamespaces = utils.findChilds(CanonicalizationMethod[0], "InclusiveNamespaces") + if (inclusiveNamespaces.length != 0) { + inclusiveNamespacesPrefixList = inclusiveNamespaces[0].getAttribute('PrefixList').split(" "); + } + } } /** * If you have a PrefixList then use it and the ancestors to add the necessary namespaces */ - if (inclusiveNamespacesPrefixList) { - var prefixList = inclusiveNamespacesPrefixList instanceof Array ? inclusiveNamespacesPrefixList : inclusiveNamespacesPrefixList.split(' '); - prefixList.forEach(function (prefix) { - if (ancestorNamespaces) { - ancestorNamespaces.forEach(function (ancestorNamespace) { - if (prefix == ancestorNamespace.prefix) { - node.setAttributeNS('http://www.w3.org/2000/xmlns/', 'xmlns:' + prefix, ancestorNamespace.namespaceURI); - } - }) - } - }) - } + if (inclusiveNamespacesPrefixList) { + var prefixList = inclusiveNamespacesPrefixList instanceof Array ? inclusiveNamespacesPrefixList : inclusiveNamespacesPrefixList.split(' '); + prefixList.forEach(function (prefix) { + if (ancestorNamespaces) { + ancestorNamespaces.forEach(function (ancestorNamespace) { + if (prefix == ancestorNamespace.prefix) { + node.setAttributeNS('http://www.w3.org/2000/xmlns/', 'xmlns:' + prefix, ancestorNamespace.namespaceURI); + } + }) + } + }) + } var res = this.processInner(node, [], defaultNs, defaultNsForPrefix, inclusiveNamespacesPrefixList); return res; From 1632e714a9af12782fdaaa43d831305e48244b5b Mon Sep 17 00:00:00 2001 From: LoneRifle Date: Sat, 23 Mar 2019 07:15:02 +0000 Subject: [PATCH 6/7] Update lib/signed-xml.js Co-Authored-By: bazzadp --- lib/signed-xml.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/signed-xml.js b/lib/signed-xml.js index ac348f8d..029f562c 100644 --- a/lib/signed-xml.js +++ b/lib/signed-xml.js @@ -452,7 +452,7 @@ SignedXml.prototype.validateReferences = function(doc) { */ if(Array.isArray(ref.transforms)){ - ref.ancestorNamespaces = findAncestorNs(doc, elemXpath); + ref.ancestorNamespaces = findAncestorNs(doc, elemXpath); } var c14nOptions = { From 9e1c6eae0d2d25a899e7e6afa26fffe4b20aecd6 Mon Sep 17 00:00:00 2001 From: pollardb Date: Sat, 23 Mar 2019 07:22:42 +0000 Subject: [PATCH 7/7] aFix logic for multiple PrefixList + fix some old whitespace issues --- lib/signed-xml.js | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/lib/signed-xml.js b/lib/signed-xml.js index 029f562c..014b98db 100644 --- a/lib/signed-xml.js +++ b/lib/signed-xml.js @@ -18,8 +18,8 @@ function FileKeyInfo(file) { this.file = file this.getKeyInfo = function(key, prefix) { - prefix = prefix || '' - prefix = prefix ? prefix + ':' : prefix + prefix = prefix || '' + prefix = prefix ? prefix + ':' : prefix return "<" + prefix + "X509Data>" } @@ -450,16 +450,15 @@ SignedXml.prototype.validateReferences = function(doc) { /** * Search for ancestor namespaces before validating references. */ - if(Array.isArray(ref.transforms)){ - - ref.ancestorNamespaces = findAncestorNs(doc, elemXpath); + if(Array.isArray(ref.transforms)){ + ref.ancestorNamespaces = findAncestorNs(doc, elemXpath); } var c14nOptions = { inclusiveNamespacesPrefixList: ref.inclusiveNamespacesPrefixList, ancestorNamespaces: ref.ancestorNamespaces }; - + var canonXml = this.getCanonXml(ref.transforms, elem[0], c14nOptions); var hash = this.findHashAlgorithm(ref.digestAlgorithm) @@ -578,7 +577,11 @@ SignedXml.prototype.loadReference = function(ref) { if (inclusiveNamespaces.length > 0) { //Should really only be one prefix list, but maybe there's some circumstances where more than one to lets handle it for (var i = 0; i