diff --git a/README.md b/README.md index c7e6f77c..26a33a83 100644 --- a/README.md +++ b/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 `` 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. - @@ -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 diff --git a/test/signature-integration-tests.spec.ts b/test/signature-integration-tests.spec.ts index 2fd41c54..a848f103 100644 --- a/test/signature-integration-tests.spec.ts +++ b/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( + 'Harry Potter', + ); + const signed = signer + .getSignedXml() + .replace("", 'Forged'); + + 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/, + ); + }); }); diff --git a/test/signature-unit-tests.spec.ts b/test/signature-unit-tests.spec.ts index 269f97e5..4d16b442 100644 --- a/test/signature-unit-tests.spec.ts +++ b/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(//, ""), + "could not find any Reference elements", + ], + [ + "no CanonicalizationMethod", + () => malform(/]*\/>/, ""), + "could not find CanonicalizationMethod/@Algorithm element", + ], + [ + "a Reference without DigestMethod", + () => malform(/]*\/>/, ""), + /^could not find DigestMethod in reference /, + ], + [ + "a DigestMethod without Algorithm", + () => malform(/]*\/>/, ""), + /^could not find Algorithm attribute in node /, + ], + [ + "a Reference without DigestValue", + () => malform(/[^<]*<\/DigestValue>/, ""), + /^could not find DigestValue node in reference /, + ], + [ + "a Reference with two DigestValues", + () => malform(/[^<]*<\/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 = "xml-cryptogithub"; - 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("")).to.throw( + "signatureAlgorithm is required", + ); + }); + it("should sign references when the Id attribute is prefixed", () => { const xml = ''; const sig = new SignedXml({