Secure getAssociatedStylesheet, and pin the CI Zulu job to JDK 8u152

.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 # 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.

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.
  * </p>
  * <p>
+ * 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.
+ * </p>
+ * <p>
  * 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.
  * </p>
@@ -101,7 +108,8 @@ public final class SecureTransformerFactory {
      * <ul>
      *   <li>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}.</li>
+     *       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.</li>
      * </ul>
      *
      * @see org.apache.commons.xml.secure
@@ -188,9 +196,35 @@ private Wrapper(final SAXTransformerFactory delegate, final Supplier<Source> 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.
+         *
+         * <p>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.</p>
+         *
+         * @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)}.
          *
+         * <p>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.</p>
+         *
          * @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.
+     *
+     * <p>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.</p>
+     */
+    private static final boolean JAVA_8 = MH_newDefaultInstance == null;
+
     /**
      * Returns a new, secure {@link TransformerFactory} of the system-default implementation.
      * <p>

src/test/java/org/apache/commons/xml/secure/AssociatedStylesheetTest.java

@@ -18,38 +18,35 @@
 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;
 
 import javax.xml.transform.Source;
 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.
  *
- * <p>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.</p>
+ * <p>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.</p>
+ *
+ * <p>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.</p>
  */
 @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