test: cover spec-required Signature elements and fail-closed signing errors

README.md

@@ -294,13 +294,18 @@ If you keep failing verification, it is worth trying to guess such a hidden tran
 
 ```javascript
 const sig = new SignedXml({
-  implicitTransforms: ["http://www.w3.org/TR/2001/REC-xml-c14n-20010315"],
+  implicitTransforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"],
   publicCert: fs.readFileSync("client_public.pem"),
 });
 sig.loadSignature(signature);
 const res = sig.checkSignature(xml);
 ```
 
+Implicit transforms run after the transforms a `<Reference>` declares. xml-crypto converts a
+node-set left after the last transform to octets with Canonical XML 1.0, so an implicit
+`http://www.w3.org/TR/2001/REC-xml-c14n-20010315` changes nothing where the transforms end in a
+node-set, such as when there are none or the last one is enveloped-signature.
+
 You might find it difficult to guess such transforms, but there are typical transforms you can try.
 
 - <http://www.w3.org/TR/2001/REC-xml-c14n-20010315>
@@ -315,7 +320,7 @@ You might find it difficult to guess such transforms, but there are typical tran
 The `SignedXml` constructor provides an abstraction for sign and verify xml documents. The object is constructed using `new SignedXml(options?: SignedXmlOptions)` where the possible options are:
 
 - `idMode` - default `null` - if the value of `wssecurity` is passed it will create/validate id's with the ws-security namespace.
-- `idAttribute` - string - default `Id` or `ID` or `id` - the name of the attribute that contains the id of the element
+- `idAttribute` - string - default `undefined` - the name of an additional attribute that holds an element's id; it is checked before `Id`, `ID` and `id`
 - `privateKey` - string or Buffer - default `null` - the private key to use for signing
 - `publicCert` - string or Buffer - default `null` - the public certificate to use for verifying
 - `signatureAlgorithm` - string - the signature algorithm to use

test/signature-integration-tests.spec.ts

@@ -1154,4 +1154,36 @@ describe("Signature integration tests", function () {
       ]);
     });
   });
+
+  it("rejects a document where a default id attribute repeats the id held in idAttribute", function () {
+    const exclusiveC14n = "http://www.w3.org/2001/10/xml-exc-c14n#";
+    const idAttribute = "AssertionID";
+    const signer = new SignedXml({
+      idAttribute,
+      privateKey: fs.readFileSync("./test/static/client.pem"),
+      canonicalizationAlgorithm: exclusiveC14n,
+      signatureAlgorithm: "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256",
+    });
+    signer.addReference({
+      xpath: "//*[local-name(.)='book']",
+      transforms: [exclusiveC14n],
+      digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256",
+    });
+    signer.computeSignature(
+      '<library><book AssertionID="b1"><title>Harry Potter</title></book></library>',
+    );
+    const signed = signer
+      .getSignedXml()
+      .replace("</library>", '<book Id="b1"><title>Forged</title></book></library>');
+
+    const verifier = new SignedXml({
+      idAttribute,
+      publicCert: fs.readFileSync("./test/static/client_public.pem"),
+    });
+    verifier.loadSignature(signer.getSignatureXml());
+
+    expect(() => verifier.checkSignature(signed)).to.throw(
+      /in order to prevent signature wrapping attack/,
+    );
+  });
 });

test/signature-unit-tests.spec.ts

@@ -1071,6 +1071,66 @@ describe("Signature unit tests", function () {
         failInvalidSignature("./test/static/invalid_signature_without_transforms_element.xml");
       });
     });
+
+    describe("reject malformed signature", function () {
+      const validXml = fs.readFileSync("./test/static/valid_signature.xml", "utf8");
+      const publicCert = fs.readFileSync("./test/static/client_public.pem");
+
+      function signatureOf(xml: string): Node {
+        const signature = xpath.select1(
+          "//*[local-name(.)='Signature' and namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#']",
+          new xmldom.DOMParser().parseFromString(xml),
+        );
+        isDomNode.assertIsNodeLike(signature);
+        return signature;
+      }
+
+      function malform(pattern: RegExp, replacement: string): string {
+        expect(validXml).to.match(pattern);
+        return validXml.replace(pattern, replacement);
+      }
+
+      const cases: Array<[string, () => string, string | RegExp]> = [
+        [
+          "no Reference",
+          () => malform(/<Reference .*<\/Reference>/, ""),
+          "could not find any Reference elements",
+        ],
+        [
+          "no CanonicalizationMethod",
+          () => malform(/<CanonicalizationMethod [^>]*\/>/, ""),
+          "could not find CanonicalizationMethod/@Algorithm element",
+        ],
+        [
+          "a Reference without DigestMethod",
+          () => malform(/<DigestMethod [^>]*\/>/, ""),
+          /^could not find DigestMethod in reference /,
+        ],
+        [
+          "a DigestMethod without Algorithm",
+          () => malform(/<DigestMethod [^>]*\/>/, "<DigestMethod/>"),
+          /^could not find Algorithm attribute in node /,
+        ],
+        [
+          "a Reference without DigestValue",
+          () => malform(/<DigestValue>[^<]*<\/DigestValue>/, ""),
+          /^could not find DigestValue node in reference /,
+        ],
+        [
+          "a Reference with two DigestValues",
+          () => malform(/<DigestValue>[^<]*<\/DigestValue>/, "$&$&"),
+          /^could not load reference for a node that contains multiple DigestValue nodes: /,
+        ],
+      ];
+
+      for (const [problem, xml, error] of cases) {
+        it(`with ${problem}`, function () {
+          const sig = new SignedXml({ publicCert });
+
+          expect(() => sig.loadSignature(signatureOf(xml()))).to.throw(error);
+        });
+      }
+    });
   });
 
   it("allow empty reference uri when signing", function () {
@@ -1098,30 +1158,6 @@ describe("Signature unit tests", function () {
     expect(URI.value, `uri should be empty but instead was ${URI.value}`).to.equal("");
   });
 
-  it("signer appends signature to a non-existing reference node", function () {
-    const xml = "<root><name>xml-crypto</name><repository>github</repository></root>";
-    const sig = new SignedXml();
-
-    sig.privateKey = fs.readFileSync("./test/static/client.pem");
-    sig.addReference({
-      xpath: "//*[local-name(.)='repository']",
-      digestAlgorithm: "http://www.w3.org/2000/09/xmldsig#sha1",
-      transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"],
-    });
-
-    try {
-      sig.computeSignature(xml, {
-        location: {
-          reference: "/root/foobar",
-          action: "append",
-        },
-      });
-      expect.fail("Expected an error to be thrown");
-    } catch (err) {
-      expect(err).not.to.be.an.instanceof(TypeError);
-    }
-  });
-
   it("signer adds existing prefixes", function () {
     function getKeyInfoContentWithAssertionId({ assertionId }) {
       return (
@@ -1460,6 +1496,34 @@ describe("Signature unit tests", function () {
     );
   });
 
+  it("should throw if a reference has no digestAlgorithm", () => {
+    const sig = new SignedXml();
+
+    expect(() =>
+      sig.addReference({
+        xpath: "//*[local-name(.)='x']",
+        transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"],
+      }),
+    ).to.throw("digestAlgorithm is required");
+  });
+
+  it("should throw if signing without a signatureAlgorithm", () => {
+    const sig = new SignedXml({
+      privateKey: fs.readFileSync("./test/static/client.pem"),
+      canonicalizationAlgorithm: "http://www.w3.org/2001/10/xml-exc-c14n#",
+    });
+
+    sig.addReference({
+      xpath: "//*[local-name(.)='x']",
+      digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256",
+      transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"],
+    });
+
+    expect(() => sig.computeSignature("<root><x/></root>")).to.throw(
+      "signatureAlgorithm is required",
+    );
+  });
+
   it("should sign references when the Id attribute is prefixed", () => {
     const xml = '<root><x xmlns:ns="urn:example" ns:Id="unique-id"/></root>';
     const sig = new SignedXml({