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 { diff --git a/log4j-core/pom.xml b/log4j-core/pom.xml index b2f34d4ab95..9cf69c39f21 100644 --- a/log4j-core/pom.xml +++ b/log4j-core/pom.xml @@ -157,6 +157,11 @@ commons-csv true + + + org.apache.commons + commons-secure-xml + 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..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 @@ -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() { * *

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 { @@ -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}). - * - *

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( @@ -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) 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; 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..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,6 +75,10 @@ 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 */ @@ -82,19 +86,20 @@ private Bundle getApiTestsBundle() throws BundleException { 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( 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..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,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 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..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,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 diff --git a/log4j-parent/pom.xml b/log4j-parent/pom.xml index 28c371abefa..e5f8bd6594d 100644 --- a/log4j-parent/pom.xml +++ b/log4j-parent/pom.xml @@ -76,6 +76,7 @@ 2.22.0 3.20.0 1.4.0 + 1.0.0 1.2.15 3.4.4 @@ -392,6 +393,12 @@ ${commons-pool2.version} + + org.apache.commons + commons-secure-xml + ${commons-secure-xml.version} + + com.conversantmedia disruptor 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/ + + + diff --git a/src/changelog/.2.x.x/4162_xml_factory_hardening.xml b/src/changelog/.2.x.x/4162_xml_factory_hardening.xml new file mode 100644 index 00000000000..9487b670a94 --- /dev/null +++ b/src/changelog/.2.x.x/4162_xml_factory_hardening.xml @@ -0,0 +1,12 @@ + + + + + Harden XML configuration parsing using the hardened JAXP factories of Apache Commons Secure XML. + +