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 ef92ca05fccd..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 @@ -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 . + 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. * 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..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 @@ -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 = + "\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 (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(), ( + "\n" + + "\">\n" + + "%eval;\n").getBytes(StandardCharsets.UTF_8)); + + final String malicious = + "\n" + + "\n" + + " %dtd;\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("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 <!DOCTYPE html>) must still parse. */ + @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: 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: 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 (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(), "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 (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 = "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()); + } + }