This is an automated email from the ASF dual-hosted git repository. ppkarwasz pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/commons-xml.git
commit 9591f9f069418c7b51d9725c2434b726977a3cdd Author: Piotr P. Karwasz <[email protected]> AuthorDate: Thu Apr 30 22:14:52 2026 +0200 Align StAX hardening with the SAX/DOM external-subset contract (#11) SAX and DOM skip the external DTD subset on non-validating parsers, so a DOCTYPE that names an external subset parses without throwing and the external fetch is ignored. The StAX path was stricter because the deny-all XMLResolver fired on the DOCTYPE itself, rejecting documents the SAX/DOM path would parse cleanly. This change brings StAX to the same contract through Zephyr's and Woodstox's native subset-skip hooks. Declared external entities still throw on every path. Assisted-By: Claude Opus 4.7 (1M context) <[email protected]> --- .../commons/xml/factory/DenyAllResolver.java | 131 ------------------- .../org/apache/commons/xml/factory/Resolvers.java | 142 +++++++++++++++++++++ .../commons/xml/factory/StockJdkProvider.java | 15 ++- .../commons/xml/factory/WoodstoxProvider.java | 49 ++++++- .../apache/commons/xml/factory/XalanProvider.java | 4 +- .../apache/commons/xml/factory/XercesProvider.java | 12 +- .../commons/xml/factory/AttackTestSupport.java | 109 +++++++++++++--- .../commons/xml/factory/DoctypeOnlyTest.java | 49 ++++++- .../commons/xml/factory/ExternalDtdTest.java | 4 +- .../apache/commons/xml/factory/NoDoctypeTest.java | 92 +++++++++++++ 10 files changed, 437 insertions(+), 170 deletions(-) diff --git a/src/main/java/org/apache/commons/xml/factory/DenyAllResolver.java b/src/main/java/org/apache/commons/xml/factory/DenyAllResolver.java deleted file mode 100644 index 46c0cf1..0000000 --- a/src/main/java/org/apache/commons/xml/factory/DenyAllResolver.java +++ /dev/null @@ -1,131 +0,0 @@ -/* - * 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.factory; - -import javax.xml.stream.XMLResolver; -import javax.xml.stream.XMLStreamException; -import javax.xml.transform.Source; -import javax.xml.transform.TransformerException; -import javax.xml.transform.URIResolver; - -import org.w3c.dom.ls.LSInput; -import org.w3c.dom.ls.LSResourceResolver; -import org.xml.sax.InputSource; -import org.xml.sax.SAXException; -import org.xml.sax.ext.EntityResolver2; - -/** - * Stateless resolvers that refuse every external lookup. - * - * <p>Four typed singleton fields are exposed, one per resolver interface; pick the one matching the API you are configuring:</p> - * <ul> - * <li>{@link #LS_RESOURCE}: {@link LSResourceResolver} for {@code SchemaFactory.setResourceResolver}.</li> - * <li>{@link #URI}: {@link URIResolver} for {@code TransformerFactory.setURIResolver} (and the same on each {@code Transformer} it produces).</li> - * <li>{@link #XML}: {@link XMLResolver} for {@code XMLInputFactory.setXMLResolver}.</li> - * <li>{@link #ENTITY2}: {@link EntityResolver2} for {@code DocumentBuilder.setEntityResolver} and {@code XMLReader.setEntityResolver}; SAX/DOM parsers - * that recognise {@link EntityResolver2} pick up the extended {@code getExternalSubset} and 4-arg {@code resolveEntity} hooks automatically.</li> - * </ul> - * - * <p>{@link XMLResolver} and {@link EntityResolver2} both declare a 4-arg {@code resolveEntity(String, String, String, String)} with identical erasure but - * different parameter semantics, return types ({@link Object} vs {@link InputSource}) and throws clauses ({@link XMLStreamException} vs {@link SAXException}), - * so they cannot coexist on the same class. That is why {@link #XML} and {@link #ENTITY2} live in separate nested types.</p> - */ -class DenyAllResolver implements LSResourceResolver, URIResolver { - - /** - * {@link EntityResolver2} flavour: refuses every external entity referenced by a SAX or DOM parser, including the external DTD subset and the 4-arg - * {@code resolveEntity} that lets a parser pass through the entity name and base URI. - */ - private static final class DenyAllEntityResolver2 extends DenyAllResolver implements EntityResolver2 { - - private DenyAllEntityResolver2() { - } - - @Override - public InputSource getExternalSubset(final String name, final String baseURI) throws SAXException { - throw new SAXException(forbiddenMessage("external-subset", null, null, name, baseURI)); - } - - @Override - public InputSource resolveEntity(final String publicId, final String systemId) throws SAXException { - throw new SAXException(forbiddenMessage(null, null, publicId, systemId, null)); - } - - @Override - public InputSource resolveEntity(final String name, final String publicId, final String baseURI, final String systemId) throws SAXException { - throw new SAXException(forbiddenMessage(name, null, publicId, systemId, baseURI)); - } - } - - /** - * {@link XMLResolver} flavour: refuses every external entity referenced by a StAX parser. - */ - private static final class DenyAllXMLResolver extends DenyAllResolver implements XMLResolver { - - private DenyAllXMLResolver() { - } - - @Override - public Object resolveEntity(final String publicID, final String systemID, final String baseURI, final String namespace) throws XMLStreamException { - throw new XMLStreamException(forbiddenMessage(null, namespace, publicID, systemID, baseURI)); - } - } - - /** - * {@link EntityResolver2} singleton; refuses every external entity lookup performed by a SAX or DOM parser. - */ - static final EntityResolver2 ENTITY2; - - /** - * {@link LSResourceResolver} singleton; refuses every {@code xs:import}/{@code xs:include}/{@code xs:redefine} lookup at schema-compile time. - */ - static final LSResourceResolver LS_RESOURCE; - /** - * {@link URIResolver} singleton; refuses every {@code xsl:import}/{@code xsl:include}/{@code document()} lookup during XSLT compile and transform. - */ - static final URIResolver URI; - /** - * {@link XMLResolver} singleton; refuses every external entity lookup performed by a StAX parser. - */ - static final XMLResolver XML; - - static { - DenyAllEntityResolver2 resolver2 = new DenyAllEntityResolver2(); - ENTITY2 = resolver2; - LS_RESOURCE = resolver2; - URI = resolver2; - XML = new DenyAllXMLResolver(); - } - - private static String forbiddenMessage(final String type, final String namespace, final String publicId, final String systemId, final String baseURI) { - return String.format("External resource fetch forbidden by hardening: type=%s, namespace=%s, publicId=%s, systemId=%s, baseURI=%s", type, namespace, - publicId, systemId, baseURI); - } - - private DenyAllResolver() { - } - - @Override - public Source resolve(final String href, final String base) throws TransformerException { - throw new TransformerException(forbiddenMessage("uri", null, null, href, base)); - } - - @Override - public LSInput resolveResource(final String type, final String namespaceURI, final String publicId, final String systemId, final String baseURI) { - throw new SecurityException(forbiddenMessage(type, namespaceURI, publicId, systemId, baseURI)); - } -} diff --git a/src/main/java/org/apache/commons/xml/factory/Resolvers.java b/src/main/java/org/apache/commons/xml/factory/Resolvers.java new file mode 100644 index 0000000..386517d --- /dev/null +++ b/src/main/java/org/apache/commons/xml/factory/Resolvers.java @@ -0,0 +1,142 @@ +/* + * 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.factory; + +import java.io.ByteArrayInputStream; +import java.io.InputStream; + +import javax.xml.stream.XMLResolver; +import javax.xml.stream.XMLStreamException; +import javax.xml.transform.TransformerException; +import javax.xml.transform.URIResolver; + +import org.w3c.dom.ls.LSResourceResolver; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; +import org.xml.sax.ext.EntityResolver2; + +/** + * Stateless resolver singletons that fix the outcome of every external lookup. + * + * <p>Two flavours are exposed, each as a typed singleton field per resolver interface:</p> + * <ul> + * <li>{@link DenyAll} refuses every lookup with an exception. Use this on schema/XSLT compile paths and on parser entity hooks where any external fetch is + * a hardening violation.</li> + * <li>{@link IgnoreAll} returns an empty input. Use this on Woodstox's DTD-subset and undeclared-entity hooks where the parse must continue with no + * replacement content.</li> + * </ul> + * + * <p>{@link XMLResolver} and {@link EntityResolver2} both declare a 4-arg {@code resolveEntity(String, String, String, String)} with identical erasure but + * different parameter semantics, return types ({@link Object} vs {@link InputSource}) and throws clauses ({@link XMLStreamException} vs {@link SAXException}), + * so they cannot coexist on the same class. Each flavour therefore exposes its {@code XMLResolver} and {@code EntityResolver2} singletons separately.</p> + */ +final class Resolvers { + + /** + * Refuses every external resource lookup with an exception. + * + * <p>The single-method resolvers ({@link LSResourceResolver}, {@link URIResolver}, {@link XMLResolver}) are exposed as lambdas; {@link EntityResolver2} + * declares three methods, so it lives in a private nested class.</p> + */ + static final class DenyAll { + + /** + * {@link EntityResolver2}: refuses every external entity lookup performed by a SAX or DOM parser. + */ + private static final class DenyAllEntityResolver2 implements EntityResolver2 { + + private DenyAllEntityResolver2() { + } + + @Override + public InputSource getExternalSubset(final String name, final String baseURI) { + // Canonical EntityResolver2 "no synthetic subset" signal; matches the behaviour of an absent resolver. Blocking happens in resolveEntity below. + return null; + } + + @Override + public InputSource resolveEntity(final String publicId, final String systemId) throws SAXException { + throw new SAXException(forbiddenMessage(null, null, publicId, systemId, null)); + } + + @Override + public InputSource resolveEntity(final String name, final String publicId, final String baseURI, final String systemId) throws SAXException { + throw new SAXException(forbiddenMessage(name, null, publicId, systemId, baseURI)); + } + } + + /** + * Refuses every external entity lookup performed by a SAX or DOM parser, including the external DTD subset. + */ + static final EntityResolver2 ENTITY2 = new DenyAllEntityResolver2(); + + /** + * Refuses every {@code xs:import}/{@code xs:include}/{@code xs:redefine} lookup at schema-compile time. + */ + static final LSResourceResolver LS_RESOURCE = (type, namespaceURI, publicId, systemId, baseURI) -> { + throw new SecurityException(forbiddenMessage(type, namespaceURI, publicId, systemId, baseURI)); + }; + + /** + * Refuses every {@code xsl:import}/{@code xsl:include}/{@code document()} lookup during XSLT compile and transform. + */ + static final URIResolver URI = (href, base) -> { + throw new TransformerException(forbiddenMessage("uri", null, null, href, base)); + }; + + /** + * Refuses every external entity lookup performed by a StAX parser. + */ + static final XMLResolver XML = (publicID, systemID, baseURI, namespace) -> { + throw new XMLStreamException(forbiddenMessage(null, namespace, publicID, systemID, baseURI)); + }; + + private DenyAll() { + } + } + + /** + * Returns an empty input for every external resource lookup so the parse can continue without replacement content. + * + * <p>Only an {@link XMLResolver} flavour is exposed: schema and XSLT compile paths must always deny imports, and SAX/DOM use the deny-all hooks plus + * {@link AndroidProvider}'s subset-aware resolver where needed.</p> + */ + static final class IgnoreAll { + + /** + * Empty {@link ByteArrayInputStream} shared across every call. {@code read()} on a zero-length array always returns {@code -1}, so reusing the + * instance is safe even though the type is technically stateful. + */ + private static final InputStream EMPTY = new ByteArrayInputStream(new byte[0]); + + /** + * Returns an empty input for every external entity lookup performed by a StAX parser. + */ + static final XMLResolver XML = (publicID, systemID, baseURI, namespace) -> EMPTY; + + private IgnoreAll() { + } + } + + private static String forbiddenMessage(final String type, final String namespace, final String publicId, final String systemId, final String baseURI) { + return String.format("External resource fetch forbidden by hardening: type=%s, namespace=%s, publicId=%s, systemId=%s, baseURI=%s", type, namespace, + publicId, systemId, baseURI); + } + + private Resolvers() { + } +} diff --git a/src/main/java/org/apache/commons/xml/factory/StockJdkProvider.java b/src/main/java/org/apache/commons/xml/factory/StockJdkProvider.java index e56c46c..54863a8 100644 --- a/src/main/java/org/apache/commons/xml/factory/StockJdkProvider.java +++ b/src/main/java/org/apache/commons/xml/factory/StockJdkProvider.java @@ -45,8 +45,10 @@ * <li><strong>{@code Limits.applyToJdk*}</strong>: required on {@link XMLInputFactory} (it rejects FSP); elsewhere defense-in-depth, pinning the limits to * JDK 25 secure values so older JDKs do not fall back to looser defaults.</li> * <li><strong>{@code ACCESS_EXTERNAL_*}</strong>: already the FSP-secure default but set to {@code ""} explicitly so a sysprop ({@code - * javax.xml.accessExternal*}) cannot loosen them. {@link XMLInputFactory} has no equivalent property, so the StAX path uses {@link DenyAllResolver} - * instead.</li> + * javax.xml.accessExternal*}) cannot loosen them. {@link XMLInputFactory} has no equivalent property, so the StAX path pairs Zephyr's + * {@value #ZEPHYR_IGNORE_EXTERNAL_DTD} property (skip the external DTD subset, lets DOCTYPE-only documents parse) with {@link Resolvers.DenyAll#XML} + * (throw on declared external entity references). Undeclared general-entity references are silently dropped: Zephyr does not raise a fatal error + * when the subset that would have declared the entity was skipped, so no extra hook is needed.</li> * </ul> * * <p>SAX hardening lives in {@link #configure(XMLReader)}: {@link SAXParserFactory} has no property API, so the {@link HardeningSAXParserFactory} wrapper @@ -64,6 +66,11 @@ final class StockJdkProvider { */ private static final String XERCES_LOAD_EXTERNAL_DTD = "http://apache.org/xml/features/nonvalidating/load-external-dtd";; + /** + * Zephyr property: skip external DTD subset loading entirely (StAX equivalent of {@link #XERCES_LOAD_EXTERNAL_DTD} {@code = false}). + */ + 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); @@ -102,8 +109,10 @@ static XMLReader configure(final XMLReader reader) { static XMLInputFactory configure(final XMLInputFactory factory) { // Required: XMLInputFactory rejects FSP, so the limits below are the only way to enable JDK XMLSecurityManager caps on the StAX path. Limits.applyToJdkStax(factory); + // Let DOCTYPE-only documents parse silently: Zephyr's StAX equivalent of XERCES_LOAD_EXTERNAL_DTD=false skips the external DTD subset entirely. + factory.setProperty(ZEPHYR_IGNORE_EXTERNAL_DTD, true); // Required: XMLInputFactory has no ACCESS_EXTERNAL_* either; an explicit deny-all resolver is the only way to block external entity fetching. - factory.setXMLResolver(DenyAllResolver.XML); + factory.setXMLResolver(Resolvers.DenyAll.XML); return factory; } diff --git a/src/main/java/org/apache/commons/xml/factory/WoodstoxProvider.java b/src/main/java/org/apache/commons/xml/factory/WoodstoxProvider.java index 7e8f843..c240616 100644 --- a/src/main/java/org/apache/commons/xml/factory/WoodstoxProvider.java +++ b/src/main/java/org/apache/commons/xml/factory/WoodstoxProvider.java @@ -17,6 +17,8 @@ package org.apache.commons.xml.factory; import javax.xml.stream.XMLInputFactory; +import javax.xml.stream.XMLResolver; +import javax.xml.stream.XMLStreamException; /** * Hardening recipe for the FasterXML Woodstox StAX implementation ({@code com.ctc.wstx:woodstox-core}). @@ -28,18 +30,55 @@ * <li><strong>{@link Limits#applyToWoodstox}</strong>: defense-in-depth. Woodstox already enforces its own caps (see {@code ReaderConfig.DEFAULT_*}), but * they are looser than the JDK 25 secure values; this call aligns them so a Woodstox-backed factory enforces the same processing limits as the JDK * parsers.</li> - * <li><strong>{@link DenyAllResolver#XML}</strong>: required. Woodstox honours {@code IS_SUPPORTING_EXTERNAL_ENTITIES=true} and {@code SUPPORT_DTD=true} by - * default and resolves external entities/DTDs through whatever {@code XMLResolver} the factory carries; without an explicit deny-all resolver, those - * lookups would proceed.</li> + * <li><strong>Three resolver hooks</strong>: required. Woodstox honours {@code IS_SUPPORTING_EXTERNAL_ENTITIES=true} and {@code SUPPORT_DTD=true} by + * default and routes lookups through three Woodstox-specific resolver properties; the hardened factory installs: + * <ul> + * <li>{@code com.ctc.wstx.dtdResolver} = {@link #DTD_SUBSET_ONLY}: returns an empty input for the external DTD subset so DOCTYPE-only + * documents parse silently, but throws on external parameter entities (which share this hook). The {@code entityName} 4th argument is + * the discriminator: {@code null} for the subset, the entity name for parameter entities (see {@code DefaultInputResolver} and + * {@code ValidatingStreamReader.findDtdExtSubset}).</li> + * <li>{@code com.ctc.wstx.entityResolver} = {@link Resolvers.DenyAll#XML}: throws on declared external general entities.</li> + * <li>{@code com.ctc.wstx.undeclaredEntityResolver} = {@link Resolvers.IgnoreAll#XML}: silently drops references to entities the parser has not + * seen declared, matching the SAX path's behaviour.</li> + * </ul> + * </li> * </ul> */ final class WoodstoxProvider { + /** Woodstox property: resolver consulted for the external DTD subset and for external parameter entities. */ + private static final String WSTX_DTD_RESOLVER = "com.ctc.wstx.dtdResolver"; + + /** Woodstox property: resolver consulted for declared external general entities. */ + private static final String WSTX_ENTITY_RESOLVER = "com.ctc.wstx.entityResolver"; + + /** Woodstox property: resolver consulted for undeclared entity references. */ + private static final String WSTX_UNDECLARED_ENTITY_RESOLVER = "com.ctc.wstx.undeclaredEntityResolver"; + + /** + * Hybrid Woodstox DTD resolver: returns the empty input for the external DTD subset, throws on external parameter entities. + * + * <p>Woodstox calls this hook with {@code entityName == null} for the subset and {@code entityName != null} for parameter-entity expansion; that + * discriminator is Woodstox-specific (the JDK Zephyr's {@code XMLResolver} always receives {@code null} as the 4th argument), so the resolver lives + * here rather than in {@link Resolvers}.</p> + */ + static final XMLResolver DTD_SUBSET_ONLY = (publicID, systemID, baseURI, entityName) -> { + if (entityName != null) { + throw new XMLStreamException("External parameter entity '" + entityName + "' refused (publicID=" + publicID + ", systemID=" + systemID + + ", baseURI=" + baseURI + ")"); + } + return Resolvers.IgnoreAll.XML.resolveEntity(publicID, systemID, baseURI, entityName); + }; + static XMLInputFactory configure(final XMLInputFactory factory) { // Defense-in-depth: align Woodstox's built-in caps with the JDK 25 secure values; Woodstox's own defaults are functional but looser. Limits.applyToWoodstox(factory); - // Required: Woodstox resolves external entities and DTDs by default; an explicit deny-all resolver is the only way to block the lookups. - factory.setXMLResolver(DenyAllResolver.XML); + // Required: empty external subset, throw on external parameter entities. + factory.setProperty(WSTX_DTD_RESOLVER, DTD_SUBSET_ONLY); + // Required: throw on declared external general entities. + factory.setProperty(WSTX_ENTITY_RESOLVER, Resolvers.DenyAll.XML); + // Required: silently drop undeclared entity references, matching the SAX path's tolerance. + factory.setProperty(WSTX_UNDECLARED_ENTITY_RESOLVER, Resolvers.IgnoreAll.XML); return factory; } diff --git a/src/main/java/org/apache/commons/xml/factory/XalanProvider.java b/src/main/java/org/apache/commons/xml/factory/XalanProvider.java index 0e16034..e7384ba 100644 --- a/src/main/java/org/apache/commons/xml/factory/XalanProvider.java +++ b/src/main/java/org/apache/commons/xml/factory/XalanProvider.java @@ -33,7 +33,7 @@ * <ul> * <li><strong>FSP</strong> ({@link XMLConstants#FEATURE_SECURE_PROCESSING}, set to {@code true}): enables Xalan's secure-processing mode, which disables * reflection-based extension functions. Required.</li> - * <li><strong>{@link DenyAllResolver#URI}</strong>: required. Xalan does not implement the JAXP 1.5 {@code ACCESS_EXTERNAL_*} attributes; a deny-all + * <li><strong>{@link Resolvers.DenyAll#URI}</strong>: required. Xalan does not implement the JAXP 1.5 {@code ACCESS_EXTERNAL_*} attributes; a deny-all * {@link javax.xml.transform.URIResolver} blocks {@code xsl:include}, {@code xsl:import}, {@code xsl:source-document} during stylesheet compilation * and {@code document()}, {@code unparsed-text()}, {@code collection()} at runtime.</li> * <li><strong>{@link HardeningTransformerFactory} + {@link HardeningTemplates} + {@link HardeningTransformer}</strong>: required. Xalan's source-document @@ -55,7 +55,7 @@ static TransformerFactory configure(final TransformerFactory factory) { setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true); // Required: Xalan does not honour JAXP 1.5 ACCESS_EXTERNAL_*; the deny-all URIResolver blocks: // xsl:import/xsl:include at compile time and document()/unparsed-text() at runtime. - factory.setURIResolver(DenyAllResolver.URI); + factory.setURIResolver(Resolvers.DenyAll.URI); // Required: Xalan's internal SAX reader is sourced from SAXParserFactory.newInstance() and only carries FSP. // We replace it with our hardened factory return new HardeningTransformerFactory((SAXTransformerFactory) factory); diff --git a/src/main/java/org/apache/commons/xml/factory/XercesProvider.java b/src/main/java/org/apache/commons/xml/factory/XercesProvider.java index 8052f48..26027f1 100644 --- a/src/main/java/org/apache/commons/xml/factory/XercesProvider.java +++ b/src/main/java/org/apache/commons/xml/factory/XercesProvider.java @@ -48,7 +48,7 @@ * JDK 8's secure values; this call pins them to the JDK 25 secure values (entity-expansion limit and {@code maxOccurs} node limit, the only two its * API exposes setters for).</li> * <li> - * <p><strong>{@code HardeningXxx} wrappers + {@link DenyAllResolver}</strong>: required. Xerces does not implement the JAXP 1.5 + * <p><strong>{@code HardeningXxx} wrappers + {@link Resolvers.DenyAll}</strong>: required. Xerces does not implement the JAXP 1.5 * {@code ACCESS_EXTERNAL_*} properties, so an explicit resolver installed on every parser/validator is the best way to block external * entity, DTD and schema fetching, without disabling those features altogether. The wrappers exist for two reasons:</p> * <ol> @@ -91,7 +91,7 @@ private static Validator hardenValidator(final Validator validator) { } catch (final SAXNotRecognizedException | SAXNotSupportedException e) { throw new HardeningException("Failed to read Xerces security manager from Validator", e); } - validator.setResourceResolver(DenyAllResolver.LS_RESOURCE); + validator.setResourceResolver(Resolvers.DenyAll.LS_RESOURCE); return validator; } @@ -101,7 +101,7 @@ private static ValidatorHandler hardenValidatorHandler(final ValidatorHandler ha } catch (final SAXNotRecognizedException | SAXNotSupportedException e) { throw new HardeningException("Failed to read Xerces security manager from ValidatorHandler", e); } - handler.setResourceResolver(DenyAllResolver.LS_RESOURCE); + handler.setResourceResolver(Resolvers.DenyAll.LS_RESOURCE); return handler; } @@ -131,7 +131,7 @@ static DocumentBuilderFactory configure(final DocumentBuilderFactory factory) { 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, DenyAllResolver.ENTITY2); + return new HardeningDocumentBuilderFactory(factory, Resolvers.DenyAll.ENTITY2); } static SAXParserFactory configure(final SAXParserFactory factory) { @@ -155,7 +155,7 @@ static XMLReader configure(final XMLReader reader) { throw new HardeningException("Failed to read Xerces security manager from XMLReader", e); } // Required: Xerces does not honour JAXP 1.5 ACCESS_EXTERNAL_*; the deny-all resolver is the only block. - reader.setEntityResolver(DenyAllResolver.ENTITY2); + reader.setEntityResolver(Resolvers.DenyAll.ENTITY2); return reader; } @@ -169,7 +169,7 @@ static SchemaFactory configure(final SchemaFactory factory) { throw new HardeningException("Failed to read Xerces security manager from SchemaFactory", e); } // Required: Xerces ignores ACCESS_EXTERNAL_*; the deny-all resolver blocks xs:import/include/redefine fetches. - factory.setResourceResolver(DenyAllResolver.LS_RESOURCE); + factory.setResourceResolver(Resolvers.DenyAll.LS_RESOURCE); // Required: routes every newSchema(Source[]) parse through an XmlFactories-hardened reader, and re-installs limits + resolver on each Validator and // ValidatorHandler since Xerces' Schema does not propagate factory state through. return new HardeningSchemaFactory(factory, XercesProvider::hardenValidator, XercesProvider::hardenValidatorHandler); diff --git a/src/test/java/org/apache/commons/xml/factory/AttackTestSupport.java b/src/test/java/org/apache/commons/xml/factory/AttackTestSupport.java index b130bf0..750b901 100644 --- a/src/test/java/org/apache/commons/xml/factory/AttackTestSupport.java +++ b/src/test/java/org/apache/commons/xml/factory/AttackTestSupport.java @@ -34,8 +34,10 @@ import javax.xml.parsers.SAXParserFactory; import javax.xml.stream.XMLEventReader; import javax.xml.stream.XMLInputFactory; +import javax.xml.stream.XMLStreamConstants; import javax.xml.stream.XMLStreamException; import javax.xml.stream.XMLStreamReader; +import javax.xml.stream.events.XMLEvent; import javax.xml.transform.ErrorListener; import javax.xml.transform.Source; import javax.xml.transform.Templates; @@ -439,6 +441,15 @@ static void assertSchemaBlocks(final Source xsd) { assertParseFails(() -> strictSchema(XmlFactories.newSchemaFactory(), xsd), "Schema compile", SAXException.class, SecurityException.class); } + /** + * Asserts a hardened Schema compilation succeeds. + * + * <p>{@link SchemaFactory#newSchema(Source)} via {@link XmlFactories#newSchemaFactory()}; positive control for DOCTYPE-only payloads.</p> + */ + static void assertSchemaCompiles(final Source xsd) { + assertParseSucceeds(() -> strictSchema(XmlFactories.newSchemaFactory(), xsd), "Schema compile"); + } + /** * Asserts a hardened Schema compilation completes without throwing. * @@ -461,6 +472,28 @@ static void assertStaxBlocks(final String payload) { assertParseFails(() -> consumeEventReader(XmlFactories.newXMLInputFactory(), payload), "StAX event", XMLStreamException.class); } + /** + * Asserts a hardened StAX parse completes without throwing and without leaked content. + * + * <p>{@link XMLStreamReader} and {@link XMLEventReader} from {@link XmlFactories#newXMLInputFactory()}; both flavours are exercised. Use this when the + * hardening guarantee is "the parse succeeds but never resolves the external resource", e.g. when the JDK's {@code ignore-external-dtd} property silently + * skips the external subset.</p> + */ + static void assertStaxDoesNotLeak(final String payload) { + assertNoLeakStrict(() -> captureStaxStreamText(XmlFactories.newXMLInputFactory(), payload), "StAX stream"); + assertNoLeakStrict(() -> captureStaxEventText(XmlFactories.newXMLInputFactory(), payload), "StAX event"); + } + + /** + * Asserts a hardened StAX parse succeeds. + * + * <p>{@link XMLStreamReader} and {@link XMLEventReader} from {@link XmlFactories#newXMLInputFactory()}; positive control for DOCTYPE-only payloads.</p> + */ + static void assertStaxParses(final String payload) { + assertParseSucceeds(() -> consumeStreamReader(XmlFactories.newXMLInputFactory(), payload), "StAX stream"); + assertParseSucceeds(() -> consumeEventReader(XmlFactories.newXMLInputFactory(), payload), "StAX event"); + } + /** * Asserts a hardened Templates compile-and-transform throws. * @@ -479,13 +512,13 @@ static void assertTemplatesBlocks(final Source xslt) { } /** - * Asserts a hardened Templates compile-and-transform either throws or completes without leaked content. + * Asserts a hardened Templates compile-and-transform succeeds. * - * <p>{@link TransformerFactory#newTemplates(Source)} via {@link XmlFactories#newTransformerFactory()} followed by transform; Saxon's TrAX path may swallow - * the entity-expansion error and produce empty output.</p> + * <p>{@link TransformerFactory#newTemplates(Source)} via {@link XmlFactories#newTransformerFactory()} followed by transform; positive control for + * DOCTYPE-only payloads.</p> */ - static void assertTemplatesBlocksOrDoesNotLeak(final Source xslt) { - assertNoLeakOrThrows(() -> templatesCompileAndTransform(xslt), "Templates", TransformerException.class); + static void assertTemplatesCompiles(final Source xslt) { + assertParseSucceeds(() -> templatesCompileAndTransform(xslt), "Templates compile"); } /** @@ -510,16 +543,6 @@ static void assertTransformerBlocks(final String payload) { "Transformer", TransformerException.class); } - /** - * Asserts a hardened identity Transformer either throws or completes without leaked content. - * - * <p>{@link Transformer#transform(Source, javax.xml.transform.Result)} via {@link XmlFactories#newTransformerFactory()}; Saxon's TrAX path may swallow - * internal errors and produce empty output.</p> - */ - static void assertTransformerBlocksOrDoesNotLeak(final String payload) { - assertNoLeakOrThrows(() -> identityTransformAndCapture(payload), "Transformer", TransformerException.class); - } - /** * Asserts a hardened identity Transformer completes without throwing and without leaked content. * @@ -530,6 +553,16 @@ static void assertTransformerDoesNotLeak(final String payload) { assertNoLeakStrict(() -> identityTransformAndCapture(payload), "Transformer"); } + /** + * Asserts a hardened identity Transformer succeeds. + * + * <p>{@link Transformer#transform(Source, javax.xml.transform.Result)} on the identity transformer from {@link XmlFactories#newTransformerFactory()}; + * positive control for DOCTYPE-only payloads.</p> + */ + static void assertTransformerTransforms(final String payload) { + assertParseSucceeds(() -> identityTransformAndCapture(payload), "Transformer"); + } + /** * Asserts a hardened Validator validation throws. * @@ -554,6 +587,18 @@ static void assertValidatorDoesNotLeak(final String xml) { "Validator"); } + /** + * Asserts a hardened Validator validation succeeds. + * + * <p>{@link Validator#validate(Source)} on a validator from {@link #BENIGN_SCHEMA} compiled via {@link XmlFactories#newSchemaFactory()}; positive control + * for DOCTYPE-only payloads.</p> + */ + static void assertValidatorValidates(final String xml) { + assertParseSucceeds( + () -> strictValidator(strictSchema(XmlFactories.newSchemaFactory(), streamSource(BENIGN_SCHEMA))).validate(streamSource(xml)), + "Validator"); + } + /** * Asserts a hardened-in-place XMLReader parse of the payload throws. * @@ -606,6 +651,40 @@ public void characters(final char[] ch, final int start, final int length) { return text.toString(); } + /** Parses the payload through a {@link XMLEventReader} and returns the accumulated character data, used by the StAX-based {@code DoesNotLeak} helper. */ + private static String captureStaxEventText(final XMLInputFactory factory, final String payload) throws Exception { + final StringBuilder text = new StringBuilder(); + final XMLEventReader events = factory.createXMLEventReader(new StringReader(payload)); + try { + while (events.hasNext()) { + final XMLEvent event = events.nextEvent(); + if (event.isCharacters()) { + text.append(event.asCharacters().getData()); + } + } + } finally { + events.close(); + } + return text.toString(); + } + + /** Parses the payload through a {@link XMLStreamReader} and returns the accumulated character data, used by the StAX-based {@code DoesNotLeak} helper. */ + private static String captureStaxStreamText(final XMLInputFactory factory, final String payload) throws Exception { + final StringBuilder text = new StringBuilder(); + final XMLStreamReader stream = factory.createXMLStreamReader(new StringReader(payload)); + try { + while (stream.hasNext()) { + final int event = stream.next(); + if (event == XMLStreamConstants.CHARACTERS || event == XMLStreamConstants.CDATA) { + text.append(stream.getText()); + } + } + } finally { + stream.close(); + } + return text.toString(); + } + /** Drains every {@link XMLEventReader} event from the factory's reader for the payload. */ private static void consumeEventReader(final XMLInputFactory factory, final String payload) throws Exception { final XMLEventReader events = factory.createXMLEventReader(new StringReader(payload)); diff --git a/src/test/java/org/apache/commons/xml/factory/DoctypeOnlyTest.java b/src/test/java/org/apache/commons/xml/factory/DoctypeOnlyTest.java index 6df4cf9..074d892 100644 --- a/src/test/java/org/apache/commons/xml/factory/DoctypeOnlyTest.java +++ b/src/test/java/org/apache/commons/xml/factory/DoctypeOnlyTest.java @@ -27,11 +27,6 @@ * <p>The SYSTEM identifier points at a deliberately bogus URL ({@code http://invalid.example.invalid/...}) under the IANA-reserved {@code .invalid} TLD: any * attempt to fetch it would raise a network error long before the test could complete, so a passing test proves the parser did not even try. The hardening * contract being verified is "skip the external DTD silently when nothing in the body needs it" rather than "reject every DOCTYPE".</p> - * - * <p>Coverage is limited to DOM and SAX. StAX is intentionally omitted: JDK Zephyr ({@code com.sun.xml.internal.stream}) routes the external DTD subset load - * through the same {@link javax.xml.stream.XMLResolver} it uses for general entities, with no equivalent of the Xerces - * {@code nonvalidating/load-external-dtd} switch, so a deny-all resolver fires on the DOCTYPE itself. Hardened StAX parsers therefore still throw on a - * SYSTEM-bearing DOCTYPE.</p> */ class DoctypeOnlyTest { @@ -40,7 +35,19 @@ class DoctypeOnlyTest { private static String payload() { return "<?xml version=\"1.0\"?>\n" + "<!DOCTYPE root SYSTEM \"" + BOGUS_DTD_URL + "\">\n" - + "<root>hello</root>\n"; + + AttackTestSupport.xmlBody("hello") + "\n"; + } + + private static String xsdPayload() { + return "<?xml version=\"1.0\"?>\n" + + "<!DOCTYPE xs:schema SYSTEM \"" + BOGUS_DTD_URL + "\">\n" + + AttackTestSupport.xsdBody("hello") + "\n"; + } + + private static String xsltPayload() { + return "<?xml version=\"1.0\"?>\n" + + "<!DOCTYPE xsl:stylesheet SYSTEM \"" + BOGUS_DTD_URL + "\">\n" + + AttackTestSupport.xsltBody("hello") + "\n"; } @Test @@ -57,6 +64,36 @@ void hardenedSaxParses() { AttackTestSupport.assertSaxParses(payload()); } + @Test + @Tag("schema") + void hardenedSchemaCompiles() { + AttackTestSupport.assertSchemaCompiles(AttackTestSupport.streamSource(xsdPayload())); + } + + @Test + @Tag("stax") + void hardenedStaxParses() { + AttackTestSupport.assertStaxParses(payload()); + } + + @Test + @Tag("trax") + void hardenedTemplatesCompiles() { + AttackTestSupport.assertTemplatesCompiles(AttackTestSupport.streamSource(xsltPayload())); + } + + @Test + @Tag("trax") + void hardenedTransformerTransforms() { + AttackTestSupport.assertTransformerTransforms(payload()); + } + + @Test + @Tag("schema") + void hardenedValidatorValidates() { + AttackTestSupport.assertValidatorValidates(payload()); + } + @Test @Tag("sax") void hardenedXmlReaderParses() { diff --git a/src/test/java/org/apache/commons/xml/factory/ExternalDtdTest.java b/src/test/java/org/apache/commons/xml/factory/ExternalDtdTest.java index c164c20..381110b 100644 --- a/src/test/java/org/apache/commons/xml/factory/ExternalDtdTest.java +++ b/src/test/java/org/apache/commons/xml/factory/ExternalDtdTest.java @@ -81,8 +81,8 @@ void hardenedSchemaDoesNotLeak() { @Test @Tag("stax") - void hardenedStaxBlocks() { - AttackTestSupport.assertStaxBlocks(xmlPayload()); + void hardenedStaxDoesNotLeak() { + AttackTestSupport.assertStaxDoesNotLeak(xmlPayload()); } @Test diff --git a/src/test/java/org/apache/commons/xml/factory/NoDoctypeTest.java b/src/test/java/org/apache/commons/xml/factory/NoDoctypeTest.java new file mode 100644 index 0000000..37aeb2e --- /dev/null +++ b/src/test/java/org/apache/commons/xml/factory/NoDoctypeTest.java @@ -0,0 +1,92 @@ +/* + * 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.factory; + +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; + +/** + * Checks that a plain document without a {@code DOCTYPE} declaration parses cleanly through every hardened JAXP surface. + * + * <p>This is the realistic 99% case for hardened input; the hardening contract being verified is "documents without a DOCTYPE parse cleanly through every + * JAXP surface" so accidental tightening (e.g. a resolver that refuses the synthetic-external-subset hook) is caught.</p> + */ +class NoDoctypeTest { + + private static String payload() { + return "<?xml version=\"1.0\"?>\n" + + AttackTestSupport.xmlBody("hello") + "\n"; + } + + private static String xsdPayload() { + return "<?xml version=\"1.0\"?>\n" + + AttackTestSupport.xsdBody("hello") + "\n"; + } + + private static String xsltPayload() { + return "<?xml version=\"1.0\"?>\n" + + AttackTestSupport.xsltBody("hello") + "\n"; + } + + @Test + @Tag("dom") + void hardenedDomParses() { + AttackTestSupport.assertDomParses(payload()); + } + + @Test + @Tag("sax") + void hardenedSaxParses() { + AttackTestSupport.assertSaxParses(payload()); + } + + @Test + @Tag("schema") + void hardenedSchemaCompiles() { + AttackTestSupport.assertSchemaCompiles(AttackTestSupport.streamSource(xsdPayload())); + } + + @Test + @Tag("stax") + void hardenedStaxParses() { + AttackTestSupport.assertStaxParses(payload()); + } + + @Test + @Tag("trax") + void hardenedTemplatesCompiles() { + AttackTestSupport.assertTemplatesCompiles(AttackTestSupport.streamSource(xsltPayload())); + } + + @Test + @Tag("trax") + void hardenedTransformerTransforms() { + AttackTestSupport.assertTransformerTransforms(payload()); + } + + @Test + @Tag("schema") + void hardenedValidatorValidates() { + AttackTestSupport.assertValidatorValidates(payload()); + } + + @Test + @Tag("sax") + void hardenedXmlReaderParses() { + AttackTestSupport.assertXmlReaderParses(payload()); + } +}
