#WithComments keeps comments on URI="" and #id references #586

Summary

When a Reference uses URI="" or a bare-name URI="#id", xml-crypto hands the referenced element to its transforms with its comments still in it, and a #WithComments canonicalization digests them. XMLDSig 1.1 §4.4.3.3 says dereferencing either form MUST delete all comment nodes. Only the scheme-based XPointers #xpointer(/) and #xpointer(id('ID')) keep them.

As a result:

• xml-crypto treats those comments as signed. Editing one after signing breaks the signature, where a conforming verifier ignores it.
• A signature xml-crypto creates this way over content that contains a comment does not verify in conforming implementations, and xml-crypto rejects their signatures over the same content.

xml-crypto cannot resolve either XPointer form: validateReference throws on the quotes in #xpointer(id('ID')) and looks for an element whose ID is xpointer(/). So for every reference it can verify, comments should be gone before the first transform.

Reproduces on master @ 675263b and on 6.1.2. #585 does not change it.

Reproduction

Save as repro.js in the repo root and run npm run build && node repro.js. It signs one URI="" and one URI="#id" reference with exc-c14n#WithComments, then verifies each as signed and with the comment text edited.

const fs = require("fs");
const { DOMParser } = require("@xmldom/xmldom");
const { SignedXml } = require("./lib/index.js");

const ENVELOPED = "http://www.w3.org/2000/09/xmldsig#enveloped-signature";
const EXC_WITH_COMMENTS = "http://www.w3.org/2001/10/xml-exc-c14n#WithComments";

function sign(xml, reference) {
  const signer = new SignedXml({
    privateKey: fs.readFileSync("./test/static/client.pem"),
    canonicalizationAlgorithm: "http://www.w3.org/2001/10/xml-exc-c14n#",
    signatureAlgorithm: "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256",
  });
  signer.addReference({ ...reference, digestAlgorithm: "http://www.w3.org/2001/04/xmlenc#sha256" });
  signer.computeSignature(xml);
  return signer.getSignedXml();
}

function verify(label, xml) {
  const verifier = new SignedXml({ publicCert: fs.readFileSync("./test/static/client_public.pem") });
  verifier.loadSignature(verifier.findSignatures(new DOMParser().parseFromString(xml))[0]);
  const valid = verifier.checkSignature(xml);
  console.log(`${label} -> ${valid}`, valid ? verifier.getSignedReferences() : "");
}

const emptyUri = sign("<root><!-- draft --><x>1</x></root>", {
  xpath: "/*",
  isEmptyUri: true,
  transforms: [ENVELOPED, EXC_WITH_COMMENTS],
});
verify('URI=""  as signed      ', emptyUri);
verify('URI=""  comment edited ', emptyUri.replace("draft", "final"));

const idUri = sign("<root><item><!-- draft --><x>1</x></item></root>", {
  xpath: "//item",
  transforms: [EXC_WITH_COMMENTS],
});
verify('URI="#_0" as signed     ', idUri);
verify('URI="#_0" comment edited', idUri.replace("draft", "final"));

Actual output

URI=""  as signed       -> true [ '<root><!-- draft --><x>1</x></root>' ]
URI=""  comment edited  -> false
URI="#_0" as signed      -> true [ '<item Id="_0"><!-- draft --><x>1</x></item>' ]
URI="#_0" comment edited -> false

Expected output

URI=""  as signed       -> true [ '<root><x>1</x></root>' ]
URI=""  comment edited  -> true [ '<root><x>1</x></root>' ]
URI="#_0" as signed      -> true [ '<item Id="_0"><x>1</x></item>' ]
URI="#_0" comment edited -> true [ '<item Id="_0"><x>1</x></item>' ]

Prior art

Each of these removes comments for URI="" and bare-name references, and keeps them only for XPointers:

• .NET: Reference.CalculateHashValue in Reference.cs calls Utils.DiscardComments.
• xmlsec: xmlSecTransformCtxSetUri in transforms.c selects xmlSecNodeSetTreeWithoutComments unless the fragment starts with #xpointer( or #xmlns(.
• Apache Santuario: ResolverFragment calls setExcludeComments(true).

With .NET's SignedXml, both signatures from the repro fail to verify. .NET's own signatures over the same references verify in .NET after the comment is edited, and master rejects them.

Affected sites

Both reach the digest through getCanonXml, in src/signed-xml.ts:

Suggested fix

Remove comment nodes from the referenced subtree before the first transform, on both of those paths.

Don't do it for every getCanonXml call. getCanonSignedInfoXml (signed-xml.ts:430) uses it too, and a #WithComments CanonicalizationMethod over SignedInfo is not a dereferenced URI, so it keeps comments. getCanonXml already deep-clones the node, so stripping the clone leaves the caller's document untouched.

The digest changes for any #WithComments reference transform over content that contains a comment, so signatures created before and after the fix will not verify with each other. That needs a README Upgrading note, like the ones added in #579, #581 and #585.

Security considerations

After the fix, adding, removing or editing a comment inside content signed through a #WithComments reference transform no longer breaks the signature, so verification rejects less for those references. Every reference that uses exc-c14n or c14n without comments, the common case, already works that way.

Comments splitting signed text are a known SAML bypass: CERT VU#475445, for example CVE-2017-11427. The defence belongs in consumers, which must not let a comment split the text they read. Relying on #WithComments to make comments tamper-evident only works against xml-crypto, not against conforming verifiers.

Suggested test coverage

• URI="" and URI="#id" with each #WithComments algorithm: the comment is absent from getSignedReferences(), and editing it after signing still verifies.
• A signature whose SignedInfo gains a comment after signing still fails with a #WithComments CanonicalizationMethod, so the fix does not reach SignedInfo.