Harden XML configuration parsing with commons-xml

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 {

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>org.apache.commons</groupId>
+      <artifactId>commons-secure-xml</artifactId>
+    </dependency>
     <!-- Alternative implementation of BlockingQueue using Conversant Disruptor for AsyncAppender -->
     <dependency>
       <groupId>com.conversantmedia</groupId>

log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/XmlConfiguration.java

@@ -19,8 +19,6 @@
 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;
@@ -35,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;
@@ -83,7 +83,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 `commons-secure-xml`; SpotBugs cannot see into the library.")
     public XmlConfiguration(final LoggerContext loggerContext, final ConfigurationSource configSource) {
         super(loggerContext, configSource);
         byte[] buffer = null;
@@ -150,11 +150,8 @@ 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();
-        factory.setNamespaceAware(true);
-
-        disableDtdProcessing(factory);
-
+        // Hardened factory, which never fetches external resources.
+        final DocumentBuilderFactory factory = SecureDocumentBuilderFactory.newDefaultNSInstance();
         if (xIncludeAware) {
             factory.setXIncludeAware(true);
         }
@@ -167,27 +164,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 +174,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, 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
                 // are resolved by the resource resolver above.
@@ -327,6 +304,9 @@ public String toString() {
      *
      * <p>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.</p>
+     *
+     * <p>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.</p>
      */
     private static final class ConfigurationSourceResolver extends DefaultHandler2 implements LSResourceResolver {
 
@@ -355,26 +335,20 @@ 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);
+                    final InputSource inputSource = toInputSource(source);
+                    inputSource.setPublicId(publicId);
+                    return inputSource;
                 }
-                inputSource.setPublicId(publicId);
-                return inputSource;
             } catch (final URISyntaxException e) {
                 throw new SAXException(e);
             }
+            // Fallback to Commons XML ignore-all floor.
+            return null;
         }
 
         /**
          * Resolves a resource imported by an XML Schema ({@code xsd:import}/{@code xsd:include}).
-         *
-         * <p>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.</p>
          */
         @Override
         public LSInput resolveResource(
@@ -385,25 +359,19 @@ public LSInput resolveResource(
                 final String baseURI) {
             try {
                 final ConfigurationSource source = toConfigurationSource(systemId, baseURI);
-                final LSInput input = domLs.createLSInput();
                 if (source != null) {
+                    final LSInput input = domLs.createLSInput();
                     input.setByteStream(source.getInputStream());
                     input.setSystemId(source.getLocation());
-                } else {
-                    input.setCharacterStream(emptyReader());
-                    input.setSystemId(systemId);
+                    input.setPublicId(publicId);
                 }
-                input.setPublicId(publicId);
-                return input;
             } catch (final URISyntaxException e) {
                 final LSException lsException = new LSException(LSException.PARSE_ERR, e.getMessage());
                 lsException.initCause(e);
                 throw lsException;
             }
-        }
-
-        private static Reader emptyReader() {
-            return new StringReader("");
+            // Fallback to Commons XML ignore-all floor.
+            return null;
         }
 
         private static ConfigurationSource toConfigurationSource(final String systemId, final String baseURI)

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;

log4j-osgi-test/src/test/java/org/apache/logging/log4j/osgi/tests/AbstractLoadBundleTest.java

@@ -75,26 +75,31 @@ private Bundle getApiTestsBundle() throws BundleException {
         return installBundle("org.apache.logging.log4j.api.test");
     }
 
+    private Bundle getSecureXmlBundle() throws BundleException {
+        return installBundle("org.apache.commons.xml.secure");
+    }
+
     /**
      * Tests starting, then stopping, then restarting, then stopping, and finally uninstalling the API and Core bundles
      */
     @Test
     public void testApiCoreStartStopStartStop() throws BundleException {
 
         final Bundle api = getApiBundle();
+        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, core);
-        doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, 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, core);
-        doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, api);
+        doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, secureXml, core);
+        doOnBundlesAndVerifyState(Bundle::stop, Bundle.RESOLVED, core, secureXml, api);
 
-        doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, core, api);
+        doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, core, secureXml, api);
     }
 
     /**
@@ -104,9 +109,10 @@ public void testApiCoreStartStopStartStop() throws BundleException {
     public void testClassNotFoundErrorLogger() throws BundleException {
 
         final Bundle api = getApiBundle();
+        final Bundle secureXml = getSecureXmlBundle();
         final Bundle core = getCoreBundle();
 
-        doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api);
+        doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, secureXml);
         // 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, secureXml, api);
+        doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, core, secureXml, api);
     }
 
     /**
@@ -138,10 +144,11 @@ public void testClassNotFoundErrorLogger() throws BundleException {
     public void testLog4J12Fragement() throws BundleException, ReflectiveOperationException {
 
         final Bundle api = getApiBundle();
+        final Bundle secureXml = getSecureXmlBundle();
         final Bundle core = getCoreBundle();
         final Bundle compat = get12ApiBundle();
 
-        doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, 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");
@@ -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, secureXml, api);
+        doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, compat, core, secureXml, api);
     }
 
     /**
@@ -166,13 +173,14 @@ public void testLog4J12Fragement() throws BundleException, ReflectiveOperationEx
     @Test
     public void testServiceLoader() throws BundleException, ReflectiveOperationException {
         final Bundle api = getApiBundle();
+        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, core, apiTests);
+        doOnBundlesAndVerifyState(Bundle::start, Bundle.ACTIVE, api, secureXml, 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, secureXml, api);
+        doOnBundlesAndVerifyState(Bundle::uninstall, Bundle.UNINSTALLED, apiTests, core, secureXml, api);
     }
 
     private static void doOnBundlesAndVerifyState(

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("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

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("org.apache.commons.xml.secure"),
                 linkBundle("org.apache.logging.log4j.core"),
                 linkBundle("com.lmax.disruptor"),
                 // required by Pax Exam's logging

log4j-parent/pom.xml

@@ -76,6 +76,7 @@
     <commons-io.version>2.22.0</commons-io.version>
     <commons-lang3.version>3.20.0</commons-lang3.version>
     <commons-logging.version>1.4.0</commons-logging.version>
+    <commons-secure-xml.version>1.0.0</commons-secure-xml.version>
     <!-- `com.conversantmedia:disruptor` version 1.2.16 requires Java 9: -->
     <conversant.disruptor.version>1.2.15</conversant.disruptor.version>
     <disruptor.version>3.4.4</disruptor.version>
@@ -392,6 +393,12 @@
         <version>${commons-pool2.version}</version>
       </dependency>
 
+      <dependency>
+        <groupId>org.apache.commons</groupId>
+        <artifactId>commons-secure-xml</artifactId>
+        <version>${commons-secure-xml.version}</version>
+      </dependency>
+
       <dependency>
         <groupId>com.conversantmedia</groupId>
         <artifactId>disruptor</artifactId>

pom.xml

@@ -572,6 +572,27 @@
     </dependencies>
   </dependencyManagement>
 
+  <repositories>
+    <!-- Repeat the super POM's Central so it is queried before the staging repository. -->
+    <repository>
+      <snapshots>
+        <enabled>false</enabled>
+      </snapshots>
+      <id>central</id>
+      <name>Central Repository</name>
+      <url>https://repo.maven.apache.org/maven2</url>
+    </repository>
+    <!-- TODO: remove once Commons Secure XML 1.0.0 is released. -->
+    <repository>
+      <snapshots>
+        <enabled>false</enabled>
+      </snapshots>
+      <id>apache.commons.staging</id>
+      <name>Apache Commons Secure XML 1.0.0 release candidate</name>
+      <url>https://repository.apache.org/content/repositories/orgapachecommons-1962/</url>
+    </repository>
+  </repositories>
+
   <build>
 
     <plugins>

src/changelog/.2.x.x/4162_xml_factory_hardening.xml

@@ -0,0 +1,12 @@
+<?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">
+  <issue id="4162" link="https://github.com/apache/logging-log4j2/pull/4162"/>
+  <description format="asciidoc">
+    Harden XML configuration parsing using the hardened JAXP factories of Apache Commons Secure XML.
+  </description>
+</entry>