diff --git a/CHANGELOG.md b/CHANGELOG.md index 02cb59d..7f4c6bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ # 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) +======= + ## 6.0.1 (2025-03-14) - Address CVEs: CVE-2025-29774 and CVE-2025-29775 diff --git a/README.md b/README.md index 2d8d82b..0e8ab9b 100644 --- a/README.md +++ b/README.md @@ -1,11 +1,33 @@ # 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/) -An xml digital signature library for node. Xml encryption is coming soon. Written in pure javascript! +--- + +# 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. -For more information visit [my blog](http://webservices20.blogspot.com/) or [my twitter](https://twitter.com/YaronNaveh). +--- ## Install @@ -161,6 +183,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#']", @@ -172,44 +199,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: diff --git a/src/XMLVerifier.ts b/src/XMLVerifier.ts new file mode 100644 index 0000000..f1df92f --- /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/index.ts b/src/index.ts index 3e4a8a4..1f59eb7 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,3 +1,8 @@ +export { C14nCanonicalization, C14nCanonicalizationWithComments } from "./c14n-canonicalization"; +export { + ExclusiveCanonicalization, + ExclusiveCanonicalizationWithComments +} from "./exclusive-canonicalization"; export { SignedXml } from "./signed-xml"; -export * from "./utils"; export * from "./types"; +export * from "./utils"; diff --git a/src/signed-xml.ts b/src/signed-xml.ts index 4f6d11e..e5a0aae 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,22 @@ 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"; + +// configuration class for Signing/Verifying XML. +// We extract relevant logic into a new XMLVerifier class export class SignedXml { idMode?: "wssecurity"; @@ -71,6 +75,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 +99,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 +110,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} */ @@ -112,6 +128,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 @@ -154,6 +171,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. } /** @@ -229,10 +249,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. * @@ -240,8 +259,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 { @@ -252,6 +271,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 +318,64 @@ 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); + + // let's clear the callback up a little bit, so we can access it's results, + const sigRes = signer.verifySignature(unverifiedSignedInfoCanon, key, this.signatureValue); + // true case + 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 +492,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; @@ -465,70 +516,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; - } - - return true; - } - findSignatures(doc: Node): Node[] { const nodes = xpath.select( "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']", @@ -575,6 +562,7 @@ export class SignedXml { const signedInfoNodes = utils.findChildren(this.signatureNode, "SignedInfo"); if (!utils.isArrayHasLength(signedInfoNodes)) { + throw new Error("no signed info node found"); } if (signedInfoNodes.length > 1) { @@ -655,6 +643,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 +760,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 +1005,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/src/types.ts b/src/types.ts index 090c944..e6b0617 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. diff --git a/test/document-tests.spec.ts b/test/document-tests.spec.ts index 57b552a..b831199 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 6ef31db..6573ca6 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 7ae94bc..c194d71 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,85 @@ 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 () { + 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); + /* 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 () { + 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); + + /* 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); + }); }); it("throws an error for a document with no `SignedInfo` node", function () { diff --git a/test/signature-integration-tests.spec.ts b/test/signature-integration-tests.spec.ts index befcab1..02da094 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 0db89a4..baa382d 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 111ed04..7ddea8e 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); }); });