MLE-27553 MLE-27554 MLE-27555 MLE-27556: Fix XXE Injection - XML External Entity Polaris medium severity issues#577
Conversation
…to XMLInputFactory
…to XMLInputFactory
There was a problem hiding this comment.
Pull request overview
This PR addresses Polaris-reported XXE (CWE-611) risks by hardening XML parsing (StAX) and transformation (JAXP TransformerFactory) to prevent external entity / external resource resolution.
Changes:
- Disable external entity resolution and DTD support on
XMLInputFactoryinstances used by aggregate XML readers. - Restrict external DTD and stylesheet access on
TransformerFactoryinstances used by document serialization. - Add warning logs when security attributes/properties are not supported by the underlying XML implementation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/main/java/com/marklogic/mapreduce/JSONDocument.java |
Hardens TransformerFactory by restricting external DTD/stylesheet access. |
src/main/java/com/marklogic/mapreduce/DOMDocument.java |
Hardens TransformerFactory by restricting external DTD/stylesheet access. |
src/main/java/com/marklogic/contentpump/CompressedAggXMLReader.java |
Hardens StAX XMLInputFactory by disabling external entities and DTDs. |
src/main/java/com/marklogic/contentpump/AggregateXMLReader.java |
Hardens StAX XMLInputFactory by disabling external entities and DTDs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
I think copilot's suggestions to separate out the double property/attribute sets into their own try/catch blocks should be implemented. It would be the safer approach to ensure that each one is at least attempted, and that one failing doesn't stop the other from trying.
…ch blocks and improve warning messages
|
Separated each setAttribute (TransformerFactory) and setProperty (XMLInputFactory) call into individual try/catch blocks. Also improved warning logs to be more descriptive and consistent. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
NeoSaber
left a comment
NeoSaber
left a comment
There was a problem hiding this comment.
The new changes look good. I would ignore copilot's updated review. It's nitpicking its own desired fix now.
abika5
left a comment
abika5
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks.
…rnal Entity Polaris medium severity issues (marklogic#577) * MLE-27554 MLE-27556: Fix XXE Injection - XML External Entity related to XMLInputFactory * MLE-27553 MLE-27555: Fix XXE Injection - XML External Entity related to TransformerFactory * MLE-27553 MLE-27554 MLE-27555 MLE-27556: Split XXE-prevention try/catch blocks and improve warning messages
…rnal Entity Polaris medium severity issues (marklogic#577) * MLE-27667 MLE-27669: Fix XXE Injection - XML External Entity related to XMLInputFactory * MLE-27666 MLE-27668: Fix XXE Injection - XML External Entity related to TransformerFactory * MLE-27666 MLE-27667 MLE-27668 MLE-27669: Split XXE-prevention try/catch blocks and improve warning messages
Problem
XML parsers were created without disabling external entity resolution (CWE-611), which could allow XXE attacks including arbitrary file reads, SSRF, and denial of service via entity expansion.
Changes
1. XMLInputFactory (StAX parsers)
Files:
AggregateXMLReader.javaCompressedAggXMLReader.javaUsed
setProperty(correct API for StAX):Disabled:
XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES = falseXMLInputFactory.SUPPORT_DTD = false2. TransformerFactory (XSLT processing)
Files:
DOMDocument.javaJSONDocument.javaUsed
setAttribute(correct API for transformers):Disabled:
XMLConstants.ACCESS_EXTERNAL_DTD = ""XMLConstants.ACCESS_EXTERNAL_STYLESHEET = ""Wrapped property/attribute configuration in
try-catch (IllegalArgumentException)Logs a warning if a property is unsupported
Testing
06mlcp test suite results
The following tests are failing:
bug17845.xmlmlcp-basic-8000.xmlmlcp-aggregates-8000.xmlmlcp-ssl-protocal-tls-49273.xmlmlcp-export-query-filter.xmlmlcp-group-host-count.xmlsame failures exist without the changes.