xxubai commented on code in PR #4119:
URL: https://github.com/apache/amoro/pull/4119#discussion_r2964271392
##########
amoro-ams/src/main/java/org/apache/amoro/server/dashboard/controller/PlatformFileInfoController.java:
##########
@@ -52,7 +55,15 @@ public void uploadFile(Context ctx) throws IOException {
// validate xml config
if (name.toLowerCase().endsWith(".xml")) {
try {
- Configuration configuration = new Configuration();
+ // Explicitly disable external entity processing to prevent XXE
attacks,
+ // regardless of the underlying XML parser implementation on the
classpath.
+ XMLInputFactory xif = XMLInputFactory.newInstance();
+ xif.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES,
false);
+ xif.setProperty(XMLInputFactory.SUPPORT_DTD, false);
+ xif.setProperty(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+ xif.createXMLStreamReader(new ByteArrayInputStream(bytes)).close();
+
+ Configuration configuration = new Configuration(false);
configuration.addResource(new ByteArrayInputStream(bytes));
Review Comment:
The XXE fix is still incomplete. The new XMLInputFactory parse only acts as
a pre-check, but the actual validation still happens in
Configuration.addResource(new ByteArrayInputStream(bytes)). In Hadoop 3.4.0,
this overload does not use the restricted parser path by default, so the XML is
reparsed in a way that can still allow DTD / external entity processing. In
other words, this adds an extra parse, but it does not fully close the XXE risk.
##########
amoro-ams/src/main/java/org/apache/amoro/server/dashboard/controller/PlatformFileInfoController.java:
##########
@@ -52,7 +55,15 @@ public void uploadFile(Context ctx) throws IOException {
// validate xml config
if (name.toLowerCase().endsWith(".xml")) {
try {
- Configuration configuration = new Configuration();
+ // Explicitly disable external entity processing to prevent XXE
attacks,
+ // regardless of the underlying XML parser implementation on the
classpath.
+ XMLInputFactory xif = XMLInputFactory.newInstance();
+ xif.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES,
false);
+ xif.setProperty(XMLInputFactory.SUPPORT_DTD, false);
+ xif.setProperty(XMLConstants.FEATURE_SECURE_PROCESSING, true);
Review Comment:
`FEATURE_SECURE_PROCESSING` is not a standard property of `XMLInputFactory`
and may throw an exception.
--
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]
