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`.
--
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]
