From a3fc5ae5bfcfbc3b256fac276a87fe2420c3c626 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Sat, 27 Jun 2026 21:39:57 +0200 Subject: [PATCH 1/7] 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 --- log4j-core/pom.xml | 5 +++ .../core/config/xml/XmlConfiguration.java | 33 ++++------------- .../osgi/tests/AbstractLoadBundleTest.java | 36 +++++++++++-------- .../log4j/osgi/tests/CoreOsgiTest.java | 1 + .../log4j/osgi/tests/DisruptorTest.java | 1 + log4j-parent/pom.xml | 7 ++++ .../.2.x.x/xml_factory_hardening.xml | 11 ++++++ 7 files changed, 53 insertions(+), 41 deletions(-) create mode 100644 src/changelog/.2.x.x/xml_factory_hardening.xml diff --git a/log4j-core/pom.xml b/log4j-core/pom.xml index b2f34d4ab95..d18948fd9ea 100644 --- a/log4j-core/pom.xml +++ b/log4j-core/pom.xml @@ -157,6 +157,11 @@ commons-csv true + + + eu.copernik + copernik-xml-factory + com.conversantmedia 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 b0a1e560227..b9a6010ddd8 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; @@ -83,7 +84,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; @@ -150,11 +151,9 @@ public XmlConfiguration(final LoggerContext loggerContext, final ConfigurationSo * @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); } @@ -167,27 +166,6 @@ static DocumentBuilder newDocumentBuilder(final boolean xIncludeAware) throws Pa 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 { @@ -198,7 +176,8 @@ private static void validateDocument(final Document document, final String schem // 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 06bb63611d2..9d370ccf5b0 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 @@ private Bundle getApiTestsBundle() throws BundleException { 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 @@ private Bundle getApiTestsBundle() throws BundleException { 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 @@ public void testApiCoreStartStopStartStop() throws BundleException { 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 @@ public void testClassNotFoundErrorLogger() throws BundleException { } 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 @@ public void testClassNotFoundErrorLogger() throws BundleException { 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 @@ public void testLog4J12Fragement() throws BundleException, ReflectiveOperationEx 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 @@ public void testLog4J12Fragement() throws BundleException, ReflectiveOperationEx @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 @@ public void testServiceLoader() throws BundleException, ReflectiveOperationExcep "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 493bb61c9e4..77d7f54cdce 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 813331210c5..018edca9f54 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 28c371abefa..f02f8aea0df 100644 --- a/log4j-parent/pom.xml +++ b/log4j-parent/pom.xml @@ -78,6 +78,7 @@ 1.4.0 1.2.15 + 0.1.2 3.4.4 0.9.0 2.50.0 @@ -404,6 +405,12 @@ ${disruptor.version} + + eu.copernik + copernik-xml-factory + ${copernik-xml-factory.version} + + org.zapodot embedded-ldap-junit 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 00000000000..6146a8e7b8a --- /dev/null +++ b/src/changelog/.2.x.x/xml_factory_hardening.xml @@ -0,0 +1,11 @@ + + + + Harden XML configuration parsing using the Commons XML hardened JAXP factories. + + \ No newline at end of file From 00505d26a2bc33a6cba07e0386b4b0af769d6e62 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Sat, 27 Jun 2026 21:54:05 +0200 Subject: [PATCH 2/7] Reference PR #4162 in the changelog entry Assisted-By: Claude Opus 4.8 --- ...{xml_factory_hardening.xml => 4162_xml_factory_hardening.xml} | 1 + 1 file changed, 1 insertion(+) rename src/changelog/.2.x.x/{xml_factory_hardening.xml => 4162_xml_factory_hardening.xml} (85%) diff --git a/src/changelog/.2.x.x/xml_factory_hardening.xml b/src/changelog/.2.x.x/4162_xml_factory_hardening.xml similarity index 85% rename from src/changelog/.2.x.x/xml_factory_hardening.xml rename to src/changelog/.2.x.x/4162_xml_factory_hardening.xml index 6146a8e7b8a..c7baff22191 100644 --- a/src/changelog/.2.x.x/xml_factory_hardening.xml +++ b/src/changelog/.2.x.x/4162_xml_factory_hardening.xml @@ -5,6 +5,7 @@ https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd" type="fixed"> + Harden XML configuration parsing using the Commons XML hardened JAXP factories. From b51877658322cb8b20c3743ad884250477298a26 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Sat, 27 Jun 2026 21:54:43 +0200 Subject: [PATCH 3/7] fix: BND warning --- .../org/apache/logging/log4j/core/config/xml/package-info.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/package-info.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/package-info.java index 248ae965cec..974bd87602a 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/package-info.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/package-info.java @@ -18,7 +18,7 @@ * Classes and interfaces supporting configuration of Log4j 2 with XML. */ @Export -@Version("2.26.0") +@Version("2.26.1") package org.apache.logging.log4j.core.config.xml; import org.osgi.annotation.bundle.Export; From a1d3af19eb5374126696c32588e01d8cb7fca590 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Sun, 28 Jun 2026 00:22:44 +0200 Subject: [PATCH 4/7] test: skip resolver-based schema validation tests on JDK 8 `XmlConfigurationSchemaTest` exercises schema validation whose `xsd:include`/`import` resources are resolved through an `LSResourceResolver`. On JDK 8 Xerces enforces the `accessExternalSchema` restriction on the resources the resolver returns; the `isCreatedByResolver` exemption that lets resolver-supplied resources bypass that check was only added in JDK 9. copernik-xml-factory sets `accessExternalSchema` to "" at API precedence (deliberately not overridable by a system property), so this resolution path cannot run on JDK 8. Skip the whole class on JDK 8 via a `@BeforeEach` assumption. The previous guard only covered `schemaIncludeRespectsAllowedProtocols` (which already passed on JDK 8), leaving the actually-failing `validConfigurationValidates` unguarded. Assisted-By: Claude Opus 4.8 (1M context) --- .../config/xml/XmlConfigurationSchemaTest.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/xml/XmlConfigurationSchemaTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/xml/XmlConfigurationSchemaTest.java index 6ab354c90ce..3ac9f875dc6 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/xml/XmlConfigurationSchemaTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/xml/XmlConfigurationSchemaTest.java @@ -18,13 +18,17 @@ import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.jupiter.api.Assumptions.assumeTrue; import java.net.URI; import java.util.Objects; +import org.apache.commons.lang3.JavaVersion; +import org.apache.commons.lang3.SystemUtils; import org.apache.logging.log4j.core.LoggerContext; import org.apache.logging.log4j.core.config.ConfigurationException; import org.apache.logging.log4j.core.config.ConfigurationSource; import org.apache.logging.log4j.test.junit.SetTestProperty; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Timeout; @@ -35,6 +39,19 @@ */ class XmlConfigurationSchemaTest { + /** + * These tests exercise schema validation whose {@code xsd:include}/{@code import} resources are resolved through an + * {@link org.w3c.dom.ls.LSResourceResolver}. On JDK 8 Xerces enforces the {@code accessExternalSchema} restriction + * on the resources the resolver returns (the {@code isCreatedByResolver} exemption that lets resolver-supplied + * resources bypass that check was only added in JDK 9), so this resolution path cannot run there. + */ + @BeforeEach + void assumeResolverBasedSchemaValidationSupported() { + assumeTrue( + SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9), + "Resolver-based schema include resolution requires JDK 9 or later."); + } + private static void load(final String name) { final URI uri; try { From 088f2c0ff591d2e0baf78d22419701e82bca3aa4 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Mon, 31 Aug 2026 13:28:30 +0200 Subject: [PATCH 5/7] Replace `copernik-xml-factory` with `commons-secure-xml` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hardened JAXP factory library was donated to Apache Commons as `org.apache.commons:commons-secure-xml`. Switch to the new coordinates, bundle symbolic name and per-factory API — `SecureDocumentBuilderFactory` and `SecureSchemaFactory` — using the `newDefault*` methods, which also opt out of JAXP pluggability. The secure factories install a non-removable fallback resolver that substitutes empty content for every resource the delegate leaves unresolved, so `ConfigurationSourceResolver` can simply return `null` on a resolution failure: a `null` return can no longer re-enable the parser's own URL resolution. Until `commons-secure-xml` 1.0.0 is released, the build resolves the SNAPSHOT from the Apache snapshots repository: the CI build job passes `-Dapache.snapshots`, the activation property of the `use-apache-snapshots` profile inherited from the ASF parent POM. The `deploy-snapshot` and `deploy-release` workflows have no equivalent input, so the branch must not be merged before that release. Assisted-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01EKPqjoEHsyMAJzTnvtDpgz --- .github/workflows/build.yaml | 4 ++ log4j-core/pom.xml | 4 +- .../core/config/xml/XmlConfiguration.java | 50 ++++++++----------- .../osgi/tests/AbstractLoadBundleTest.java | 40 +++++++-------- .../log4j/osgi/tests/CoreOsgiTest.java | 2 +- .../log4j/osgi/tests/DisruptorTest.java | 2 +- log4j-parent/pom.xml | 14 +++--- .../.2.x.x/4162_xml_factory_hardening.xml | 4 +- 8 files changed, 57 insertions(+), 63 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 558b326df82..c0399768a9c 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -43,6 +43,10 @@ jobs: 17 site-enabled: true reproducibility-check-enabled: false + # Resolve the `commons-secure-xml` SNAPSHOT from the Apache snapshots repository + # (activates the `use-apache-snapshots` profile of the ASF parent POM). + # Remove once `commons-secure-xml` 1.0.0 is released. + maven-args: -Dapache.snapshots deploy-snapshot: needs: build diff --git a/log4j-core/pom.xml b/log4j-core/pom.xml index d18948fd9ea..9cf69c39f21 100644 --- a/log4j-core/pom.xml +++ b/log4j-core/pom.xml @@ -159,8 +159,8 @@ - eu.copernik - copernik-xml-factory + org.apache.commons + commons-secure-xml 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 b9a6010ddd8..2e2e894daae 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,12 +16,9 @@ */ 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; -import java.io.Reader; -import java.io.StringReader; import java.net.URI; import java.net.URISyntaxException; import java.time.Instant; @@ -36,6 +33,8 @@ import javax.xml.validation.Schema; import javax.xml.validation.SchemaFactory; import javax.xml.validation.Validator; +import org.apache.commons.xml.secure.SecureDocumentBuilderFactory; +import org.apache.commons.xml.secure.SecureSchemaFactory; import org.apache.logging.log4j.core.LoggerContext; import org.apache.logging.log4j.core.config.AbstractConfiguration; import org.apache.logging.log4j.core.config.Configuration; @@ -84,7 +83,7 @@ public class XmlConfiguration extends AbstractConfiguration implements Reconfigu @SuppressFBWarnings( value = "XXE_DOCUMENT", - justification = "The parsers are hardened by `copernik-xml-factory`; SpotBugs cannot see into the library.") + justification = "The parsers are hardened by `commons-secure-xml`; SpotBugs cannot see into the library.") public XmlConfiguration(final LoggerContext loggerContext, final ConfigurationSource configSource) { super(loggerContext, configSource); byte[] buffer = null; @@ -151,9 +150,9 @@ public XmlConfiguration(final LoggerContext loggerContext, final ConfigurationSo * @throws ParserConfigurationException if a DocumentBuilder cannot be created, which satisfies the configuration requested. */ static DocumentBuilder newDocumentBuilder(final boolean xIncludeAware) throws ParserConfigurationException { - // Hardened factory: by the library's contract it and what it produces never fetch external resources. - final DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory(); - factory.setNamespaceAware(true); + // Hardened factory: by the `commons-secure-xml` contract it and what it produces never fetch external + // resources. + final DocumentBuilderFactory factory = SecureDocumentBuilderFactory.newDefaultNSInstance(); if (xIncludeAware) { factory.setXIncludeAware(true); } @@ -176,8 +175,9 @@ private static void validateDocument(final Document document, final String schem // a schema has its own modularity features (`xsd:include`/`xsd:import`). final Document schemaDocument = newDocumentBuilder(false).parse(ConfigurationSourceResolver.toInputSource(schemaSource)); - // Hardened factory: by the library's contract it and what it produces never fetch external resources. - final SchemaFactory factory = XmlFactories.newSchemaFactory(); + // Hardened factory: by the `commons-secure-xml` contract it and what it produces never fetch external + // resources. + final SchemaFactory factory = SecureSchemaFactory.newDefaultInstance(); 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. @@ -306,6 +306,9 @@ public String toString() { * *

This adds support for the Log4j URI conventions (such as the {@code classpath:} scheme) and subjects every * referenced resource to the {@code ALLOWED_PROTOCOLS} restrictions.

+ * + *

Returning {@code null} for an unresolved resource is safe: the {@code commons-secure-xml} fallback resolver + * substitutes empty content instead of letting the parser fetch the resource itself.

*/ private static final class ConfigurationSourceResolver extends DefaultHandler2 implements LSResourceResolver { @@ -334,13 +337,10 @@ public InputSource resolveEntity( throws SAXException { try { final ConfigurationSource source = toConfigurationSource(systemId, baseURI); - final InputSource inputSource; - if (source != null) { - inputSource = toInputSource(source); - } else { - inputSource = new InputSource(emptyReader()); - inputSource.setSystemId(systemId); + if (source == null) { + return null; } + final InputSource inputSource = toInputSource(source); inputSource.setPublicId(publicId); return inputSource; } catch (final URISyntaxException e) { @@ -350,10 +350,6 @@ public InputSource resolveEntity( /** * Resolves a resource imported by an XML Schema ({@code xsd:import}/{@code xsd:include}). - * - *

Returns an empty input when the resource cannot be resolved, instead of returning {@code null}: a - * {@code null} return would let the parser fall back to its own URL resolution, bypassing the - * {@code ALLOWED_PROTOCOLS} restrictions.

*/ @Override public LSInput resolveResource( @@ -364,14 +360,12 @@ public LSInput resolveResource( final String baseURI) { try { final ConfigurationSource source = toConfigurationSource(systemId, baseURI); - final LSInput input = domLs.createLSInput(); - if (source != null) { - input.setByteStream(source.getInputStream()); - input.setSystemId(source.getLocation()); - } else { - input.setCharacterStream(emptyReader()); - input.setSystemId(systemId); + if (source == null) { + return null; } + final LSInput input = domLs.createLSInput(); + input.setByteStream(source.getInputStream()); + input.setSystemId(source.getLocation()); input.setPublicId(publicId); return input; } catch (final URISyntaxException e) { @@ -381,10 +375,6 @@ public LSInput resolveResource( } } - private static Reader emptyReader() { - return new StringReader(""); - } - private static ConfigurationSource toConfigurationSource(final String systemId, final String baseURI) throws URISyntaxException { if (systemId == null) { 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 9d370ccf5b0..d66b7765c4f 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,8 +75,8 @@ private Bundle getApiTestsBundle() throws BundleException { return installBundle("org.apache.logging.log4j.api.test"); } - private Bundle getXmlFactoryBundle() throws BundleException { - return installBundle("eu.copernik.xml.factory"); + private Bundle getSecureXmlBundle() throws BundleException { + return installBundle("org.apache.commons.xml.secure"); } /** @@ -86,20 +86,20 @@ private Bundle getXmlFactoryBundle() throws BundleException { public void testApiCoreStartStopStartStop() throws BundleException { final Bundle api = getApiBundle(); - final Bundle xmlFactory = getXmlFactoryBundle(); + final Bundle secureXml = getSecureXmlBundle(); 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, xmlFactory, core); - doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, xmlFactory, api); + doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, secureXml, core); + doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, secureXml, api); // 2nd start-stop - doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, xmlFactory, core); - doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, xmlFactory, api); + doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, secureXml, core); + doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, secureXml, api); - doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, core, xmlFactory, api); + doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, core, secureXml, api); } /** @@ -109,10 +109,10 @@ public void testApiCoreStartStopStartStop() throws BundleException { public void testClassNotFoundErrorLogger() throws BundleException { final Bundle api = getApiBundle(); - final Bundle xmlFactory = getXmlFactoryBundle(); + final Bundle secureXml = getSecureXmlBundle(); final Bundle core = getCoreBundle(); - doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, xmlFactory); + doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, secureXml); // fails if LOG4J2-1637 is not fixed try { core.start(); @@ -132,8 +132,8 @@ public void testClassNotFoundErrorLogger() throws BundleException { } assertEquals(Bundle.ACTIVE, core.getState(), String.format("`%s` bundle state mismatch", core)); - doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, xmlFactory, api); - doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, core, xmlFactory, api); + doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, secureXml, api); + doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, core, secureXml, api); } /** @@ -144,11 +144,11 @@ public void testClassNotFoundErrorLogger() throws BundleException { public void testLog4J12Fragement() throws BundleException, ReflectiveOperationException { final Bundle api = getApiBundle(); - final Bundle xmlFactory = getXmlFactoryBundle(); + final Bundle secureXml = getSecureXmlBundle(); final Bundle core = getCoreBundle(); final Bundle compat = get12ApiBundle(); - doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, xmlFactory, core); + doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, secureXml, core); final Class coreClassFromCore = core.loadClass("org.apache.logging.log4j.core.Core"); final Class levelClassFrom12API = core.loadClass("org.apache.log4j.Level"); @@ -163,8 +163,8 @@ public void testLog4J12Fragement() throws BundleException, ReflectiveOperationEx levelClassFromAPI.getClassLoader(), "expected 1.2 API Level NOT to have the same class loader as API Level"); - doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, xmlFactory, api); - doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, compat, core, xmlFactory, api); + doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, secureXml, api); + doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, compat, core, secureXml, api); } /** @@ -173,14 +173,14 @@ public void testLog4J12Fragement() throws BundleException, ReflectiveOperationEx @Test public void testServiceLoader() throws BundleException, ReflectiveOperationException { final Bundle api = getApiBundle(); - final Bundle xmlFactory = getXmlFactoryBundle(); + final Bundle secureXml = getSecureXmlBundle(); 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, xmlFactory, core, apiTests); + doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, secureXml, core, apiTests); final Class osgiServiceLocatorTest = apiTests.loadClass("org.apache.logging.log4j.test.util.OsgiServiceLocatorTest"); @@ -195,8 +195,8 @@ public void testServiceLoader() throws BundleException, ReflectiveOperationExcep "org.apache.logging.log4j.core.impl.Log4jProvider", services.get(0).getClass().getName()); - doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, apiTests, core, xmlFactory, api); - doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, apiTests, core, xmlFactory, api); + doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, apiTests, core, secureXml, api); + doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, apiTests, core, secureXml, 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 77d7f54cdce..9dcc2acc09c 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,7 +46,7 @@ public class CoreOsgiTest { public Option[] config() { return options( linkBundle("org.apache.logging.log4j.api"), - linkBundle("eu.copernik.xml.factory"), + linkBundle("org.apache.commons.xml.secure"), 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 018edca9f54..96bf98eb59d 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,7 +52,7 @@ public class DisruptorTest { public Option[] config() { return options( linkBundle("org.apache.logging.log4j.api"), - linkBundle("eu.copernik.xml.factory"), + linkBundle("org.apache.commons.xml.secure"), 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 f02f8aea0df..a4066fe5c4f 100644 --- a/log4j-parent/pom.xml +++ b/log4j-parent/pom.xml @@ -76,9 +76,9 @@ 2.22.0 3.20.0 1.4.0 + 1.0.0-SNAPSHOT 1.2.15 - 0.1.2 3.4.4 0.9.0 2.50.0 @@ -393,6 +393,12 @@ ${commons-pool2.version} + + org.apache.commons + commons-secure-xml + ${commons-secure-xml.version} + + com.conversantmedia disruptor @@ -405,12 +411,6 @@ ${disruptor.version} - - eu.copernik - copernik-xml-factory - ${copernik-xml-factory.version} - - org.zapodot embedded-ldap-junit diff --git a/src/changelog/.2.x.x/4162_xml_factory_hardening.xml b/src/changelog/.2.x.x/4162_xml_factory_hardening.xml index c7baff22191..9487b670a94 100644 --- a/src/changelog/.2.x.x/4162_xml_factory_hardening.xml +++ b/src/changelog/.2.x.x/4162_xml_factory_hardening.xml @@ -7,6 +7,6 @@ type="fixed"> - Harden XML configuration parsing using the Commons XML hardened JAXP factories. + Harden XML configuration parsing using the hardened JAXP factories of Apache Commons Secure XML. - \ No newline at end of file + From ba893d759dc7a22b85096fa0e6fa3a5e4ab41a86 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Mon, 31 Aug 2026 13:29:16 +0200 Subject: [PATCH 6/7] fix: comments --- .github/workflows/build.yaml | 3 +- .../core/config/xml/XmlConfiguration.java | 31 +++++++++---------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index c0399768a9c..673b7b04789 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -44,9 +44,8 @@ jobs: site-enabled: true reproducibility-check-enabled: false # Resolve the `commons-secure-xml` SNAPSHOT from the Apache snapshots repository - # (activates the `use-apache-snapshots` profile of the ASF parent POM). # Remove once `commons-secure-xml` 1.0.0 is released. - maven-args: -Dapache.snapshots + maven-args: -Puse-apache-snapshots deploy-snapshot: needs: build 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 2e2e894daae..1c198797a8a 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 @@ -150,8 +150,7 @@ public XmlConfiguration(final LoggerContext loggerContext, final ConfigurationSo * @throws ParserConfigurationException if a DocumentBuilder cannot be created, which satisfies the configuration requested. */ static DocumentBuilder newDocumentBuilder(final boolean xIncludeAware) throws ParserConfigurationException { - // Hardened factory: by the `commons-secure-xml` contract it and what it produces never fetch external - // resources. + // Hardened factory, which never fetches external resources. final DocumentBuilderFactory factory = SecureDocumentBuilderFactory.newDefaultNSInstance(); if (xIncludeAware) { factory.setXIncludeAware(true); @@ -175,8 +174,7 @@ private static void validateDocument(final Document document, final String schem // a schema has its own modularity features (`xsd:include`/`xsd:import`). final Document schemaDocument = newDocumentBuilder(false).parse(ConfigurationSourceResolver.toInputSource(schemaSource)); - // Hardened factory: by the `commons-secure-xml` contract it and what it produces never fetch external - // resources. + // Hardened factory, which never fetches external resources. final SchemaFactory factory = SecureSchemaFactory.newDefaultInstance(); factory.setResourceResolver(ConfigurationSourceResolver.INSTANCE); // The system id is the base URI against which the schema's `xsd:include`/`xsd:import` resources @@ -337,15 +335,16 @@ public InputSource resolveEntity( throws SAXException { try { final ConfigurationSource source = toConfigurationSource(systemId, baseURI); - if (source == null) { - return null; + if (source != null) { + final InputSource inputSource = toInputSource(source); + inputSource.setPublicId(publicId); + return inputSource; } - final InputSource inputSource = toInputSource(source); - inputSource.setPublicId(publicId); - return inputSource; } catch (final URISyntaxException e) { throw new SAXException(e); } + // Fallback to Commons XML ignore-all floor. + return null; } /** @@ -360,19 +359,19 @@ public LSInput resolveResource( final String baseURI) { try { final ConfigurationSource source = toConfigurationSource(systemId, baseURI); - if (source == null) { - return null; + if (source != null) { + final LSInput input = domLs.createLSInput(); + input.setByteStream(source.getInputStream()); + input.setSystemId(source.getLocation()); + input.setPublicId(publicId); } - final LSInput input = domLs.createLSInput(); - input.setByteStream(source.getInputStream()); - input.setSystemId(source.getLocation()); - input.setPublicId(publicId); - return input; } catch (final URISyntaxException e) { final LSException lsException = new LSException(LSException.PARSE_ERR, e.getMessage()); lsException.initCause(e); throw lsException; } + // Fallback to Commons XML ignore-all floor. + return null; } private static ConfigurationSource toConfigurationSource(final String systemId, final String baseURI) From cfebb6b275389eb29f2edf0973fa9f764568af24 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Thu, 3 Sep 2026 07:04:33 +0200 Subject: [PATCH 7/7] Use the Commons Secure XML 1.0.0 release candidate Bump org.apache.commons:commons-secure-xml from 1.0.0-SNAPSHOT to 1.0.0 and add the temporary staging repository https://repository.apache.org/content/repositories/orgapachecommons-1962/ after Central, so the vote gets downstream CI results. Drop the -Puse-apache-snapshots profile from the CI workflows, which the release version no longer needs. Remove the staging repository once 1.0.0 is released. Assisted-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0167e29ScPEdfzJnEFm95imK --- .github/workflows/build.yaml | 3 --- log4j-parent/pom.xml | 2 +- pom.xml | 21 +++++++++++++++++++++ 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 673b7b04789..558b326df82 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -43,9 +43,6 @@ jobs: 17 site-enabled: true reproducibility-check-enabled: false - # Resolve the `commons-secure-xml` SNAPSHOT from the Apache snapshots repository - # Remove once `commons-secure-xml` 1.0.0 is released. - maven-args: -Puse-apache-snapshots deploy-snapshot: needs: build diff --git a/log4j-parent/pom.xml b/log4j-parent/pom.xml index a4066fe5c4f..e5f8bd6594d 100644 --- a/log4j-parent/pom.xml +++ b/log4j-parent/pom.xml @@ -76,7 +76,7 @@ 2.22.0 3.20.0 1.4.0 - 1.0.0-SNAPSHOT + 1.0.0 1.2.15 3.4.4 diff --git a/pom.xml b/pom.xml index be528c33bb2..6311cad09b7 100644 --- a/pom.xml +++ b/pom.xml @@ -572,6 +572,27 @@ + + + + + false + + central + Central Repository + https://repo.maven.apache.org/maven2 + + + + + false + + apache.commons.staging + Apache Commons Secure XML 1.0.0 release candidate + https://repository.apache.org/content/repositories/orgapachecommons-1962/ + + +