Rather than branching on the implementation class, {@link #harden(DocumentBuilderFactory)} probes what the factory supports and adapts:
+ *Required for implementations that do not honour JAXP 1.5 {@code ACCESS_EXTERNAL_*} (the external Xerces distribution): the factory carries no resolver + * of its own, so it has to be set on each builder.
+ */ + private static final class HardeningDocumentBuilderFactory extends DelegatingDocumentBuilderFactory { + + private final EntityResolver resolver; + + HardeningDocumentBuilderFactory(final DocumentBuilderFactory delegate, final EntityResolver resolver) { + super(delegate); + this.resolver = resolver; + } + + @Override + public DocumentBuilder newDocumentBuilder() throws ParserConfigurationException { + final DocumentBuilder builder = super.newDocumentBuilder(); + builder.setEntityResolver(resolver); + return builder; + } + } + + /** Class name of Android's Harmony-based {@link DocumentBuilderFactory}, which exposes no hardening surface. */ + private static final String ANDROID_DOCUMENT_BUILDER_FACTORY = "org.apache.harmony.xml.parsers.DocumentBuilderFactoryImpl"; + + /** Xerces feature: load the external DTD subset for non-validating parsers. */ + private static final String XERCES_LOAD_EXTERNAL_DTD = "http://apache.org/xml/features/nonvalidating/load-external-dtd"; + + static DocumentBuilderFactory harden(final DocumentBuilderFactory factory) { + // Android exposes no FSP, ACCESS_EXTERNAL_* or attribute API, and KXmlParser drops user-defined entities; nothing to apply. + if (ANDROID_DOCUMENT_BUILDER_FACTORY.equals(factory.getClass().getName())) { + return factory; + } + // Required: enables the implementation's security manager, which carries the limits. + setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true); + // Optional, implementation-based: JDK attribute limits or Xerces' SecurityManager. + Limits.tryApply(factory); + // Optional: skip the external DTD subset on non-validating parsers so DOCTYPE-only documents parse without a blocked fetch attempt. + setOptionalFeature(factory, XERCES_LOAD_EXTERNAL_DTD, false); + // ACCESS_EXTERNAL_* support is the dividing capability between JAXP 1.5 implementations and older ones. + if (trySetAttribute(factory, XMLConstants.ACCESS_EXTERNAL_DTD, "") + && trySetAttribute(factory, XMLConstants.ACCESS_EXTERNAL_SCHEMA, "")) { + // Honoured: the JAXP 1.5 properties block external fetches, so the bare factory is already hardened. + return factory; + } + // Rejected: external Xerces ignores ACCESS_EXTERNAL_*; install a deny-all resolver on every DocumentBuilder. + return new HardeningDocumentBuilderFactory(factory, Resolvers.DenyAll.ENTITY2); + } + + private DocumentBuilderHardener() { + } +} diff --git a/src/main/java/org/apache/commons/xml/JaxpSetters.java b/src/main/java/org/apache/commons/xml/JaxpSetters.java index 55c7f623..e4ffa052 100644 --- a/src/main/java/org/apache/commons/xml/JaxpSetters.java +++ b/src/main/java/org/apache/commons/xml/JaxpSetters.java @@ -55,6 +55,20 @@ static void setAttribute(final DocumentBuilderFactory factory, final String attr apply(factory, "attribute", attribute, () -> factory.setAttribute(attribute, value)); } + /** @return {@code true} if the attribute was applied, {@code false} if the implementation rejected it. */ + static boolean trySetAttribute(final DocumentBuilderFactory factory, final String attribute, final Object value) { + try { + factory.setAttribute(attribute, value); + return true; + } catch (final Exception e) { + return false; + } + } + + static void setOptionalAttribute(final DocumentBuilderFactory factory, final String attribute, final Object value) { + trySetAttribute(factory, attribute, value); + } + static void setAttribute(final TransformerFactory factory, final String attribute, final Object value) { apply(factory, "attribute", attribute, () -> factory.setAttribute(attribute, value)); } @@ -63,6 +77,14 @@ static void setFeature(final DocumentBuilderFactory factory, final String featur apply(factory, "feature", feature, () -> factory.setFeature(feature, value)); } + static void setOptionalFeature(final DocumentBuilderFactory factory, final String feature, final boolean value) { + try { + factory.setFeature(feature, value); + } catch (final Exception e) { + // Ignored: the implementation does not recognise this feature. + } + } + static void setFeature(final SAXParserFactory factory, final String feature, final boolean value) { apply(factory, "feature", feature, () -> factory.setFeature(feature, value)); } diff --git a/src/main/java/org/apache/commons/xml/Limits.java b/src/main/java/org/apache/commons/xml/Limits.java index d84f8e05..475b9218 100644 --- a/src/main/java/org/apache/commons/xml/Limits.java +++ b/src/main/java/org/apache/commons/xml/Limits.java @@ -17,6 +17,7 @@ package org.apache.commons.xml; import static org.apache.commons.xml.JaxpSetters.setAttribute; +import static org.apache.commons.xml.JaxpSetters.setOptionalAttribute; import static org.apache.commons.xml.JaxpSetters.setProperty; import java.util.Collections; @@ -216,6 +217,10 @@ final class Limits { * Woodstox property: maximum number of entity expansions in a single parse. */ private static final String WSTX_MAX_ENTITY_COUNT = "com.ctc.wstx.maxEntityCount"; + /** + * Class name of the external Apache Xerces {@link DocumentBuilderFactory}, whose limits live on a {@code SecurityManager} rather than JDK attributes. + */ + private static final String EXTERNAL_XERCES_DOCUMENT_BUILDER_FACTORY = "org.apache.xerces.jaxp.DocumentBuilderFactoryImpl"; static { final MapExternal Xerces carries its limits on an {@code org.apache.xerces.util.SecurityManager} instance. Every other implementation (the stock JDK and any + * future attribute-based parser) takes the JDK limit attributes. Neither path throws if the implementation declines a limit.
* * @param factory The target factory to modify. */ - static void applyToJdkDom(final DocumentBuilderFactory factory) { - JDK_LIMITS.forEach((name, supplier) -> setAttribute(factory, name, Integer.toString(supplier.getAsInt()))); + static void tryApply(final DocumentBuilderFactory factory) { + if (EXTERNAL_XERCES_DOCUMENT_BUILDER_FACTORY.equals(factory.getClass().getName())) { + // Install a fresh SecurityManager pinned to JDK 25 limits, replacing Xerces' built-in caps which are looser than even JDK 8. + final Object securityManager = newSecurityManager(); + applyToXerces(securityManager); + setAttribute(factory, XercesProvider.XERCES_SECURITY_MANAGER_PROPERTY, securityManager); + return; + } + // Pin the JDK attribute limits to JDK 25 secure values; skip silently any attribute the implementation does not recognise. + JDK_LIMITS.forEach((name, supplier) -> setOptionalAttribute(factory, name, Integer.toString(supplier.getAsInt()))); } /** @@ -304,6 +320,14 @@ static void applyToXerces(final Object securityManager) { } } + private static Object newSecurityManager() { + try { + return Class.forName("org.apache.xerces.util.SecurityManager").getDeclaredConstructor().newInstance(); + } catch (final ReflectiveOperationException e) { + throw new HardeningException("Failed to instantiate org.apache.xerces.util.SecurityManager; expected Xerces to be on the classpath", e); + } + } + private static int getElementAttributeLimit() { return read(SP_ELEMENT_ATTRIBUTE_LIMIT, DEFAULT_ELEMENT_ATTRIBUTE_LIMIT); } diff --git a/src/main/java/org/apache/commons/xml/StockJdkProvider.java b/src/main/java/org/apache/commons/xml/StockJdkProvider.java index fa315574..99a7d8d2 100644 --- a/src/main/java/org/apache/commons/xml/StockJdkProvider.java +++ b/src/main/java/org/apache/commons/xml/StockJdkProvider.java @@ -22,7 +22,6 @@ import static org.apache.commons.xml.JaxpSetters.setProperty; import javax.xml.XMLConstants; -import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.SAXParserFactory; import javax.xml.stream.XMLInputFactory; import javax.xml.transform.TransformerFactory; @@ -72,19 +71,6 @@ final class StockJdkProvider { */ private static final String ZEPHYR_IGNORE_EXTERNAL_DTD = "http://java.sun.com/xml/stream/properties/ignore-external-dtd"; - static DocumentBuilderFactory configure(final DocumentBuilderFactory factory) { - // Required: enables the JDK XMLSecurityManager limits. - setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true); - // Let DOCTYPE-only documents parse silently without SSRF: skip the external DTD subset on non-validating parsers. - setFeature(factory, XERCES_LOAD_EXTERNAL_DTD, false); - // Defense-in-depth: pin to JDK 25 limits so older JDKs do not fall back to looser secure values. - Limits.applyToJdkDom(factory); - // Defense-in-depth: already FSP-secure defaults, set explicitly so they are not relaxed via system property. - setAttribute(factory, XMLConstants.ACCESS_EXTERNAL_DTD, ""); - setAttribute(factory, XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); - return factory; - } - static SAXParserFactory configure(final SAXParserFactory factory) { // Required: enables the JDK XMLSecurityManager limits. setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true); diff --git a/src/main/java/org/apache/commons/xml/XercesProvider.java b/src/main/java/org/apache/commons/xml/XercesProvider.java index 7b89be1a..4634c9f9 100644 --- a/src/main/java/org/apache/commons/xml/XercesProvider.java +++ b/src/main/java/org/apache/commons/xml/XercesProvider.java @@ -20,9 +20,6 @@ import static org.apache.commons.xml.JaxpSetters.setFeature; import javax.xml.XMLConstants; -import javax.xml.parsers.DocumentBuilder; -import javax.xml.parsers.DocumentBuilderFactory; -import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; import javax.xml.validation.Schema; @@ -30,7 +27,6 @@ import javax.xml.validation.Validator; import javax.xml.validation.ValidatorHandler; -import org.xml.sax.EntityResolver; import org.xml.sax.SAXNotRecognizedException; import org.xml.sax.SAXNotSupportedException; import org.xml.sax.XMLReader; @@ -39,7 +35,7 @@ * Hardening recipes for the external Apache Xerces distribution (the {@code xerces:xercesImpl} artifact). * *Factory classes live in the {@code org.apache.xerces.*} package. External Xerces does not ship a {@code TransformerFactory}, {@code XMLInputFactory} or - * {@code XPathFactory}, so this class only handles DOM, SAX and Schema factories.
+ * {@code XPathFactory}, so this class only handles SAX and Schema factories. DOM hardening lives in {@link DocumentBuilderHardener}. * *Hardening recipe applied to every factory below uses the same building blocks:
*Sets the deny-all {@link EntityResolver} on every {@link DocumentBuilder} produced; required because {@link DocumentBuilderFactory} carries no - * resolver of its own and Xerces does not honour JAXP 1.5 {@code ACCESS_EXTERNAL_*}.
- */ - private static final class HardeningDocumentBuilderFactory extends DelegatingDocumentBuilderFactory { - - private final EntityResolver resolver; - - HardeningDocumentBuilderFactory(final DocumentBuilderFactory delegate, final EntityResolver resolver) { - super(delegate); - this.resolver = resolver; - } - - @Override - public DocumentBuilder newDocumentBuilder() throws ParserConfigurationException { - final DocumentBuilder builder = super.newDocumentBuilder(); - builder.setEntityResolver(resolver); - return builder; - } - } - private static Validator hardenValidator(final Validator validator) { try { Limits.applyToXerces(validator.getProperty(XERCES_SECURITY_MANAGER_PROPERTY)); @@ -114,27 +86,6 @@ private static ValidatorHandler hardenValidatorHandler(final ValidatorHandler ha /** Xerces feature: load the external DTD subset for non-validating parsers. */ private static final String XERCES_LOAD_EXTERNAL_DTD = "http://apache.org/xml/features/nonvalidating/load-external-dtd"; - private static Object newSecurityManager() { - try { - return Class.forName("org.apache.xerces.util.SecurityManager").getDeclaredConstructor().newInstance(); - } catch (final ReflectiveOperationException e) { - throw new HardeningException("Failed to instantiate org.apache.xerces.util.SecurityManager; expected Xerces to be on the classpath", e); - } - } - - static DocumentBuilderFactory configure(final DocumentBuilderFactory factory) { - // Required: enables Xerces' built-in SecurityManager (which is what carries the limits). - setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true); - // Let DOCTYPE-only documents parse silently without SSRF: skip the external DTD subset on non-validating parsers. - setFeature(factory, XERCES_LOAD_EXTERNAL_DTD, false); - // Defense-in-depth: install a fresh SecurityManager pinned to JDK 25 limits, replacing Xerces' built-in caps which are looser than even JDK 8. - final Object securityManager = newSecurityManager(); - Limits.applyToXerces(securityManager); - factory.setAttribute(XERCES_SECURITY_MANAGER_PROPERTY, securityManager); - // Required: Xerces does not honour JAXP 1.5 ACCESS_EXTERNAL_*; the wrapper installs a deny-all resolver on every DocumentBuilder. - return new HardeningDocumentBuilderFactory(factory, Resolvers.DenyAll.ENTITY2); - } - static SAXParserFactory configure(final SAXParserFactory factory) { // Required: enables Xerces' built-in SecurityManager (which is what carries the limits). setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true); diff --git a/src/main/java/org/apache/commons/xml/XmlFactories.java b/src/main/java/org/apache/commons/xml/XmlFactories.java index b97c59f9..62282567 100644 --- a/src/main/java/org/apache/commons/xml/XmlFactories.java +++ b/src/main/java/org/apache/commons/xml/XmlFactories.java @@ -73,19 +73,6 @@ */ public final class XmlFactories { - static DocumentBuilderFactory dispatch(final DocumentBuilderFactory factory) { - switch (factory.getClass().getName()) { - case "com.sun.org.apache.xerces.internal.jaxp.DocumentBuilderFactoryImpl": - return StockJdkProvider.configure(factory); - case "org.apache.harmony.xml.parsers.DocumentBuilderFactoryImpl": - return AndroidProvider.configure(factory); - case "org.apache.xerces.jaxp.DocumentBuilderFactoryImpl": - return XercesProvider.configure(factory); - default: - throw noProvider(factory); - } - } - private static SAXParserFactory dispatch(final SAXParserFactory factory) { switch (factory.getClass().getName()) { case "com.sun.org.apache.xerces.internal.jaxp.SAXParserFactoryImpl": @@ -197,16 +184,15 @@ public static XMLReader harden(final XMLReader reader) { /** * Returns a fresh, hardened {@link DocumentBuilderFactory}. * - *Beyond the three universal guarantees on {@link XmlFactories}, XInclude resolution is disabled. Calling - * {@link DocumentBuilderFactory#setXIncludeAware(boolean) setXIncludeAware(true)} on the returned factory does not re-enable resolution; a parse that - * encounters an {@code xi:include} element fails.
+ *Enabling XInclude: {@link DocumentBuilderFactory#setXIncludeAware(boolean) setXIncludeAware(true)} on its own does not make XInclude + * usable, because an included resource is fetched like any other external resource and is therefore blocked, failing the parse. A caller that genuinely + * wants XInclude must, in addition to enabling awareness, install a custom {@link org.xml.sax.EntityResolver} that permits those specific lookups.
* * @return a hardened factory. - * @throws IllegalStateException if the underlying JAXP implementation is not recognised by any bundled hardening recipe, or if the matching recipe cannot - * apply its settings to it. + * @throws IllegalStateException if a required hardening setting cannot be applied to the underlying implementation. */ public static DocumentBuilderFactory newDocumentBuilderFactory() { - return dispatch(DocumentBuilderFactory.newInstance()); + return DocumentBuilderHardener.harden(DocumentBuilderFactory.newInstance()); } /** diff --git a/src/test/java/org/apache/commons/xml/SchemaLocationDomTest.java b/src/test/java/org/apache/commons/xml/SchemaLocationDomTest.java new file mode 100644 index 00000000..8b873e33 --- /dev/null +++ b/src/test/java/org/apache/commons/xml/SchemaLocationDomTest.java @@ -0,0 +1,112 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.xml; + +import static org.apache.commons.xml.AttackTestSupport.LEAKED_MARKER; +import static org.apache.commons.xml.AttackTestSupport.resourceUrl; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +import javax.xml.XMLConstants; +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; + +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.w3c.dom.Document; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; +import org.xml.sax.SAXParseException; +import org.xml.sax.helpers.DefaultHandler; + +/** + * Checks that a hardened {@link DocumentBuilderFactory} performing JAXP 1.2 XSD validation does not fetch an external schema named by an + * {@code xsi:noNamespaceSchemaLocation} hint in the instance document. + * + *The instance is empty {@code
The test runs only where the implementation supports JAXP 1.2 schema-language XSD validation (the stock JDK and external Xerces do; Android does not), so + * it skips on parsers without it.
+ */ +@Tag("dom") +class SchemaLocationDomTest { + + /** JAXP 1.2 property selecting the schema language used by {@link DocumentBuilderFactory#setValidating(boolean)}. */ + private static final String SCHEMA_LANGUAGE = "http://java.sun.com/xml/jaxp/properties/schemaLanguage"; + + private static final String INSTANCE = "schema-location-instance.xml"; + + /** Name of the external schema the instance points at; both block mechanisms name it in the failure message. */ + private static final String SCHEMA = "schema-location.xsd"; + + @Test + void hardenedBlocksExternalSchemaFetch() { + assumeTrue(supportsSchemaLanguage(), "parser does not support JAXP 1.2 schema-language XSD validation"); + final DocumentBuilderFactory factory = enableXsdValidation(XmlFactories.newDocumentBuilderFactory()); + // The schemaLocation fetch is denied and surfaced as a fatal error rather than a silent fetch. The attribution is implementation-specific, so assert + // only that the failure names the external schema, not the mechanism (accessExternalSchema on the JDK, the deny-all resolver on external Xerces). + final SAXException thrown = assertThrows(SAXException.class, () -> parse(factory)); + assertTrue(thrown.getMessage() != null && thrown.getMessage().contains(SCHEMA), + "Block must reference the external schema, got: " + thrown.getMessage()); + } + + @Test + void unconfiguredFetchesExternalSchema() throws Exception { + assumeTrue(supportsSchemaLanguage(), "parser does not support JAXP 1.2 schema-language XSD validation"); + final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, false); + // Positive control: without hardening the external schema is fetched and its default attribute is inlined into the DOM. + final Document document = parse(enableXsdValidation(factory)); + assertEquals(LEAKED_MARKER, document.getDocumentElement().getAttribute("leak"), + "Permissive parse should have fetched the external schema and inlined its default attribute."); + } + + private static DocumentBuilderFactory enableXsdValidation(final DocumentBuilderFactory factory) { + factory.setNamespaceAware(true); + factory.setValidating(true); + factory.setAttribute(SCHEMA_LANGUAGE, XMLConstants.W3C_XML_SCHEMA_NS_URI); + return factory; + } + + private static Document parse(final DocumentBuilderFactory factory) throws Exception { + final DocumentBuilder builder = factory.newDocumentBuilder(); + builder.setErrorHandler(new DefaultHandler() { + @Override + public void error(final SAXParseException exception) throws SAXException { + throw exception; + } + }); + return builder.parse(new InputSource(resourceUrl(INSTANCE).toString())); + } + + private static boolean supportsSchemaLanguage() { + try { + final DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setValidating(true); + factory.setAttribute(SCHEMA_LANGUAGE, XMLConstants.W3C_XML_SCHEMA_NS_URI); + return true; + } catch (final Exception e) { + return false; + } + } +} diff --git a/src/test/java/org/apache/commons/xml/UnsupportedXmlImplementationTest.java b/src/test/java/org/apache/commons/xml/UnsupportedXmlImplementationTest.java index c0f304a4..863fdf4d 100644 --- a/src/test/java/org/apache/commons/xml/UnsupportedXmlImplementationTest.java +++ b/src/test/java/org/apache/commons/xml/UnsupportedXmlImplementationTest.java @@ -23,16 +23,18 @@ import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; import org.junit.jupiter.api.Test; /** - * Verifies that an unknown factory class surfaces {@link IllegalStateException} with a message naming the class. + * Verifies that an implementation which does not honour the required secure-processing feature surfaces {@link IllegalStateException} with a message naming the + * class. */ class UnsupportedXmlImplementationTest { /** - * A stand-in factory class whose fully qualified name is not matched by any bundled hardening recipe. + * A stand-in factory that rejects the secure-processing feature, like a JAXP implementation that does not recognise it. */ public static final class FakeDocumentBuilderFactory extends DocumentBuilderFactory { @@ -52,8 +54,8 @@ public Object getAttribute(final String name) { } @Override - public void setFeature(final String name, final boolean value) { - // no-op + public void setFeature(final String name, final boolean value) throws ParserConfigurationException { + throw new ParserConfigurationException("feature not recognised: " + name); } @Override @@ -63,10 +65,10 @@ public boolean getFeature(final String name) { } @Test - void dispatchRejectsUnknownFactory() { + void hardenRejectsUnsecurableFactory() { final IllegalStateException thrown = assertThrows( IllegalStateException.class, - () -> XmlFactories.dispatch(new FakeDocumentBuilderFactory())); + () -> DocumentBuilderHardener.harden(new FakeDocumentBuilderFactory())); assertNotNull(thrown.getMessage()); assertTrue(thrown.getMessage().contains(FakeDocumentBuilderFactory.class.getName()), "Exception message must name the unsupported class: " + thrown.getMessage()); diff --git a/src/test/resources/leaked/schema-location-instance.xml b/src/test/resources/leaked/schema-location-instance.xml new file mode 100644 index 00000000..3caf442d --- /dev/null +++ b/src/test/resources/leaked/schema-location-instance.xml @@ -0,0 +1,4 @@ + + +