+ * 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 { *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 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);