From 414da2f31af7753ed3917565e22ff7e3deb1d51d Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 1 Sep 2026 12:14:19 +0200 Subject: [PATCH 1/3] Pin the CI Zulu job to JDK 8u152 The Zulu job exists to cover the oldest JDK 8 behavior the securing has to work against, but 8u201 already carries the 8u162 backport wave. Pin it to 8u152, the last release before XSLTC's getAssociatedStylesheet began honoring the XMLReader carried by a SAXSource: through 8u152 it self-provisions a parser instead, which the newer job cannot exercise. jdk.xml.overrideDefaultParser arrived in the same wave, so OverrideDefaultParserTest now skips where the runtime does not recognize the feature rather than failing on it. Assisted-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01CLnTBsvmYtxzNTWVGNyz33 --- .github/workflows/maven.yml | 7 +++--- .../xml/secure/OverrideDefaultParserTest.java | 22 +++++++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index f231076b..83b3c052 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -42,8 +42,9 @@ jobs: # - Distribution variance, one job each: # * Semeru 8 and 21 OpenJ9's class library is the most divergent runtime setup-java offers, # and the most plausible source of surprises in factory lookup and class loading. - # * Zulu Pinned to an old 8 patch level. - # Verifies the securing degrades gracefully on a runtime predating the later jdk.xml.* backports. + # * Zulu Pinned to 8u152, the last release before XSLTC's getAssociatedStylesheet started honoring the XMLReader + # carried by a SAXSource (it self-provisioned one through 8u152, and honors it from 8u162 on). + # Also predates the later jdk.xml.* backports, so it covers the oldest behavior the securing has to degrade against. # * GraalVM Runs the suite as a native image via the `native-xalan` profile: # the JAXP providers resolve at build time under the closed-world assumption, a different code path from # the JVM's run-time ServiceLoader lookup. 25 is the latest GraalVM release. @@ -75,7 +76,7 @@ jobs: java-version: 21 distribution: semeru - os: ubuntu-latest - java-version: 8.0.201 + java-version: 8.0.152 distribution: zulu # native-image resolves the JAXP providers at build time under the closed-world assumption, a different # code path from the JVM's run-time ServiceLoader lookup. 25 is the latest GraalVM release. diff --git a/src/test/java/org/apache/commons/xml/secure/OverrideDefaultParserTest.java b/src/test/java/org/apache/commons/xml/secure/OverrideDefaultParserTest.java index 1ada79b8..d5b640e0 100644 --- a/src/test/java/org/apache/commons/xml/secure/OverrideDefaultParserTest.java +++ b/src/test/java/org/apache/commons/xml/secure/OverrideDefaultParserTest.java @@ -22,6 +22,7 @@ import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assumptions.assumeFalse; +import static org.junit.jupiter.api.Assumptions.assumeTrue; import java.io.StringWriter; @@ -54,6 +55,23 @@ class OverrideDefaultParserTest { /** Package prefix of the JDK's built-in parsers, the family a {@code false} feature value pins. */ private static final String JDK_INTERNAL_PREFIX = "com.sun.org.apache.xerces.internal."; + /** {@code true} where the runtime's factories know {@value SecureSAXParserFactory#OVERRIDE_DEFAULT_PARSER}; JDK 8 gained it in 8u162. */ + private static final boolean SUPPORTS_FEATURE = probeFeature(); + + private static boolean probeFeature() { + try { + TransformerFactory.newInstance().setFeature(FEATURE, true); + return true; + } catch (final Exception e) { + return false; + } + } + + /** Skips a test on a runtime whose factories do not recognize the feature, where there is no selection to observe. */ + private static void assumeFeatureSupported() { + assumeTrue(SUPPORTS_FEATURE, "runtime does not recognize " + FEATURE); + } + private static String transform(final TransformerFactory factory, final String text) throws Exception { final Transformer transformer = factory.newTransformer(AttackTestSupport.streamSource(AttackTestSupport.xsltBody(text))); final StringWriter out = new StringWriter(); @@ -73,6 +91,7 @@ private static boolean xercesOnClasspath() { @Test void schemaFactoryReadsFeatureAtCreation() throws Exception { assumeFalse(AttackTestSupport.IS_ANDROID); + assumeFeatureSupported(); final SchemaFactory factory = SecureSchemaFactory.newDefaultInstance(); assertFalse(factory.getFeature(FEATURE)); assertFalse(((SecureSchema) factory.newSchema(AttackTestSupport.streamSource(AttackTestSupport.BENIGN_SCHEMA))).overrideDefaultParser); @@ -97,6 +116,7 @@ void secureReaderFollowsFlag() throws Exception { @Test void transformerFactoryReadsFeatureAtCreation() throws Exception { assumeFalse(AttackTestSupport.IS_ANDROID); + assumeFeatureSupported(); final TransformerFactory factory = SecureTransformerFactory.newDefaultInstance(); assertFalse(factory.getFeature(FEATURE)); assertFalse(((SecureTemplates) factory.newTemplates(AttackTestSupport.streamSource(AttackTestSupport.xsltBody("probe")))).overrideDefaultParser); @@ -110,6 +130,7 @@ void transformerFactoryReadsFeatureAtCreation() throws Exception { @DisabledInNativeImage void transformSucceedsUnderBothParserFamilies() throws Exception { assumeFalse(AttackTestSupport.IS_ANDROID); + assumeFeatureSupported(); final TransformerFactory factory = SecureTransformerFactory.newDefaultInstance(); // Feature false (the JDK's default): stylesheet and source parse through the pinned platform parser. assertTrue(transform(factory, "pinned").contains("pinned")); @@ -121,6 +142,7 @@ void transformSucceedsUnderBothParserFamilies() throws Exception { @Test void xPathFactoryReadsFeatureAtCreation() throws Exception { assumeFalse(AttackTestSupport.IS_ANDROID); + assumeFeatureSupported(); final XPathFactory factory = SecureXPathFactory.newDefaultInstance(); assertFalse(factory.getFeature(FEATURE)); assertFalse(((SecureXPath) factory.newXPath()).overrideDefaultParser); From dad641f6b57295383bc32b8f1881c64276c54723 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 1 Sep 2026 12:28:07 +0200 Subject: [PATCH 2/3] Secure both untrusted inputs of getAssociatedStylesheet The PI scan and the href it yields are each derived from the scanned document, and each reached an implementation unfiltered. The scan: Apache Xalan provisions its own parser instead of using the one a SAXSource carries (XALANJ-2849), and the JDK's XSLTC did the same before 8u162, so on Java 8 the document was scanned by a parser this library never configured. Pre-parse to a DOM for those engines, keying on Java 8 as a whole since the patch levels are indistinguishable through any API. Include a SAXSource that carries the caller's own reader: an engine that reaches this path drops that reader anyway, so the choice is only between this parse and the engine's own. The href: XSLTC-lineage engines resolve it during the scan, before the factory's URIResolver is installed, and hand back a live Source naming the absolutized URI, which compiling would then fetch. Route it through the floor like any other content-named reference, so a caller resolver opts it in and everything else resolves to empty. Saxon already behaved this way, so the engines now agree. Assisted-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01CLnTBsvmYtxzNTWVGNyz33 --- .../xml/secure/SecureTransformerFactory.java | 60 +++++++++++++++--- .../xml/secure/AssociatedStylesheetTest.java | 63 +++++++++++++------ 2 files changed, 96 insertions(+), 27 deletions(-) diff --git a/src/main/java/org/apache/commons/xml/secure/SecureTransformerFactory.java b/src/main/java/org/apache/commons/xml/secure/SecureTransformerFactory.java index 744ba854..f7dfa13e 100644 --- a/src/main/java/org/apache/commons/xml/secure/SecureTransformerFactory.java +++ b/src/main/java/org/apache/commons/xml/secure/SecureTransformerFactory.java @@ -31,6 +31,7 @@ import javax.xml.transform.Templates; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerConfigurationException; +import javax.xml.transform.TransformerException; import javax.xml.transform.TransformerFactory; import javax.xml.transform.TransformerFactoryConfigurationError; import javax.xml.transform.URIResolver; @@ -63,6 +64,12 @@ * {@code Transformer.transform(Source, Result)} time. *

*

+ * The {@code href} an {@code xml-stylesheet} processing instruction names is content of the document being scanned, so + * {@link TransformerFactory#getAssociatedStylesheet(Source, String, String, String) getAssociatedStylesheet} treats it as any other content-named reference: + * install a {@link URIResolver} resolving that href to compile the stylesheet it points at. Without one the returned {@link Source} carries empty content + * rather than naming the URI, so compiling it cannot fetch a stylesheet the parsed document chose. + *

+ *

* The {@link javax.xml.transform.sax.SAXTransformerFactory} extension methods ({@code newTransformerHandler(..)}, {@code newTemplatesHandler()}, * {@code newXMLFilter(..)}), if reachable by casting the returned factory, produce objects carrying the same guarantees. *

@@ -101,7 +108,8 @@ public final class SecureTransformerFactory { *
    *
  • A {@link SAXSource} that carries its own {@link XMLReader} is trusted as-is: the caller is expected to supply a secure reader (via * {@link SecureSAXParserFactory#newInstance()}) in that case. The same applies to the SAX events a caller feeds into a handler, and to a parent reader a - * caller sets on a returned {@link XMLFilter}.
    • + * caller sets on a returned {@link XMLFilter}. The exception is {@code getAssociatedStylesheet} on an engine that drops the reader (Apache Xalan, and + * the JDK's XSLTC on Java 8): there the document is pre-parsed into a DOM instead, since the reader would otherwise be replaced by the engine's own. *
* * @see org.apache.commons.xml.secure @@ -188,9 +196,35 @@ private Wrapper(final SAXTransformerFactory delegate, final Supplier emp @Override public Source getAssociatedStylesheet(final Source source, final String media, final String title, final String charset) throws TransformerConfigurationException { - // Xalan's getAssociatedStylesheet drops a SAXSource's reader and self-provisions its own to scan for xml-stylesheet PIs (XALANJ-2849). - final Source secure = isXalan(delegate) ? secureSourceToDom(source) : SecureSAXParserFactory.secure(source, overrideDefaultParser()); - return delegate.getAssociatedStylesheet(secure, media, title, charset); + // Xalan's getAssociatedStylesheet drops a SAXSource's reader and self-provisions its own to scan for xml-stylesheet PIs (XALANJ-2849), and the + // JDK's XSLTC did the same before 8u162; hand those a DOM so no parser but ours ever sees the document. + final Source secure = isXalan(delegate) || JAVA_8 ? secureSourceToDom(source) : SecureSAXParserFactory.secure(source, overrideDefaultParser()); + return floorAssociated(delegate.getAssociatedStylesheet(secure, media, title, charset), secure.getSystemId()); + } + + /** + * Routes the href an {@code xml-stylesheet} PI yielded through the floor, so a URI distilled from untrusted content is opted in by the caller's + * resolver or resolved to empty like any other content-named reference. + * + *

XSLTC-lineage engines resolve the href during the scan, before they install the factory's {@link URIResolver}, and hand back a live + * {@link SAXSource} naming the absolutized URI; compiling it, the one documented use of this method, would then fetch it. Saxon already floors the href + * itself and returns an empty source, so flooring here is also what makes the engines agree.

+ * + * @param associated The delegate's result; {@code null} when no PI matched. + * @param base The system id of the scanned document, the base the href was resolved against. + * @return The caller resolver's source for an opted-in href, an empty source otherwise, or {@code null} when no PI matched. + * @throws TransformerConfigurationException if the floor rejects the href, which it does when {@value SecureException#THROW_ON_UNRESOLVED} is set. + */ + private Source floorAssociated(final Source associated, final String base) throws TransformerConfigurationException { + if (associated == null || associated.getSystemId() == null) { + // No PI matched, or the engine already floored the href to a source that names no URI. + return associated; + } + try { + return floor.resolve(associated.getSystemId(), base); + } catch (final TransformerException e) { + throw new TransformerConfigurationException("Failed to resolve the associated stylesheet " + associated.getSystemId(), e); + } } @Override @@ -311,19 +345,22 @@ private TransformerHandler secure(final TransformerHandler handler) { } /** - * Parses a reader-less source into a DOM through a secure, namespace-aware {@link javax.xml.parsers.DocumentBuilder} and returns a {@link DOMSource} + * Parses a stream or SAX source into a DOM through a secure, namespace-aware {@link javax.xml.parsers.DocumentBuilder} and returns a {@link DOMSource} * carrying its system id, so the consumer walks the tree instead of provisioning its own reader. Any other source is left to * {@link SecureSAXParserFactory#secure(Source, boolean)}. * + *

A {@link SAXSource} carrying the caller's own reader is pre-parsed here too, unlike everywhere else in this class: an engine that reaches this + * method drops that reader anyway, so honoring it is not among the options — the choice is only between this parse and the engine's unsecured one.

+ * * @param source The source to scan for an associated stylesheet. - * @return A {@link DOMSource} for a reader-less source, otherwise the result of {@link SecureSAXParserFactory#secure(Source, boolean)}. + * @return A {@link DOMSource} for a stream or SAX source, otherwise the result of {@link SecureSAXParserFactory#secure(Source, boolean)}. * @throws TransformerConfigurationException if the source cannot be parsed. * @throws FactoryConfigurationError Thrown from a factory in case of a {@link java.util.ServiceConfigurationError service * configuration error} or if the implementation is not available or cannot be instantiated. * @throws SecureException Thrown if a (non-Android) factory cannot support the secure processing feature {@link XMLConstants#FEATURE_SECURE_PROCESSING}. */ private Source secureSourceToDom(final Source source) throws TransformerConfigurationException { - if (source instanceof StreamSource || source instanceof SAXSource && ((SAXSource) source).getXMLReader() == null) { + if (source instanceof StreamSource || source instanceof SAXSource) { final InputSource inputSource = SAXSource.sourceToInputSource(source); if (inputSource != null) { try { @@ -365,6 +402,15 @@ public void setURIResolver(final URIResolver resolver) { private static final MethodHandle MH_newDefaultInstance = MethodHandleFactory.findStatic(TransformerFactory.class, "newDefaultInstance"); + /** + * {@code true} on Java 8, detected by the absence of {@code TransformerFactory.newDefaultInstance()}, which arrived in Java 9. + * + *

The JDK's XSLTC only began honoring the {@link XMLReader} carried by a {@link SAXSource} in {@code getAssociatedStylesheet} in 8u162; through 8u152 it + * provisions its own parser, exactly as Apache Xalan does. Java 8 as a whole is used as the boundary rather than the patch level: the two are + * indistinguishable through any API, and a runtime that old has already chosen correctness of configuration over the cost of a DOM pre-parse.

+ */ + private static final boolean JAVA_8 = MH_newDefaultInstance == null; + /** * Returns a new, secure {@link TransformerFactory} of the system-default implementation. *

diff --git a/src/test/java/org/apache/commons/xml/secure/AssociatedStylesheetTest.java b/src/test/java/org/apache/commons/xml/secure/AssociatedStylesheetTest.java index 25b98f3b..7f3f0703 100644 --- a/src/test/java/org/apache/commons/xml/secure/AssociatedStylesheetTest.java +++ b/src/test/java/org/apache/commons/xml/secure/AssociatedStylesheetTest.java @@ -18,6 +18,7 @@ package org.apache.commons.xml.secure; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -25,31 +26,27 @@ import javax.xml.transform.TransformerConfigurationException; import javax.xml.transform.TransformerFactory; import javax.xml.transform.sax.SAXSource; +import javax.xml.transform.stream.StreamSource; import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; +import org.xml.sax.InputSource; /** - * Checks that {@code getAssociatedStylesheet} scans for {@code xml-stylesheet} PIs without fetching an external DTD declared in the document prolog. + * Checks the two untrusted inputs {@code getAssociatedStylesheet} handles: the document it scans, and the href the {@code xml-stylesheet} PI names. * - *

The PI scan parses the prolog, where a {@code DOCTYPE} with an external subset is processed before the root element. On Apache Xalan the scan runs on a - * reader the engine provisions itself, ignoring a secure reader passed in a {@link SAXSource} (XALANJ-2849); the wrapper works around - * that by handing Xalan a {@code DOMSource} it pre-parsed through a secure {@code DocumentBuilder}. The JDK's XSLTC honors the secure reader directly. Either - * way the external DTD resolves to empty instead of being fetched. Tagged {@code trax}, so it runs on the stock JDK, Apache Xalan, Saxon, and the Android - * runtime.

+ *

The scan parses the prolog, where a {@code DOCTYPE} with an external subset is processed before the root element. Apache Xalan runs it on a reader the + * engine provisions itself, ignoring one passed in a {@link SAXSource} (XALANJ-2849), and the JDK's XSLTC did the same before 8u162; the wrapper hands those a + * {@code DOMSource} it pre-parsed through a secure {@code DocumentBuilder}, so the external DTD resolves to empty instead of being fetched.

+ * + *

The href is attacker-controlled content, so the wrapper routes it through the same floor as any other content-named reference: unresolved by default, + * fetched only where a caller's {@code URIResolver} opts it in. Compiling the returned Source is the one documented use of this method, so returning it live + * would be handing back a URI the document chose. Tagged {@code trax}, so it runs on the stock JDK, Apache Xalan, Saxon, and the Android runtime.

*/ @Tag("trax") class AssociatedStylesheetTest { - /** The PI was found (non-null); where the engine exposes a system id, it points at the declared stylesheet. */ - private static void assertAssociatedStylesheet(final Source associated) { - assertNotNull(associated, "expected the associated stylesheet PI to be found"); - if (associated.getSystemId() != null) { - assertTrue(associated.getSystemId().endsWith("included.xsl"), "unexpected associated stylesheet: " + associated.getSystemId()); - } - } - private static TransformerFactory secureFactory() { final TransformerFactory factory = SecureTransformerFactory.newInstance(); factory.setErrorListener(AttackTestSupport.STRICT_REPORTER); @@ -58,20 +55,46 @@ private static TransformerFactory secureFactory() { @Test void secureGetAssociatedStylesheetIgnoresExternalDtd() throws TransformerConfigurationException { - // The prolog declares an unreachable external DTD; the secure parse resolves it to empty rather than fetching it, so the PI scan completes and finds - // the stylesheet instead of throwing on a fetch. (The returned Source's shape is engine-specific: XSLTC and Xalan point it at included.xsl, while Saxon - // resolves the href through its own floor and returns an empty source; both mean the scan ran without fetching the DTD.) + // The prolog declares an unreachable external DTD; the secure scan resolves it to empty rather than fetching it, so the lookup completes instead of + // throwing. The PI is found, and its href is floored, so what comes back names no URI. final Source associated = secureFactory() .getAssociatedStylesheet(AttackTestSupport.resourceSource("associated-stylesheet.xml"), null, null, null); - assertAssociatedStylesheet(associated); + assertNotNull(associated, "expected the associated stylesheet PI to be found"); + assertNull(associated.getSystemId(), "the PI href must not come back as a live URI: " + associated.getSystemId()); + } + + @Test + void secureGetAssociatedStylesheetIgnoresExternalDtdWithCallerReader() throws Exception { + // Same scan through a SAXSource carrying a caller-supplied secure reader. Xalan and Java 8 XSLTC drop that reader, so this shape has to be pre-parsed + // like the reader-less one rather than passed through. + final SAXSource source = new SAXSource(SecureSAXParserFactory.newInstance().newSAXParser().getXMLReader(), + new InputSource(AttackTestSupport.resourceUrl("associated-stylesheet.xml").toString())); + final Source associated = secureFactory().getAssociatedStylesheet(source, null, null, null); + assertNotNull(associated, "expected the associated stylesheet PI to be found"); + assertNull(associated.getSystemId(), "the PI href must not come back as a live URI: " + associated.getSystemId()); + } + + @Test + void secureGetAssociatedStylesheetOptsInThroughResolver() throws TransformerConfigurationException { + // A caller resolver is consulted for the href exactly as for any other reference. What comes back names the opted-in stylesheet rather than nothing, + // which is what separates an opt-in from the floored default; the floor still re-parses it through a secure reader, so the shape is its own. + final StreamSource opted = new StreamSource(AttackTestSupport.resourceUrl("included.xsl").toString()); + final TransformerFactory factory = secureFactory(); + factory.setURIResolver((href, base) -> href != null && href.endsWith("included.xsl") ? opted : null); + final Source associated = factory + .getAssociatedStylesheet(AttackTestSupport.resourceSource("associated-stylesheet-plain.xml"), null, null, null); + assertNotNull(associated, "expected the associated stylesheet PI to be found"); + assertNotNull(associated.getSystemId(), "an opted-in href must resolve to the caller's stylesheet, not to the empty default"); + assertTrue(associated.getSystemId().endsWith("included.xsl"), "unexpected associated stylesheet: " + associated.getSystemId()); } @Test void secureGetAssociatedStylesheetReturnsStylesheet() throws TransformerConfigurationException { - // Positive control: a plain document with no DOCTYPE resolves its xml-stylesheet PI end to end. + // Positive control: a plain document with no DOCTYPE is scanned end to end and its PI found, with the href floored. final Source associated = secureFactory() .getAssociatedStylesheet(AttackTestSupport.resourceSource("associated-stylesheet-plain.xml"), null, null, null); - assertAssociatedStylesheet(associated); + assertNotNull(associated, "expected the associated stylesheet PI to be found"); + assertNull(associated.getSystemId(), "the PI href must not come back as a live URI: " + associated.getSystemId()); } @Test From 19f678dec17d5484ec91565c20a242e4c4a6771a Mon Sep 17 00:00:00 2001 From: Gary Gregory Date: Tue, 1 Sep 2026 07:36:27 -0400 Subject: [PATCH 3/3] Add comment. --- .github/workflows/maven.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index 83b3c052..15115d9a 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -76,7 +76,7 @@ jobs: java-version: 21 distribution: semeru - os: ubuntu-latest - java-version: 8.0.152 + java-version: 8.0.152 # Not the latest, see comment above. distribution: zulu # native-image resolves the JAXP providers at build time under the closed-world assumption, a different # code path from the JVM's run-time ServiceLoader lookup. 25 is the latest GraalVM release.