This is an automated email from the ASF dual-hosted git repository. ppkarwasz pushed a commit to branch feature/ignore-all-resolver-floors in repository https://gitbox.apache.org/repos/asf/commons-xml.git
commit 7b3d2ad7abdcb96a76096e9f5b54c68ec0ab61dd Author: Piotr P. Karwasz <[email protected]> AuthorDate: Sat Aug 1 22:30:21 2026 +0200 Extend the ignore-all floor to Saxon resource resolution Saxon was the odd one out: its ALLOWED_PROTOCOLS="" lockdown rejected every unresolved external reference (for example an external DTD subset) with an exception, where the other implementations resolve it to empty content through a floor. Drop the ALLOWED_PROTOCOLS setting and install a nature-aware ignore-all ResourceResolver on the HardenedConfiguration instead: - XML, XSLT and XSD natures resolve to Saxon's EmptySource, so an unresolved xsl:include/xsl:import compiles as an empty module and doc()/document() return the empty sequence. - Text and binary natures resolve to an empty StreamSource, so unparsed-text() yields the empty string. - External-entity and DTD natures return null so the lookup falls through to the hardened reader's entity-resolver floor, keeping caller allow-listing on the reader working. The floor backs every resolution chain ahead of Saxon's direct-fetch fallback. A setResourceResolver override re-wraps any resolver installed later (including through the plain-JAXP TransformerFactory.setURIResolver route, which replaces the Configuration resolver wholesale) with the floor as its fallback, and an empty CollectionFinder covers fn:collection, the one channel that bypasses the resource resolver. Saxon now passes the DOCTYPE-only and external-DTD suites like the other stacks; tests where implementations still diverge on unresolved xsl:import/include (XSLTC and Xalan reject the emptied module, Saxon compiles it) accept either outcome and assert no leak. Pin the new nested class in ShadingFootprintTest. Assisted-By: Claude Fable 5 <[email protected]> --- .../java/org/apache/commons/xml/SaxonProvider.java | 62 +++++++++++++++++++--- .../org/apache/commons/xml/DoctypeOnlyTest.java | 10 ++-- .../commons/xml/EntityResolverFloorTest.java | 20 +++++-- .../apache/commons/xml/TemplatesImportTest.java | 2 +- .../apache/commons/xml/TemplatesIncludeTest.java | 2 +- 5 files changed, 75 insertions(+), 21 deletions(-) diff --git a/src/main/java/org/apache/commons/xml/SaxonProvider.java b/src/main/java/org/apache/commons/xml/SaxonProvider.java index a6c5002..ad9bfa6 100644 --- a/src/main/java/org/apache/commons/xml/SaxonProvider.java +++ b/src/main/java/org/apache/commons/xml/SaxonProvider.java @@ -17,15 +17,24 @@ package org.apache.commons.xml; +import java.io.StringReader; + import javax.xml.transform.TransformerFactory; import javax.xml.transform.TransformerFactoryConfigurationError; +import javax.xml.transform.stream.StreamSource; import javax.xml.xpath.XPathFactory; import org.xml.sax.XMLReader; import net.sf.saxon.Configuration; +import net.sf.saxon.functions.CollectionFn; import net.sf.saxon.jaxp.SaxonTransformerFactory; +import net.sf.saxon.lib.ChainedResourceResolver; +import net.sf.saxon.lib.CollectionFinder; +import net.sf.saxon.lib.EmptySource; import net.sf.saxon.lib.Feature; +import net.sf.saxon.lib.ResourceRequest; +import net.sf.saxon.lib.ResourceResolver; import net.sf.saxon.xpath.XPathFactoryImpl; /** @@ -42,29 +51,68 @@ final class SaxonProvider { * * <ol> * <li><b>SAX layer.</b> {@link #makeParser} hands every {@link XMLReader} Saxon would otherwise use through - * {@link XmlFactories#harden(XMLReader)}, which routes it to the matching bundled hardening recipe. DOCTYPE, external entities and XInclude - * are refused at parse time.</li> - * <li><b>URI-resolution layer.</b> {@link Feature#ALLOWED_PROTOCOLS} is set to the empty string. This blocks XSLT inclusions {@code xsl:include}, + * {@link XmlFactories#harden(XMLReader)}, which routes it to the matching bundled hardening recipe. External DTDs, entities and XInclude + * resolve to empty content at parse time.</li> + * <li><b>Resource-resolution layer.</b> A non-removable ignore-all {@link ResourceResolver} floor backs every resolution chain ({@code xsl:include}, * {@code xsl:import}, {@code xsl:source-document}, and the XPath/XSLT functions {@code fn:doc}, {@code fn:document}, {@code fn:unparsed-text}, - * {@code fn:collection}, {@code fn:json-doc} and {@code fn:transform}.</li> + * {@code fn:json-doc} and {@code fn:transform}) ahead of Saxon's direct-fetch fallback, resolving whatever a caller-set resolver leaves unresolved to + * empty content. {@code fn:collection} bypasses the resource resolver and fetches directly, so an empty {@link CollectionFinder} supplies its ignore + * outcome instead.</li> * <li><b>Extension-function layer.</b> {@link Feature#ALLOW_EXTERNAL_FUNCTIONS} is disabled, so reflection-based extension calls cannot be used to * sidestep the URI restrictions.</li> * </ol> */ private static class HardenedConfiguration extends Configuration { + /** + * Ignore-all resource-resolution floor: resolves whatever the resolvers ahead of it leave unresolved to empty content, so the external resource is + * neither fetched nor leaked. + * + * <p>Nature-aware, because Saxon's consumers accept different shapes of "empty".</p> + */ + private static final ResourceResolver IGNORE_ALL_FLOOR = request -> { + if (ResourceRequest.EXTERNAL_ENTITY_NATURE.equals(request.nature) || ResourceRequest.DTD_NATURE.equals(request.nature)) { + // Fall through to the parser's own EntityResolver, which Saxon chains behind this resolver: on a hardened reader that is the + // FallbackIgnoreEntityResolver2 floor, so caller allow-listing keeps working and the lookup still ends in empty content, not a fetch. + return null; + } + if (ResourceRequest.XML_NATURE.equals(request.nature) || ResourceRequest.XSLT_NATURE.equals(request.nature) + || ResourceRequest.XSD_NATURE.equals(request.nature)) { + // EmptySource makes xsl:include/xsl:import substitute an empty stylesheet module and doc()/document() return the empty sequence. + return EmptySource.getInstance(); + } + // Text and binary consumers need actual empty content: unparsed-text() yields the empty string. + return new StreamSource(new StringReader("")); + }; + + /** Collection-level ignore: {@code fn:collection()} and {@code fn:uri-collection()} resolve to an empty collection instead of fetching. */ + private static final CollectionFinder EMPTY_COLLECTION_FINDER = (context, collectionURI) -> CollectionFn.EMPTY_COLLECTION; + private HardenedConfiguration() { // Extension-function layer: turn off Saxon's reflection-based extension calls. Without this an attacker could bypass URI restrictions through // user-supplied Java extensions. setBooleanProperty(Feature.ALLOW_EXTERNAL_FUNCTIONS, false); - // URI-resolution layer: empty string disallows every URI scheme. Saxon front-ends the existing ResourceResolver with a ProtocolRestrictor; a - // later setResourceResolver call would cancel this filter, so this stays last in the constructor. - setConfigurationProperty(Feature.ALLOWED_PROTOCOLS, ""); + // Resource-resolution layer: the floor backs every resolution chain ahead of Saxon's direct-fetch fallback (installing it here also keeps the + // default catalog resolver out); the setResourceResolver override below keeps it non-removable. fn:collection is the one channel that bypasses + // the resolver, closed by the empty collection finder. + super.setResourceResolver(IGNORE_ALL_FLOOR); + setCollectionFinder(EMPTY_COLLECTION_FINDER); // Use the parser below for both style and source: setStyleParserClass("#DEFAULT"); setSourceParserClass("#DEFAULT"); } + /** + * Keeps the floor underneath any resolver installed later. + * + * <p>The plain JAXP routes ({@code TransformerFactory.setURIResolver}, {@code setAttribute} with Saxon's resolver-valued keys) replace the + * Configuration resolver wholesale rather than chaining to it, so the incoming resolver is re-wrapped with the floor as its fallback.</p> + */ + @Override + public void setResourceResolver(final ResourceResolver resolver) { + super.setResourceResolver(resolver == null ? IGNORE_ALL_FLOOR : new ChainedResourceResolver(resolver, IGNORE_ALL_FLOOR)); + } + /** * Saxon's hook for instantiating a new SAX parser. */ diff --git a/src/test/java/org/apache/commons/xml/DoctypeOnlyTest.java b/src/test/java/org/apache/commons/xml/DoctypeOnlyTest.java index 18d86c0..1c78212 100644 --- a/src/test/java/org/apache/commons/xml/DoctypeOnlyTest.java +++ b/src/test/java/org/apache/commons/xml/DoctypeOnlyTest.java @@ -27,9 +27,7 @@ * * <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 best-effort "skip the external DTD silently when nothing in the body needs it": most surfaces parse the document, but an - * implementation-prescribed rejection stays acceptable (Saxon's {@code ALLOWED_PROTOCOLS} restrictor refuses the subset lookup outright), so the TrAX cases - * accept either outcome.</p> + * contract being verified is "skip the external DTD silently when nothing in the body needs it" rather than "reject every DOCTYPE".</p> */ class DoctypeOnlyTest { @@ -82,15 +80,13 @@ void hardenedStaxParses() { @Test @Tag("trax") void hardenedTemplatesCompiles() { - // Saxon prescribes a rejection here: its ALLOWED_PROTOCOLS restrictor refuses the subset lookup before the ignore-all floor can empty it. - AttackTestSupport.assertTemplatesBlocksOrDoesNotLeak(AttackTestSupport.streamSource(xsltPayload())); + AttackTestSupport.assertTemplatesCompiles(AttackTestSupport.streamSource(xsltPayload())); } @Test @Tag("trax") void hardenedTransformerTransforms() { - // Saxon prescribes a rejection here: its ALLOWED_PROTOCOLS restrictor refuses the subset lookup before the ignore-all floor can empty it. - AttackTestSupport.assertTransformerBlocksOrDoesNotLeak(payload()); + AttackTestSupport.assertTransformerTransforms(payload()); } @Test diff --git a/src/test/java/org/apache/commons/xml/EntityResolverFloorTest.java b/src/test/java/org/apache/commons/xml/EntityResolverFloorTest.java index 8af9715..96c1bd9 100644 --- a/src/test/java/org/apache/commons/xml/EntityResolverFloorTest.java +++ b/src/test/java/org/apache/commons/xml/EntityResolverFloorTest.java @@ -24,6 +24,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.StringReader; +import java.io.StringWriter; import java.net.URL; import javax.xml.parsers.DocumentBuilder; @@ -38,6 +39,7 @@ import javax.xml.transform.TransformerException; import javax.xml.transform.TransformerFactory; import javax.xml.transform.URIResolver; +import javax.xml.transform.stream.StreamResult; import javax.xml.validation.SchemaFactory; import org.junit.jupiter.api.Assumptions; @@ -60,7 +62,7 @@ * <p>The observable contract on every hardened factory is the same: a resource the caller resolves (returns a non-null value) is allowed, but anything the * caller does not resolve is resolved to empty content instead of fetched, so a resolver that resolves nothing leaves the block in place. Most * factories enforce this with a {@link FallbackIgnoreEntityResolver2}-style floor that consults the caller and returns empty on a {@code null} return; Saxon - * enforces the equivalent through its {@code ALLOWED_PROTOCOLS} restrictor. Every resolver channel is exercised: the SAX/DOM + * enforces the equivalent through an ignore-all {@code ResourceResolver} floor on its {@code Configuration}. Every resolver channel is exercised: the SAX/DOM * {@link EntityResolver}, the StAX {@link XMLResolver}, the schema {@link LSResourceResolver} and the XSLT {@link URIResolver}.</p> */ class EntityResolverFloorTest { @@ -386,14 +388,22 @@ void transformerResolvesAllowListed() { void transformerDeniesUnlisted() { final TransformerFactory factory = hardenedTransformerFactory(); factory.setURIResolver((href, base) -> null); - assertParseFails(() -> factory.newTemplates(AttackTestSupport.resourceSource("with-import.xsl")), "Stylesheet import", TransformerException.class); + // XSLTC and Xalan reject the emptied import at compile time; Saxon compiles it as an empty module, so transform and assert the import did not leak. + try { + final StringWriter sink = new StringWriter(); + factory.newTemplates(AttackTestSupport.resourceSource("with-import.xsl")).newTransformer() + .transform(AttackTestSupport.streamSource("<root/>"), new StreamResult(sink)); + assertFalse(sink.toString().contains(AttackTestSupport.LEAKED_MARKER), "unlisted stylesheet import leaked"); + } catch (final TransformerException blocked) { + // Acceptable: rejected at compile rather than resolved to empty. + } } /** * A hardened {@link TransformerFactory} with a re-throwing error listener. XSLTC and Xalan enforce the block through the - * {@link FallbackIgnoreURIResolver} floor; Saxon enforces it through its {@code ALLOWED_PROTOCOLS} restrictor. Either way a caller-set resolver that - * returns {@code null} cannot re-open the fetch. The strict listener is required because interpretive Xalan routes a blocked {@code xsl:import} through the - * error listener and would otherwise recover and compile instead of throwing (XSLTC and Saxon throw regardless). + * {@link FallbackIgnoreURIResolver} floor; Saxon enforces it through the ignore-all resolver floor on its {@code Configuration}. Either way a caller-set + * resolver that returns {@code null} cannot re-open the fetch. The strict listener is required because interpretive Xalan routes a blocked + * {@code xsl:import} through the error listener and would otherwise recover and compile instead of throwing. */ private static TransformerFactory hardenedTransformerFactory() { final TransformerFactory factory = XmlFactories.newTransformerFactory(); diff --git a/src/test/java/org/apache/commons/xml/TemplatesImportTest.java b/src/test/java/org/apache/commons/xml/TemplatesImportTest.java index 28d5395..3a1768b 100644 --- a/src/test/java/org/apache/commons/xml/TemplatesImportTest.java +++ b/src/test/java/org/apache/commons/xml/TemplatesImportTest.java @@ -29,6 +29,6 @@ class TemplatesImportTest { @Test void hardenedTemplatesBlocks() { - AttackTestSupport.assertTemplatesBlocks(AttackTestSupport.resourceSource("with-import.xsl")); + AttackTestSupport.assertTemplatesBlocksOrDoesNotLeak(AttackTestSupport.resourceSource("with-import.xsl")); } } diff --git a/src/test/java/org/apache/commons/xml/TemplatesIncludeTest.java b/src/test/java/org/apache/commons/xml/TemplatesIncludeTest.java index 425110c..a20a706 100644 --- a/src/test/java/org/apache/commons/xml/TemplatesIncludeTest.java +++ b/src/test/java/org/apache/commons/xml/TemplatesIncludeTest.java @@ -29,6 +29,6 @@ class TemplatesIncludeTest { @Test void hardenedTemplatesBlocks() { - AttackTestSupport.assertTemplatesBlocks(AttackTestSupport.resourceSource("with-include.xsl")); + AttackTestSupport.assertTemplatesBlocksOrDoesNotLeak(AttackTestSupport.resourceSource("with-include.xsl")); } }
