Fix transform processing regression (#379)

node-saml · Sep 14, 2023 · 110dd7c · 110dd7c
1 parent fa2922f
commit 110dd7c
Show file tree

Hide file tree

Showing 9 changed files with 111 additions and 82 deletions.
diff --git a/src/c14n-canonicalization.ts b/src/c14n-canonicalization.ts
@@ -167,11 +167,14 @@ export class C14nCanonicalization implements CanonicalizationOrTransformationAlg
     return { rendered: res.join(""), newDefaultNs };
   }
 
-  processInner(node: Node, prefixesInScope, defaultNs, defaultNsForPrefix, ancestorNamespaces) {
+  /**
+   * @param node Node
+   */
+  processInner(node, prefixesInScope, defaultNs, defaultNsForPrefix, ancestorNamespaces) {
     if (xpath.isComment(node)) {
       return this.renderComment(node);
     }
-    if (xpath.isComment(node)) {
+    if (node.data) {
       return utils.encodeSpecialCharactersInText(node.data);
     }
 
@@ -198,7 +201,7 @@ export class C14nCanonicalization implements CanonicalizationOrTransformationAlg
       return res.join("");
     }
 
-    return "";
+    throw new Error(`Unable to canonicalize node type: ${node.nodeType}`);
   }
 
   // Thanks to deoxxa/xml-c14n for comment renderer
@@ -247,7 +250,7 @@ export class C14nCanonicalization implements CanonicalizationOrTransformationAlg
    * @param node
    * @api public
    */
-  process(node: Node, options: CanonicalizationOrTransformationAlgorithmProcessOptions) {
+  process(node: Node, options: CanonicalizationOrTransformationAlgorithmProcessOptions): string {
     options = options || {};
     const defaultNs = options.defaultNs || "";
     const defaultNsForPrefix = options.defaultNsForPrefix || {};

diff --git a/src/enveloped-signature.ts b/src/enveloped-signature.ts
@@ -8,7 +8,7 @@ import type {
 
 export class EnvelopedSignature implements CanonicalizationOrTransformationAlgorithm {
   includeComments = false;
-  process(node: Node, options: CanonicalizationOrTransformationAlgorithmProcessOptions) {
+  process(node: Node, options: CanonicalizationOrTransformationAlgorithmProcessOptions): Node {
     if (null == options.signatureNode) {
       const signature = xpath.select1(
         "./*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']",

diff --git a/src/exclusive-canonicalization.ts b/src/exclusive-canonicalization.ts
@@ -167,6 +167,9 @@ export class ExclusiveCanonicalization implements CanonicalizationOrTransformati
     return { rendered: res.join(""), newDefaultNs: newDefaultNs };
   }
 
+  /**
+   * @param node Node
+   */
   processInner(
     node,
     prefixesInScope,
@@ -181,32 +184,36 @@ export class ExclusiveCanonicalization implements CanonicalizationOrTransformati
       return utils.encodeSpecialCharactersInText(node.data);
     }
 
-    let i;
-    let pfxCopy;
-    const ns = this.renderNs(
-      node,
-      prefixesInScope,
-      defaultNs,
-      defaultNsForPrefix,
-      inclusiveNamespacesPrefixList,
-    );
-    const res = ["<", node.tagName, ns.rendered, this.renderAttrs(node), ">"];
-
-    for (i = 0; i < node.childNodes.length; ++i) {
-      pfxCopy = prefixesInScope.slice(0);
-      res.push(
-        this.processInner(
-          node.childNodes[i],
-          pfxCopy,
-          ns.newDefaultNs,
-          defaultNsForPrefix,
-          inclusiveNamespacesPrefixList,
-        ),
+    if (xpath.isElement(node)) {
+      let i;
+      let pfxCopy;
+      const ns = this.renderNs(
+        node,
+        prefixesInScope,
+        defaultNs,
+        defaultNsForPrefix,
+        inclusiveNamespacesPrefixList,
       );
+      const res = ["<", node.tagName, ns.rendered, this.renderAttrs(node), ">"];
+
+      for (i = 0; i < node.childNodes.length; ++i) {
+        pfxCopy = prefixesInScope.slice(0);
+        res.push(
+          this.processInner(
+            node.childNodes[i],
+            pfxCopy,
+            ns.newDefaultNs,
+            defaultNsForPrefix,
+            inclusiveNamespacesPrefixList,
+          ),
+        );
+      }
+
+      res.push("</", node.tagName, ">");
+      return res.join("");
     }
 
-    res.push("</", node.tagName, ">");
-    return res.join("");
+    throw new Error(`Unable to exclusive canonicalize node type: ${node.nodeType}`);
   }
 
   // Thanks to deoxxa/xml-c14n for comment renderer
@@ -250,13 +257,11 @@ export class ExclusiveCanonicalization implements CanonicalizationOrTransformati
   }
 
   /**
-   * Perform canonicalization of the given node
+   * Perform canonicalization of the given element node
    *
-   * @param {Node} node
-   * @return {String}
    * @api public
    */
-  process(node, options: CanonicalizationOrTransformationAlgorithmProcessOptions) {
+  process(elem: Element, options: CanonicalizationOrTransformationAlgorithmProcessOptions): string {
     options = options || {};
     let inclusiveNamespacesPrefixList = options.inclusiveNamespacesPrefixList || [];
     const defaultNs = options.defaultNs || "";
@@ -267,7 +272,7 @@ export class ExclusiveCanonicalization implements CanonicalizationOrTransformati
      * If the inclusiveNamespacesPrefixList has not been explicitly provided then look it up in CanonicalizationMethod/InclusiveNamespaces
      */
     if (!utils.isArrayHasLength(inclusiveNamespacesPrefixList)) {
-      const CanonicalizationMethod = utils.findChildren(node, "CanonicalizationMethod");
+      const CanonicalizationMethod = utils.findChildren(elem, "CanonicalizationMethod");
       if (CanonicalizationMethod.length !== 0) {
         const inclusiveNamespaces = utils.findChildren(
           CanonicalizationMethod[0],
@@ -289,7 +294,7 @@ export class ExclusiveCanonicalization implements CanonicalizationOrTransformati
         if (ancestorNamespaces) {
           ancestorNamespaces.forEach(function (ancestorNamespace) {
             if (prefix === ancestorNamespace.prefix) {
-              node.setAttributeNS(
+              elem.setAttributeNS(
                 "http://www.w3.org/2000/xmlns/",
                 `xmlns:${prefix}`,
                 ancestorNamespace.namespaceURI,
@@ -301,7 +306,7 @@ export class ExclusiveCanonicalization implements CanonicalizationOrTransformati
     }
 
     const res = this.processInner(
-      node,
+      elem,
       [],
       defaultNs,
       defaultNsForPrefix,

diff --git a/src/signed-xml.ts b/src/signed-xml.ts
@@ -471,12 +471,11 @@ export class SignedXml {
   }
 
   validateReferences(doc: Document) {
-    for (const ref of this.references) {
-      if (!this.validateReference(ref, doc)) {
-        return false;
-      }
-    }
-    return true;
+    return (
+      Array.isArray(this.references) &&
+      this.references.length > 0 &&
+      this.references.every((ref) => this.validateReference(ref, doc))
+    );
   }
 
   /**
@@ -595,39 +594,38 @@ export class SignedXml {
           .flatMap((namespace) => (namespace.getAttribute("PrefixList") ?? "").split(" "))
           .filter((value) => value.length > 0);
       }
+    }
 
-      if (utils.isArrayHasLength(this.implicitTransforms)) {
-        this.implicitTransforms.forEach(function (t) {
-          transforms.push(t);
-        });
-      }
-
-      /**
-       * DigestMethods take an octet stream rather than a node set. If the output of the last transform is a node set, we
-       * need to canonicalize the node set to an octet stream using non-exclusive canonicalization. If there are no
-       * transforms, we need to canonicalize because URI dereferencing for a same-document reference will return a node-set.
-       * @see:
-       * https://www.w3.org/TR/xmldsig-core1/#sec-DigestMethod
-       * https://www.w3.org/TR/xmldsig-core1/#sec-ReferenceProcessingModel
-       * https://www.w3.org/TR/xmldsig-core1/#sec-Same-Document
-       */
-      if (
-        transforms.length === 0 ||
-        transforms[transforms.length - 1] ===
-          "http://www.w3.org/2000/09/xmldsig#enveloped-signature"
-      ) {
-        transforms.push("http://www.w3.org/TR/2001/REC-xml-c14n-20010315");
-      }
-
-      this.addReference({
-        transforms,
-        digestAlgorithm: digestAlgo,
-        uri: xpath.isElement(refNode) ? utils.findAttr(refNode, "URI")?.value : undefined,
-        digestValue,
-        inclusiveNamespacesPrefixList,
-        isEmptyUri: false,
+    if (utils.isArrayHasLength(this.implicitTransforms)) {
+      this.implicitTransforms.forEach(function (t) {
+        transforms.push(t);
       });
     }
+
+    /**
+     * DigestMethods take an octet stream rather than a node set. If the output of the last transform is a node set, we
+     * need to canonicalize the node set to an octet stream using non-exclusive canonicalization. If there are no
+     * transforms, we need to canonicalize because URI dereferencing for a same-document reference will return a node-set.
+     * @see:
+     * https://www.w3.org/TR/xmldsig-core1/#sec-DigestMethod
+     * https://www.w3.org/TR/xmldsig-core1/#sec-ReferenceProcessingModel
+     * https://www.w3.org/TR/xmldsig-core1/#sec-Same-Document
+     */
+    if (
+      transforms.length === 0 ||
+      transforms[transforms.length - 1] === "http://www.w3.org/2000/09/xmldsig#enveloped-signature"
+    ) {
+      transforms.push("http://www.w3.org/TR/2001/REC-xml-c14n-20010315");
+    }
+
+    this.addReference({
+      transforms,
+      digestAlgorithm: digestAlgo,
+      uri: xpath.isElement(refNode) ? utils.findAttr(refNode, "URI")?.value : undefined,
+      digestValue,
+      inclusiveNamespacesPrefixList,
+      isEmptyUri: false,
+    });
   }
 
   /**

diff --git a/test/c14nWithComments-unit-tests.spec.ts b/test/c14nWithComments-unit-tests.spec.ts
@@ -9,9 +9,12 @@ const compare = function (xml, xpathArg, expected, inclusiveNamespacesPrefixList
   const doc = new xmldom.DOMParser().parseFromString(xml);
   const elem = xpath.select1(xpathArg, doc);
   const can = new c14nWithComments();
-  const result = can.process(elem, { inclusiveNamespacesPrefixList }).toString();
-
-  expect(result).to.equal(expected);
+  if (xpath.isElement(elem)) {
+    const result = can.process(elem, { inclusiveNamespacesPrefixList }).toString();
+    expect(result).to.equal(expected);
+  } else {
+    throw new Error("Element not found.");
+  }
 };
 
 describe("Exclusive canonicalization with comments", function () {

diff --git a/test/canonicalization-unit-tests.spec.ts b/test/canonicalization-unit-tests.spec.ts
@@ -15,14 +15,18 @@ const compare = function (
   const doc = new xmldom.DOMParser().parseFromString(xml);
   const elem = xpath.select1(xpathArg, doc);
   const can = new ExclusiveCanonicalization();
-  const result = can
-    .process(elem, {
-      inclusiveNamespacesPrefixList,
-      defaultNsForPrefix,
-    })
-    .toString();
-
-  expect(expected).to.equal(result);
+  if (xpath.isElement(elem)) {
+    const result = can
+      .process(elem, {
+        inclusiveNamespacesPrefixList,
+        defaultNsForPrefix,
+      })
+      .toString();
+
+    expect(expected).to.equal(result);
+  } else {
+    throw new Error("Invalid element");
+  }
 };
 
 describe("Canonicalization unit tests", function () {

diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts
@@ -877,6 +877,10 @@ describe("Signature unit tests", function () {
     passValidSignature("./test/static/valid_signature_with_unused_prefixes.xml");
   });
 
+  it("verifies valid signature without transforms element", 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");
   });
@@ -918,6 +922,10 @@ describe("Signature unit tests", function () {
     );
   });
 
+  it("fails invalid signature without transforms element", function () {
+    failInvalidSignature("./test/static/invalid_signature_without_transforms_element.xml");
+  });
+
   it("allow empty reference uri when signing", function () {
     const xml = "<root><x /></root>";
     const sig = new SignedXml();

diff --git a/test/static/invalid_signature_without_transforms_element.xml b/test/static/invalid_signature_without_transforms_element.xml
@@ -0,0 +1,4 @@
+<?xml version="1.0"?>
+<library><book ID="bookid"><name>some tampered text</name></book><Signature xmlns="http://www.w3.org/2000/09/xmldsig#"><SignedInfo><CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/><SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/><Reference URI="#bookid"><DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/><DigestValue>LsMoqo1d6Sqh8DKLp00MK0fSBDA=</DigestValue></Reference></SignedInfo><SignatureValue>OR1SYcyU18qELj+3DX/bW/r5DqueuyPAnNFEh3hNKFaj8ZKLB/mdsR9w8GDBCmZ2
+lsCTEvJqWC37oF8rm2eBSonNbdBnA+TM6Y22C8rffVzaoM3zpNoeWMH2LwFmpdKB
+UXOMWVExEaz/s4fOcyv1ajVuk42I3nl0xcD95+i7PjY=</SignatureValue></Signature></library>
diff --git a/test/static/valid_signature_without_transforms_element.xml b/test/static/valid_signature_without_transforms_element.xml
@@ -0,0 +1,4 @@
+<?xml version="1.0"?>
+<library><book ID="bookid"><name>some text</name></book><Signature xmlns="http://www.w3.org/2000/09/xmldsig#"><SignedInfo><CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/><SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/><Reference URI="#bookid"><DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/><DigestValue>LsMoqo1d6Sqh8DKLp00MK0fSBDA=</DigestValue></Reference></SignedInfo><SignatureValue>OR1SYcyU18qELj+3DX/bW/r5DqueuyPAnNFEh3hNKFaj8ZKLB/mdsR9w8GDBCmZ2
+lsCTEvJqWC37oF8rm2eBSonNbdBnA+TM6Y22C8rffVzaoM3zpNoeWMH2LwFmpdKB
+UXOMWVExEaz/s4fOcyv1ajVuk42I3nl0xcD95+i7PjY=</SignatureValue></Signature></library>