This is an automated email from the ASF dual-hosted git repository.
garydgregory pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/commons-secure-xml.git
The following commit(s) were added to refs/heads/main by this push:
new 9192933 Secure getAssociatedStylesheet, and pin the CI Zulu job to
JDK 8u152 (#74)
9192933 is described below
commit 91929333d96bf213f97040de63b7c34690fd98f6
Author: Piotr P. Karwasz <[email protected]>
AuthorDate: Tue Sep 1 13:38:43 2026 +0200
Secure getAssociatedStylesheet, and pin the CI Zulu job to JDK 8u152 (#74)
* 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 <[email protected]>
Claude-Session: https://claude.ai/code/session_01CLnTBsvmYtxzNTWVGNyz33
* 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 <[email protected]>
Claude-Session: https://claude.ai/code/session_01CLnTBsvmYtxzNTWVGNyz33
* Add comment.
---------
Co-authored-by: Gary Gregory <[email protected]>
---
.github/workflows/maven.yml | 7 +--
.../xml/secure/SecureTransformerFactory.java | 60 ++++++++++++++++++---
.../xml/secure/AssociatedStylesheetTest.java | 63 +++++++++++++++-------
.../xml/secure/OverrideDefaultParserTest.java | 22 ++++++++
4 files changed, 122 insertions(+), 30 deletions(-)
diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml
index f231076..15115d9 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 # 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.
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 744ba85..f7dfa13 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.
* </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>
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 25b98f3..7f3f070 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.
*
- * <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
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 1ada79b..d5b640e 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);
