You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@logging.apache.org by pk...@apache.org on 2026/06/27 19:40:06 UTC
(logging-log4j2) branch feat/2.x/xml-factory-hardening created (now 810da54997)
This is an automated email from the ASF dual-hosted git repository.
ppkarwasz pushed a change to branch feat/2.x/xml-factory-hardening
in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
at 810da54997 Harden XML configuration parsing with `copernik-xml-factory`
This branch includes the following new commits:
new 810da54997 Harden XML configuration parsing with `copernik-xml-factory`
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(logging-log4j2) 01/01: Harden XML configuration parsing with `copernik-xml-factory`
Posted by pk...@apache.org.
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 <pk...@apache.org>
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 <no...@anthropic.com>
---
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