From 043fac58d7d0eb34e08fb57236be9efbec47b799 Mon Sep 17 00:00:00 2001 From: Chandra Gorantla Date: Wed, 12 Aug 2026 12:07:43 +0530 Subject: [PATCH 1/5] NMS-20206: Fix XXE in XML collector 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. --- .../AbstractXmlCollectionHandler.java | 18 ++++ .../AbstractXmlCollectionHandlerTest.java | 99 +++++++++++++++++++ 2 files changed, 117 insertions(+) diff --git a/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java b/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java index ef92ca05fccd..04cb42c0db9c 100644 --- a/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java +++ b/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java @@ -46,6 +46,7 @@ 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; @@ -489,6 +490,18 @@ protected Document getXmlDocument(InputStream is, Request request) throws Except is = preProcessHtml(request, is); is = applyXsltTransformation(request, is); DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + // Harden against XXE: the parsed content comes from a collected (potentially + // attacker-controlled) source. We do not use disallow-doctype-decl here because the + // pre-parse-html feature legitimately produces documents with a ; + // instead we forbid resolving any external entities/DTDs, which blocks both the + // in-band (external general entity) and out-of-band (external parameter entity / + // external DTD) XXE vectors while still allowing benign, entity-free DOCTYPEs. + 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.setExpandEntityReferences(false); factory.setIgnoringComments(true); factory.setNamespaceAware(true); DocumentBuilder builder = factory.newDocumentBuilder(); @@ -524,6 +537,11 @@ protected InputStream applyXsltTransformation(Request request, InputStream is) t if (!xsltFile.exists()) return is; TransformerFactory factory = TransformerFactory.newInstance(); + // Harden against XXE/SSRF: the input being transformed is collected (potentially + // attacker-controlled) content, so forbid resolution of external DTDs/stylesheets. + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); Source xslt = new StreamSource(xsltFile); Transformer transformer = factory.newTransformer(xslt); ByteArrayOutputStream baos = new ByteArrayOutputStream(); diff --git a/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java b/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java index 150f0a602539..364111c095b8 100644 --- a/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java +++ b/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java @@ -28,6 +28,10 @@ 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; @@ -35,6 +39,7 @@ import org.junit.Test; import org.opennms.netmgt.model.OnmsAssetRecord; import org.opennms.netmgt.model.OnmsNode; +import org.w3c.dom.Document; /** * The Test Class for AbstractXmlCollectionHandler. @@ -69,4 +74,98 @@ public void testParseString() throws Exception { Assert.assertEquals(jsonContent, json); } + /** + * NMS-20206: collected XML can be attacker-controlled. An external general entity that + * points at a local file (the in-band XXE file-read vector) must never be resolved into + * the parsed document. We assert the secret file content never reaches the DOM (the + * parser may instead reject the reference outright - either outcome is safe). + * + * @throws Exception the exception + */ + @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 = + "\n" + + " ]>\n" + + "&xxe;"; + 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) { + // The parser rejecting the disabled external entity is equally acceptable. + } + } + + /** + * NMS-20206: the out-of-band vector uses an external parameter entity that pulls an + * external DTD. With external-parameter-entities and load-external-dtd disabled, the + * local file referenced through the parameter entity must not be read into the document. + * + * @throws Exception the exception + */ + @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)); + + final String malicious = + "\n" + + "\n" + + " \">\n" + + " %eval;\n" + + "]>\n" + + "&exfil;"; + 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("Parameter entity was resolved - OOB XXE not blocked (leaked: " + text + ")", + text.contains("OOB_SECRET_SENTINEL")); + } catch (Exception expected) { + // Rejecting the disabled parameter entity is equally acceptable. + } + } + + /** + * NMS-20206: the hardening must not use disallow-doctype-decl, because the pre-parse-html + * feature legitimately produces documents that begin with a benign, entity-free DOCTYPE + * (e.g. <!DOCTYPE html>). Such documents must still parse. + * + * @throws Exception the exception + */ + @Test + public void testBenignDoctypeStillParses() throws Exception { + final DefaultXmlCollectionHandler handler = new DefaultXmlCollectionHandler(); + final String withDoctype = "\nok"; + 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: the XXE hardening must not break collection of normal, entity-free XML. + * + * @throws Exception the exception + */ + @Test + public void testWellFormedXmlStillParses() throws Exception { + final DefaultXmlCollectionHandler handler = new DefaultXmlCollectionHandler(); + final String ok = "ok"; + 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()); + } + } From 4ea3f16dbe1ca46443db02bbeb1b7c97e77573ba Mon Sep 17 00:00:00 2001 From: Chandra Gorantla Date: Wed, 12 Aug 2026 20:28:21 +0530 Subject: [PATCH 2/5] NMS-20206: Handle review comments --- .../AbstractXmlCollectionHandler.java | 25 ++--- .../AbstractXmlCollectionHandlerTest.java | 93 +++++++++++++------ 2 files changed, 79 insertions(+), 39 deletions(-) diff --git a/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java b/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java index 04cb42c0db9c..a17a76a2bb77 100644 --- a/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java +++ b/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java @@ -490,18 +490,13 @@ protected Document getXmlDocument(InputStream is, Request request) throws Except is = preProcessHtml(request, is); is = applyXsltTransformation(request, is); DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); - // Harden against XXE: the parsed content comes from a collected (potentially - // attacker-controlled) source. We do not use disallow-doctype-decl here because the - // pre-parse-html feature legitimately produces documents with a ; - // instead we forbid resolving any external entities/DTDs, which blocks both the - // in-band (external general entity) and out-of-band (external parameter entity / - // external DTD) XXE vectors while still allowing benign, entity-free DOCTYPEs. + // Block XXE: forbid external entities/DTDs. Not disallow-doctype-decl, since + // pre-parse-html produces a benign . 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.setExpandEntityReferences(false); factory.setIgnoringComments(true); factory.setNamespaceAware(true); DocumentBuilder builder = factory.newDocumentBuilder(); @@ -537,11 +532,19 @@ protected InputStream applyXsltTransformation(Request request, InputStream is) t if (!xsltFile.exists()) return is; TransformerFactory factory = TransformerFactory.newInstance(); - // Harden against XXE/SSRF: the input being transformed is collected (potentially - // attacker-controlled) content, so forbid resolution of external DTDs/stylesheets. + // Block XXE/SSRF via external DTDs/stylesheets. Best-effort: some factories + // (e.g. Xalan) reject these attributes with IllegalArgumentException. factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); - factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, ""); - factory.setAttribute(XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ""); + 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); + } Source xslt = new StreamSource(xsltFile); Transformer transformer = factory.newTransformer(xslt); ByteArrayOutputStream baos = new ByteArrayOutputStream(); diff --git a/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java b/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java index 364111c095b8..6c64728f1f9f 100644 --- a/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java +++ b/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java @@ -35,10 +35,15 @@ 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; /** @@ -74,14 +79,7 @@ public void testParseString() throws Exception { Assert.assertEquals(jsonContent, json); } - /** - * NMS-20206: collected XML can be attacker-controlled. An external general entity that - * points at a local file (the in-band XXE file-read vector) must never be resolved into - * the parsed document. We assert the secret file content never reaches the DOM (the - * parser may instead reject the reference outright - either outcome is safe). - * - * @throws Exception the exception - */ + /** 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"); @@ -100,29 +98,32 @@ public void testInBandExternalEntityIsNotResolved() throws Exception { Assert.assertFalse("External entity was resolved - XXE not blocked (leaked: " + text + ")", text.contains("TOP_SECRET_SENTINEL")); } catch (Exception expected) { - // The parser rejecting the disabled external entity is equally acceptable. + // Rejecting the entity outright is equally safe. } } - /** - * NMS-20206: the out-of-band vector uses an external parameter entity that pulls an - * external DTD. With external-parameter-entities and load-external-dtd disabled, the - * local file referenced through the parameter entity must not be read into the document. - * - * @throws Exception the exception - */ + /** 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(), ( + "\n" + + "\">\n" + + "%eval;\n").getBytes(StandardCharsets.UTF_8)); + final String malicious = "\n" + "\n" + - " \">\n" + - " %eval;\n" + + " \n" + + " %dtd;\n" + "]>\n" + "&exfil;"; final DefaultXmlCollectionHandler handler = new DefaultXmlCollectionHandler(); @@ -130,20 +131,14 @@ public void testOutOfBandParameterEntityIsNotResolved() throws Exception { final Document doc = handler.getXmlDocument( new ByteArrayInputStream(malicious.getBytes(StandardCharsets.UTF_8)), null); final String text = doc.getElementsByTagName("val").item(0).getTextContent(); - Assert.assertFalse("Parameter entity was resolved - OOB XXE not blocked (leaked: " + text + ")", + Assert.assertFalse("External DTD was fetched - OOB XXE not blocked (leaked: " + text + ")", text.contains("OOB_SECRET_SENTINEL")); } catch (Exception expected) { - // Rejecting the disabled parameter entity is equally acceptable. + // Rejecting the undefined entity (because the external DTD was not loaded) is safe. } } - /** - * NMS-20206: the hardening must not use disallow-doctype-decl, because the pre-parse-html - * feature legitimately produces documents that begin with a benign, entity-free DOCTYPE - * (e.g. <!DOCTYPE html>). Such documents must still parse. - * - * @throws Exception the exception - */ + /** NMS-20206: a benign DOCTYPE (e.g. pre-parse-html's <!DOCTYPE html>) must still parse. */ @Test public void testBenignDoctypeStillParses() throws Exception { final DefaultXmlCollectionHandler handler = new DefaultXmlCollectionHandler(); @@ -154,6 +149,48 @@ public void testBenignDoctypeStillParses() throws Exception { 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 = + "\n" + + " ]>\n" + + "v&ver;"; + 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 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 = + "\n" + + "\n" + + " \n" + + " xslt-ran\n" + + " \n" + + ""; + 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 = "raw"; + 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: the XXE hardening must not break collection of normal, entity-free XML. * From 32376ef30fd9da327560dcc354b68df73f1ff408 Mon Sep 17 00:00:00 2001 From: Chandra Gorantla Date: Wed, 12 Aug 2026 21:31:05 +0530 Subject: [PATCH 3/5] NMS-20206: Handle some more review comments 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. --- .../AbstractXmlCollectionHandler.java | 28 ++++++++++-- .../AbstractXmlCollectionHandlerTest.java | 44 ++++++++++++++++--- 2 files changed, 63 insertions(+), 9 deletions(-) diff --git a/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java b/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java index a17a76a2bb77..65ee3b41879d 100644 --- a/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java +++ b/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java @@ -50,9 +50,11 @@ 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; @@ -60,6 +62,9 @@ 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; @@ -532,9 +537,8 @@ protected InputStream applyXsltTransformation(Request request, InputStream is) t if (!xsltFile.exists()) return is; TransformerFactory factory = TransformerFactory.newInstance(); - // Block XXE/SSRF via external DTDs/stylesheets. Best-effort: some factories - // (e.g. Xalan) reject these attributes with IllegalArgumentException. 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) { @@ -545,17 +549,33 @@ protected InputStream applyXsltTransformation(Request request, InputStream is) t } catch (IllegalArgumentException e) { LOG.debug("TransformerFactory {} does not support {}; skipping", factory.getClass().getName(), XMLConstants.ACCESS_EXTERNAL_STYLESHEET); } - Source xslt = new StreamSource(xsltFile); + // 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. * diff --git a/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java b/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java index 6c64728f1f9f..70a90a5508ec 100644 --- a/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java +++ b/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java @@ -191,11 +191,45 @@ public void testXsltTransformationStillWorks() throws Exception { Assert.assertEquals("xslt-ran", doc.getElementsByTagName("val").item(0).getTextContent()); } - /** - * NMS-20206: the XXE hardening must not break collection of normal, entity-free XML. - * - * @throws Exception the exception - */ + /** 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(), ( + "\n" + + "\n" + + " \n" + + " \n" + + " \n" + + "").getBytes(StandardCharsets.UTF_8)); + + final String malicious = + "\n" + + " ]>\n" + + "&xxe;"; + + 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(); From e55f74739a1dea4ce0e4921c851132abf1fffe21 Mon Sep 17 00:00:00 2001 From: Chandra Gorantla Date: Wed, 12 Aug 2026 22:45:53 +0530 Subject: [PATCH 4/5] NMS-20206: Add some more protections --- .../AbstractXmlCollectionHandler.java | 53 ++++++++++--------- .../AbstractXmlCollectionHandlerTest.java | 35 ++++++++++++ 2 files changed, 63 insertions(+), 25 deletions(-) diff --git a/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java b/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java index 65ee3b41879d..64e115e8c94d 100644 --- a/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java +++ b/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java @@ -53,18 +53,16 @@ import javax.xml.parsers.SAXParserFactory; import javax.xml.transform.Source; import javax.xml.transform.Transformer; +import javax.xml.transform.TransformerException; import javax.xml.transform.TransformerFactory; +import javax.xml.transform.URIResolver; 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; @@ -102,6 +100,8 @@ import org.w3c.dom.Document; import org.w3c.dom.Node; import org.w3c.dom.NodeList; +import org.xml.sax.InputSource; +import org.xml.sax.XMLReader; /** * The Abstract Class XML Collection Handler. @@ -494,14 +494,7 @@ protected Document getXmlDocument(String urlString, Request request) throws Exce protected Document getXmlDocument(InputStream is, Request request) throws Exception { 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 . - 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); + DocumentBuilderFactory factory = newSecureDocumentBuilderFactory(); factory.setIgnoringComments(true); factory.setNamespaceAware(true); DocumentBuilder builder = factory.newDocumentBuilder(); @@ -536,23 +529,17 @@ protected InputStream applyXsltTransformation(Request request, InputStream is) t File xsltFile = new File(xsltFilename); if (!xsltFile.exists()) return is; + // The collected source XML is untrusted, so block XXE/SSRF: deny external URI + // resolution (xsl:import/include and document()) and parse with an entity-hardened reader. TransformerFactory factory = TransformerFactory.newInstance(); 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. + final URIResolver denyExternal = (href, base) -> { + throw new TransformerException("External resource resolution disabled for collector XSLT: " + href); + }; + factory.setURIResolver(denyExternal); Source xslt = new SAXSource(newSecureXmlReader(), new InputSource(xsltFile.toURI().toString())); Transformer transformer = factory.newTransformer(xslt); + transformer.setURIResolver(denyExternal); ByteArrayOutputStream baos = new ByteArrayOutputStream(); try { Source source = new SAXSource(newSecureXmlReader(), new InputSource(is)); @@ -563,6 +550,22 @@ protected InputStream applyXsltTransformation(Request request, InputStream is) t } } + /** + * Builds a namespace-aware DocumentBuilderFactory with external entity/DTD resolution disabled (XXE-safe). + */ + private static DocumentBuilderFactory newSecureDocumentBuilderFactory() throws Exception { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + // Block XXE: forbid external entities/DTDs. Not disallow-doctype-decl, since + // pre-parse-html produces a benign . + 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.setNamespaceAware(true); + return factory; + } + /** * Builds a namespace-aware XMLReader with external entity/DTD resolution disabled (XXE-safe). */ diff --git a/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java b/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java index 70a90a5508ec..90d4af5f016d 100644 --- a/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java +++ b/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java @@ -229,6 +229,41 @@ public void testXsltSourceExternalEntityIsNotResolved() throws Exception { } } + /** NMS-20206: XSLT document() on a URI from collected data must not read files / issue requests. */ + @Test + public void testXsltDocumentFunctionIsBlocked() throws Exception { + final File secret = File.createTempFile("nms20206-doc", ".xml"); + secret.deleteOnExit(); + Files.write(secret.toPath(), "DOC_SECRET_SENTINEL".getBytes(StandardCharsets.UTF_8)); + + // Stylesheet resolves document() using a URI taken from the (attacker-controlled) source. + final File xslt = File.createTempFile("nms20206-doc", ".xsl"); + xslt.deleteOnExit(); + Files.write(xslt.toPath(), ( + "\n" + + "\n" + + " \n" + + " \n" + + " \n" + + "").getBytes(StandardCharsets.UTF_8)); + + final String source = "" + secret.toURI() + ""; + + 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(source.getBytes(StandardCharsets.UTF_8)), request); + final String text = doc.getElementsByTagName("val").item(0).getTextContent(); + Assert.assertFalse("document() resolved a source URI - SSRF/file-read not blocked (leaked: " + text + ")", + text.contains("DOC_SECRET_SENTINEL")); + } catch (Exception expected) { + // A rejecting URIResolver aborting the transform is equally safe. + } + } + /** NMS-20206: the XXE hardening must not break collection of normal, entity-free XML. */ @Test public void testWellFormedXmlStillParses() throws Exception { From 134ccf57ea0eef79074c1a7b3eeb8dbe3a0924e3 Mon Sep 17 00:00:00 2001 From: Chandra Gorantla Date: Thu, 13 Aug 2026 10:36:34 +0530 Subject: [PATCH 5/5] NMS-20206: Update docs --- .../collectors/xml.adoc | 7 ++++ .../AbstractXmlCollectionHandler.java | 32 +++++++++++++------ .../AbstractXmlCollectionHandlerTest.java | 10 +++--- 3 files changed, 36 insertions(+), 13 deletions(-) diff --git a/docs/modules/reference/pages/performance-data-collection/collectors/xml.adoc b/docs/modules/reference/pages/performance-data-collection/collectors/xml.adoc index e876b27deb16..8c1ec2c4fd3d 100644 --- a/docs/modules/reference/pages/performance-data-collection/collectors/xml.adoc +++ b/docs/modules/reference/pages/performance-data-collection/collectors/xml.adoc @@ -135,6 +135,13 @@ Configure the system proxy settings via <` object. diff --git a/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java b/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java index 64e115e8c94d..d638c548b2cc 100644 --- a/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java +++ b/protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java @@ -496,7 +496,6 @@ protected Document getXmlDocument(InputStream is, Request request) throws Except is = applyXsltTransformation(request, is); DocumentBuilderFactory factory = newSecureDocumentBuilderFactory(); factory.setIgnoringComments(true); - factory.setNamespaceAware(true); DocumentBuilder builder = factory.newDocumentBuilder(); StringWriter writer = new StringWriter(); IOUtils.copy(is, writer, StandardCharsets.UTF_8); @@ -557,10 +556,7 @@ private static DocumentBuilderFactory newSecureDocumentBuilderFactory() throws E DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); // Block XXE: forbid external entities/DTDs. Not disallow-doctype-decl, since // pre-parse-html produces a benign . - 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); + disableExternalEntities(factory::setFeature); factory.setXIncludeAware(false); factory.setNamespaceAware(true); return factory; @@ -572,13 +568,31 @@ private static DocumentBuilderFactory newSecureDocumentBuilderFactory() throws E 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); + disableExternalEntities(spf::setFeature); return spf.newSAXParser().getXMLReader(); } + @FunctionalInterface + private interface FeatureSetter { + void setFeature(String name, boolean value) throws Exception; + } + + /** + * Applies the XXE hardening features shared by the DOM and SAX parsers. + * external-general/parameter-entities are SAX-standard and required; load-external-dtd is + * Xerces-specific and set best-effort so a non-Xerces provider does not fail parsing outright. + */ + private static void disableExternalEntities(FeatureSetter parser) throws Exception { + parser.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); + parser.setFeature("http://xml.org/sax/features/external-general-entities", false); + parser.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + try { + parser.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + } catch (Exception e) { + LOG.debug("XML parser does not support the load-external-dtd feature; skipping"); + } + } + /** * Pre-process HTML. * diff --git a/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java b/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java index 90d4af5f016d..c97ccb68eeb0 100644 --- a/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java +++ b/protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java @@ -35,6 +35,7 @@ import java.util.HashMap; import java.util.Map; +import javax.xml.transform.TransformerException; import javax.xml.xpath.XPath; import javax.xml.xpath.XPathConstants; import javax.xml.xpath.XPathFactory; @@ -45,6 +46,7 @@ import org.opennms.netmgt.model.OnmsNode; import org.opennms.protocols.xml.config.Request; import org.w3c.dom.Document; +import org.xml.sax.SAXException; /** * The Test Class for AbstractXmlCollectionHandler. @@ -97,7 +99,7 @@ public void testInBandExternalEntityIsNotResolved() throws Exception { 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) { + } catch (SAXException | TransformerException expected) { // Rejecting the entity outright is equally safe. } } @@ -133,7 +135,7 @@ public void testOutOfBandParameterEntityIsNotResolved() throws Exception { 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) { + } catch (SAXException | TransformerException expected) { // Rejecting the undefined entity (because the external DTD was not loaded) is safe. } } @@ -224,7 +226,7 @@ public void testXsltSourceExternalEntityIsNotResolved() throws Exception { 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) { + } catch (SAXException | TransformerException expected) { // Rejecting the entity outright is equally safe. } } @@ -259,7 +261,7 @@ public void testXsltDocumentFunctionIsBlocked() throws Exception { final String text = doc.getElementsByTagName("val").item(0).getTextContent(); Assert.assertFalse("document() resolved a source URI - SSRF/file-read not blocked (leaked: " + text + ")", text.contains("DOC_SECRET_SENTINEL")); - } catch (Exception expected) { + } catch (SAXException | TransformerException expected) { // A rejecting URIResolver aborting the transform is equally safe. } }