Merge pull request #46 from ppkarwasz/fix/associated-stylesheet-domsource
Harden getAssociatedStylesheet on Xalan via a DOMSource
diff --git a/src/main/java/org/apache/commons/xml/HardeningTransformerFactory.java b/src/main/java/org/apache/commons/xml/HardeningTransformerFactory.java
index 8c9abdf..cc25ddd 100644
--- a/src/main/java/org/apache/commons/xml/HardeningTransformerFactory.java
+++ b/src/main/java/org/apache/commons/xml/HardeningTransformerFactory.java
@@ -17,19 +17,27 @@
package org.apache.commons.xml;
+import java.io.IOException;
import java.util.function.Supplier;
+import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.ParserConfigurationException;
import javax.xml.transform.ErrorListener;
import javax.xml.transform.Source;
import javax.xml.transform.Templates;
import javax.xml.transform.Transformer;
import javax.xml.transform.TransformerConfigurationException;
import javax.xml.transform.URIResolver;
+import javax.xml.transform.dom.DOMSource;
import javax.xml.transform.sax.SAXSource;
import javax.xml.transform.sax.SAXTransformerFactory;
import javax.xml.transform.sax.TemplatesHandler;
import javax.xml.transform.sax.TransformerHandler;
+import javax.xml.transform.stream.StreamSource;
+import org.w3c.dom.Document;
+import org.xml.sax.InputSource;
+import org.xml.sax.SAXException;
import org.xml.sax.XMLFilter;
import org.xml.sax.XMLReader;
@@ -95,7 +103,45 @@ public URIResolver getURIResolver() {
@Override
public Source getAssociatedStylesheet(final Source source, final String media, final String title, final String charset)
throws TransformerConfigurationException {
- return delegate.getAssociatedStylesheet(SAXParserHardener.hardenSource(source), media, title, charset);
+ // Xalan's getAssociatedStylesheet drops a SAXSource's reader and self-provisions its own to scan for xml-stylesheet PIs (XALANJ-2849).
+ final Source hardened = isXalan(delegate) ? hardenSourceToDom(source) : SAXParserHardener.hardenSource(source);
+ return delegate.getAssociatedStylesheet(hardened, media, title, charset);
+ }
+
+ /**
+ * Whether the delegate is Apache Xalan (either its interpretive or its XSLTC factory), whose {@code getAssociatedStylesheet} ignores a SAXSource reader.
+ *
+ * @param factory The delegate factory.
+ * @return Whether the delegate is an {@code org.apache.xalan.} implementation.
+ */
+ private static boolean isXalan(final SAXTransformerFactory factory) {
+ return factory.getClass().getName().startsWith("org.apache.xalan.");
+ }
+
+ /**
+ * Parses a reader-less source into a DOM through a hardened, 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 SAXParserHardener#hardenSource(Source)}.
+ *
+ * @param source The source to scan for an associated stylesheet.
+ * @return A {@link DOMSource} for a reader-less source, otherwise the result of {@link SAXParserHardener#hardenSource(Source)}.
+ * @throws TransformerConfigurationException if the source cannot be parsed.
+ */
+ private static Source hardenSourceToDom(final Source source) throws TransformerConfigurationException {
+ if (source instanceof StreamSource || source instanceof SAXSource && ((SAXSource) source).getXMLReader() == null) {
+ final InputSource inputSource = SAXSource.sourceToInputSource(source);
+ if (inputSource != null) {
+ try {
+ final DocumentBuilderFactory factory = DocumentBuilderHardener.harden(DocumentBuilderFactory.newInstance());
+ factory.setNamespaceAware(true);
+ final Document document = factory.newDocumentBuilder().parse(inputSource);
+ return new DOMSource(document, inputSource.getSystemId());
+ } catch (final ParserConfigurationException | SAXException | IOException e) {
+ throw new TransformerConfigurationException("Failed to parse the source for associated-stylesheet lookup", e);
+ }
+ }
+ }
+ return SAXParserHardener.hardenSource(source);
}
@Override
diff --git a/src/test/java/org/apache/commons/xml/AssociatedStylesheetTest.java b/src/test/java/org/apache/commons/xml/AssociatedStylesheetTest.java
new file mode 100644
index 0000000..c4bb03f
--- /dev/null
+++ b/src/test/java/org/apache/commons/xml/AssociatedStylesheetTest.java
@@ -0,0 +1,84 @@
+/*
+ * 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;
+
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+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 org.junit.jupiter.api.Assumptions;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Checks that {@code getAssociatedStylesheet} scans for {@code xml-stylesheet} PIs without fetching an external DTD declared in the document prolog.
+ *
+ * <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 hardened reader passed in a {@link javax.xml.transform.sax.SAXSource} (XALANJ-2849); the wrapper works around
+ * that by handing Xalan a {@code DOMSource} it pre-parsed through a hardened {@code DocumentBuilder}. The JDK's XSLTC honors the hardened 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>
+ */
+@Tag("trax")
+class AssociatedStylesheetTest {
+
+ private static TransformerFactory hardenedFactory() {
+ final TransformerFactory factory = XmlFactories.newTransformerFactory();
+ factory.setErrorListener(AttackTestSupport.STRICT_REPORTER);
+ return factory;
+ }
+
+ @Test
+ void hardenedGetAssociatedStylesheetIgnoresExternalDtd() throws TransformerConfigurationException {
+ // The prolog declares an unreachable external DTD; the hardened 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.)
+ final Source associated = hardenedFactory()
+ .getAssociatedStylesheet(AttackTestSupport.resourceSource("associated-stylesheet.xml"), null, null, null);
+ assertAssociatedStylesheet(associated);
+ }
+
+ @Test
+ void hardenedGetAssociatedStylesheetReturnsStylesheet() throws TransformerConfigurationException {
+ // Positive control: a plain document with no DOCTYPE resolves its xml-stylesheet PI end to end.
+ final Source associated = hardenedFactory()
+ .getAssociatedStylesheet(AttackTestSupport.resourceSource("associated-stylesheet-plain.xml"), null, null, null);
+ assertAssociatedStylesheet(associated);
+ }
+
+ /** 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());
+ }
+ }
+
+ @Test
+ void unconfiguredGetAssociatedStylesheetFetchesExternalDtd() {
+ // Leak/discrimination control: the unconfigured engine attempts to fetch the unreachable external DTD and fails. Android's KXmlParser does not fetch
+ // external DTDs, so it has nothing to demonstrate here.
+ Assumptions.assumeFalse(AttackTestSupport.IS_ANDROID, "Android's KXmlParser does not fetch external DTDs");
+ assertThrows(TransformerConfigurationException.class, () -> TransformerFactory.newInstance()
+ .getAssociatedStylesheet(AttackTestSupport.resourceSource("associated-stylesheet.xml"), null, null, null));
+ }
+}
diff --git a/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java b/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java
index 4629163..f7bb39a 100644
--- a/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java
+++ b/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java
@@ -43,8 +43,9 @@
*
* <p>Using {@code jdependency}, the same library {@code maven-shade-plugin}'s {@code minimizeJar} uses, this test computes each entry point's transitive class
* closure over the compiled {@code target/classes} and pins it to an expected set. It keeps each hardener from silently regaining a dependency on classes it
- * should not need (for example a sibling resolver floor or another hardener), so TrAX, XPath and schema build only on the shared SAX path while only the public
- * {@link XmlFactories} entry pulls the whole library. Update the expected sets deliberately: a change here is a change to what a downstream shade includes.</p>
+ * should not need (for example a sibling resolver floor or another hardener), so XPath and schema build only on the shared SAX path, TrAX additionally on the
+ * DOM path its Xalan getAssociatedStylesheet rewrite parses through, while only the public {@link XmlFactories} entry pulls the whole library. Update the
+ * expected sets deliberately: a change here is a change to what a downstream shade includes.</p>
*
* <p>The test reads the compiled {@code .class} files from the code-source location, which only exists on a regular JVM: a native image carries no bytecode (and
* nobody shades one), so the test is disabled there, just as it is excluded from the Android test compile.</p>
@@ -69,12 +70,13 @@ class ShadingFootprintTest {
private static final Set<String> STAX_HARDENER = set("StaxHardener", "HardeningXMLInputFactory", "FallbackIgnoreXMLResolver", HARDENING_EXCEPTION);
/**
- * TrAX, XPath and schema re-harden their sub-parsers through {@link SAXParserHardener#harden(Source)}, so each builds on the full SAX closure below.
+ * TrAX, XPath and schema re-harden their sub-parsers through {@link SAXParserHardener#hardenSource(Source)}, so each builds on the full SAX closure below; TrAX
+ * additionally parses the Xalan {@code getAssociatedStylesheet} source through the DOM hardener, so its closure carries that set too.
*/
private static final Set<String> TRANSFORMER_HARDENER = saxParsersHardenerPlus("TransformerHardener", "HardeningTransformerFactory",
"HardeningTransformer", "HardeningTransformerHandler", "HardeningTemplates", "HardeningTemplatesHandler", "HardeningXMLFilter",
"FallbackIgnoreURIResolver", "SaxonProvider", "SaxonProvider$1", "SaxonProvider$HardenedConfiguration"
- , "SaxonProvider$SaxonProviderConfigurer");
+ , "SaxonProvider$SaxonProviderConfigurer", "DocumentBuilderHardener", "HardeningDocumentBuilder", "HardeningDocumentBuilderFactory");
private static final Set<String> XPATH_HARDENER = saxParsersHardenerPlus("XPathHardener", "FallbackIgnoreURIResolver", "SaxonProvider", "SaxonProvider$1",
"SaxonProvider$HardenedConfiguration", "SaxonProvider$SaxonProviderConfigurer");
diff --git a/src/test/resources/leaked/associated-stylesheet-plain.xml b/src/test/resources/leaked/associated-stylesheet-plain.xml
new file mode 100644
index 0000000..29958c0
--- /dev/null
+++ b/src/test/resources/leaked/associated-stylesheet-plain.xml
@@ -0,0 +1,4 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!-- SPDX-License-Identifier: Apache-2.0 -->
+<?xml-stylesheet type="text/xsl" href="included.xsl"?>
+<root/>
diff --git a/src/test/resources/leaked/associated-stylesheet.xml b/src/test/resources/leaked/associated-stylesheet.xml
new file mode 100644
index 0000000..0955158
--- /dev/null
+++ b/src/test/resources/leaked/associated-stylesheet.xml
@@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!-- SPDX-License-Identifier: Apache-2.0 -->
+<!DOCTYPE root SYSTEM "does-not-exist.dtd">
+<?xml-stylesheet type="text/xsl" href="included.xsl"?>
+<root/>
