diff --git a/CHANGELOG.md b/CHANGELOG.md index 02cb59d..6aee9c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## 6.0.1 (2025-03-14) - - Address CVEs: CVE-2025-29774 and CVE-2025-29775 +- Address CVEs: CVE-2025-29774 and CVE-2025-29775 --- diff --git a/README.md b/README.md index 2d8d82b..ecfa261 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 3e4a8a4..3c82b7a 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..fddb916 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 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..4208ecf 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 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); }); });