Skip to content

Commit 34e739f

Browse files
committed
Reset per-signature state in loadSignature
signatureAlgorithm, canonicalizationAlgorithm, signatureValue, and keyInfo were only assigned when the corresponding node existed in the signature being loaded, so a reused instance silently inherited values from a previously loaded signature when the new one omitted them. A document whose <SignatureValue> was emptied would still verify against the value left over from the previous verification. Reset all four fields at the top of loadSignature and add a regression test covering the stale-SignatureValue case on a reused verifier.
1 parent 16520bc commit 34e739f

2 files changed

Lines changed: 32 additions & 0 deletions

File tree

src/signed-xml.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -702,6 +702,16 @@ export class SignedXml {
702702
* @param signatureNode The XML node or string representing the signature.
703703
*/
704704
loadSignature(signatureNode: Node | string): void {
705+
// Reset all per-signature state before parsing. The fields below are only
706+
// assigned when the corresponding node exists in the new signature, so
707+
// without this reset a reused instance would silently inherit values
708+
// (algorithm, signature value, key material) from a previously loaded
709+
// signature when the new one omits them.
710+
this.signatureAlgorithm = undefined;
711+
this.canonicalizationAlgorithm = undefined;
712+
this.signatureValue = "";
713+
this.keyInfo = null;
714+
705715
const signatureNodeParsed =
706716
typeof signatureNode === "string" ? utils.parseXml(signatureNode) : signatureNode;
707717
this.signatureNode = signatureNodeParsed;

test/xmldsig-verifier.spec.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -405,6 +405,28 @@ describe("XmlDSigVerifier", function () {
405405
expectValidResult(verifier.verifySignature(signedXml), 1);
406406
});
407407

408+
it("does not reuse the previous document's SignatureValue when the next document omits it", function () {
409+
const signedXml = createChainSignedXml(xml);
410+
// Same document, but with an emptied <SignatureValue>. A verifier that
411+
// leaks state would fall back to the value loaded on the previous call
412+
// and wrongly report success.
413+
const stripped = signedXml.replace(
414+
/<SignatureValue>[^<]*<\/SignatureValue>/,
415+
"<SignatureValue></SignatureValue>",
416+
);
417+
expect(stripped).to.not.equal(signedXml);
418+
419+
const verifier = new XmlDSigVerifier({
420+
keySelector: {
421+
getCertFromKeyInfo: () => chainPublicCert,
422+
},
423+
security: { truststore: [rootCert] },
424+
});
425+
426+
expectValidResult(verifier.verifySignature(signedXml));
427+
expectInvalidResult(verifier.verifySignature(stripped), "invalid signature");
428+
});
429+
408430
it("verifies using a certificate actually extracted from the document's KeyInfo", function () {
409431
// Unlike the constant-callback tests above, this exercises the real
410432
// extraction path: the cert comes out of <KeyInfo>, not the test closure.

0 commit comments

Comments
 (0)