Skip to content

NMS-20206: Fix XXE in XML collector #8782

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cgorantla wants to merge 5 commits into
base: foundation-2023
Choose a base branch
from
Open

NMS-20206: Fix XXE in XML collector #8782

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
043fac5
NMS-20206: Fix XXE in XML collector
cgorantla Aug 12, 2026
4ea3f16
NMS-20206: Handle review comments
cgorantla Aug 12, 2026
32376ef
NMS-20206: Handle some more review comments
cgorantla Aug 12, 2026
e55f747
NMS-20206: Add some more protections
cgorantla Aug 12, 2026
134ccf5
NMS-20206: Update docs
cgorantla Aug 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 43 additions & 2 deletions ...s/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,19 +46,25 @@
import java.util.List;
import java.util.Map;

import javax.xml.XMLConstants;
import javax.xml.namespace.NamespaceContext;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.SAXParserFactory;
import javax.xml.transform.Source;
import javax.xml.transform.Transformer;
import javax.xml.transform.TransformerFactory;
import javax.xml.transform.sax.SAXSource;
import javax.xml.transform.stream.StreamResult;
import javax.xml.transform.stream.StreamSource;
import javax.xml.xpath.XPath;
import javax.xml.xpath.XPathConstants;
import javax.xml.xpath.XPathExpressionException;
import javax.xml.xpath.XPathFactory;

import org.xml.sax.InputSource;
import org.xml.sax.XMLReader;

import org.apache.commons.io.IOUtils;
import org.apache.commons.lang.StringUtils;
import org.joda.time.DateTime;
Expand Down Expand Up @@ -489,6 +495,13 @@ protected Document getXmlDocument(InputStream is, Request request) throws Except
is = preProcessHtml(request, is);
is = applyXsltTransformation(request, is);
DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
// Block XXE: forbid external entities/DTDs. Not disallow-doctype-decl, since
// pre-parse-html produces a benign <!DOCTYPE html>.
factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
factory.setXIncludeAware(false);
factory.setIgnoringComments(true);
factory.setNamespaceAware(true);
DocumentBuilder builder = factory.newDocumentBuilder();
Expand Down Expand Up @@ -524,17 +537,45 @@ protected InputStream applyXsltTransformation(Request request, InputStream is) t
if (!xsltFile.exists())
return is;
TransformerFactory factory = TransformerFactory.newInstance();
Source xslt = new StreamSource(xsltFile);
factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
// Best-effort; Xalan rejects these. The SAXSource readers below are the real guard.
try {
factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
} catch (IllegalArgumentException e) {
LOG.debug("TransformerFactory {} does not support {}; skipping", factory.getClass().getName(), XMLConstants.ACCESS_EXTERNAL_DTD);
}
try {
factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, "");
} catch (IllegalArgumentException e) {
LOG.debug("TransformerFactory {} does not support {}; skipping", factory.getClass().getName(), XMLConstants.ACCESS_EXTERNAL_STYLESHEET);
}
// Parse the stylesheet and the (attacker-controlled) source with hardened readers so
// XXE is blocked even when the factory (e.g. Xalan) ignores the attributes above.
Source xslt = new SAXSource(newSecureXmlReader(), new InputSource(xsltFile.toURI().toString()));
Transformer transformer = factory.newTransformer(xslt);
ByteArrayOutputStream baos = new ByteArrayOutputStream();
try {
transformer.transform(new StreamSource(is), new StreamResult(baos));
Source source = new SAXSource(newSecureXmlReader(), new InputSource(is));
transformer.transform(source, new StreamResult(baos));
return new ByteArrayInputStream(baos.toByteArray());
} finally {
IOUtils.closeQuietly(is);
}
}

/**
* Builds a namespace-aware XMLReader with external entity/DTD resolution disabled (XXE-safe).
*/
private static XMLReader newSecureXmlReader() throws Exception {
SAXParserFactory spf = SAXParserFactory.newInstance();
spf.setNamespaceAware(true);
spf.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
spf.setFeature("http://xml.org/sax/features/external-general-entities", false);
spf.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
spf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
return spf.newSAXParser().getXMLReader();
}

/**
* Pre-process HTML.
*
Expand Down
170 changes: 170 additions & 0 deletions ...l/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,23 @@

package org.opennms.protocols.xml.collector;

import java.io.ByteArrayInputStream;
import java.io.File;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.util.HashMap;
import java.util.Map;

import javax.xml.xpath.XPath;
import javax.xml.xpath.XPathConstants;
import javax.xml.xpath.XPathFactory;

import org.junit.Assert;
import org.junit.Test;
import org.opennms.netmgt.model.OnmsAssetRecord;
import org.opennms.netmgt.model.OnmsNode;
import org.opennms.protocols.xml.config.Request;
import org.w3c.dom.Document;

/**
* The Test Class for AbstractXmlCollectionHandler.
Expand Down Expand Up @@ -69,4 +79,164 @@ public void testParseString() throws Exception {
Assert.assertEquals(jsonContent, json);
}

/** NMS-20206: in-band XXE - an external general entity must not read a local file into the DOM. */
@Test
public void testInBandExternalEntityIsNotResolved() throws Exception {
final File secret = File.createTempFile("nms20206-secret", ".txt");
secret.deleteOnExit();
Files.write(secret.toPath(), "TOP_SECRET_SENTINEL".getBytes(StandardCharsets.UTF_8));

final String malicious =
"<?xml version=\"1.0\"?>\n" +
"<!DOCTYPE stats [ <!ENTITY xxe SYSTEM \"" + secret.toURI() + "\"> ]>\n" +
"<stats><val>&xxe;</val></stats>";
final DefaultXmlCollectionHandler handler = new DefaultXmlCollectionHandler();
try {
final Document doc = handler.getXmlDocument(
new ByteArrayInputStream(malicious.getBytes(StandardCharsets.UTF_8)), null);
final String text = doc.getElementsByTagName("val").item(0).getTextContent();
Assert.assertFalse("External entity was resolved - XXE not blocked (leaked: " + text + ")",
text.contains("TOP_SECRET_SENTINEL"));
} catch (Exception expected) {
// Rejecting the entity outright is equally safe.
}
}

/** NMS-20206: out-of-band XXE - an external parameter entity / external DTD must not be fetched. */
@Test
public void testOutOfBandParameterEntityIsNotResolved() throws Exception {
final File secret = File.createTempFile("nms20206-oob", ".txt");
secret.deleteOnExit();
Files.write(secret.toPath(), "OOB_SECRET_SENTINEL".getBytes(StandardCharsets.UTF_8));

// The nested parameter-entity trick is only well-formed in an EXTERNAL DTD, which is
// exactly what the real attack fetches. An unhardened parser loads this and expands
// &exfil; to the file contents; the hardening must prevent the external DTD load.
final File dtd = File.createTempFile("nms20206-oob", ".dtd");
dtd.deleteOnExit();
Files.write(dtd.toPath(), (
"<!ENTITY % file SYSTEM \"" + secret.toURI() + "\">\n" +
"<!ENTITY % eval \"<!ENTITY exfil '%file;'>\">\n" +
"%eval;\n").getBytes(StandardCharsets.UTF_8));

final String malicious =
"<?xml version=\"1.0\"?>\n" +
"<!DOCTYPE stats [\n" +
" <!ENTITY % dtd SYSTEM \"" + dtd.toURI() + "\">\n" +
" %dtd;\n" +
"]>\n" +
"<stats><val>&exfil;</val></stats>";
final DefaultXmlCollectionHandler handler = new DefaultXmlCollectionHandler();
try {
final Document doc = handler.getXmlDocument(
new ByteArrayInputStream(malicious.getBytes(StandardCharsets.UTF_8)), null);
final String text = doc.getElementsByTagName("val").item(0).getTextContent();
Assert.assertFalse("External DTD was fetched - OOB XXE not blocked (leaked: " + text + ")",
text.contains("OOB_SECRET_SENTINEL"));
} catch (Exception expected) {
// Rejecting the undefined entity (because the external DTD was not loaded) is safe.
}
}

/** NMS-20206: a benign DOCTYPE (e.g. pre-parse-html's &lt;!DOCTYPE html&gt;) must still parse. */
@Test
public void testBenignDoctypeStillParses() throws Exception {
final DefaultXmlCollectionHandler handler = new DefaultXmlCollectionHandler();
final String withDoctype = "<!DOCTYPE html>\n<stats><val>ok</val></stats>";
final Document doc = handler.getXmlDocument(
new ByteArrayInputStream(withDoctype.getBytes(StandardCharsets.UTF_8)), null);
Assert.assertNotNull(doc);
Assert.assertEquals("ok", doc.getElementsByTagName("val").item(0).getTextContent());
}

/** NMS-20206: internal entities must still expand (read via XPath string(), as collection does). */
@Test
public void testInternalEntityIsExpanded() throws Exception {
final DefaultXmlCollectionHandler handler = new DefaultXmlCollectionHandler();
final String xml =
"<?xml version=\"1.0\"?>\n" +
"<!DOCTYPE stats [ <!ENTITY ver \"1.2.3\"> ]>\n" +
"<stats><val>v&ver;</val></stats>";
final Document doc = handler.getXmlDocument(
new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8)), null);
final XPath xpath = XPathFactory.newInstance().newXPath();
final String value = (String) xpath.evaluate("string(/stats/val)", doc, XPathConstants.STRING);
Assert.assertEquals("v1.2.3", value);
}

/** NMS-20206: XSLT collection must still work (TransformerFactory hardening is best-effort). */
@Test
public void testXsltTransformationStillWorks() throws Exception {
// Stylesheet rewrites <val> to a fixed marker, so the assertion fails if the
// transform is skipped (rather than a no-op identity transform that proves nothing).
final File xslt = File.createTempFile("nms20206-xslt", ".xsl");
xslt.deleteOnExit();
final String stylesheet =
"<?xml version=\"1.0\"?>\n" +
"<xsl:stylesheet version=\"1.0\" xmlns:xsl=\"http://www.w3.org/1999/XSL/Transform\">\n" +
" <xsl:template match=\"/\">\n" +
" <stats><val>xslt-ran</val></stats>\n" +
" </xsl:template>\n" +
"</xsl:stylesheet>";
Files.write(xslt.toPath(), stylesheet.getBytes(StandardCharsets.UTF_8));

final Request request = new Request();
request.addParameter("xslt-source-file", xslt.getAbsolutePath());

final DefaultXmlCollectionHandler handler = new DefaultXmlCollectionHandler();
final String xml = "<stats><val>raw</val></stats>";
final Document doc = handler.getXmlDocument(
new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8)), request);
Assert.assertNotNull(doc);
Assert.assertEquals("xslt-ran", doc.getElementsByTagName("val").item(0).getTextContent());
}

/** NMS-20206: XXE in the collected source must be blocked during XSLT too (Xalan ignores ACCESS_EXTERNAL_*). */
@Test
public void testXsltSourceExternalEntityIsNotResolved() throws Exception {
final File secret = File.createTempFile("nms20206-xslt-src", ".txt");
secret.deleteOnExit();
Files.write(secret.toPath(), "XSLT_SRC_SENTINEL".getBytes(StandardCharsets.UTF_8));

// Copy-through stylesheet: a resolved source entity would surface in the output.
final File xslt = File.createTempFile("nms20206-xslt-src", ".xsl");
xslt.deleteOnExit();
Files.write(xslt.toPath(), (
"<?xml version=\"1.0\"?>\n" +
"<xsl:stylesheet version=\"1.0\" xmlns:xsl=\"http://www.w3.org/1999/XSL/Transform\">\n" +
" <xsl:template match=\"/\">\n" +
" <stats><val><xsl:value-of select=\"/stats/val\"/></val></stats>\n" +
" </xsl:template>\n" +
"</xsl:stylesheet>").getBytes(StandardCharsets.UTF_8));

final String malicious =
"<?xml version=\"1.0\"?>\n" +
"<!DOCTYPE stats [ <!ENTITY xxe SYSTEM \"" + secret.toURI() + "\"> ]>\n" +
"<stats><val>&xxe;</val></stats>";

final Request request = new Request();
request.addParameter("xslt-source-file", xslt.getAbsolutePath());

final DefaultXmlCollectionHandler handler = new DefaultXmlCollectionHandler();
try {
final Document doc = handler.getXmlDocument(
new ByteArrayInputStream(malicious.getBytes(StandardCharsets.UTF_8)), request);
final String text = doc.getElementsByTagName("val").item(0).getTextContent();
Assert.assertFalse("Source entity resolved during XSLT - XXE not blocked (leaked: " + text + ")",
text.contains("XSLT_SRC_SENTINEL"));
} catch (Exception expected) {
// Rejecting the entity outright is equally safe.
}
}

/** NMS-20206: the XXE hardening must not break collection of normal, entity-free XML. */
@Test
public void testWellFormedXmlStillParses() throws Exception {
final DefaultXmlCollectionHandler handler = new DefaultXmlCollectionHandler();
final String ok = "<stats><val>ok</val></stats>";
final Document doc = handler.getXmlDocument(new ByteArrayInputStream(ok.getBytes(StandardCharsets.UTF_8)), null);
Assert.assertNotNull(doc);
Assert.assertEquals("ok", doc.getElementsByTagName("val").item(0).getTextContent());
}

}
Loading