NMS-20206: Fix XXE in XML collector

docs/modules/reference/pages/performance-data-collection/collectors/xml.adoc

@@ -135,6 +135,13 @@ Configure the system proxy settings via <<operation:deep-dive/admin/configuratio
 | n/a
 |===
 
+[NOTE]
+====
+The XSLT stylesheet must be self-contained.
+Because the collected XML can come from an untrusted endpoint, the transformer blocks all external references to prevent XML External Entity (XXE) and server-side request forgery attacks: `xsl:include`, `xsl:import`, the `document()` function, and external DTDs referenced by the collected XML are not resolved.
+Secure processing is also enabled, which disables XSLT extension functions (for example, EXSLT or `java:` extensions).
+====
+
 === HTTP headers
 
 If the endpoint being collected requires additional headers, you can add them as part of the `<request>` object.

protocols/xml/src/main/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandler.java

@@ -46,14 +46,18 @@
 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.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;
@@ -96,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.
@@ -488,9 +494,8 @@ 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();
+        DocumentBuilderFactory factory = newSecureDocumentBuilderFactory();
         factory.setIgnoringComments(true);
-        factory.setNamespaceAware(true);
         DocumentBuilder builder = factory.newDocumentBuilder();
         StringWriter writer = new StringWriter();
         IOUtils.copy(is, writer, StandardCharsets.UTF_8);
@@ -523,18 +528,71 @@ 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();
-        Source xslt = new StreamSource(xsltFile);
+        factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+        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 {
-            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 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 <!DOCTYPE html>.
+        disableExternalEntities(factory::setFeature);
+        factory.setXIncludeAware(false);
+        factory.setNamespaceAware(true);
+        return factory;
+    }
+
+    /**
+     * 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);
+        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.
      *

protocols/xml/src/test/java/org/opennms/protocols/xml/collector/AbstractXmlCollectionHandlerTest.java

@@ -28,13 +28,25 @@
 
 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.transform.TransformerException;
+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;
+import org.xml.sax.SAXException;
 
 /**
  * The Test Class for AbstractXmlCollectionHandler.
@@ -69,4 +81,199 @@ 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 (SAXException | TransformerException 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 (SAXException | TransformerException 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 (SAXException | TransformerException expected) {
+            // Rejecting the entity outright is equally safe.
+        }
+    }
+
+    /** 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(), "<r>DOC_SECRET_SENTINEL</r>".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(), (
+                "<?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=\"document(/stats/uri)\"/></val></stats>\n" +
+                "  </xsl:template>\n" +
+                "</xsl:stylesheet>").getBytes(StandardCharsets.UTF_8));
+
+        final String source = "<stats><uri>" + secret.toURI() + "</uri></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(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 (SAXException | TransformerException 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 {
+        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());
+    }
+
 }