From f9b36822a075f95b7430f645a7ee393a128db152 Mon Sep 17 00:00:00 2001 From: parallels999 <109294935+parallels999@users.noreply.github.com> Date: Fri, 23 Aug 2024 21:02:58 -0500 Subject: [PATCH 1/9] Exports C14nCanonicalization, ExclusiveCanonicalization (#471) --- src/index.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/index.ts b/src/index.ts index 3e4a8a4c..0e72578a 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,3 +1,8 @@ export { SignedXml } from "./signed-xml"; +export { C14nCanonicalization, C14nCanonicalizationWithComments } from "./c14n-canonicalization"; +export { + ExclusiveCanonicalization, + ExclusiveCanonicalizationWithComments, +} from "./exclusive-canonicalization"; export * from "./utils"; export * from "./types"; From c2a7d7ab3be82c2ce8aa4890fa194b881e9b7944 Mon Sep 17 00:00:00 2001 From: SecureSAML Date: Wed, 12 Mar 2025 21:40:51 -0400 Subject: [PATCH 2/9] Ensure that they agree on loading References from canon XML --- src/signed-xml.ts | 69 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 64 insertions(+), 5 deletions(-) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index e5d80af7..70d27199 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -25,6 +25,7 @@ import * as signatureAlgorithms from "./signature-algorithms"; import * as crypto from "crypto"; import * as isDomNode from "@xmldom/is-dom-node"; + export class SignedXml { idMode?: "wssecurity"; idAttributes: string[]; @@ -87,6 +88,8 @@ export class SignedXml { "http://www.w3.org/2000/09/xmldsig#enveloped-signature": envelopedSignatures.EnvelopedSignature, }; + // TODO: In V7.x we may consider deprecating sha1 + /** * To add a new hash algorithm create a new class that implements the {@link HashAlgorithm} interface, and register it here. More info: {@link https://github.com/node-saml/xml-crypto#customizing-algorithms|Customizing Algorithms} */ @@ -96,6 +99,8 @@ export class SignedXml { "http://www.w3.org/2001/04/xmlenc#sha512": hashAlgorithms.Sha512, }; + // TODO: In V7.x we may consider deprecating sha1 + /** * To add a new signature algorithm create a new class that implements the {@link SignatureAlgorithm} interface, and register it here. More info: {@link https://github.com/node-saml/xml-crypto#customizing-algorithms|Customizing Algorithms} */ @@ -252,6 +257,45 @@ export class SignedXml { this.signedXml = xml; const doc = new xmldom.DOMParser().parseFromString(xml); + // mutate the this.references to our new list after we have the document + // Ideally we should have been able to load the Signature and it's references in one go + // However, in the .loadSignature() method we don't necessarily have the underlying document + // It is only provided here. And we need the underlying document if we want to keep the inclusive namespaces + + // signedInfoCanon is unsigned here, we will show that it is signed in later step (B) + const signedInfoCanon = this.getCanonSignedInfoXml(doc) + + // type checking + // ensure that it is not empty + if (!(signedInfoCanon)) { + throw new Error(`Canonical signed info not be empty, ${signedInfoCanon}`); + } + + + // now we know that only the "signedInfoCanon" is signed by key + // parse it into a signedInfo node + const parsedSignedInfo = new xmldom.DOMParser().parseFromString(signedInfoCanon, "text/xml"); + + const signedInfoDoc = parsedSignedInfo.documentElement; + if (!signedInfoDoc) { + throw new Error('Could not parse signedInfoCanon into a document') + } + + // reset the references. Previous references loaded cannot be trusted + // only references from our new re-parsed signedInfo node + this.references = []; + + const references = xpath.select( + "/*[local-name()='SignedInfo']/*[local-name()='Reference']", + signedInfoDoc + ); + if (!utils.isArrayHasLength(references)) { + throw new Error("could not find any Reference elements"); + } + + for (const reference of references) { + this.loadReference(reference); + } if (!this.getReferences().every((ref) => this.validateReference(ref, doc))) { if (callback) { @@ -262,12 +306,15 @@ export class SignedXml { return false; } - const signedInfoCanon = this.getCanonSignedInfoXml(doc); + // (Stage B authentication step, show that the signedInfoCanon is signed) + + // first find the key & signature algorithm, this should match const signer = this.findSignatureAlgorithm(this.signatureAlgorithm); const key = this.getCertFromKeyInfo(this.keyInfo) || this.publicCert || this.privateKey; if (key == null) { throw new Error("KeyInfo or publicCert or privateKey is required to validate signature"); } + if (callback) { signer.verifySignature(signedInfoCanon, key, this.signatureValue, callback); } else { @@ -522,11 +569,23 @@ export class SignedXml { this.signatureAlgorithm = signatureAlgorithm.value as SignatureAlgorithmType; } + const signedInfoNodes = utils.findChildren(this.signatureNode, "SignedInfo"); + if (!utils.isArrayHasLength(signedInfoNodes)) { + throw new Error('no signed info node found') + } + + // try to operate over the c14n version of signedInfo (however still not the safe as previously) + // this forces the initial .getReferences() API call to always return references that are loaded under the canonical signede info + // in the case that the client access the .references **before** signature verification + + const tempCanon = this.getCanonXml(["http://www.w3.org/2001/10/xml-exc-c14n#"], signedInfoNodes[0]); + const s = new xmldom.DOMParser().parseFromString(tempCanon, "text/xml"); + const signedInfoDoc = s.documentElement; + + this.references = []; - const references = xpath.select( - ".//*[local-name(.)='SignedInfo']/*[local-name(.)='Reference']", - signatureNode, - ); + const references = utils.findChildren(signedInfoDoc, "Reference") + if (!utils.isArrayHasLength(references)) { throw new Error("could not find any Reference elements"); } From b901de880231a1ea4a222464ef22ef898ad63cbb Mon Sep 17 00:00:00 2001 From: SecureSAML Date: Wed, 12 Mar 2025 21:51:22 -0400 Subject: [PATCH 3/9] Fix specific exploits --- src/signed-xml.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index 70d27199..37025f61 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -631,11 +631,11 @@ export class SignedXml { if (nodes.length === 0) { throw new Error(`could not find DigestValue node in reference ${refNode.toString()}`); } - const firstChild = nodes[0].firstChild; - if (!firstChild || !("data" in firstChild)) { + const firstChild = nodes[0]; + if (!firstChild) { throw new Error(`could not find the value of DigestValue in ${nodes[0].toString()}`); } - const digestValue = firstChild.data; + const digestValue = firstChild.textContent; // use textContent which SHOULD be part of signing input const transforms: string[] = []; let inclusiveNamespacesPrefixList: string[] = []; @@ -685,11 +685,12 @@ export class SignedXml { ) { transforms.push("http://www.w3.org/TR/2001/REC-xml-c14n-20010315"); } + const refUri = isDomNode.isElementNode(refNode) ? (refNode.getAttribute("URI") || undefined) : undefined; this.addReference({ transforms, digestAlgorithm: digestAlgo, - uri: isDomNode.isElementNode(refNode) ? utils.findAttr(refNode, "URI")?.value : undefined, + uri: refUri, digestValue, inclusiveNamespacesPrefixList, isEmptyUri: false, From ea1ad2418f89523656d7bb2fc1b8cd80673b3bad Mon Sep 17 00:00:00 2001 From: SecureSAML Date: Wed, 12 Mar 2025 21:54:38 -0400 Subject: [PATCH 4/9] Clarify Comments --- src/signed-xml.ts | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index 37025f61..03c6203d 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -257,10 +257,9 @@ export class SignedXml { this.signedXml = xml; const doc = new xmldom.DOMParser().parseFromString(xml); - // mutate the this.references to our new list after we have the document - // Ideally we should have been able to load the Signature and it's references in one go - // However, in the .loadSignature() method we don't necessarily have the underlying document - // It is only provided here. And we need the underlying document if we want to keep the inclusive namespaces + // reset the references. Previous references loaded cannot be trusted + // only references from our new re-parsed signedInfo node + this.references = []; // signedInfoCanon is unsigned here, we will show that it is signed in later step (B) const signedInfoCanon = this.getCanonSignedInfoXml(doc) @@ -271,9 +270,7 @@ export class SignedXml { throw new Error(`Canonical signed info not be empty, ${signedInfoCanon}`); } - - // now we know that only the "signedInfoCanon" is signed by key - // parse it into a signedInfo node + // unsigned, verify later to keep with consistent callback behavior const parsedSignedInfo = new xmldom.DOMParser().parseFromString(signedInfoCanon, "text/xml"); const signedInfoDoc = parsedSignedInfo.documentElement; @@ -281,9 +278,6 @@ export class SignedXml { throw new Error('Could not parse signedInfoCanon into a document') } - // reset the references. Previous references loaded cannot be trusted - // only references from our new re-parsed signedInfo node - this.references = []; const references = xpath.select( "/*[local-name()='SignedInfo']/*[local-name()='Reference']", @@ -293,6 +287,12 @@ export class SignedXml { throw new Error("could not find any Reference elements"); } + // mutate the this.references to our new list after we have the document + // Ideally we should have been able to load the Signature and it's references in one go + // However, in the .loadSignature() method we don't necessarily have the underlying document + // It is only provided here. And we need the underlying document if we want to keep the inclusive namespaces + + for (const reference of references) { this.loadReference(reference); } From d2c9f8c13ed2b62bdcade04e367422e493855e9c Mon Sep 17 00:00:00 2001 From: SecureSAML Date: Wed, 12 Mar 2025 22:07:38 -0400 Subject: [PATCH 5/9] Document changelog to signedReferences API --- CHANGELOG.md | 9 ++++++++ README.md | 61 +++++++++++++++++++++++++++------------------------- 2 files changed, 41 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f83d6c06..452ae888 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,14 @@ # Changelog +## 6.1.0 (...) + +# Changelog + +- Introduced new .signedReferences property of signature to help prevent signature wrapping attacks. +- After calling .checkSignature() with your public certificate, obtain .signedReferences to use. Array of signed strings by the certificate + + + ## 6.0.0 (2024-01-26) ### 💣 Major Changes diff --git a/README.md b/README.md index 2d8d82bc..7af2a49d 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,18 @@ # xml-crypto + +# Upgrading + +The `.getReferences() AND the .references` API is deprecated. +Please do not attempt to access it. The content in there should be treated as unsigned. + +Instead, we strongly encourage users to migrate to the .signedReferences API. See the `Verifying XML document` section +We understand that this may take a lot of efforts to migrate, feel free to ask for help. +This will help prevent future XML signature wrapping attacks in the future. + +`` + + ![Build](https://github.com/node-saml/xml-crypto/actions/workflows/ci.yml/badge.svg) [![Gitpod Ready-to-Code](https://img.shields.io/badge/Gitpod-Ready--to--Code-blue?logo=gitpod)](https://gitpod.io/from-referrer/) @@ -159,8 +172,13 @@ var select = require("xml-crypto").xpath, fs = require("fs"); var xml = fs.readFileSync("signed.xml").toString(); + var doc = new dom().parseFromString(xml); +// DO NOT attempt to parse whatever data object you have here +// i.e. BAD: parseAssertion(doc), +// good: see below + var signature = select( doc, "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", @@ -172,44 +190,29 @@ try { } catch (ex) { console.log(ex); } + + ``` In order to protect from some attacks we must check the content we want to use is the one that has been signed: ```javascript -// Roll your own -const elem = xpath.select("/xpath_to_interesting_element", doc); -const uri = sig.getReferences()[0].uri; // might not be 0; it depends on the document -const id = uri[0] === "#" ? uri.substring(1) : uri; -if ( - elem.getAttribute("ID") != id && - elem.getAttribute("Id") != id && - elem.getAttribute("id") != id -) { - throw new Error("The interesting element was not the one verified by the signature"); +if (!res) { + throw "Invalid Signature" } +// good: The XML Signature has been verified, meaning some subset of XML is verified. +var signedBytes = sig.signedReferences; -// Get the validated element directly from a reference -const elem = sig.references[0].getValidatedElement(); // might not be 0; it depends on the document -const matchingReference = xpath.select1("/xpath_to_interesting_element", elem); -if (!isDomNode.isNodeLike(matchingReference)) { - throw new Error("The interesting element was not the one verified by the signature"); -} +var authenticatedDoc = new dom().parseFromString(signedBytes[0]); // take the first of the signed references +// load SAML or whatever from now +// obtain the assertion XML from here +// use only authenticated data +let signedAssertionNode = extractAssertion(authenticatedDoc); +let parsedAssertion = parseAssertion(signedAssertionNode) +return parsedAssertion; // now return the client, the signed Assertion -// Use the built-in method -const elem = xpath.select1("/xpath_to_interesting_element", doc); -try { - const matchingReference = sig.validateElementAgainstReferences(elem, doc); -} catch { - throw new Error("The interesting element was not the one verified by the signature"); -} -// Use the built-in method with a an xpath expression -try { - const matchingReference = sig.validateReferenceWithXPath("/xpath_to_interesting_element", doc); -} catch { - throw new Error("The interesting element was not the one verified by the signature"); -} +// BAD example: DO not use the .getReferences() API. ``` Note: From b0192abe7c3c8677feab0fad54d1ba25c210e17d Mon Sep 17 00:00:00 2001 From: SecureSAML Date: Wed, 12 Mar 2025 22:36:35 -0400 Subject: [PATCH 6/9] Adds signedReferences API --- src/signed-xml.ts | 41 ++++++++++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/src/signed-xml.ts b/src/signed-xml.ts index 03c6203d..8fc85b35 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -117,6 +117,7 @@ export class SignedXml { }; static noop = () => null; + public signedReferences: string[] = []; /** * The SignedXml constructor provides an abstraction for sign and verify xml documents. The object is constructed using @@ -159,6 +160,9 @@ export class SignedXml { this.CanonicalizationAlgorithms; this.HashAlgorithms; this.SignatureAlgorithms; + // this populates only after verifying the signature + // array of bytes that are cryptographically authenticated + this.signedReferences = []; // TODO: should we rename this to something better. } /** @@ -270,7 +274,8 @@ export class SignedXml { throw new Error(`Canonical signed info not be empty, ${signedInfoCanon}`); } - // unsigned, verify later to keep with consistent callback behavior + // unfortunately I've decided to keep using the unverified signedInfoCanon first for now. + // I don't want to modify callback behavior too much const parsedSignedInfo = new xmldom.DOMParser().parseFromString(signedInfoCanon, "text/xml"); const signedInfoDoc = parsedSignedInfo.documentElement; @@ -315,18 +320,35 @@ export class SignedXml { throw new Error("KeyInfo or publicCert or privateKey is required to validate signature"); } - if (callback) { - signer.verifySignature(signedInfoCanon, key, this.signatureValue, callback); + + // let's clear the callback up a little bit, so we can access it's results, + // and decide whether to reset signature value or not + const sigRes = signer.verifySignature(signedInfoCanon, key, this.signatureValue); + // true case + if (sigRes === true) { + if (callback) { + callback(null, true); + } else { + return true; + } + } else { - const verified = signer.verifySignature(signedInfoCanon, key, this.signatureValue); + // false case + // reset the signedReferences back to empty array + // I would have preferred to start by verifying the signedInfoCanon first, if that's OK + // but that may cause some breaking changes? + this.signedReferences = []; - if (verified === false) { + if (callback) { + callback(new Error( + `invalid signature: the signature value ${this.signatureValue} is incorrect`, + )); + return; // return early + } else { throw new Error( `invalid signature: the signature value ${this.signatureValue} is incorrect`, ); } - - return true; } } @@ -521,6 +543,11 @@ export class SignedXml { return false; } + // This step can only be done after we have verified the signedInfo + // we verified that they have same hash + // so, the canonXml and only the canonXml can be trusted + // append this to signedReferences + this.signedReferences.push(canonXml); return true; } From 4a2a48ceba7a05897f33648d38ceb7c530e49b4d Mon Sep 17 00:00:00 2001 From: SecureSAML Date: Tue, 8 Apr 2025 07:14:01 -0400 Subject: [PATCH 7/9] V7 prototype --- src/XMLVerifier.ts | 186 +++++++++++++++++++++++++++++++++++++++++++++ src/signed-xml.ts | 78 ++----------------- src/types.ts | 2 + 3 files changed, 193 insertions(+), 73 deletions(-) create mode 100644 src/XMLVerifier.ts diff --git a/src/XMLVerifier.ts b/src/XMLVerifier.ts new file mode 100644 index 00000000..f1df92f6 --- /dev/null +++ b/src/XMLVerifier.ts @@ -0,0 +1,186 @@ +import type { + Reference, + SignedXmlOptions, +} from "./types"; + +import * as xpath from "xpath"; +import * as xmldom from "@xmldom/xmldom"; +import * as utils from "./utils"; +import * as crypto from "crypto"; +import { SignedXml } from "./signed-xml"; + + +// used to verify XML Signatures class +class XMLVerifier { + // xmlSignatureOptions, XML signature options, i.e. IdMode + // keyInfoProvider, function: finds a trusted given, given optionally the keyInfo + + private signatureOptions: SignedXmlOptions; + private signedXMLInstance: SignedXml + // private keyInfoProvider; + // this is designed to throw error, but maybe we should do boolean isntead + private referencePrevalidator: (ref: Reference) => void; + + constructor(xmlSignatureOptions: SignedXmlOptions = {}, referencePrevalidator: (ref: Reference) => void) { + this.signatureOptions = xmlSignatureOptions; + this.signedXMLInstance = new SignedXml(xmlSignatureOptions); + // this.keyInfoProvider = keyInfoProvider; + this.referencePrevalidator = referencePrevalidator; + } + + getAuthenticatedReferencesWithCallback(signature: Node, contextXml: string, keyInfoProvider: (keyInfo: Node) => crypto.KeyObject, callback : (err: unknown, authenticatedReferences: string[]) => void) { + try { + callback(null, this.getAuthenticatedReferences(signature, contextXml, keyInfoProvider)); + } catch (e) { + callback(e, []); + } + } + + /** + * Validates the signature of the provided XML document synchronously using the configured key info provider. + * + * @param xml The XML document containing the signature to be validated. + * @returns an array of utf-8 encoded bytes which are authenticated by the KeyInfoProvider + * Note: This function does NOT return a boolean value. + * Please DO NOT rely on the length of the array to make security decisions + * Only use the **contents** of the returned array to make security decisions. + * @throws Error if no key info resolver is provided. + */ + getAuthenticatedReferences(signature: Node, contextXml: string, keyInfoProvider: (keyInfo: Node) => crypto.KeyObject): string[] { + // I: authenticate the keying material + const signer = this.findSignatureAlgorithm(this.signatureAlgorithm); + // Now it returns a crypto.KeyObject, forcing user to distinguish between which type to use + const key = this.getCertFromKeyInfo(this.keyInfo); + if (key == null) { + throw new Error("KeyInfo or publicCert or privateKey is required to validate signature"); + } + + + // II: authenticate signedInfo utf-8 encoded canonical XML string. + const doc = new xmldom.DOMParser().parseFromString(contextXml); + + const unverifiedSignedInfoCanon = this.getCanonSignedInfoXml(doc); + if (!unverifiedSignedInfoCanon) { + throw new Error("Canonical signed info cannot be empty"); + } + + // let's clear the callback up a little bit, so we can access it's results, + // and decide whether to reset signature value or not + const sigRes = signer.verifySignature(unverifiedSignedInfoCanon, key, this.signatureValue); + // true case + if (sigRes === true) { + // continue on + } else { + throw new Error(`invalid signature: the signature value ${this.signatureValue} is incorrect`) + } + + // unverifiedSignedInfoCanon is verified + + // unsigned, verify later to keep with consistent callback behavior + const signedInfo = new xmldom.DOMParser().parseFromString( + unverifiedSignedInfoCanon, + "text/xml", + ); + + const unverifiedSignedInfoDoc = signedInfo.documentElement; + if (!unverifiedSignedInfoDoc) { + throw new Error("Could not parse unverifiedSignedInfoCanon into a document"); + } + + const references = utils.findChildren(unverifiedSignedInfoDoc, "Reference"); + if (!utils.isArrayHasLength(references)) { + throw new Error("could not find any Reference elements"); + } + + // load each reference Node + const unmarshalledReference = references.map((r) => this.loadReferenceNode(r)); + + // now authenticate each Reference i.e. verify the Digest Value + // map & return the utf-8 canon XML from each Reference i.e. the same digest input + return unmarshalledReference.map((refObj) => this.getVerifiedBytes(refObj, doc)); + } + + // returns a Reference object + private loadReferenceNode(ref: Node): Reference { + + return ref; // TODO + } + + + + // processes a Reference node to get the authenticated bytes + private getVerifiedBytes(ref: Reference, doc: Document): string { + + + const uri = ref.uri?.[0] === "#" ? ref.uri.substring(1) : ref.uri; + let elem: xpath.SelectSingleReturnType = null; + for (const attr of this.idAttributes) { + const tmp_elemXpath = `//*[@*[local-name(.)='${attr}']='${uri}']`; + const tmp_elem = xpath.select(tmp_elemXpath, doc); + if (utils.isArrayHasLength(tmp_elem)) { + num_elements_for_id += tmp_elem.length; + + if (num_elements_for_id > 1) { + throw new Error( + "Cannot validate a document which contains multiple elements with the " + + "same value for the ID / Id / Id attributes, in order to prevent " + + "signature wrapping attack.", + ); + } + + elem = tmp_elem[0]; + } + } + // TODO, fix private issues? + const canonXml = this.signedXMLInstance.getCanonReferenceXml(doc, ref, elem); + const hash = this.signedXMLInstance.findHashAlgorithm(ref.digestAlgorithm); + const digest = hash.getHash(canonXml); + + if (!utils.validateDigestValue(digest, ref.digestValue)) { + throw new Error(`invalid signature: for uri ${ref.uri} calculated digest is ${digest} but the xml to validate supplies digest ${ref.digestValue}`) + } + return canonXml; + } + + + + // TODO maybe prevalidate a reference. Ideally this should be handled at the processReference stage + // but this would help to abstract the function away for SAML. + private preValidateReference(ref: Reference, contextDoc: Document): void { + // assert that there are only 5 nodes. + const uri = ref.uri; + // spurious pre-verifications + if (uri === "") { + elem = xpath.select1("//*", doc); + } else if (uri?.indexOf("'") !== -1) { + // xpath injection + throw new Error("Cannot validate a uri with quotes inside it"); + } else { + let num_elements_for_id = 0; + for (const attr of this.idAttributes) { + const tmp_elemXpath = `//*[@*[local-name(.)='${attr}']='${uri}']`; + const tmp_elem = xpath.select(tmp_elemXpath, doc); + if (utils.isArrayHasLength(tmp_elem)) { + num_elements_for_id += tmp_elem.length; + + if (num_elements_for_id > 1) { + throw new Error( + "Cannot validate a document which contains multiple elements with the " + + "same value for the ID / Id / Id attributes, in order to prevent " + + "signature wrapping attack.", + ); + } + + elem = tmp_elem[0]; + ref.xpath = tmp_elemXpath; + } + } + } + + // ref + if (ref.transforms.length >= 5) { + throw new Error('...') + } + } + +} \ No newline at end of file diff --git a/src/signed-xml.ts b/src/signed-xml.ts index 5ef7f2e2..8615b38e 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -25,6 +25,8 @@ import * as signatureAlgorithms from "./signature-algorithms"; import * as crypto from "crypto"; import * as isDomNode from "@xmldom/is-dom-node"; +// configuration class for Signing/Verifying XML. +// We extract relevant logic into a new XMLVerifier class export class SignedXml { idMode?: "wssecurity"; @@ -238,10 +240,9 @@ export class SignedXml { * Validates the signature of the provided XML document synchronously using the configured key info provider. * * @param xml The XML document containing the signature to be validated. - * @returns `true` if the signature is valid * @throws Error if no key info resolver is provided. */ - checkSignature(xml: string): boolean; + _checkSignature(xml: string): string[]; /** * Validates the signature of the provided XML document synchronously using the configured key info provider. * @@ -249,8 +250,8 @@ export class SignedXml { * @param callback Callback function to handle the validation result asynchronously. * @throws Error if the last parameter is provided and is not a function, or if no key info resolver is provided. */ - checkSignature(xml: string, callback: (error: Error | null, isValid?: boolean) => void): void; - checkSignature( + _checkSignature(xml: string, callback: (error: Error | null, isValid?: boolean) => void): void; + _checkSignature( xml: string, callback?: (error: Error | null, isValid?: boolean) => void, ): unknown { @@ -495,75 +496,6 @@ export class SignedXml { throw new Error("No references passed validation"); } - private validateReference(ref: Reference, doc: Document) { - const uri = ref.uri?.[0] === "#" ? ref.uri.substring(1) : ref.uri; - let elem: xpath.SelectSingleReturnType = null; - - if (uri === "") { - elem = xpath.select1("//*", doc); - } else if (uri?.indexOf("'") !== -1) { - // xpath injection - throw new Error("Cannot validate a uri with quotes inside it"); - } else { - let num_elements_for_id = 0; - for (const attr of this.idAttributes) { - const tmp_elemXpath = `//*[@*[local-name(.)='${attr}']='${uri}']`; - const tmp_elem = xpath.select(tmp_elemXpath, doc); - if (utils.isArrayHasLength(tmp_elem)) { - num_elements_for_id += tmp_elem.length; - - if (num_elements_for_id > 1) { - throw new Error( - "Cannot validate a document which contains multiple elements with the " + - "same value for the ID / Id / Id attributes, in order to prevent " + - "signature wrapping attack.", - ); - } - - elem = tmp_elem[0]; - ref.xpath = tmp_elemXpath; - } - } - } - - ref.getValidatedNode = (xpathSelector?: string) => { - xpathSelector = xpathSelector || ref.xpath; - if (typeof xpathSelector !== "string" || ref.validationError != null) { - return null; - } - const selectedValue = xpath.select1(xpathSelector, doc); - return isDomNode.isNodeLike(selectedValue) ? selectedValue : null; - }; - - if (!isDomNode.isNodeLike(elem)) { - const validationError = new Error( - `invalid signature: the signature references an element with uri ${ref.uri} but could not find such element in the xml`, - ); - ref.validationError = validationError; - return false; - } - - const canonXml = this.getCanonReferenceXml(doc, ref, elem); - const hash = this.findHashAlgorithm(ref.digestAlgorithm); - const digest = hash.getHash(canonXml); - - if (!utils.validateDigestValue(digest, ref.digestValue)) { - const validationError = new Error( - `invalid signature: for uri ${ref.uri} calculated digest is ${digest} but the xml to validate supplies digest ${ref.digestValue}`, - ); - ref.validationError = validationError; - - return false; - } - // This step can only be done after we have verified the signedInfo - // we verified that they have same hash - // so, the canonXml and only the canonXml can be trusted - // append this to signedReferences - this.signedReferences.push(canonXml); - - return true; - } - findSignatures(doc: Node): Node[] { const nodes = xpath.select( "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", diff --git a/src/types.ts b/src/types.ts index 090c944e..e6b06176 100644 --- a/src/types.ts +++ b/src/types.ts @@ -104,6 +104,8 @@ export interface ComputeSignatureOptions { /** * Represents a reference node for XML digital signature. + * + * Should be internal only */ export interface Reference { // The XPath expression that selects the data to be signed. From 055c5dd1bd210af76a8665aa92b4a3115fe90aa0 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Wed, 9 Apr 2025 08:06:57 -0500 Subject: [PATCH 8/9] CVE-2025-29774 and CVE-2025-29775 (#494) --- src/signed-xml.ts | 116 ++++++++++++++++-- test/saml-response-tests.spec.ts | 74 +++++++++++ test/static/invalid_saml_no_signed_info.xml | 9 ++ .../saml_multiple_signed_info_nodes.xml | 1 + test/static/saml_wrapped_signed_info_node.xml | 1 + .../static/valid_saml_with_digest_comment.xml | 7 ++ 6 files changed, 195 insertions(+), 13 deletions(-) create mode 100644 test/static/invalid_saml_no_signed_info.xml create mode 100644 test/static/saml_multiple_signed_info_nodes.xml create mode 100644 test/static/saml_wrapped_signed_info_node.xml create mode 100644 test/static/valid_saml_with_digest_comment.xml diff --git a/src/signed-xml.ts b/src/signed-xml.ts index e5d80af7..4f6d11ec 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -252,26 +252,72 @@ export class SignedXml { this.signedXml = xml; const doc = new xmldom.DOMParser().parseFromString(xml); + // Reset the references as only references from our re-parsed signedInfo node can be trusted + this.references = []; + + const unverifiedSignedInfoCanon = this.getCanonSignedInfoXml(doc); + if (!unverifiedSignedInfoCanon) { + if (callback) { + callback(new Error("Canonical signed info cannot be empty"), false); + return; + } + + throw new Error("Canonical signed info cannot be empty"); + } + + // unsigned, verify later to keep with consistent callback behavior + const parsedUnverifiedSignedInfo = new xmldom.DOMParser().parseFromString( + unverifiedSignedInfoCanon, + "text/xml", + ); + + const unverifiedSignedInfoDoc = parsedUnverifiedSignedInfo.documentElement; + if (!unverifiedSignedInfoDoc) { + if (callback) { + callback(new Error("Could not parse unverifiedSignedInfoCanon into a document"), false); + return; + } + + throw new Error("Could not parse unverifiedSignedInfoCanon into a document"); + } + + const references = utils.findChildren(unverifiedSignedInfoDoc, "Reference"); + if (!utils.isArrayHasLength(references)) { + if (callback) { + callback(new Error("could not find any Reference elements"), false); + return; + } + + throw new Error("could not find any Reference elements"); + } + + // TODO: In a future release we'd like to load the Signature and its References at the same time, + // however, in the `.loadSignature()` method we don't have the entire document, + // which we need to to keep the inclusive namespaces + for (const reference of references) { + this.loadReference(reference); + } if (!this.getReferences().every((ref) => this.validateReference(ref, doc))) { if (callback) { - callback(new Error("Could not validate all references")); + callback(new Error("Could not validate all references"), false); return; } return false; } - const signedInfoCanon = this.getCanonSignedInfoXml(doc); + // Stage B: Take the signature algorithm and key and verify the SignatureValue against the canonicalized SignedInfo const signer = this.findSignatureAlgorithm(this.signatureAlgorithm); const key = this.getCertFromKeyInfo(this.keyInfo) || this.publicCert || this.privateKey; if (key == null) { throw new Error("KeyInfo or publicCert or privateKey is required to validate signature"); } + if (callback) { - signer.verifySignature(signedInfoCanon, key, this.signatureValue, callback); + signer.verifySignature(unverifiedSignedInfoCanon, key, this.signatureValue, callback); } else { - const verified = signer.verifySignature(signedInfoCanon, key, this.signatureValue); + const verified = signer.verifySignature(unverifiedSignedInfoCanon, key, this.signatureValue); if (verified === false) { throw new Error( @@ -295,6 +341,11 @@ export class SignedXml { if (signedInfo.length === 0) { throw new Error("could not find SignedInfo element in the message"); } + if (signedInfo.length > 1) { + throw new Error( + "could not get canonicalized signed info for a signature that contains multiple SignedInfo nodes", + ); + } if ( this.canonicalizationAlgorithm === "http://www.w3.org/TR/2001/REC-xml-c14n-20010315" || @@ -522,11 +573,43 @@ export class SignedXml { this.signatureAlgorithm = signatureAlgorithm.value as SignatureAlgorithmType; } - this.references = []; - const references = xpath.select( - ".//*[local-name(.)='SignedInfo']/*[local-name(.)='Reference']", - signatureNode, + const signedInfoNodes = utils.findChildren(this.signatureNode, "SignedInfo"); + if (!utils.isArrayHasLength(signedInfoNodes)) { + throw new Error("no signed info node found"); + } + if (signedInfoNodes.length > 1) { + throw new Error("could not load signature that contains multiple SignedInfo nodes"); + } + + // Try to operate on the c14n version of `signedInfo`. This forces the initial `getReferences()` + // API call to always return references that are loaded under the canonical `SignedInfo` + // in the case that the client access the `.references` **before** signature verification. + + // Ensure canonicalization algorithm is exclusive, otherwise we'd need the entire document + let canonicalizationAlgorithmForSignedInfo = this.canonicalizationAlgorithm; + if ( + !canonicalizationAlgorithmForSignedInfo || + canonicalizationAlgorithmForSignedInfo === + "http://www.w3.org/TR/2001/REC-xml-c14n-20010315" || + canonicalizationAlgorithmForSignedInfo === + "http://www.w3.org/TR/2001/REC-xml-c14n-20010315#WithComments" + ) { + canonicalizationAlgorithmForSignedInfo = "http://www.w3.org/2001/10/xml-exc-c14n#"; + } + + const temporaryCanonSignedInfo = this.getCanonXml( + [canonicalizationAlgorithmForSignedInfo], + signedInfoNodes[0], + ); + const temporaryCanonSignedInfoXml = new xmldom.DOMParser().parseFromString( + temporaryCanonSignedInfo, + "text/xml", ); + const signedInfoDoc = temporaryCanonSignedInfoXml.documentElement; + + this.references = []; + const references = utils.findChildren(signedInfoDoc, "Reference"); + if (!utils.isArrayHasLength(references)) { throw new Error("could not find any Reference elements"); } @@ -572,11 +655,15 @@ export class SignedXml { if (nodes.length === 0) { throw new Error(`could not find DigestValue node in reference ${refNode.toString()}`); } - const firstChild = nodes[0].firstChild; - if (!firstChild || !("data" in firstChild)) { - throw new Error(`could not find the value of DigestValue in ${nodes[0].toString()}`); + if (nodes.length > 1) { + throw new Error( + `could not load reference for a node that contains multiple DigestValue nodes: ${refNode.toString()}`, + ); + } + const digestValue = nodes[0].textContent; + if (!digestValue) { + throw new Error(`could not find the value of DigestValue in ${refNode.toString()}`); } - const digestValue = firstChild.data; const transforms: string[] = []; let inclusiveNamespacesPrefixList: string[] = []; @@ -626,11 +713,14 @@ export class SignedXml { ) { transforms.push("http://www.w3.org/TR/2001/REC-xml-c14n-20010315"); } + const refUri = isDomNode.isElementNode(refNode) + ? refNode.getAttribute("URI") || undefined + : undefined; this.addReference({ transforms, digestAlgorithm: digestAlgo, - uri: isDomNode.isElementNode(refNode) ? utils.findAttr(refNode, "URI")?.value : undefined, + uri: refUri, digestValue, inclusiveNamespacesPrefixList, isEmptyUri: false, diff --git a/test/saml-response-tests.spec.ts b/test/saml-response-tests.spec.ts index b6ee8263..7ae94bc8 100644 --- a/test/saml-response-tests.spec.ts +++ b/test/saml-response-tests.spec.ts @@ -92,4 +92,78 @@ describe("SAML response tests", function () { // This doesn't matter, just want to make sure that we don't fail due to unknown algorithm expect(() => sig.checkSignature(xml)).to.throw(/^invalid signature/); }); + + it("throws an error for a document with no `SignedInfo` node", function () { + const xml = fs.readFileSync("./test/static/invalid_saml_no_signed_info.xml", "utf-8"); + const doc = new xmldom.DOMParser().parseFromString(xml); + const node = xpath.select1( + "/*/*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", + doc, + ); + + isDomNode.assertIsNodeLike(node); + const sig = new SignedXml(); + const feidePublicCert = fs.readFileSync("./test/static/feide_public.pem"); + sig.publicCert = feidePublicCert; + + expect(() => sig.loadSignature(node)).to.throw("no signed info node found"); + }); + + it("test validation ignores an additional wrapped `SignedInfo` node", function () { + const xml = fs.readFileSync("./test/static/saml_wrapped_signed_info_node.xml", "utf-8"); + const doc = new xmldom.DOMParser().parseFromString(xml); + const assertion = xpath.select1("//*[local-name(.)='Assertion']", doc); + isDomNode.assertIsNodeLike(assertion); + const signature = xpath.select1( + "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", + assertion, + ); + isDomNode.assertIsNodeLike(signature); + + const sig = new SignedXml(); + sig.publicCert = fs.readFileSync("./test/static/saml_external_ns.pem"); + sig.loadSignature(signature); + expect(sig.getReferences().length).to.equal(1); + const checkSignatureResult = sig.checkSignature(xml); + expect(checkSignatureResult).to.be.true; + }); + + it("test signature throws if multiple `SignedInfo` nodes are found", function () { + const xml = fs.readFileSync("./test/static/saml_multiple_signed_info_nodes.xml", "utf-8"); + const doc = new xmldom.DOMParser().parseFromString(xml); + const assertion = xpath.select1("//*[local-name(.)='Assertion'][1]", doc); + isDomNode.assertIsNodeLike(assertion); + const signature = xpath.select1( + "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", + assertion, + ); + isDomNode.assertIsNodeLike(signature); + + const sig = new SignedXml(); + sig.publicCert = fs.readFileSync("./test/static/saml_external_ns.pem"); + expect(() => sig.loadSignature(signature)).to.throw( + "could not load signature that contains multiple SignedInfo nodes", + ); + }); + + describe("for a SAML response with a digest value comment", () => { + it("loads digest value from text content instead of comment", function () { + const xml = fs.readFileSync("./test/static/valid_saml_with_digest_comment.xml", "utf-8"); + const doc = new xmldom.DOMParser().parseFromString(xml); + const assertion = xpath.select1("//*[local-name(.)='Assertion']", doc); + isDomNode.assertIsNodeLike(assertion); + const signature = xpath.select1( + "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", + assertion, + ); + isDomNode.assertIsNodeLike(signature); + const sig = new SignedXml(); + sig.publicCert = fs.readFileSync("./test/static/feide_public.pem"); + + sig.loadSignature(signature); + + expect(sig.getReferences()[0].digestValue).to.equal("RnNjoyUguwze5w2R+cboyTHlkQk="); + expect(sig.checkSignature(xml)).to.be.false; + }); + }); }); diff --git a/test/static/invalid_saml_no_signed_info.xml b/test/static/invalid_saml_no_signed_info.xml new file mode 100644 index 00000000..cfc34ca1 --- /dev/null +++ b/test/static/invalid_saml_no_signed_info.xml @@ -0,0 +1,9 @@ +https://openidp.feide.no + + + dkONrkxW+LSuDvnNMG/mWYFa47d2WGyapLhXSTYqrlT9Td+tT7ciojNJ55WTaPaCMt7IrGtIxxskPAZIjdIn5pRyDxHr0joWxzZ7oZHCOI1CnQV5HjOq+rzzmEN2LctCZ6S4hbL7SQ1qJ3vp2BCXAygy4tmJOURQdnk0KLwwRS8= +MIICizCCAfQCCQCY8tKaMc0BMjANBgkqhkiG9w0BAQUFADCBiTELMAkGA1UEBhMCTk8xEjAQBgNVBAgTCVRyb25kaGVpbTEQMA4GA1UEChMHVU5JTkVUVDEOMAwGA1UECxMFRmVpZGUxGTAXBgNVBAMTEG9wZW5pZHAuZmVpZGUubm8xKTAnBgkqhkiG9w0BCQEWGmFuZHJlYXMuc29sYmVyZ0B1bmluZXR0Lm5vMB4XDTA4MDUwODA5MjI0OFoXDTM1MDkyMzA5MjI0OFowgYkxCzAJBgNVBAYTAk5PMRIwEAYDVQQIEwlUcm9uZGhlaW0xEDAOBgNVBAoTB1VOSU5FVFQxDjAMBgNVBAsTBUZlaWRlMRkwFwYDVQQDExBvcGVuaWRwLmZlaWRlLm5vMSkwJwYJKoZIhvcNAQkBFhphbmRyZWFzLnNvbGJlcmdAdW5pbmV0dC5ubzCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAt8jLoqI1VTlxAZ2axiDIThWcAOXdu8KkVUWaN/SooO9O0QQ7KRUjSGKN9JK65AFRDXQkWPAu4HlnO4noYlFSLnYyDxI66LCr71x4lgFJjqLeAvB/GqBqFfIZ3YK/NrhnUqFwZu63nLrZjcUZxNaPjOOSRSDaXpv1kb5k3jOiSGECAwEAATANBgkqhkiG9w0BAQUFAAOBgQBQYj4cAafWaYfjBU2zi1ElwStIaJ5nyp/s/8B8SAPK2T79McMyccP3wSW13LHkmM1jwKe3ACFXBvqGQN0IbcH49hu0FKhYFM/GPDJcIHFBsiyMBXChpye9vBaTNEBCtU3KjjyG0hRT2mAQ9h+bkPmOvlEo/aH0xR68Z9hw4PF13w==https://openidp.feide.no + + + RnNjoyUguwze5w2R+cboyTHlkQk=aw5711jKP7xragunjRRCAD4mT4xKHc37iohBpQDbdSomD3ksOSB96UZQp0MtaC3xlVSkMtYw85Om96T2q2xrxLLYVA50eFJEMMF7SCVPStWTVjBlaCuOPEQxIaHyJs9Sy3MCEfbBh4Pqn9IJBd1kzwdlCrWWjAmksbFFg5wHQJA= +MIICizCCAfQCCQCY8tKaMc0BMjANBgkqhkiG9w0BAQUFADCBiTELMAkGA1UEBhMCTk8xEjAQBgNVBAgTCVRyb25kaGVpbTEQMA4GA1UEChMHVU5JTkVUVDEOMAwGA1UECxMFRmVpZGUxGTAXBgNVBAMTEG9wZW5pZHAuZmVpZGUubm8xKTAnBgkqhkiG9w0BCQEWGmFuZHJlYXMuc29sYmVyZ0B1bmluZXR0Lm5vMB4XDTA4MDUwODA5MjI0OFoXDTM1MDkyMzA5MjI0OFowgYkxCzAJBgNVBAYTAk5PMRIwEAYDVQQIEwlUcm9uZGhlaW0xEDAOBgNVBAoTB1VOSU5FVFQxDjAMBgNVBAsTBUZlaWRlMRkwFwYDVQQDExBvcGVuaWRwLmZlaWRlLm5vMSkwJwYJKoZIhvcNAQkBFhphbmRyZWFzLnNvbGJlcmdAdW5pbmV0dC5ubzCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAt8jLoqI1VTlxAZ2axiDIThWcAOXdu8KkVUWaN/SooO9O0QQ7KRUjSGKN9JK65AFRDXQkWPAu4HlnO4noYlFSLnYyDxI66LCr71x4lgFJjqLeAvB/GqBqFfIZ3YK/NrhnUqFwZu63nLrZjcUZxNaPjOOSRSDaXpv1kb5k3jOiSGECAwEAATANBgkqhkiG9w0BAQUFAAOBgQBQYj4cAafWaYfjBU2zi1ElwStIaJ5nyp/s/8B8SAPK2T79McMyccP3wSW13LHkmM1jwKe3ACFXBvqGQN0IbcH49hu0FKhYFM/GPDJcIHFBsiyMBXChpye9vBaTNEBCtU3KjjyG0hRT2mAQ9h+bkPmOvlEo/aH0xR68Z9hw4PF13w==_6c5dcaa3053321ff4d63785fbc3f67c59a129cde82passport-samlurn:oasis:names:tc:SAML:2.0:ac:classes:PasswordbergieHenriBergiusHenri Bergiushenri.bergius@nemein.combergie@rnd.feide.no8216c78fe244502efa13f62e6615c94acb7bdf3ebergieHenriBergiusHenri Bergiushenri.bergius@nemein.combergie@rnd.feide.no8216c78fe244502efa13f62e6615c94acb7bdf3e \ No newline at end of file diff --git a/test/static/saml_multiple_signed_info_nodes.xml b/test/static/saml_multiple_signed_info_nodes.xml new file mode 100644 index 00000000..aed22887 --- /dev/null +++ b/test/static/saml_multiple_signed_info_nodes.xml @@ -0,0 +1 @@ +https://app.onelogin.com/saml/metadata/164679https://app.onelogin.com/saml/metadata/164679Gx0mTydMn1k6804jZBrdUrZmbV4=jGst6BnAC9xOeqa6hKNPsoMm2TY=oHEPKtwoCbfq1QRm2pjx35zVMqAsti4nQU+3ws8EUJUXHmPG2EoX3HBkb7D2wN4m+ZFrdwARUpNJlhhOIz/eG4jES6ar0tvlNN3qE5cqcQhwZHyRARLnTlERqyZU9Qm729DnAGBeXCdMb736zi16onOIVPNA63LRTzUIxhyZqypDCf1wd6me/ur6UUgH11nYOu4JDYx0iWNkXc1Nad7vkF9oMPeO1QsMxuZSIVH4tvdJkue+qAnu2l+dFJb0LPfm+xmIC0FBo+VX1ECCWRoUZIxjotQfAM6yZpHIi5fNqPXkVyN9fYoUEa9CafqHlc4tAAdgAgGeOqA3jWeC8ZnOVA==MIIEHjCCAwagAwIBAgIBATANBgkqhkiG9w0BAQUFADBnMQswCQYDVQQGEwJVUzETMBEGA1UECAwKQ2FsaWZvcm5pYTEVMBMGA1UEBwwMU2FudGEgTW9uaWNhMREwDwYDVQQKDAhPbmVMb2dpbjEZMBcGA1UEAwwQYXBwLm9uZWxvZ2luLmNvbTAeFw0xMzA1MjcwODU1MTNaFw0xODA1MjcwODU1MTNaMGcxCzAJBgNVBAYTAlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMRUwEwYDVQQHDAxTYW50YSBNb25pY2ExETAPBgNVBAoMCE9uZUxvZ2luMRkwFwYDVQQDDBBhcHAub25lbG9naW4uY29tMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAoXoc7IFZQRv+SwJ15zjIl9touwY5e6b7H4vn3OtOUByjOKHUX8VX0TpbAV2ctZE2GSALx1AGuQAv6O4MVUH+qn/2IAiBY3a7zKN07UBsya7xFMQVHuGE6EiBAs9jpA9wjvYMPRkS5wYZcwjpTQSZK7zFPPtobG8K/1vDbm/tWZjNLmZmQePmXpwrQAuC0+NlzlmnjoQYB2xp2NaTUK9JnnmuB5qev3dpUwlYGSJpf+HUIoxuo8IpxAXOymq1d6tEEJgU1kR2sa7o1sSRFo31YeW/qYCP/gcLJZo3MRUDFe0g5MHeliFue9DsKYUsC6qwAD3gc+MI47buiD6Msu11cwIDAQABo4HUMIHRMAwGA1UdEwEB/wQCMAAwHQYDVR0OBBYEFAAJFJRIlpQQSFsuNdeq7FkTJIH4MIGRBgNVHSMEgYkwgYaAFAAJFJRIlpQQSFsuNdeq7FkTJIH4oWukaTBnMQswCQYDVQQGEwJVUzETMBEGA1UECAwKQ2FsaWZvcm5pYTEVMBMGA1UEBwwMU2FudGEgTW9uaWNhMREwDwYDVQQKDAhPbmVMb2dpbjEZMBcGA1UEAwwQYXBwLm9uZWxvZ2luLmNvbYIBATAOBgNVHQ8BAf8EBAMCBPAwDQYJKoZIhvcNAQEFBQADggEBAB9zN+g6N4sUBE61RaMUH2LSHWwOtfhL64i7pjHjvZa47/qcV/S0Yyd4IE44ho9i2N+AM79d34mThc30oK5aVxOKphKf+xM/cOyVaWIeqr+dCbkY/0OpLEwWOh9VSgOizRO3evLMurbtR892LbSK/Td3hG5jfwoHD23nHH87Dv/3KyZox9MkJdY2DXOHGGIcsqoIifaTyNZyhW6RgwEujQ6LjsaolP1YoeV85TZFKTLa1Ta7ZLUVUC2UJWqz+kRlsyGxf+E/ZmJ7hSq0ZBVHrVOyXjCcFn6X0/W5SrpOmN3fZYcj8Bp6vhB0cJk9qpjgWOP2RCuBdHZVawjCjIaE+bc=anyone@gmail.comForNodeJSurn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransportkartik.cds@gmail.comKartikCDShttps://app.onelogin.com/saml/metadata/164679kartik.cds@gmail.comForNodeJSurn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransportkartik.cds@gmail.comKartikCDS \ No newline at end of file diff --git a/test/static/saml_wrapped_signed_info_node.xml b/test/static/saml_wrapped_signed_info_node.xml new file mode 100644 index 00000000..b7eb19d1 --- /dev/null +++ b/test/static/saml_wrapped_signed_info_node.xml @@ -0,0 +1 @@ +https://app.onelogin.com/saml/metadata/164679https://app.onelogin.com/saml/metadata/164679jGst6BnAC9xOeqa6hKNPsoMm2TY=Gx0mTydMn1k6804jZBrdUrZmbV4=oHEPKtwoCbfq1QRm2pjx35zVMqAsti4nQU+3ws8EUJUXHmPG2EoX3HBkb7D2wN4m+ZFrdwARUpNJlhhOIz/eG4jES6ar0tvlNN3qE5cqcQhwZHyRARLnTlERqyZU9Qm729DnAGBeXCdMb736zi16onOIVPNA63LRTzUIxhyZqypDCf1wd6me/ur6UUgH11nYOu4JDYx0iWNkXc1Nad7vkF9oMPeO1QsMxuZSIVH4tvdJkue+qAnu2l+dFJb0LPfm+xmIC0FBo+VX1ECCWRoUZIxjotQfAM6yZpHIi5fNqPXkVyN9fYoUEa9CafqHlc4tAAdgAgGeOqA3jWeC8ZnOVA==MIIEHjCCAwagAwIBAgIBATANBgkqhkiG9w0BAQUFADBnMQswCQYDVQQGEwJVUzETMBEGA1UECAwKQ2FsaWZvcm5pYTEVMBMGA1UEBwwMU2FudGEgTW9uaWNhMREwDwYDVQQKDAhPbmVMb2dpbjEZMBcGA1UEAwwQYXBwLm9uZWxvZ2luLmNvbTAeFw0xMzA1MjcwODU1MTNaFw0xODA1MjcwODU1MTNaMGcxCzAJBgNVBAYTAlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMRUwEwYDVQQHDAxTYW50YSBNb25pY2ExETAPBgNVBAoMCE9uZUxvZ2luMRkwFwYDVQQDDBBhcHAub25lbG9naW4uY29tMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAoXoc7IFZQRv+SwJ15zjIl9touwY5e6b7H4vn3OtOUByjOKHUX8VX0TpbAV2ctZE2GSALx1AGuQAv6O4MVUH+qn/2IAiBY3a7zKN07UBsya7xFMQVHuGE6EiBAs9jpA9wjvYMPRkS5wYZcwjpTQSZK7zFPPtobG8K/1vDbm/tWZjNLmZmQePmXpwrQAuC0+NlzlmnjoQYB2xp2NaTUK9JnnmuB5qev3dpUwlYGSJpf+HUIoxuo8IpxAXOymq1d6tEEJgU1kR2sa7o1sSRFo31YeW/qYCP/gcLJZo3MRUDFe0g5MHeliFue9DsKYUsC6qwAD3gc+MI47buiD6Msu11cwIDAQABo4HUMIHRMAwGA1UdEwEB/wQCMAAwHQYDVR0OBBYEFAAJFJRIlpQQSFsuNdeq7FkTJIH4MIGRBgNVHSMEgYkwgYaAFAAJFJRIlpQQSFsuNdeq7FkTJIH4oWukaTBnMQswCQYDVQQGEwJVUzETMBEGA1UECAwKQ2FsaWZvcm5pYTEVMBMGA1UEBwwMU2FudGEgTW9uaWNhMREwDwYDVQQKDAhPbmVMb2dpbjEZMBcGA1UEAwwQYXBwLm9uZWxvZ2luLmNvbYIBATAOBgNVHQ8BAf8EBAMCBPAwDQYJKoZIhvcNAQEFBQADggEBAB9zN+g6N4sUBE61RaMUH2LSHWwOtfhL64i7pjHjvZa47/qcV/S0Yyd4IE44ho9i2N+AM79d34mThc30oK5aVxOKphKf+xM/cOyVaWIeqr+dCbkY/0OpLEwWOh9VSgOizRO3evLMurbtR892LbSK/Td3hG5jfwoHD23nHH87Dv/3KyZox9MkJdY2DXOHGGIcsqoIifaTyNZyhW6RgwEujQ6LjsaolP1YoeV85TZFKTLa1Ta7ZLUVUC2UJWqz+kRlsyGxf+E/ZmJ7hSq0ZBVHrVOyXjCcFn6X0/W5SrpOmN3fZYcj8Bp6vhB0cJk9qpjgWOP2RCuBdHZVawjCjIaE+bc=anyone@gmail.comForNodeJSurn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransportkartik.cds@gmail.comKartikCDShttps://app.onelogin.com/saml/metadata/164679kartik.cds@gmail.comForNodeJSurn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransportkartik.cds@gmail.comKartikCDS \ No newline at end of file diff --git a/test/static/valid_saml_with_digest_comment.xml b/test/static/valid_saml_with_digest_comment.xml new file mode 100644 index 00000000..d0996872 --- /dev/null +++ b/test/static/valid_saml_with_digest_comment.xml @@ -0,0 +1,7 @@ +https://openidp.feide.no +https://openidp.feide.no + + + RnNjoyUguwze5w2R+cboyTHlkQk=aw5711jKP7xragunjRRCAD4mT4xKHc37iohBpQDbdSomD3ksOSB96UZQp0MtaC3xlVSkMtYw85Om96T2q2xrxLLYVA50eFJEMMF7SCVPStWTVjBlaCuOPEQxIaHyJs9Sy3MCEfbBh4Pqn9IJBd1kzwdlCrWWjAmksbFFg5wHQJA= +MIICizCCAfQCCQCY8tKaMc0BMjANBgkqhkiG9w0BAQUFADCBiTELMAkGA1UEBhMCTk8xEjAQBgNVBAgTCVRyb25kaGVpbTEQMA4GA1UEChMHVU5JTkVUVDEOMAwGA1UECxMFRmVpZGUxGTAXBgNVBAMTEG9wZW5pZHAuZmVpZGUubm8xKTAnBgkqhkiG9w0BCQEWGmFuZHJlYXMuc29sYmVyZ0B1bmluZXR0Lm5vMB4XDTA4MDUwODA5MjI0OFoXDTM1MDkyMzA5MjI0OFowgYkxCzAJBgNVBAYTAk5PMRIwEAYDVQQIEwlUcm9uZGhlaW0xEDAOBgNVBAoTB1VOSU5FVFQxDjAMBgNVBAsTBUZlaWRlMRkwFwYDVQQDExBvcGVuaWRwLmZlaWRlLm5vMSkwJwYJKoZIhvcNAQkBFhphbmRyZWFzLnNvbGJlcmdAdW5pbmV0dC5ubzCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEAt8jLoqI1VTlxAZ2axiDIThWcAOXdu8KkVUWaN/SooO9O0QQ7KRUjSGKN9JK65AFRDXQkWPAu4HlnO4noYlFSLnYyDxI66LCr71x4lgFJjqLeAvB/GqBqFfIZ3YK/NrhnUqFwZu63nLrZjcUZxNaPjOOSRSDaXpv1kb5k3jOiSGECAwEAATANBgkqhkiG9w0BAQUFAAOBgQBQYj4cAafWaYfjBU2zi1ElwStIaJ5nyp/s/8B8SAPK2T79McMyccP3wSW13LHkmM1jwKe3ACFXBvqGQN0IbcH49hu0FKhYFM/GPDJcIHFBsiyMBXChpye9vBaTNEBCtU3KjjyG0hRT2mAQ9h+bkPmOvlEo/aH0xR68Z9hw4PF13w==test@example.compassport-samlurn:oasis:names:tc:SAML:2.0:ac:classes:PasswordbergieHenriBergiusHenri Bergiushenri.bergius@nemein.combergie@rnd.feide.no8216c78fe244502efa13f62e6615c94acb7bdf3ebergieHenriBergiusHenri Bergiushenri.bergius@nemein.combergie@rnd.feide.no8216c78fe244502efa13f62e6615c94acb7bdf3e + \ No newline at end of file From cc24755d3b170ba6991a573c8091b96a341405c7 Mon Sep 17 00:00:00 2001 From: Chris Barth Date: Wed, 9 Apr 2025 09:30:56 -0500 Subject: [PATCH 9/9] Introduce new .getSignedReferences() function of signature to help prevent signature wrapping attacks (#495) --- README.md | 58 +++++++-------- src/index.ts | 4 +- src/signed-xml.ts | 93 +++++++++++++++++++----- test/document-tests.spec.ts | 17 +++++ test/hmac-tests.spec.ts | 3 + test/saml-response-tests.spec.ts | 8 ++ test/signature-integration-tests.spec.ts | 5 ++ test/signature-unit-tests.spec.ts | 40 ++++++---- test/wsfed-metadata-tests.spec.ts | 1 + 9 files changed, 166 insertions(+), 63 deletions(-) diff --git a/README.md b/README.md index 2d8d82bc..ecfa2612 100644 --- a/README.md +++ b/README.md @@ -3,9 +3,18 @@ ![Build](https://github.com/node-saml/xml-crypto/actions/workflows/ci.yml/badge.svg) [![Gitpod Ready-to-Code](https://img.shields.io/badge/Gitpod-Ready--to--Code-blue?logo=gitpod)](https://gitpod.io/from-referrer/) -An xml digital signature library for node. Xml encryption is coming soon. Written in pure javascript! +--- -For more information visit [my blog](http://webservices20.blogspot.com/) or [my twitter](https://twitter.com/YaronNaveh). +# Upgrading + +The `.getReferences()` AND the `.references` APIs are deprecated. +Please do not attempt to access them. The content in them should be treated as unsigned. + +Instead, we strongly encourage users to migrate to the `.getSignedReferences()` API. See the [Verifying XML document](#verifying-xml-documents) section +We understand that this may take a lot of efforts to migrate, feel free to ask for help. +This will help prevent future XML signature wrapping attacks. + +--- ## Install @@ -161,6 +170,11 @@ var select = require("xml-crypto").xpath, var xml = fs.readFileSync("signed.xml").toString(); var doc = new dom().parseFromString(xml); +// DO NOT attempt to parse whatever data object you have here in `doc` +// and then use it to verify the signature. This can lead to security issues. +// i.e. BAD: parseAssertion(doc), +// good: see below + var signature = select( doc, "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", @@ -177,39 +191,21 @@ try { In order to protect from some attacks we must check the content we want to use is the one that has been signed: ```javascript -// Roll your own -const elem = xpath.select("/xpath_to_interesting_element", doc); -const uri = sig.getReferences()[0].uri; // might not be 0; it depends on the document -const id = uri[0] === "#" ? uri.substring(1) : uri; -if ( - elem.getAttribute("ID") != id && - elem.getAttribute("Id") != id && - elem.getAttribute("id") != id -) { - throw new Error("The interesting element was not the one verified by the signature"); +if (!res) { + throw "Invalid Signature"; } +// good: The XML Signature has been verified, meaning some subset of XML is verified. +var signedBytes = sig.getSignedReferences(); -// Get the validated element directly from a reference -const elem = sig.references[0].getValidatedElement(); // might not be 0; it depends on the document -const matchingReference = xpath.select1("/xpath_to_interesting_element", elem); -if (!isDomNode.isNodeLike(matchingReference)) { - throw new Error("The interesting element was not the one verified by the signature"); -} +var authenticatedDoc = new dom().parseFromString(signedBytes[0]); // Take the first signed reference +// It is now safe to load SAML, obtain the assertion XML, or do whatever else is needed. +// Be sure to only use authenticated data. +let signedAssertionNode = extractAssertion(authenticatedDoc); +let parsedAssertion = parseAssertion(signedAssertionNode); -// Use the built-in method -const elem = xpath.select1("/xpath_to_interesting_element", doc); -try { - const matchingReference = sig.validateElementAgainstReferences(elem, doc); -} catch { - throw new Error("The interesting element was not the one verified by the signature"); -} +return parsedAssertion; // This the correctly verified signed Assertion -// Use the built-in method with a an xpath expression -try { - const matchingReference = sig.validateReferenceWithXPath("/xpath_to_interesting_element", doc); -} catch { - throw new Error("The interesting element was not the one verified by the signature"); -} +// BAD example: DO not use the .getReferences() API. ``` Note: diff --git a/src/index.ts b/src/index.ts index 0e72578a..3c82b7a8 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,8 +1,8 @@ -export { SignedXml } from "./signed-xml"; export { C14nCanonicalization, C14nCanonicalizationWithComments } from "./c14n-canonicalization"; export { ExclusiveCanonicalization, ExclusiveCanonicalizationWithComments, } from "./exclusive-canonicalization"; -export * from "./utils"; +export { SignedXml } from "./signed-xml"; export * from "./types"; +export * from "./utils"; diff --git a/src/signed-xml.ts b/src/signed-xml.ts index 4f6d11ec..fddb9162 100644 --- a/src/signed-xml.ts +++ b/src/signed-xml.ts @@ -1,7 +1,10 @@ import type { CanonicalizationAlgorithmType, + CanonicalizationOrTransformAlgorithmType, CanonicalizationOrTransformationAlgorithm, + CanonicalizationOrTransformationAlgorithmProcessOptions, ComputeSignatureOptions, + ErrorFirstCallback, GetKeyInfoContentArgs, HashAlgorithm, HashAlgorithmType, @@ -9,21 +12,19 @@ import type { SignatureAlgorithm, SignatureAlgorithmType, SignedXmlOptions, - CanonicalizationOrTransformAlgorithmType, - ErrorFirstCallback, - CanonicalizationOrTransformationAlgorithmProcessOptions, } from "./types"; -import * as xpath from "xpath"; +import * as isDomNode from "@xmldom/is-dom-node"; import * as xmldom from "@xmldom/xmldom"; -import * as utils from "./utils"; +import * as crypto from "crypto"; +import { deprecate } from "util"; +import * as xpath from "xpath"; import * as c14n from "./c14n-canonicalization"; -import * as execC14n from "./exclusive-canonicalization"; import * as envelopedSignatures from "./enveloped-signature"; +import * as execC14n from "./exclusive-canonicalization"; import * as hashAlgorithms from "./hash-algorithms"; import * as signatureAlgorithms from "./signature-algorithms"; -import * as crypto from "crypto"; -import * as isDomNode from "@xmldom/is-dom-node"; +import * as utils from "./utils"; export class SignedXml { idMode?: "wssecurity"; @@ -71,6 +72,14 @@ export class SignedXml { */ private references: Reference[] = []; + /** + * Contains the canonicalized XML of the references that were validly signed. + * + * This populates with the canonical XML of the reference only after + * verifying the signature is cryptographically authentic. + */ + private signedReferences: string[] = []; + /** * To add a new transformation algorithm create a new class that implements the {@link TransformationAlgorithm} interface, and register it here. More info: {@link https://github.com/node-saml/xml-crypto#customizing-algorithms|Customizing Algorithms} */ @@ -87,6 +96,8 @@ export class SignedXml { "http://www.w3.org/2000/09/xmldsig#enveloped-signature": envelopedSignatures.EnvelopedSignature, }; + // TODO: In v7.x we may consider deprecating sha1 + /** * To add a new hash algorithm create a new class that implements the {@link HashAlgorithm} interface, and register it here. More info: {@link https://github.com/node-saml/xml-crypto#customizing-algorithms|Customizing Algorithms} */ @@ -96,6 +107,8 @@ export class SignedXml { "http://www.w3.org/2001/04/xmlenc#sha512": hashAlgorithms.Sha512, }; + // TODO: In v7.x we may consider deprecating sha1 + /** * To add a new signature algorithm create a new class that implements the {@link SignatureAlgorithm} interface, and register it here. More info: {@link https://github.com/node-saml/xml-crypto#customizing-algorithms|Customizing Algorithms} */ @@ -252,6 +265,7 @@ export class SignedXml { this.signedXml = xml; const doc = new xmldom.DOMParser().parseFromString(xml); + // Reset the references as only references from our re-parsed signedInfo node can be trusted this.references = []; @@ -298,34 +312,62 @@ export class SignedXml { this.loadReference(reference); } + /* eslint-disable-next-line deprecation/deprecation */ if (!this.getReferences().every((ref) => this.validateReference(ref, doc))) { + /* Trustworthiness can only be determined if SignedInfo's (which holds References' DigestValue(s) + which were validated at this stage) signature is valid. Execution does not proceed to validate + signature phase thus each References' DigestValue must be considered to be untrusted (attacker + might have injected any data with new new references and/or recalculated new DigestValue for + altered Reference(s)). Returning any content via `signedReferences` would give false sense of + trustworthiness if/when SignedInfo's (which holds references' DigestValues) signature is not + valid(ated). Put simply: if one fails, they are all not trustworthy. + */ + this.signedReferences = []; if (callback) { callback(new Error("Could not validate all references"), false); return; } + // We return false because some references validated, but not all + // We should actually be throwing an error here, but that would be a breaking change + // See https://www.w3.org/TR/xmldsig-core/#sec-CoreValidation return false; } - // Stage B: Take the signature algorithm and key and verify the SignatureValue against the canonicalized SignedInfo + // (Stage B authentication step, show that the `signedInfoCanon` is signed) + + // First find the key & signature algorithm, these should match + // Stage B: Take the signature algorithm and key and verify the `SignatureValue` against the canonicalized `SignedInfo` const signer = this.findSignatureAlgorithm(this.signatureAlgorithm); const key = this.getCertFromKeyInfo(this.keyInfo) || this.publicCert || this.privateKey; if (key == null) { throw new Error("KeyInfo or publicCert or privateKey is required to validate signature"); } - if (callback) { - signer.verifySignature(unverifiedSignedInfoCanon, key, this.signatureValue, callback); + // Check the signature verification to know whether to reset signature value or not. + const sigRes = signer.verifySignature(unverifiedSignedInfoCanon, key, this.signatureValue); + if (sigRes === true) { + if (callback) { + callback(null, true); + } else { + return true; + } } else { - const verified = signer.verifySignature(unverifiedSignedInfoCanon, key, this.signatureValue); + // Ideally, we would start by verifying the `signedInfoCanon` first, + // but that may cause some breaking changes, so we'll handle that in v7.x. + // If we were validating `signedInfoCanon` first, we wouldn't have to reset this array. + this.signedReferences = []; - if (verified === false) { + if (callback) { + callback( + new Error(`invalid signature: the signature value ${this.signatureValue} is incorrect`), + ); + return; // return early + } else { throw new Error( `invalid signature: the signature value ${this.signatureValue} is incorrect`, ); } - - return true; } } @@ -442,6 +484,7 @@ export class SignedXml { elem = elemOrXpath; } + /* eslint-disable-next-line deprecation/deprecation */ for (const ref of this.getReferences()) { const uri = ref.uri?.[0] === "#" ? ref.uri.substring(1) : ref.uri; @@ -525,6 +568,11 @@ export class SignedXml { return false; } + // This step can only be done after we have verified the `signedInfo`. + // We verified that they have same hash, + // thus the `canonXml` and _only_ the `canonXml` can be trusted. + // Append this to `signedReferences`. + this.signedReferences.push(canonXml); return true; } @@ -655,6 +703,7 @@ export class SignedXml { if (nodes.length === 0) { throw new Error(`could not find DigestValue node in reference ${refNode.toString()}`); } + if (nodes.length > 1) { throw new Error( `could not load reference for a node that contains multiple DigestValue nodes: ${refNode.toString()}`, @@ -771,8 +820,17 @@ export class SignedXml { }); } - getReferences(): Reference[] { - return this.references; + /** + * @deprecated Use `.getSignedReferences()` instead. + * Returns the list of references. + */ + getReferences = deprecate( + () => this.references, + "getReferences() is deprecated. Use `.getSignedReferences()` instead.", + ); + + getSignedReferences() { + return [...this.signedReferences]; } /** @@ -1007,6 +1065,7 @@ export class SignedXml { prefix = prefix || ""; prefix = prefix ? `${prefix}:` : prefix; + /* eslint-disable-next-line deprecation/deprecation */ for (const ref of this.getReferences()) { const nodes = xpath.selectWithResolver(ref.xpath ?? "", doc, this.namespaceResolver); diff --git a/test/document-tests.spec.ts b/test/document-tests.spec.ts index 57b552af..b8311994 100644 --- a/test/document-tests.spec.ts +++ b/test/document-tests.spec.ts @@ -21,6 +21,7 @@ describe("Document tests", function () { const result = sig.checkSignature(xml); expect(result).to.be.true; + expect(sig.getSignedReferences().length).to.equal(1); }); it("test with a document (using StringKeyInfo)", function () { @@ -39,6 +40,7 @@ describe("Document tests", function () { const result = sig.checkSignature(xml); expect(result).to.be.true; + expect(sig.getSignedReferences().length).to.equal(1); }); }); @@ -51,10 +53,13 @@ describe("Validated node references tests", function () { sig.loadSignature(sig.findSignatures(doc)[0]); const validSignature = sig.checkSignature(xml); expect(validSignature).to.be.true; + expect(sig.getSignedReferences().length).to.equal(1); + /* eslint-disable-next-line deprecation/deprecation */ const ref = sig.getReferences()[0]; const result = ref.getValidatedNode(); expect(result?.toString()).to.equal(doc.toString()); + expect(sig.getSignedReferences().length).to.equal(1); }); it("should not return references if the document is not validly signed", function () { @@ -64,10 +69,13 @@ describe("Validated node references tests", function () { sig.loadSignature(sig.findSignatures(doc)[0]); const validSignature = sig.checkSignature(xml); expect(validSignature).to.be.false; + expect(sig.getSignedReferences().length).to.equal(0); + /* eslint-disable-next-line deprecation/deprecation */ const ref = sig.getReferences()[1]; const result = ref.getValidatedNode(); expect(result).to.be.null; + expect(sig.getSignedReferences().length).to.equal(0); }); it("should return `null` if the selected node isn't found", function () { @@ -78,7 +86,9 @@ describe("Validated node references tests", function () { sig.loadSignature(sig.findSignatures(doc)[0]); const validSignature = sig.checkSignature(xml); expect(validSignature).to.be.true; + expect(sig.getSignedReferences().length).to.equal(1); + /* eslint-disable-next-line deprecation/deprecation */ const ref = sig.getReferences()[0]; const result = ref.getValidatedNode("/non-existent-node"); expect(result).to.be.null; @@ -92,12 +102,15 @@ describe("Validated node references tests", function () { sig.loadSignature(sig.findSignatures(doc)[0]); const validSignature = sig.checkSignature(xml); expect(validSignature).to.be.true; + expect(sig.getSignedReferences().length).to.equal(1); + /* eslint-disable-next-line deprecation/deprecation */ const ref = sig.getReferences()[0]; const result = ref.getValidatedNode( "//*[local-name()='Attribute' and @Name='mail']/*[local-name()='AttributeValue']/text()", ); expect(result?.nodeValue).to.equal("henri.bergius@nemein.com"); + expect(sig.getSignedReferences().length).to.equal(1); }); it("should return `null` if the selected node isn't validly signed", function () { @@ -107,11 +120,15 @@ describe("Validated node references tests", function () { sig.loadSignature(sig.findSignatures(doc)[0]); const validSignature = sig.checkSignature(xml); expect(validSignature).to.be.false; + expect(sig.getSignedReferences().length).to.equal(0); + /* eslint-disable-next-line deprecation/deprecation */ const ref = sig.getReferences()[0]; const result = ref.getValidatedNode( "//*[local-name()='Attribute' and @Name='mail']/*[local-name()='AttributeValue']/text()", ); expect(result).to.be.null; + // Not all references verified, so no references should be in `.getSignedReferences()`. + expect(sig.getSignedReferences().length).to.equal(0); }); }); diff --git a/test/hmac-tests.spec.ts b/test/hmac-tests.spec.ts index 6ef31db0..6573ca6b 100644 --- a/test/hmac-tests.spec.ts +++ b/test/hmac-tests.spec.ts @@ -22,6 +22,7 @@ describe("HMAC tests", function () { const result = sig.checkSignature(xml); expect(result).to.be.true; + expect(sig.getSignedReferences().length).to.equal(1); }); it("test HMAC signature with incorrect key", function () { @@ -39,6 +40,7 @@ describe("HMAC tests", function () { sig.loadSignature(signature); expect(() => sig.checkSignature(xml)).to.throw(/^invalid signature/); + expect(sig.getSignedReferences().length).to.equal(0); }); it("test create and validate HMAC signature", function () { @@ -69,5 +71,6 @@ describe("HMAC tests", function () { const result = verify.checkSignature(sig.getSignedXml()); expect(result).to.be.true; + expect(verify.getSignedReferences().length).to.equal(1); }); }); diff --git a/test/saml-response-tests.spec.ts b/test/saml-response-tests.spec.ts index 7ae94bc8..4208ecfa 100644 --- a/test/saml-response-tests.spec.ts +++ b/test/saml-response-tests.spec.ts @@ -20,6 +20,7 @@ describe("SAML response tests", function () { const result = sig.checkSignature(xml); expect(result).to.be.true; + expect(sig.getSignedReferences().length).to.equal(1); }); it("test validating wrapped assertion signature", function () { @@ -43,6 +44,7 @@ describe("SAML response tests", function () { "same value for the ID / Id / Id attributes, in order to prevent " + "signature wrapping attack.", ).to.throw(); + expect(sig.getSignedReferences().length).to.equal(0); }); it("test validating SAML response where a namespace is defined outside the signed element", function () { @@ -58,6 +60,7 @@ describe("SAML response tests", function () { sig.loadSignature(signature); const result = sig.checkSignature(xml); expect(result).to.be.true; + expect(sig.getSignedReferences().length).to.equal(1); }); it("test reference id does not contain quotes", function () { @@ -91,6 +94,7 @@ describe("SAML response tests", function () { sig.loadSignature(signature); // This doesn't matter, just want to make sure that we don't fail due to unknown algorithm expect(() => sig.checkSignature(xml)).to.throw(/^invalid signature/); + expect(sig.getSignedReferences().length).to.equal(0); }); it("throws an error for a document with no `SignedInfo` node", function () { @@ -123,9 +127,11 @@ describe("SAML response tests", function () { const sig = new SignedXml(); sig.publicCert = fs.readFileSync("./test/static/saml_external_ns.pem"); sig.loadSignature(signature); + /* eslint-disable-next-line deprecation/deprecation */ expect(sig.getReferences().length).to.equal(1); const checkSignatureResult = sig.checkSignature(xml); expect(checkSignatureResult).to.be.true; + expect(sig.getSignedReferences().length).to.equal(1); }); it("test signature throws if multiple `SignedInfo` nodes are found", function () { @@ -162,8 +168,10 @@ describe("SAML response tests", function () { sig.loadSignature(signature); + /* eslint-disable-next-line deprecation/deprecation */ expect(sig.getReferences()[0].digestValue).to.equal("RnNjoyUguwze5w2R+cboyTHlkQk="); expect(sig.checkSignature(xml)).to.be.false; + expect(sig.getSignedReferences().length).to.equal(0); }); }); }); diff --git a/test/signature-integration-tests.spec.ts b/test/signature-integration-tests.spec.ts index befcab1f..02da0949 100644 --- a/test/signature-integration-tests.spec.ts +++ b/test/signature-integration-tests.spec.ts @@ -83,6 +83,7 @@ describe("Signature integration tests", function () { const result = sig.checkSignature(xml); expect(result).to.be.true; + expect(sig.getSignedReferences().length).to.equal(1); }); it("add canonicalization if output of transforms will be a node-set rather than an octet stream", function () { @@ -110,6 +111,7 @@ describe("Signature integration tests", function () { const result = sig.checkSignature(childXml ?? ""); expect(result).to.be.true; + expect(sig.getSignedReferences().length).to.equal(1); }); it("signature with inclusive namespaces", function () { @@ -128,6 +130,7 @@ describe("Signature integration tests", function () { const result = sig.checkSignature(childXml ?? ""); expect(result).to.be.true; + expect(sig.getSignedReferences().length).to.equal(1); }); it("signature with inclusive namespaces with unix line separators", function () { @@ -149,6 +152,7 @@ describe("Signature integration tests", function () { const result = sig.checkSignature(childXml ?? ""); expect(result).to.be.true; + expect(sig.getSignedReferences().length).to.equal(1); }); it("signature with inclusive namespaces with windows line separators", function () { @@ -170,6 +174,7 @@ describe("Signature integration tests", function () { const result = sig.checkSignature(childXml ?? ""); expect(result).to.be.true; + expect(sig.getSignedReferences().length).to.equal(1); }); it("should create single root xml document when signing inner node", function () { diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 0db89a49..baa382db 100644 --- a/test/signature-unit-tests.spec.ts +++ b/test/signature-unit-tests.spec.ts @@ -816,7 +816,9 @@ describe("Signature unit tests", function () { const checkedSignature = sig.checkSignature(xml); expect(checkedSignature).to.be.true; + /* eslint-disable-next-line deprecation/deprecation */ expect(sig.getReferences().length).to.equal(3); + expect(sig.getSignedReferences().length).to.equal(3); const digests = [ "b5GCZ2xpP5T7tbLWBTkOl4CYupQ=", @@ -829,7 +831,9 @@ describe("Signature unit tests", function () { const matchedReference = sig.validateElementAgainstReferences(firstGrandchild, doc); expect(matchedReference).to.not.be.false; + /* eslint-disable-next-line deprecation/deprecation */ for (let i = 0; i < sig.getReferences().length; i++) { + /* eslint-disable-next-line deprecation/deprecation */ const ref = sig.getReferences()[i]; const expectedUri = `#_${i}`; expect( @@ -857,7 +861,7 @@ describe("Signature unit tests", function () { }); describe("pass verify signature", function () { - function verifySignature(xml: string, idMode?: "wssecurity") { + function loadSignature(xml: string, idMode?: "wssecurity") { const doc = new xmldom.DOMParser().parseFromString(xml); const node = xpath.select1( "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", @@ -867,25 +871,35 @@ describe("Signature unit tests", function () { const sig = new SignedXml({ idMode }); sig.publicCert = fs.readFileSync("./test/static/client_public.pem"); sig.loadSignature(node); - try { - const res = sig.checkSignature(xml); - return res; - } catch (e) { - return false; - } + return sig; } function passValidSignature(file: string, mode?: "wssecurity") { const xml = fs.readFileSync(file, "utf8"); - const res = verifySignature(xml, mode); - expect(res, "expected signature to be valid, but it was reported invalid").to.equal(true); + const sig = loadSignature(xml, mode); + const res = sig.checkSignature(xml); + expect(res, "expected all signatures to be valid, but some reported invalid").to.be.true; + /* eslint-disable-next-line deprecation/deprecation */ + expect(sig.getSignedReferences().length).to.equal(sig.getReferences().length); } function failInvalidSignature(file: string, idMode?: "wssecurity") { const xml = fs.readFileSync(file).toString(); - const res = verifySignature(xml, idMode); - expect(res, "expected signature to be invalid, but it was reported valid").to.equal(false); + const sig = loadSignature(xml, idMode); + const res = sig.checkSignature(xml); + expect(res, "expected a signature to be invalid, but all were reported valid").to.be.false; + expect(sig.getSignedReferences().length).to.equal(0); + } + + function throwsValidatingSignature(file: string, idMode?: "wssecurity") { + const xml = fs.readFileSync(file).toString(); + const sig = loadSignature(xml, idMode); + expect( + () => sig.checkSignature(xml), + "expected an error to be thrown because signatures couldn't be checked for validity", + ).to.throw(); + expect(sig.getSignedReferences().length).to.equal(0); } it("verifies valid signature", function () { @@ -920,8 +934,8 @@ describe("Signature unit tests", function () { passValidSignature("./test/static/valid_signature_without_transforms_element.xml"); }); - it("fails invalid signature - signature value", function () { - failInvalidSignature("./test/static/invalid_signature - signature value.xml"); + it("throws validating signature - signature value", function () { + throwsValidatingSignature("./test/static/invalid_signature - signature value.xml"); }); it("fails invalid signature - hash", function () { diff --git a/test/wsfed-metadata-tests.spec.ts b/test/wsfed-metadata-tests.spec.ts index 111ed041..7ddea8e8 100644 --- a/test/wsfed-metadata-tests.spec.ts +++ b/test/wsfed-metadata-tests.spec.ts @@ -20,5 +20,6 @@ describe("WS-Fed Metadata tests", function () { const result = sig.checkSignature(xml); expect(result).to.be.true; + expect(sig.getSignedReferences().length).to.equal(1); }); });