NMS-20206: Fix XXE in XML collector - #8782
Conversation
Disable external entity/DTD resolution in AbstractXmlCollectionHandler's DocumentBuilderFactory and TransformerFactory so collected XML cannot read local files or trigger out-of-band requests. Avoids disallow-doctype-decl to keep pre-parse-html working. Adds regression tests.
marshallmassengill
left a comment
There was a problem hiding this comment.
You may want to have the comments shortened/adjusted but this looks good.
marshallmassengill
left a comment
There was a problem hiding this comment.
Found a couple blockers on deeper review:
-
setAttribute(ACCESS_EXTERNAL_*) throws at runtime — xslt-source-file collection breaks entirely. AbstractXmlCollectionHandler.java:541-542. Xalan 2.7.3 ships in $OPENNMS_HOME/lib/ and registers META-INF/services/javax.xml.transform.TransformerFactory, so TransformerFactory.newInstance() resolves to org.apache.xalan.processor.TransformerFactoryImpl in the collectd JVM — which rejects both properties with IllegalArgumentException: Not supported: http://javax.xml.XMLConstants/property/accessExternalDTD (reproduced on JDK 11 and 21). Unchecked, so it propagates out of applyXsltTransformation and fails every collection cycle. Wrap each setAttribute in its own try/catch, as OWASP's cheat sheet does.
-
setExpandEntityReferences(false) silently corrupts collected values. AbstractXmlCollectionHandler.java:502. With the real runtime stack, + v&ver; makes xpath.evaluate(..., XPathConstants.STRING) — line 231, how every collected value is read — return v instead of v1.2.3. Reproduced on JDK 11, 17 and 21; also on the JDK parser alone at 17/21. No error, just wrong metrics. It contributes nothing to the fix either: the three feature flags already block external entities, both XXE payloads stay blocked with this line removed. Delete it.
Disable external entity/DTD resolution in the DocumentBuilder and, for the xslt-source-file path, parse the stylesheet and collected source through a hardened XMLReader (SAXSource) so XXE is blocked even under Xalan. Adds regression tests.
marshallmassengill
left a comment
There was a problem hiding this comment.
Still looks like one blocker:
The try/catch makes the XSLT path silently unprotected in production, which is worse than the crash it replaced. AbstractXmlCollectionHandler.java:536-544. In a real install Xalan 2.7.3 wins the JAXP lookup, so both setAttribute calls take the IllegalArgumentException branch and log at debug. FEATURE_SECURE_PROCESSING alone does not stop Xalan resolving external entities in the source document — verified on JDK 11 and 21 with $OPENNMS_HOME/lib/'s actual jars:
impl=org.apache.xalan.processor.TransformerFactoryImpl
set accessExternalDTD SKIPPED (IAE)
RESULT leaked=true -> <out>TOP_SECRET_SENTINEL</out>
The leaked file content lands in the transform output, which then becomes the DOM. So for any source configured with xslt-source-file, NMS-20206 is still exploitable after this PR. testXsltTransformationStillWorks can't catch it: surefire resolves the JDK's XSLTC, where the attributes apply and the transform is blocked.
Possible fix: stop feeding untrusted bytes to the transformer's own parser. Parse with the DocumentBuilderFactory that is already hardened and pass a DOMSource:
DOMSource source = new DOMSource(hardenedBuilder.parse(is));
transformer.transform(source, new StreamResult(baos));
marshallmassengill
left a comment
There was a problem hiding this comment.
Found another blocker:
Blocked references fail open with corrupt output, and nothing is logged. applyXsltTransformation (AbstractXmlCollectionHandler.java:522) sets no ErrorListener, so the TransformerException thrown by denyExternal (line 535) goes to Xalan's default listener, which prints to System.err and lets the transform continue.
No exception escapes applyXsltTransformation in any of these cases. Collection then either records wrong values or dies downstream in the DOM parse with SAXParseException: XML document structures must start and end within the same entity, whose cause appears only on stderr. Set an ErrorListener on both the factory and the transformer that logs via LOG and rethrows, so a blocked reference fails the collection loudly.
Disable external entity/DTD resolution in AbstractXmlCollectionHandler's DocumentBuilderFactory and TransformerFactory so collected XML cannot read local files or trigger out-of-band requests.
Adds regression tests.
External References