ppkarwasz commented on code in PR #4198:
URL: https://github.com/apache/logging-log4j2/pull/4198#discussion_r3811007570
##########
log4j-1.2-api/src/main/java/org/apache/log4j/xml/Log4jEntityResolver.java:
##########
@@ -46,6 +48,7 @@ public InputSource resolveEntity(final String publicId, final
String systemId) {
}
return new InputSource(in);
}
- return null;
+ LOGGER.warn("Ignoring external resource with public ID [{}] and system
ID [{}].", publicId, systemId);
+ return new InputSource(new
ByteArrayInputStream(Constants.EMPTY_BYTE_ARRAY));
Review Comment:
This goes in the right direction in my opinion, but the main question
remains: why are we making this change? What do users gain by upgrading?
As far as I can tell, nothing. Configuration files are trusted data. An
attacker who can modify a Log4j configuration file will almost certainly:
- have write access to the rest of the classpath as well,
- be able to do considerably more damage than an SSRF, e.g. instantiate any
class on the classpath and call its setters.
Meanwhile, by Hyrum's law, some deployment out there **does** rely on
fetching entities from an external file, and we would be breaking it. To
silence a SAST warning? To correct a `@SuppressFBWarnings` justification I
wrote some years ago?
I would rather take the same route as #4161: don't forbid the usage, gate it
behind an additional configuration toggle.
`ConfigurationSource.fromUri`/`fromResource` are the methods we use to fetch
the configuration file in the first place, so restricting external entity
fetches to what those methods allow seems reasonable:
```java
// Resolve using `ConfigurationSource`
if (systemId != null) {
try {
final ConfigurationSource source = ConfigurationSource.fromUri(new
URI(systemId));
if (source != null) {
final InputSource inputSource = new
InputSource(source.getInputStream());
inputSource.setSystemId(systemId);
return inputSource;
}
} catch (final URISyntaxException e) {
LOGGER.warn("Resolution error: invalid URI {}", systemId, e);
}
}
// Fall back to empty resource
LOGGER.warn("Resolution error: unable to resolve {}", systemId);
return new InputSource(new ByteArrayInputStream(Constants.EMPTY_BYTE_ARRAY));
```
This doesn't prevent anyone from keeping their current configuration. It
only asks them to explicitly **opt in** to discouraged practices, such as
fetching resources over `http`.
Are we recommending external entities in 2026? Certainly not. But we don't
recommend running a Log4j 1 configuration on Log4j 2 Core, or writing a
`log4j2.properties` file either, and both are still supported. That choice
isn't ours to make.
--
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]
