Introduce new .getSignedReferences() function of signature to help prevent signature wrapping attacks

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:

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";

src/signed-xml.ts

@@ -1,29 +1,30 @@
 import type {
   CanonicalizationAlgorithmType,
+  CanonicalizationOrTransformAlgorithmType,
   CanonicalizationOrTransformationAlgorithm,
+  CanonicalizationOrTransformationAlgorithmProcessOptions,
   ComputeSignatureOptions,
+  ErrorFirstCallback,
   GetKeyInfoContentArgs,
   HashAlgorithm,
   HashAlgorithmType,
   Reference,
   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);
 

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("[email protected]");
+    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);
   });
 });