disable external entity resolution in log4j 1.x XmlConfiguration

[jmestwa-coder]

The Log4j 1.x XmlConfiguration reader resolves external XML entities.

• Log4jEntityResolver only handles log4j.dtd and returns null otherwise, so the parser falls back to default resolution and fetches external entities
• a legacy log4j.xml with a SYSTEM entity can read local files or reach network URLs

Fix, per review:

• DTD validation stays enabled as before
• Log4jEntityResolver now resolves any external resource other than the bundled log4j.dtd to an empty source and logs a status warning, so the parser never falls back to default resolution regardless of the JAXP provider

Test loads a config with an external entity and asserts it is not resolved.

Checklist

• Base your changes on 2.x branch
• ./mvnw verify succeeds
• Non-trivial changes contain an entry file in the src/changelog/.2.x.x directory
• Tests are provided

[ramanathan1504]

@jmestwa-coder Thanks for the PR! I appreciate your patience. I'll try to get it closed ASAP.

[ramanathan1504]

@vy @ppkarwasz could one of you take a look? I did rather hand this over than guess on a security related things.

[ppkarwasz]

As the name suggests, this setting applies to non-validating parsers. Resolving the external subset is required in validating parsers and can not be turned off.

[jmestwa-coder]

right, missed that this feature only applies to non-validating parsers. dropped the feature-based approach entirely in favor of the resolver.

[ppkarwasz]

Hi @jmestwa-coder,

Thank you for working on this.

I do not think the proposed parser features are the right approach for Log4j 1 XML configuration files.

1. These features are implementation-specific rather than required by JAXP. Xerces and the JDK’s Xerces fork support them, but other JAXP providers are not required to do so. Silently swallowing ParserConfigurationException would therefore produce provider-dependent, best-effort hardening: some parsers would be restricted, while others would continue resolving external resources.

2. As noted above, http://apache.org/xml/features/nonvalidating/load-external-dtd only applies to non-validating parsers. It does not prevent a validating parser from loading the external subset.

3. The parser already uses an EntityResolver. JAXP parsers are required to resolve external subsets and entities through that resolver, so we can enforce a provider-independent policy there: resolve the bundled log4j.dtd normally and return an empty source for every other external resource.

4. We also have the incubating commons-xml project, whose purpose is exactly to centralize this kind of XML parser hardening instead of requiring every project to reproduce, and often cargo-cult, the same collection of implementation-specific settings.

Log4j configuration files are considered trusted, operator-controlled sources. Completely disabling external resources therefore provides limited additional protection, while potentially breaking existing configurations. The Log4j 1 compatibility layer needs to account for more than 25 years of configuration files deployed in real applications.

I therefore suggest that we:

• Keep DTD validation enabled, but constrain external resource resolution through Log4jEntityResolver. This would be similar to the approach proposed for log4j-core in Resolve XML configuration resources through ConfigurationSource #4161.
• Address the existing validation behavior separately. Validation is currently enabled, but validation errors are only written to the status logger and do not prevent the configuration from being used.

For the validation issue, we should consider one of the following approaches:

• Fix the bundled log4j.dtd and our example configurations so that all supported configurations validate without warnings.
• Make the bundled log4j.dtd stricter, so that it accepts only appenders and layouts that the Log4j 1 bridge can translate into Log4j 2 Core equivalents.

Custom Log4j 1 appenders have worked again since 2.17.1, but their use is strongly discouraged, so the second option may be worth considering carefully.

[garydgregory]

I generally agree with @ppkarwasz. In my view, the primary consideration should be backward compatibility. When I use the Log4j 1 compatibility layer, it's because I need it, not because I want to. As a result, compatibility needs to be as robust and faithful as possible.

Over time, both my transitive dependencies and my own applications will (or should) complete their migration to Log4j 2. Until then, compatibility takes precedence.

[jmestwa-coder]

makes sense, reworked it along those lines:

• reverted the parser features and setValidating(false); dtd validation stays enabled as before
• Log4jEntityResolver still serves the bundled log4j.dtd, but now returns an empty source instead of null for anything else, so the policy is enforced through the resolver and doesn't depend on the JAXP provider
• it logs a status warning with the public/system id when it ignores a resource, so configs that relied on external entities get a visible hint instead of a silent behavior change
• test unchanged, full log4j-1.2-api suite passes

agreed the validation behavior is a separate topic, so I left the log4j.dtd cleanup/strictening out of this PR. commons-xml looks like the right long-term home for the generic hardening once it's usable.