Repository navigation
Xml-crypto doesnt properly handle dtd #524
Description
Xml-crypto uses xmldom to parse xml. Xmldom does not support dtd, however dtds can affect the meaning of the document.
For example:
<foo>&:bar;</foo>And
<!DOCTYPE foo [<!ENTITY :bar "admin">]>
<foo>&:bar;</foo>Have different meanings and different canonical forms. xml-crypto should give these documents different hashes and different digital signatures. However it thinks they are the same.
This is not a huge deal in practise as most downstream users also use xmldom. However it could be a big deal if a downstream user was parsing documents with something more compliant with the xml spec like happydom
Suggested fix: just error if the doc has a dtd, its an obscure feature nobody uses. If that is too restrictive (some people add them as document identifiers) error on dtds containing an internal subset.
Activity
markstos commentedon Nov 11, 2025
I chatted with the GPT-OSS model, and it agreed with the feedback here, suggesting:
Since it DTD parsing already wasn't working, we aren't breaking any existing cases, only behavior more strictly and "secure by default" for this case.
shunkica commentedon Jan 27, 2026
This is invalid xml:
Per https://www.w3.org/TR/REC-xml-names/#Conformance
bawolff commentedon Jan 27, 2026
Its not namespace well formed but it would be non-namespace well formed. Namespaces are an optional part of xml.
Regardless most xml implementations seem to accept entity names with colons in them.
In any case, if you are really concerned you could relace the colon with À (as in the pre-composed U+00C0, not the decomposed form) and do the exact same thing.
shunkica commentedon Jan 27, 2026
C14N is inherently namespace-aware, therefore namespace well-formedness is a prerequisite for proper C14N. As is always the case with XML, your mileage may vary with different implementations...
In any case, just throwing an error if the doc has a DTD would be an over-restriction, because it would also break cases where the DTD does not affect the content which is being signed.
Migrating from
xmldomtolibxml2-wasmcould solve this problem, aslibxml2-wasmsupports expanding entities and has built-in canonicalization.ahacker1-securesaml commentedon Apr 8, 2026
I disagree with migrating to libxml2-wasm. libxml2 is a big codebase with lots of potential vulnerabilities (memory leaks ...).
let's keep with xmldom.
And we shouldn't support loading external entities in XML.
cjbarth commentedon Apr 9, 2026
If we don't support DTD, then it is reasonable if we throw an exception when we encounter one; that is the safer path against unexpected behavior. We could include a flag to allow ignoring DTD, which would restore the current behavior. A PR for this is welcome IMO.
cjbarth commentedon Oct 13, 2026
Decision for 7.0:
computeSignature()andcheckSignature()throw when the document has a DOCTYPE. Any DOCTYPE, and with no option to allow one.Why throw. XML 1.0 §5.1 requires a processor to expand the entities and supply the default attributes that the internal subset declares, and Canonical XML §1.1 depends on that. xmldom does neither, so for such a document the canonical form here is not the one the specifications define. The conformant choices are to process the declaration or to refuse the document. Processing it means building DTD support on top of xmldom, which nobody has asked for and which brings entity-expansion attacks with it. Refusing keeps every signature this library does produce or verify correct.
Why any DOCTYPE, and not only one with an internal subset. Canonical XML §2.1 requires the behavior of a validating processor, which reads an external subset too.
<!DOCTYPE foo SYSTEM "x.dtd">can change the canonical form as much as an internal subset can.Why no opt-out. An option to allow a DOCTYPE would switch the incorrect canonical form back on. It can be added if someone shows a need for it.
Where. In the two methods that parse a document themselves. The canonicalization classes take a DOM the caller parsed, and are not changed by this.
This rejects documents that 6.x signs and verifies, so 6.4 warns first: #661.