From c894565cf66ae939f63d6e1364adad38a17ddfea Mon Sep 17 00:00:00 2001 From: mattcasters Date: Fri, 28 Aug 2026 21:34:05 +0200 Subject: [PATCH] Issue #8163 : Harden XmlInputStream StAX factory against XXE Disable DTD processing and external entity resolution on StAX XMLInputFactory via XmlParserFactoryProducer, and use that factory from XmlInputStream and the remaining raw/partial StAX call sites. --- THREAT_MODEL.md | 5 +- .../org/apache/hop/core/xml/XmlFormatter.java | 3 +- .../core/xml/XmlParserFactoryProducer.java | 23 +++++++++ .../org/apache/hop/core/xml/XmlUtilsTest.java | 48 +++++++++++++++++++ .../excelinput/staxpoi/StaxUtil.java | 7 +-- .../transforms/webservices/WebService.java | 5 +- .../advancedxmloutput/AdvancedXmlOutput.java | 11 ++--- .../xml/xmlinputstream/XmlInputStream.java | 4 +- .../xmlinputstream/XmlInputStreamTest.java | 28 +++++++++++ 9 files changed, 113 insertions(+), 21 deletions(-) diff --git a/THREAT_MODEL.md b/THREAT_MODEL.md index 7de2dd274f7..bcf7186cd20 100644 --- a/THREAT_MODEL.md +++ b/THREAT_MODEL.md @@ -156,7 +156,10 @@ where strict outbound TLS verification is required (see §9). ([`XmlParserFactoryProducer.java`](core/src/main/java/org/apache/hop/core/xml/XmlParserFactoryProducer.java)) with external general/parameter entities and external DTD loading disabled and `FEATURE_SECURE_PROCESSING` on — XXE file-read/SSRF and entity-expansion are mitigated (verified empirically). The - same secure parser is used by the Hop Server remote-add endpoints. + same secure parser is used by the Hop Server remote-add endpoints. StAX ingest + (`XmlInputStream` and other `XMLInputFactory` sites) uses + `createSecureXmlInputFactory()`, which disables DTD processing and external + entities. - **Credential storage — NOT confidential by default.** Connection passwords in metadata are by default only **reversibly obfuscated, not encrypted**: the built-in `Hop` encoder ([`HopTwoWayPasswordEncoder.java`](core/src/main/java/org/apache/hop/core/encryption/HopTwoWayPasswordEncoder.java)) XORs against a diff --git a/core/src/main/java/org/apache/hop/core/xml/XmlFormatter.java b/core/src/main/java/org/apache/hop/core/xml/XmlFormatter.java index 82915d4101c..cd27af78ee0 100644 --- a/core/src/main/java/org/apache/hop/core/xml/XmlFormatter.java +++ b/core/src/main/java/org/apache/hop/core/xml/XmlFormatter.java @@ -37,7 +37,8 @@ public class XmlFormatter { private static final String TRANSFORM_PREFIX = " "; - private static XMLInputFactory INPUT_FACTORY = XMLInputFactory.newInstance(); + private static XMLInputFactory INPUT_FACTORY = + XmlParserFactoryProducer.createSecureXmlInputFactory(); private static XMLOutputFactory OUTPUT_FACTORY = XMLOutputFactory.newInstance(); static { diff --git a/core/src/main/java/org/apache/hop/core/xml/XmlParserFactoryProducer.java b/core/src/main/java/org/apache/hop/core/xml/XmlParserFactoryProducer.java index 07db9f27517..84c3d3ada65 100644 --- a/core/src/main/java/org/apache/hop/core/xml/XmlParserFactoryProducer.java +++ b/core/src/main/java/org/apache/hop/core/xml/XmlParserFactoryProducer.java @@ -21,6 +21,7 @@ import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParserFactory; +import javax.xml.stream.XMLInputFactory; import javax.xml.validation.SchemaFactory; import org.apache.hop.core.Const; import org.apache.hop.core.logging.LogChannel; @@ -143,4 +144,26 @@ public static SchemaFactory createSecureSchemaFactory(String schemaLanguage) return factory; } + + /** + * Creates an instance of {@link XMLInputFactory} with DTD processing and external entity + * resolution disabled to protect against XML External Entity (XXE) attacks and XML entity + * expansion bombs. + * + *

{@link XMLConstants#ACCESS_EXTERNAL_DTD} and {@link XMLConstants#ACCESS_EXTERNAL_SCHEMA} are + * set when the StAX provider recognizes them. Woodstox (the factory on Hop's runtime classpath) + * does not, so those two calls are best-effort. + */ + public static XMLInputFactory createSecureXmlInputFactory() { + XMLInputFactory factory = XMLInputFactory.newInstance(); + factory.setProperty(XMLInputFactory.SUPPORT_DTD, false); + factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false); + try { + factory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); + factory.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); + } catch (IllegalArgumentException e) { + // Property not supported by this StAX provider + } + return factory; + } } diff --git a/core/src/test/java/org/apache/hop/core/xml/XmlUtilsTest.java b/core/src/test/java/org/apache/hop/core/xml/XmlUtilsTest.java index d7682559aec..94ed7795766 100644 --- a/core/src/test/java/org/apache/hop/core/xml/XmlUtilsTest.java +++ b/core/src/test/java/org/apache/hop/core/xml/XmlUtilsTest.java @@ -18,15 +18,21 @@ package org.apache.hop.core.xml; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.File; +import java.io.StringReader; import java.nio.file.Files; import java.nio.file.Path; import javax.xml.XMLConstants; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.SAXParserFactory; +import javax.xml.stream.XMLInputFactory; +import javax.xml.stream.XMLStreamConstants; +import javax.xml.stream.XMLStreamException; +import javax.xml.stream.XMLStreamReader; import javax.xml.validation.SchemaFactory; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -104,4 +110,46 @@ void secureSchemaFactoryStillResolvesLocalSchemaReference(@TempDir Path tempDir) assertDoesNotThrow(() -> schemaFactory.newSchema(including)); } + + @Test + void secureXmlInputFactoryDisablesDtdAndExternalEntities() { + XMLInputFactory factory = XmlParserFactoryProducer.createSecureXmlInputFactory(); + + assertFalse((Boolean) factory.getProperty(XMLInputFactory.SUPPORT_DTD)); + assertFalse((Boolean) factory.getProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES)); + } + + @Test + void secureXmlInputFactoryDoesNotResolveExternalEntities(@TempDir Path tempDir) throws Exception { + Path secret = tempDir.resolve("secret.txt"); + Files.writeString(secret, "CANARY_SECRET_VALUE"); + String xml = + "" + + " ]>" + + "&xxe;"; + + XMLInputFactory factory = XmlParserFactoryProducer.createSecureXmlInputFactory(); + StringBuilder text = new StringBuilder(); + XMLStreamReader streamReader = factory.createXMLStreamReader(new StringReader(xml)); + try { + while (streamReader.hasNext()) { + int event = streamReader.next(); + if (event == XMLStreamConstants.CHARACTERS || event == XMLStreamConstants.CDATA) { + text.append(streamReader.getText()); + } + } + } catch (XMLStreamException e) { + // Expected when DTD processing is disabled + assertFalse(text.toString().contains("CANARY_SECRET_VALUE")); + return; + } finally { + streamReader.close(); + } + + assertFalse( + text.toString().contains("CANARY_SECRET_VALUE"), + "external entity content must not appear in the parse result"); + } } diff --git a/plugins/transforms/excel/src/main/java/org/apache/hop/pipeline/transforms/excelinput/staxpoi/StaxUtil.java b/plugins/transforms/excel/src/main/java/org/apache/hop/pipeline/transforms/excelinput/staxpoi/StaxUtil.java index c340f109308..b3127063704 100644 --- a/plugins/transforms/excel/src/main/java/org/apache/hop/pipeline/transforms/excelinput/staxpoi/StaxUtil.java +++ b/plugins/transforms/excel/src/main/java/org/apache/hop/pipeline/transforms/excelinput/staxpoi/StaxUtil.java @@ -18,6 +18,7 @@ package org.apache.hop.pipeline.transforms.excelinput.staxpoi; import javax.xml.stream.XMLInputFactory; +import org.apache.hop.core.xml.XmlParserFactoryProducer; import org.apache.poi.ss.SpreadsheetVersion; public class StaxUtil { @@ -67,10 +68,6 @@ public static final int parseColumnNumber(String columnIndicator) { } public static final XMLInputFactory safeXMLInputFactory() { - XMLInputFactory factory = XMLInputFactory.newInstance(); - // To prevent from XXE attacks - factory.setProperty(XMLInputFactory.SUPPORT_DTD, Boolean.FALSE); - factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, Boolean.FALSE); - return factory; + return XmlParserFactoryProducer.createSecureXmlInputFactory(); } } diff --git a/plugins/transforms/webservices/src/main/java/org/apache/hop/pipeline/transforms/webservices/WebService.java b/plugins/transforms/webservices/src/main/java/org/apache/hop/pipeline/transforms/webservices/WebService.java index 3c5f68e0fdf..3ce7a7ec680 100644 --- a/plugins/transforms/webservices/src/main/java/org/apache/hop/pipeline/transforms/webservices/WebService.java +++ b/plugins/transforms/webservices/src/main/java/org/apache/hop/pipeline/transforms/webservices/WebService.java @@ -36,7 +36,6 @@ import java.util.Hashtable; import java.util.Iterator; import java.util.List; -import javax.xml.XMLConstants; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.stream.XMLInputFactory; @@ -906,9 +905,7 @@ private void compatibleProcessRows( // TODO Very empirical : see if we can do something better here try { - XMLInputFactory vFactory = XMLInputFactory.newInstance(); - vFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, ""); - vFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); + XMLInputFactory vFactory = XmlParserFactoryProducer.createSecureXmlInputFactory(); XMLStreamReader vReader = vFactory.createXMLStreamReader(stringReader); Object[] outputRowData = RowDataUtil.allocateRowData(data.outputRowMeta.size()); diff --git a/plugins/transforms/xml/src/main/java/org/apache/hop/pipeline/transforms/xml/advancedxmloutput/AdvancedXmlOutput.java b/plugins/transforms/xml/src/main/java/org/apache/hop/pipeline/transforms/xml/advancedxmloutput/AdvancedXmlOutput.java index 12548429280..2bf45a3ee05 100644 --- a/plugins/transforms/xml/src/main/java/org/apache/hop/pipeline/transforms/xml/advancedxmloutput/AdvancedXmlOutput.java +++ b/plugins/transforms/xml/src/main/java/org/apache/hop/pipeline/transforms/xml/advancedxmloutput/AdvancedXmlOutput.java @@ -48,6 +48,7 @@ import org.apache.hop.core.row.RowDataUtil; import org.apache.hop.core.util.Utils; import org.apache.hop.core.vfs.HopVfs; +import org.apache.hop.core.xml.XmlParserFactoryProducer; import org.apache.hop.i18n.BaseMessages; import org.apache.hop.pipeline.Pipeline; import org.apache.hop.pipeline.PipelineMeta; @@ -64,7 +65,8 @@ public class AdvancedXmlOutput extends BaseTransform" + + " ]>" + + "&xxe;"; + xmlInputStreamMeta.setFilename(createTestFile(xml)); + + assertThrows(HopException.class, this::doTest); + + for (Object[] row : rl.getWritten()) { + for (Object cell : row) { + if (cell instanceof String value) { + assertFalse(value.contains("CANARY_SECRET_VALUE")); + } + } + } + } + private void doTest() throws HopException { XmlInputStream xmlInputStream = new XmlInputStream(