checkSignature leaves unauthenticated content in getSignedReferences() when it throws

[cjbarth]

Summary

getSignedReferences() is meant to be the one output of verification a caller can trust. signedReferences "populates with the canonical XML of the reference only after verifying the signature is cryptographically authentic". That does not hold when checkSignature throws.

Each reference is digested, and its canonical XML pushed into signedReferences and ref.signedReference, before the signature algorithm and key are resolved and before SignatureValue is checked. Only the two failure paths that do not throw (return false and invalid signature: …) clear them. Any exception thrown after the first reference has been digested leaves unauthenticated content behind.

Digests are not keyed, so that content is attacker-chosen: making a reference's digest match needs no key.

Affected versions

getSignedReferences() shipped in 6.1.0 (#495). 6.1.0 through 6.1.2 and master (2ac129b) are affected, and the 6.x branch has the same ordering.

Who is exposed

• Not affected: a caller that treats a throw as failure and does not read getSignedReferences() afterwards, which is the pattern the README shows. As far as I can tell node-saml is not affected either: src/xml.ts builds a new SignedXml per certificate and reads getSignedReferences() only after checkSignature returns true.
• Affected: a caller that catches the exception and then reads getSignedReferences() or ref.signedReference, or that treats a non-empty getSignedReferences() as success, gets content nobody signed. So does one that keeps using the same SignedXml instance after a throw.

Triggers

Each is a throw reached after at least one reference has been digested.

Error	Who controls it
signature algorithm '…' is not supported	the document: any unregistered SignatureMethod, including hmac-sha1, which is off until enableHMAC()
signatureAlgorithm is required	the document: omit SignatureMethod, unless the verifier configures one
hash algorithm '…' is not supported, canonicalization algorithm '…' is not supported on a later Reference	the document
Cannot validate a document which contains multiple elements with the same value for the ID … on a later Reference	the document; the wrapping guard itself leaks the references checked before it
KeyInfo or publicCert or privateKey is required to validate signature	verifier configuration
anything thrown by getCertFromKeyInfo or verifySignature	e.g. Unknown DER format. from SignedXml.getCertFromKeyInfo given a malformed embedded certificate

For the Reference rows the document does not even need a valid signature, because SignedInfo is never checked.

Reproduction

import { SignedXml } from "xml-crypto";
import * as fs from "fs";

const publicCert = fs.readFileSync("./test/static/client_public.pem");

// The verifier never gets as far as checking the HMAC, so the key does not matter.
const forger = new SignedXml({
  privateKey: "any key at all",
  canonicalizationAlgorithm: "http://www.w3.org/2001/10/xml-exc-c14n#",
  signatureAlgorithm: "http://www.w3.org/2000/09/xmldsig#hmac-sha1",
});
forger.enableHMAC();
forger.addReference({
  xpath: "//*[local-name(.)='assertion']",
  transforms: ["http://www.w3.org/2001/10/xml-exc-c14n#"],
  digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256",
});
forger.computeSignature("<response><assertion>forged</assertion></response>");

const sig = new SignedXml({ publicCert });
sig.loadSignature(forger.getSignatureXml());
try {
  sig.checkSignature(forger.getSignedXml());
} catch (e) {
  console.log(e.message); // signature algorithm 'http://www.w3.org/2000/09/xmldsig#hmac-sha1' is not supported
}
console.log(sig.getSignedReferences()); // [ '<assertion Id="_0">forged</assertion>' ]

Expected: []
Actual: the forged assertion.

Root cause

validateReference (src/signed-xml.ts:595-596) pushes each reference whose digest matches into this.signedReferences and sets ref.signedReference as it goes. checkSignature then resolves the signature algorithm (:357) and the key (:358-361), and verifies SignatureValue (:364).

The comment at :372-374 acknowledges the ordering and plans to verify SignedInfo first in 7.x. Until then, the non-throwing failure paths at :336-339 and :375-378 clear the arrays to compensate, but no exception path does.

Suggested fix (non-breaking)

Keep the order of the checks, so every document still gets the same return value or error it gets today, but publish nothing until SignatureValue has verified:

• Collect each reference's canonical XML locally, and assign signedReferences and ref.signedReference only on success.
• Start each check with signedReferences empty, so a check that throws leaves it empty, as return false already does.
• Have a successful check keep appending to what earlier successful checks on the instance published, as it does today.

Verifying SignedInfo first, the 7.x plan, would also fix this, but it changes which error a document with more than one problem produces.

Tests

• For each trigger above: checkSignature throws the error it throws today, and afterwards getSignedReferences() is empty and every ref.signedReference is undefined.
• A check that throws does not leave behind the references of an earlier successful check on the same instance.

🤖 Generated with Claude Code