diff --git a/src/signed-xml.ts b/src/signed-xml.ts index 663d3d0e..c6a43625 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -427,12 +427,16 @@ export class SignedXml { return this.getCanonXml([this.canonicalizationAlgorithm], signedInfo[0], c14nOptions); } - private getCanonReferenceXml(doc: Document, ref: Reference, node: Node) { + private getCanonReferenceXml(_doc: Document, ref: Reference, node: Node) { /** - * Search for ancestor namespaces before canonicalization. + * Derive ancestor namespaces from the specific node being digested, not by + * re-running ref.xpath. findAncestorNs uses only the first XPath match, so + * when multiple references are created for the same xpath pattern (e.g. via + * addAllReferences), references beyond the first are digested with the wrong + * ancestor namespace scope. */ - if (Array.isArray(ref.transforms)) { - ref.ancestorNamespaces = utils.findAncestorNs(doc, ref.xpath, this.namespaceResolver); + if (isDomNode.isElementNode(node)) { + ref.ancestorNamespaces = utils.findAncestorNsForNode(node); } const c14nOptions = { @@ -819,10 +823,6 @@ export class SignedXml { throw new Error("digestAlgorithm is required"); } - if (!utils.isArrayHasLength(transforms)) { - throw new Error("transforms must contain at least one transform algorithm"); - } - this.references.push({ xpath, transforms, @@ -1155,32 +1155,36 @@ export class SignedXml { referenceElem.setAttribute("Type", ref.type); } - const transformsElem = signatureDoc.createElementNS( - signatureNamespace, - `${currentPrefix}Transforms`, - ); - - for (const trans of ref.transforms || []) { - const transform = this.findCanonicalizationAlgorithm(trans); - const transformElem = signatureDoc.createElementNS( + if (utils.isArrayHasLength(ref.transforms)) { + const transformsElem = signatureDoc.createElementNS( signatureNamespace, - `${currentPrefix}Transform`, + `${currentPrefix}Transforms`, ); - transformElem.setAttribute("Algorithm", transform.getAlgorithmName()); - if (utils.isArrayHasLength(ref.inclusiveNamespacesPrefixList)) { - const inclusiveNamespacesElem = signatureDoc.createElementNS( - transform.getAlgorithmName(), - "InclusiveNamespaces", + for (const trans of ref.transforms) { + const transform = this.findCanonicalizationAlgorithm(trans); + const transformElem = signatureDoc.createElementNS( + signatureNamespace, + `${currentPrefix}Transform`, ); - inclusiveNamespacesElem.setAttribute( - "PrefixList", - ref.inclusiveNamespacesPrefixList.join(" "), - ); - transformElem.appendChild(inclusiveNamespacesElem); + transformElem.setAttribute("Algorithm", transform.getAlgorithmName()); + + if (utils.isArrayHasLength(ref.inclusiveNamespacesPrefixList)) { + const inclusiveNamespacesElem = signatureDoc.createElementNS( + transform.getAlgorithmName(), + "InclusiveNamespaces", + ); + inclusiveNamespacesElem.setAttribute( + "PrefixList", + ref.inclusiveNamespacesPrefixList.join(" "), + ); + transformElem.appendChild(inclusiveNamespacesElem); + } + + transformsElem.appendChild(transformElem); } - transformsElem.appendChild(transformElem); + referenceElem.appendChild(transformsElem); } // Get the canonicalized XML @@ -1201,7 +1205,6 @@ export class SignedXml { ); digestValueElem.textContent = digestAlgorithm.getHash(canonXml); - referenceElem.appendChild(transformsElem); referenceElem.appendChild(digestMethodElem); referenceElem.appendChild(digestValueElem); @@ -1272,7 +1275,7 @@ export class SignedXml { const canonXml = node.cloneNode(true); // Deep clone let transformedXml: Node | string = canonXml; - transforms.forEach((transformName) => { + (transforms ?? []).forEach((transformName) => { if (isDomNode.isNodeLike(transformedXml)) { // If, after processing, `transformedNode` is a string, we can't do anymore transforms on it const transform = this.findCanonicalizationAlgorithm(transformName); @@ -1287,6 +1290,27 @@ export class SignedXml { //if only y is the node to sign then a string would be without the definition of the p namespace. probably xmldom toString() should have added it. }); + // When the transform chain produces a DOM node (including the no-transform + // case), apply C14N so that the digest is computed over a canonical byte + // sequence. This mirrors what loadReference does on the verification side: + // it appends C14N when the transform list is empty or ends with + // enveloped-signature, ensuring signing and verification agree on the bytes. + if (typeof transformedXml === "string") { + return transformedXml; + } + + // When there are no transforms, the XMLDSig processing model requires the + // node-set to be serialized via C14N before digesting. This mirrors what + // loadReference already does on the verification side (it appends C14N when + // the transform list is empty), so that signing and verification compute the + // same digest. + if (!utils.isArrayHasLength(transforms)) { + const c14n = this.findCanonicalizationAlgorithm( + "http://www.w3.org/TR/2001/REC-xml-c14n-20010315", + ); + return String(c14n.process(transformedXml, options)); + } + return transformedXml.toString(); } diff --git a/src/types.ts b/src/types.ts index 89c0b304..426b8b02 100644 --- a/src/types.ts +++ b/src/types.ts @@ -127,7 +127,9 @@ export interface Reference { xpath?: string; // An array of transforms to be applied to the data before signing. - transforms: ReadonlyArray; + // When absent or empty, no Transforms element is emitted and the referenced + // node is digested directly (after C14N, per the XMLDSig processing model). + transforms?: ReadonlyArray; // The algorithm used to calculate the digest value of the data. digestAlgorithm: HashAlgorithmType; diff --git a/src/utils.ts b/src/utils.ts index 466b252e..034a6329 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -236,6 +236,49 @@ function isElementSubset(docSubset: Node[]): docSubset is Element[] { return docSubset.every((node) => isDomNode.isElementNode(node)); } +/** Deduplicate and filter ancestor namespaces for a given subset element. */ +function buildAncestorNsForElement(element: Element): NamespacePrefix[] { + const ancestorNs = collectAncestorNamespaces(element); + const ancestorNsWithoutDuplicate: NamespacePrefix[] = []; + for (let i = 0; i < ancestorNs.length; i++) { + let notOnTheList = true; + for (const v in ancestorNsWithoutDuplicate) { + if (ancestorNsWithoutDuplicate[v].prefix === ancestorNs[i].prefix) { + notOnTheList = false; + break; + } + } + + if (notOnTheList) { + ancestorNsWithoutDuplicate.push(ancestorNs[i]); + } + } + + // Remove namespaces which are already declared in the subset with the same prefix + const returningNs: NamespacePrefix[] = []; + const subsetNsPrefix = findNSPrefix(element); + for (const ns of ancestorNsWithoutDuplicate) { + if (ns.prefix !== subsetNsPrefix) { + returningNs.push(ns); + } + } + + return returningNs; +} + +/** + * Extract ancestor namespaces for a specific element node. + * Prefer this over `findAncestorNs` when the target element is already known, + * since `findAncestorNs` re-executes an XPath and uses the first match — + * which is incorrect when multiple nodes are referenced individually. + * + * @param element - The element whose ancestor namespace declarations to collect + * @returns i.e. [{prefix: "saml", namespaceURI: "urn:oasis:names:tc:SAML:2.0:assertion"}] + */ +export function findAncestorNsForNode(element: Element): NamespacePrefix[] { + return buildAncestorNsForElement(element); +} + /** * Extract ancestor namespaces in order to import it to root of document subset * which is being canonicalized for non-exclusive c14n. @@ -264,33 +307,7 @@ export function findAncestorNs( throw new Error("Document subset must be list of elements"); } - // Remove duplicate on ancestor namespace - const ancestorNs = collectAncestorNamespaces(docSubset[0]); - const ancestorNsWithoutDuplicate: NamespacePrefix[] = []; - for (let i = 0; i < ancestorNs.length; i++) { - let notOnTheList = true; - for (const v in ancestorNsWithoutDuplicate) { - if (ancestorNsWithoutDuplicate[v].prefix === ancestorNs[i].prefix) { - notOnTheList = false; - break; - } - } - - if (notOnTheList) { - ancestorNsWithoutDuplicate.push(ancestorNs[i]); - } - } - - // Remove namespaces which are already declared in the subset with the same prefix - const returningNs: NamespacePrefix[] = []; - const subsetNsPrefix = findNSPrefix(docSubset[0]); - for (const ancestorNs of ancestorNsWithoutDuplicate) { - if (ancestorNs.prefix !== subsetNsPrefix) { - returningNs.push(ancestorNs); - } - } - - return returningNs; + return buildAncestorNsForElement(docSubset[0]); } export function validateDigestValue(digest, expectedDigest) { diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index c0dcf136..290ce685 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -908,8 +908,7 @@ describe("Signature unit tests", function () { ref.uri, `wrong uri for index ${i}. expected: ${expectedUri} actual: ${ref.uri}`, ).to.equal(expectedUri); - expect(ref.transforms.length).to.equal(1); - expect(ref.transforms[0]).to.equal("http://www.w3.org/2001/10/xml-exc-c14n#"); + expect(ref.transforms).to.deep.equal(["http://www.w3.org/2001/10/xml-exc-c14n#"]); expect(ref.digestValue).to.equal(digests[i]); expect(ref.digestAlgorithm).to.equal("http://www.w3.org/2000/09/xmldsig#sha1"); } @@ -1074,6 +1073,108 @@ describe("Signature unit tests", function () { expect(URI.value, `uri should be empty but instead was ${URI.value}`).to.equal(""); }); + for (const { label, transforms } of [ + { label: "omitted transforms property", transforms: undefined }, + { label: "empty transforms array", transforms: [] as string[] }, + ]) { + it(`omits Transforms element when no transforms are specified (${label})`, function () { + const xml = ""; + const sig = new SignedXml(); + sig.privateKey = fs.readFileSync("./test/static/client.pem"); + sig.addReference({ + xpath: "//*[local-name(.)='x']", + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + uri: "#ref1", + digestValue: "", + inclusiveNamespacesPrefixList: [], + isEmptyUri: false, + ...(transforms !== undefined ? { transforms } : {}), + }); + sig.canonicalizationAlgorithm = "http://www.w3.org/TR/2001/REC-xml-c14n-20010315"; + sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"; + sig.computeSignature(xml); + const signedXml = sig.getSignedXml(); + const doc = new xmldom.DOMParser().parseFromString(signedXml); + const transformNodes = xpath.select("//*[local-name(.)='Transforms']", doc); + expect(transformNodes, "Transforms element should be absent when no transforms specified").to + .have.length(0); + }); + } + + it("signs and verifies correctly with no transforms (round-trip)", function () { + const xml = ""; + const sig = new SignedXml(); + sig.privateKey = fs.readFileSync("./test/static/client.pem"); + sig.publicCert = fs.readFileSync("./test/static/client_public.pem"); + sig.addReference({ + xpath: "//*[local-name(.)='x']", + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + uri: "#ref1", + digestValue: "", + inclusiveNamespacesPrefixList: [], + isEmptyUri: false, + }); + sig.canonicalizationAlgorithm = "http://www.w3.org/TR/2001/REC-xml-c14n-20010315"; + sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"; + sig.computeSignature(xml); + const signedXml = sig.getSignedXml(); + + const doc = new xmldom.DOMParser().parseFromString(signedXml); + const sigNode = xpath.select1( + "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", + doc, + ); + isDomNode.assertIsNodeLike(sigNode); + + const verifySig = new SignedXml(); + verifySig.publicCert = fs.readFileSync("./test/static/client_public.pem"); + verifySig.loadSignature(sigNode); + const result = verifySig.checkSignature(signedXml); + expect(result, "expected signature to verify successfully").to.be.true; + }); + + it("correctly canonicalizes no-transform references under different ancestor namespace scopes", function () { + // Two elements live under different namespace scopes. Without the + // fix, findAncestorNs(doc, ref.xpath) always uses the first XPath match + // (item1's scope), so item2 is digested with the wrong ancestor namespaces + // and verification fails. + const xml = + "" + + "
one
" + + "
two
" + + "
"; + const sig = new SignedXml(); + sig.privateKey = fs.readFileSync("./test/static/client.pem"); + sig.publicCert = fs.readFileSync("./test/static/client_public.pem"); + for (const id of ["item1", "item2"]) { + sig.addReference({ + xpath: `//*[@Id='${id}']`, + digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256", + uri: `#${id}`, + digestValue: "", + inclusiveNamespacesPrefixList: [], + isEmptyUri: false, + }); + } + sig.canonicalizationAlgorithm = "http://www.w3.org/TR/2001/REC-xml-c14n-20010315"; + sig.signatureAlgorithm = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"; + sig.computeSignature(xml); + const signedXml = sig.getSignedXml(); + + const doc = new xmldom.DOMParser().parseFromString(signedXml); + const sigNode = xpath.select1( + "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", + doc, + ); + isDomNode.assertIsNodeLike(sigNode); + + const verifySig = new SignedXml(); + verifySig.publicCert = fs.readFileSync("./test/static/client_public.pem"); + verifySig.loadSignature(sigNode); + const result = verifySig.checkSignature(signedXml); + expect(result, "expected signature to verify successfully").to.be.true; + }); + it("signer appends signature to a non-existing reference node", function () { const xml = "xml-cryptogithub"; const sig = new SignedXml();