ppkarwasz commented on code in PR #11250:
URL: https://github.com/apache/seatunnel/pull/11250#discussion_r3506278535
##########
seatunnel-connectors-v2/connector-file/connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/source/reader/XmlReadStrategy.java:
##########
@@ -104,7 +121,7 @@ public void readProcess(
Map<String, String> partitionsMap,
String currentFileName)
throws IOException {
- SAXReader saxReader = new SAXReader();
+ SAXReader saxReader = createSecureSaxReader();
Review Comment:
We're bootstrapping an [Apache Commons
XML](https://github.com/apache/commons-xml) project to provide this kind of
hardening, tested across JAXP implementations at runtime (not tied to Xerces
specifically). We don't plan to support dom4j directly, but once it lands
you'll be able to harden any existing `SAXReader` in place:
```java
saxReader.setXMLReader(XmlFactories.harden(saxReader.getXMLReader()));
```
`harden()` throws if it can't find a way to secure the reader, rather than
silently doing nothing. `SAXReader.createDefault()` is another option already
available in dom4j, but it swallows `SAXException` internally, so a
misconfigured or unusual `XMLReader` implementation could silently stay
insecure.
Not a blocker for this PR either way, just flagging it if you prefer to rely
on a small external library instead of maintaining the hardening (and tests) in
SeaTunnel.
##########
seatunnel-connectors-v2/connector-file/connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/source/reader/XmlReadStrategy.java:
##########
@@ -169,6 +186,25 @@ public void readProcess(
});
}
+ /**
+ * Configure the XML reader with XXE-safe defaults before parsing
user-controlled file contents.
+ */
+ private SAXReader createSecureSaxReader() {
+ SAXReader saxReader = new SAXReader();
+ try {
+ saxReader.setFeature(DISALLOW_DOCTYPE_DECL, true);
+ saxReader.setFeature(EXTERNAL_GENERAL_ENTITIES, false);
+ saxReader.setFeature(EXTERNAL_PARAMETER_ENTITIES, false);
+ saxReader.setFeature(LOAD_EXTERNAL_DTD, false);
Review Comment:
It might be useful to add `FEATURE_SECURE_PROCESSING` to harden the reader
against entity expansion attacks. The default is `true` in the JDK, but `false`
if Apache Xerces happens to be on the classpath.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
