Issue #8163 : Harden XmlInputStream StAX factory against XXE

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

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 {

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.
+   *
+   * <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;
+  }
 }

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 =
+        "<?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");
+  }
 }

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();
   }
 }

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());

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<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 @@ private static String stripLeadingXmlDeclaration(String fragment) {
     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;

plugins/transforms/xml/src/main/java/org/apache/hop/pipeline/transforms/xml/xmlinputstream/XmlInputStream.java

@@ -23,7 +23,6 @@
 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.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 @@ private void resetElementCounters() {
   @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()

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 @@ void multiLineDataReadAsMultipleElements() throws HopException, IOException {
     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(