This is an automated email from the ASF dual-hosted git repository. ppkarwasz pushed a commit to branch feat/2.x/xml-factory-hardening in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit 810da54997c11d22e879985c98783d80c37266b9 Author: Piotr P. Karwasz <[email protected]> AuthorDate: Sat Jun 27 21:39:57 2026 +0200 Harden XML configuration parsing with `copernik-xml-factory` Replace the hand-rolled `DocumentBuilderFactory` hardening in `XmlConfiguration` with the hardened JAXP factories from `eu.copernik:copernik-xml-factory`, and harden the schema-validation path the same way: * `newDocumentBuilder` uses `XmlFactories.newDocumentBuilderFactory()`, * `validateDocument` uses `XmlFactories.newSchemaFactory()`. By the library's contract these factories, and everything they produce, never fetch external entities, DTDs or schemas, across the JAXP implementations the library recognizes. The OSGi integration tests link the `eu.copernik.xml.factory` bundle. Assisted-By: Claude Opus 4.8 <[email protected]> --- log4j-core/pom.xml | 5 +++ .../log4j/core/config/xml/XmlConfiguration.java | 33 ++++---------------- .../log4j/osgi/tests/AbstractLoadBundleTest.java | 36 +++++++++++++--------- .../logging/log4j/osgi/tests/CoreOsgiTest.java | 1 + .../logging/log4j/osgi/tests/DisruptorTest.java | 1 + log4j-parent/pom.xml | 7 +++++ src/changelog/.2.x.x/xml_factory_hardening.xml | 11 +++++++ 7 files changed, 53 insertions(+), 41 deletions(-) diff --git a/log4j-core/pom.xml b/log4j-core/pom.xml index b2f34d4ab9..d18948fd9e 100644 --- a/log4j-core/pom.xml +++ b/log4j-core/pom.xml @@ -157,6 +157,11 @@ <artifactId>commons-csv</artifactId> <optional>true</optional> </dependency> + <!-- Hardened JAXP factories for XML configuration parsing --> + <dependency> + <groupId>eu.copernik</groupId> + <artifactId>copernik-xml-factory</artifactId> + </dependency> <!-- Alternative implementation of BlockingQueue using Conversant Disruptor for AsyncAppender --> <dependency> <groupId>com.conversantmedia</groupId> diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/XmlConfiguration.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/XmlConfiguration.java index 06800f991d..ee983d1355 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/XmlConfiguration.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/XmlConfiguration.java @@ -16,6 +16,7 @@ */ package org.apache.logging.log4j.core.config.xml; +import eu.copernik.xml.factory.XmlFactories; import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; @@ -79,7 +80,7 @@ public class XmlConfiguration extends AbstractConfiguration implements Reconfigu @SuppressFBWarnings( value = "XXE_DOCUMENT", - justification = "The `newDocumentBuilder` method disables DTD processing.") + justification = "The parsers are hardened by `copernik-xml-factory`; SpotBugs cannot see into the library.") public XmlConfiguration(final LoggerContext loggerContext, final ConfigurationSource configSource) { super(loggerContext, configSource); byte[] buffer = null; @@ -146,11 +147,9 @@ public class XmlConfiguration extends AbstractConfiguration implements Reconfigu * @throws ParserConfigurationException if a DocumentBuilder cannot be created, which satisfies the configuration requested. */ static DocumentBuilder newDocumentBuilder(final boolean xIncludeAware) throws ParserConfigurationException { - final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + // Hardened factory: by the library's contract it and what it produces never fetch external resources. + final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); factory.setNamespaceAware(true); - - disableDtdProcessing(factory); - if (xIncludeAware) { factory.setXIncludeAware(true); } @@ -163,27 +162,6 @@ public class XmlConfiguration extends AbstractConfiguration implements Reconfigu return builder; } - private static void disableDtdProcessing(final DocumentBuilderFactory factory) { - factory.setValidating(false); - factory.setExpandEntityReferences(false); - setFeature(factory, "http://xml.org/sax/features/external-general-entities";, false); - setFeature(factory, "http://xml.org/sax/features/external-parameter-entities";, false); - setFeature(factory, "http://apache.org/xml/features/nonvalidating/load-external-dtd";, false); - } - - private static void setFeature( - final DocumentBuilderFactory factory, final String featureName, final boolean value) { - try { - factory.setFeature(featureName, value); - } catch (final ParserConfigurationException e) { - LOGGER.warn( - "The DocumentBuilderFactory [{}] does not support the feature [{}]: {}", factory, featureName, e); - } catch (final AbstractMethodError err) { - LOGGER.warn( - "The DocumentBuilderFactory [{}] is out of date and does not support setFeature: {}", factory, err); - } - } - private static void validateDocument(final Document document, final String schemaLocation) throws ConfigurationException { try { @@ -194,7 +172,8 @@ public class XmlConfiguration extends AbstractConfiguration implements Reconfigu // a schema has its own modularity features (`xsd:include`/`xsd:import`). final Document schemaDocument = newDocumentBuilder(false).parse(ConfigurationSourceResolver.toInputSource(schemaSource)); - final SchemaFactory factory = SchemaFactory.newInstance(XMLConstants.W3C_XML_SCHEMA_NS_URI); + // Hardened factory: by the library's contract it and what it produces never fetch external resources. + final SchemaFactory factory = XmlFactories.newSchemaFactory(); factory.setResourceResolver(ConfigurationSourceResolver.INSTANCE); // The system id is the base URI against which the schema's `xsd:include`/`xsd:import` resources // are resolved by the resource resolver above. diff --git a/log4j-osgi-test/src/test/java/org/apache/logging/log4j/osgi/tests/AbstractLoadBundleTest.java b/log4j-osgi-test/src/test/java/org/apache/logging/log4j/osgi/tests/AbstractLoadBundleTest.java index 06bb63611d..9d370ccf5b 100644 --- a/log4j-osgi-test/src/test/java/org/apache/logging/log4j/osgi/tests/AbstractLoadBundleTest.java +++ b/log4j-osgi-test/src/test/java/org/apache/logging/log4j/osgi/tests/AbstractLoadBundleTest.java @@ -75,6 +75,10 @@ abstract class AbstractLoadBundleTest { return installBundle("org.apache.logging.log4j.api.test"); } + private Bundle getXmlFactoryBundle() throws BundleException { + return installBundle("eu.copernik.xml.factory"); + } + /** * Tests starting, then stopping, then restarting, then stopping, and finally uninstalling the API and Core bundles */ @@ -82,19 +86,20 @@ abstract class AbstractLoadBundleTest { public void testApiCoreStartStopStartStop() throws BundleException { final Bundle api = getApiBundle(); + final Bundle xmlFactory = getXmlFactoryBundle(); final Bundle core = getCoreBundle(); assertEquals(Bundle.INSTALLED, api.getState(), "api is not in INSTALLED state"); assertEquals(Bundle.INSTALLED, core.getState(), "core is not in INSTALLED state"); // 1st start-stop - doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, core); - doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, api); + doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, xmlFactory, core); + doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, xmlFactory, api); // 2nd start-stop - doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, core); - doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, api); + doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, xmlFactory, core); + doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, xmlFactory, api); - doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, core, api); + doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, core, xmlFactory, api); } /** @@ -104,9 +109,10 @@ abstract class AbstractLoadBundleTest { public void testClassNotFoundErrorLogger() throws BundleException { final Bundle api = getApiBundle(); + final Bundle xmlFactory = getXmlFactoryBundle(); final Bundle core = getCoreBundle(); - doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api); + doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, xmlFactory); // fails if LOG4J2-1637 is not fixed try { core.start(); @@ -126,8 +132,8 @@ abstract class AbstractLoadBundleTest { } assertEquals(Bundle.ACTIVE, core.getState(), String.format("`%s` bundle state mismatch", core)); - doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, api); - doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, core, api); + doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, xmlFactory, api); + doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, core, xmlFactory, api); } /** @@ -138,10 +144,11 @@ abstract class AbstractLoadBundleTest { public void testLog4J12Fragement() throws BundleException, ReflectiveOperationException { final Bundle api = getApiBundle(); + final Bundle xmlFactory = getXmlFactoryBundle(); final Bundle core = getCoreBundle(); final Bundle compat = get12ApiBundle(); - doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, core); + doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, xmlFactory, core); final Class<?> coreClassFromCore = core.loadClass("org.apache.logging.log4j.core.Core"); final Class<?> levelClassFrom12API = core.loadClass("org.apache.log4j.Level"); @@ -156,8 +163,8 @@ abstract class AbstractLoadBundleTest { levelClassFromAPI.getClassLoader(), "expected 1.2 API Level NOT to have the same class loader as API Level"); - doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, api); - doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, compat, core, api); + doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, xmlFactory, api); + doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, compat, core, xmlFactory, api); } /** @@ -166,13 +173,14 @@ abstract class AbstractLoadBundleTest { @Test public void testServiceLoader() throws BundleException, ReflectiveOperationException { final Bundle api = getApiBundle(); + final Bundle xmlFactory = getXmlFactoryBundle(); final Bundle core = getCoreBundle(); final Bundle apiTests = getApiTestsBundle(); final Class<?> osgiServiceLocator = api.loadClass("org.apache.logging.log4j.util.OsgiServiceLocator"); assertTrue((boolean) osgiServiceLocator.getMethod("isAvailable").invoke(null), "OsgiServiceLocator is active"); - doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, core, apiTests); + doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, xmlFactory, core, apiTests); final Class<?> osgiServiceLocatorTest = apiTests.loadClass("org.apache.logging.log4j.test.util.OsgiServiceLocatorTest"); @@ -187,8 +195,8 @@ abstract class AbstractLoadBundleTest { "org.apache.logging.log4j.core.impl.Log4jProvider", services.get(0).getClass().getName()); - doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, apiTests, core, api); - doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, apiTests, core, api); + doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, apiTests, core, xmlFactory, api); + doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, apiTests, core, xmlFactory, api); } private static void doOnBundlesAndVerifyState( diff --git a/log4j-osgi-test/src/test/java/org/apache/logging/log4j/osgi/tests/CoreOsgiTest.java b/log4j-osgi-test/src/test/java/org/apache/logging/log4j/osgi/tests/CoreOsgiTest.java index 493bb61c9e..77d7f54cdc 100644 --- a/log4j-osgi-test/src/test/java/org/apache/logging/log4j/osgi/tests/CoreOsgiTest.java +++ b/log4j-osgi-test/src/test/java/org/apache/logging/log4j/osgi/tests/CoreOsgiTest.java @@ -46,6 +46,7 @@ public class CoreOsgiTest { public Option[] config() { return options( linkBundle("org.apache.logging.log4j.api"), + linkBundle("eu.copernik.xml.factory"), linkBundle("org.apache.logging.log4j.core"), linkBundle("org.apache.logging.log4j.1.2.api").start(false), // required by Pax Exam's logging diff --git a/log4j-osgi-test/src/test/java/org/apache/logging/log4j/osgi/tests/DisruptorTest.java b/log4j-osgi-test/src/test/java/org/apache/logging/log4j/osgi/tests/DisruptorTest.java index 813331210c..018edca9f5 100644 --- a/log4j-osgi-test/src/test/java/org/apache/logging/log4j/osgi/tests/DisruptorTest.java +++ b/log4j-osgi-test/src/test/java/org/apache/logging/log4j/osgi/tests/DisruptorTest.java @@ -52,6 +52,7 @@ public class DisruptorTest { public Option[] config() { return options( linkBundle("org.apache.logging.log4j.api"), + linkBundle("eu.copernik.xml.factory"), linkBundle("org.apache.logging.log4j.core"), linkBundle("com.lmax.disruptor"), // required by Pax Exam's logging diff --git a/log4j-parent/pom.xml b/log4j-parent/pom.xml index 9a7b5aa2c8..5ea9c62023 100644 --- a/log4j-parent/pom.xml +++ b/log4j-parent/pom.xml @@ -78,6 +78,7 @@ <commons-logging.version>1.3.5</commons-logging.version> <!-- `com.conversantmedia:disruptor` version 1.2.16 requires Java 9: --> <conversant.disruptor.version>1.2.15</conversant.disruptor.version> + <copernik-xml-factory.version>0.1.2</copernik-xml-factory.version> <disruptor.version>3.4.4</disruptor.version> <embedded-ldap.version>0.9.0</embedded-ldap.version> <error-prone-annotations.version>2.38.0</error-prone-annotations.version> @@ -404,6 +405,12 @@ <version>${disruptor.version}</version> </dependency> + <dependency> + <groupId>eu.copernik</groupId> + <artifactId>copernik-xml-factory</artifactId> + <version>${copernik-xml-factory.version}</version> + </dependency> + <dependency> <groupId>org.zapodot</groupId> <artifactId>embedded-ldap-junit</artifactId> diff --git a/src/changelog/.2.x.x/xml_factory_hardening.xml b/src/changelog/.2.x.x/xml_factory_hardening.xml new file mode 100644 index 0000000000..6146a8e7b8 --- /dev/null +++ b/src/changelog/.2.x.x/xml_factory_hardening.xml @@ -0,0 +1,11 @@ +<?xml version="1.0" encoding="UTF-8"?> +<entry xmlns="https://logging.apache.org/xml/ns"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation=" + https://logging.apache.org/xml/ns + https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"; + type="fixed"> + <description format="asciidoc"> + Harden XML configuration parsing using the Commons XML hardened JAXP factories. + </description> +</entry> \ No newline at end of file
