This is an automated email from the ASF dual-hosted git repository.
hansva pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/hop.git
The following commit(s) were added to refs/heads/main by this push:
new 4e9d4268fd Issue #8163 : Harden XmlInputStream StAX factory against
XXE (#8164)
4e9d4268fd is described below
commit 4e9d4268fd07859605e3d3ce51f5785ac82908db
Author: Matt Casters <[email protected]>
AuthorDate: Mon Aug 31 20:27:44 2026 +0200
Issue #8163 : Harden XmlInputStream StAX factory against XXE (#8164)
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 ++-
.../java/org/apache/hop/core/xml/XmlFormatter.java | 3 +-
.../hop/core/xml/XmlParserFactoryProducer.java | 23 +++++++++++
.../java/org/apache/hop/core/xml/XmlUtilsTest.java | 48 ++++++++++++++++++++++
.../transforms/excelinput/staxpoi/StaxUtil.java | 7 +---
.../transforms/webservices/WebService.java | 5 +--
.../xml/advancedxmloutput/AdvancedXmlOutput.java | 11 ++---
.../xml/xmlinputstream/XmlInputStream.java | 4 +-
.../xml/xmlinputstream/XmlInputStreamTest.java | 28 +++++++++++++
9 files changed, 113 insertions(+), 21 deletions(-)
diff --git a/THREAT_MODEL.md b/THREAT_MODEL.md
index 7de2dd274f..bcf7186cd2 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 82915d4101..cd27af78ee 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 @@ import org.apache.hop.core.exception.HopRuntimeException;
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 07db9f2751..84c3d3ada6 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.XMLConstants;
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 class XmlParserFactoryProducer {
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.
+ *
+ * <p>{@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 d7682559ae..94ed779576 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 @@ class XmlUtilsTest {
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 =
+ "<?xml version=\"1.0\"?>"
+ + "<!DOCTYPE foo [ <!ENTITY xxe SYSTEM \""
+ + secret.toUri()
+ + "\"> ]>"
+ + "<root>&xxe;</root>";
+
+ 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 c340f10930..b312706370 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 class StaxUtil {
}
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 3c5f68e0fd..3ce7a7ec68 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.Date;
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 @@ public class WebService extends
BaseTransform<WebServiceMeta, WebServiceData> {
// 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 1254842928..2bf45a3ee0 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.IValueMeta;
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<AdvancedXmlOutputMeta, Adva
private static final String EOL = "\n";
private static final XMLOutputFactory XML_OUT_FACTORY =
XMLOutputFactory.newInstance();
- private static final XMLInputFactory XML_IN_FACTORY =
createSecureInputFactory();
+ private static final XMLInputFactory XML_IN_FACTORY =
+ XmlParserFactoryProducer.createSecureXmlInputFactory();
/** Writes every byte to two underlying streams (e.g. file + in-memory
capture). */
private static final class TeeOutputStream extends OutputStream {
@@ -990,13 +992,6 @@ public class AdvancedXmlOutput extends
BaseTransform<AdvancedXmlOutputMeta, Adva
return s;
}
- private static XMLInputFactory createSecureInputFactory() {
- XMLInputFactory f = XMLInputFactory.newInstance();
- f.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
- f.setProperty(XMLInputFactory.SUPPORT_DTD, false);
- return f;
- }
-
/** Test hook: returns the current data object's writer (so unit tests can
inject a mock). */
protected XMLStreamWriter getWriter() {
return data == null ? null : data.writer;
diff --git
a/plugins/transforms/xml/src/main/java/org/apache/hop/pipeline/transforms/xml/xmlinputstream/XmlInputStream.java
b/plugins/transforms/xml/src/main/java/org/apache/hop/pipeline/transforms/xml/xmlinputstream/XmlInputStream.java
index 7550606ff6..99a32289ff 100644
---
a/plugins/transforms/xml/src/main/java/org/apache/hop/pipeline/transforms/xml/xmlinputstream/XmlInputStream.java
+++
b/plugins/transforms/xml/src/main/java/org/apache/hop/pipeline/transforms/xml/xmlinputstream/XmlInputStream.java
@@ -23,7 +23,6 @@ import java.util.ArrayList;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
-import javax.xml.stream.XMLInputFactory;
import javax.xml.stream.XMLStreamConstants;
import javax.xml.stream.XMLStreamException;
import javax.xml.stream.events.Attribute;
@@ -42,6 +41,7 @@ import org.apache.hop.core.row.RowDataUtil;
import org.apache.hop.core.row.RowMeta;
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.lineage.LineageFileIoEmitter;
import org.apache.hop.lineage.model.FileIoOperation;
@@ -651,7 +651,7 @@ public class XmlInputStream extends
BaseTransform<XmlInputStreamMeta, XmlInputSt
@Override
public boolean init() {
if (super.init()) {
- data.staxInstance = XMLInputFactory.newInstance(); // could select the
parser later on
+ data.staxInstance =
XmlParserFactoryProducer.createSecureXmlInputFactory();
data.staxInstance.setProperty("javax.xml.stream.isCoalescing", false);
data.filenr = 0;
if
(getPipelineMeta().findPreviousTransforms(getTransformMeta()).isEmpty()
diff --git
a/plugins/transforms/xml/src/test/java/org/apache/hop/pipeline/transforms/xml/xmlinputstream/XmlInputStreamTest.java
b/plugins/transforms/xml/src/test/java/org/apache/hop/pipeline/transforms/xml/xmlinputstream/XmlInputStreamTest.java
index b02a63a171..94142f3731 100644
---
a/plugins/transforms/xml/src/test/java/org/apache/hop/pipeline/transforms/xml/xmlinputstream/XmlInputStreamTest.java
+++
b/plugins/transforms/xml/src/test/java/org/apache/hop/pipeline/transforms/xml/xmlinputstream/XmlInputStreamTest.java
@@ -17,6 +17,7 @@
package org.apache.hop.pipeline.transforms.xml.xmlinputstream;
import static org.junit.jupiter.api.Assertions.assertEquals;
+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 static org.mockito.ArgumentMatchers.any;
@@ -350,6 +351,33 @@ class XmlInputStreamTest {
assertEquals("other data", rl.getWritten().get(3)[3]);
}
+ @Test
+ void doesNotResolveExternalEntities() throws Exception {
+ File secret = File.createTempFile("xxe-secret", ".txt");
+ secret.deleteOnExit();
+ try (Writer writer = new PrintWriter(secret, "UTF8")) {
+ writer.write("CANARY_SECRET_VALUE");
+ }
+
+ String xml =
+ "<?xml version=\"1.0\" encoding=\"UTF-8\"?>"
+ + "<!DOCTYPE root [ <!ENTITY xxe SYSTEM \""
+ + secret.toURI()
+ + "\"> ]>"
+ + "<root>&xxe;</root>";
+ 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(
