ppkarwasz commented on code in PR #4144:
URL: https://github.com/apache/logging-log4j2/pull/4144#discussion_r3395301905
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/XmlConfiguration.java:
##########
@@ -84,24 +88,9 @@
}
final InputSource source = new InputSource(new
ByteArrayInputStream(buffer));
source.setSystemId(configSource.getLocation());
- final DocumentBuilder documentBuilder = newDocumentBuilder(true);
- Document document;
- try {
- document = documentBuilder.parse(source);
- } catch (final Exception e) {
- // LOG4J2-1127
- final Throwable throwable = Throwables.getRootCause(e);
- if (throwable instanceof UnsupportedOperationException) {
- LOGGER.warn(
- "The DocumentBuilder {} does not support an
operation: {}."
- + "Trying again without XInclude...",
- documentBuilder,
- e);
- document = newDocumentBuilder(false).parse(source);
- } else {
- throw e;
- }
- }
+ final boolean xIncludeAware =
PropertiesUtil.getProperties().getBooleanProperty(ENABLE_XINCLUDE_PROP);
+ final DocumentBuilder documentBuilder =
newDocumentBuilder(xIncludeAware);
+ final Document document = documentBuilder.parse(source);
Review Comment:
This is **exactly** the finding this PR was meant to make impossible, which
is what makes it depressing: delegating to `copernik-xml-factory` was supposed
to *shield* this code from XXE/SSRF findings by centralizing the JAXP
hardening, not attract them.
Two things might have triggered it:
1. **Cargo-culting.** It saw `documentBuilder.parse(source)` with none of
the usual `setFeature(...)` cargo-culted next to the call, and read that
absence as "unhardened parser", even though the hardening now lives in
`XmlFactories.newDocumentBuilder(...)`.
2. **Deeper dataflow analysis.** It understood that external entities and
`xi:include` targets *can* be resolved via `ConfigurationSource.fromUri`.
I am afraid it is (1), the depressing case: the tool would be firing
precisely because we *stopped* cargo-culting, and any SAST tool that rewards
inline boilerplate will keep re-raising this.
If it is (2), that is a good sign, because it means the tool engaged with
the real dataflow and just needs an explanation: the custom `EntityResolver`
resolves through the *same* `ConfigurationSource.fromUri` method that loads the
configuration file itself, propagating the trust we already place in that file
to the resources it references. No trust boundary is crossed, resolution stays
bounded by `log4j2.configurationAllowedProtocols`, and the default resolver is
**never** invoked.
--
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]
