ppkarwasz commented on PR #4198: URL: https://github.com/apache/logging-log4j2/pull/4198#issuecomment-5151312925
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](https://github.com/apache/commons-xml), 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](https://logging.apache.org/security.html#threat-common-sources-configuration). 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 #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. -- 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]
