From 87c7172992180fa600fe2254e0df492cef8cc41e Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Sun, 28 Jun 2026 17:35:18 +0200 Subject: [PATCH 1/2] Base SAXParserFactory hardening on feature support Replace the per-implementation class-name dispatch for SAX with a single capability-driven recipe, mirroring the DocumentBuilderFactory rework. A new SAXParserHardener probes feature and property support and adapts: - harden(SAXParserFactory) sets FSP (skipped on Android's Expat, which rejects it) and wraps the factory in the now-universal HardeningSAXParserFactory, since SAXParserFactory exposes no property API. - hardenReader(XMLReader) sets FSP, skips the external DTD subset where supported, applies limits and probes ACCESS_EXTERNAL_*: the bare reader is returned when the JAXP 1.5 properties are honoured (stock JDK), otherwise a deny-all EntityResolver is installed (external Xerces). - Limits.tryApply(XMLReader) picks the Xerces SecurityManager vs JDK limit-property path by class name (the JDK reader is itself a Xerces fork exposing the same property, so a probe cannot tell them apart). - JaxpSetters gains setOptionalFeature(XMLReader) and trySetProperty(XMLReader). StockJdkProvider and XercesProvider lose their SAX recipes; XercesProvider now handles only Schema factories. HardeningSAXParserFactory becomes a single-argument wrapper that returns a HardeningSAXParser, which exposes the hardened reader and a matching SAX 1 Parser (cast or XMLReaderAdapter, lazily) so consumers reaching the parser through either path, including Xalan's identity transformer, get the same hardening. Android keeps its fixups rather than becoming a pass-through. Expat ignores external fetches silently when no resolver is set, so a subset-aware deny-all resolver is installed to fail on external entities while still letting an unused external subset load, and an ExpatReaderWrapper rejects the unsupported namespace-prefixes feature eagerly instead of failing mid-parse. DOM (KXmlParser) remains a pass-through. Behaviour change: the returned SAXParserFactory no longer forces namespace awareness. The previous StockJdkProvider and XercesProvider SAX recipes called setNamespaceAware(true); whether to parse namespace-aware is the caller's decision, not a hardening concern, so it is now left at the implementation default (DOM never set it). The internal Source-parsing path keeps it on: XmlFactories.harden(Source) sets namespace awareness itself, since schema, XSLT and validator sources are parsed in a namespace-aware context. Adds SchemaLocationSaxTest (the SAX counterpart of SchemaLocationDomTest, reusing AttackTestSupport's shared strict reporter) and a SAX case in UnsupportedXmlImplementationTest. AndroidProvider is removed. Assisted-By: Claude Opus 4.8 (1M context) --- .../apache/commons/xml/AndroidProvider.java | 195 ------------------ .../commons/xml/HardeningSAXParser.java | 65 ++++++ .../xml/HardeningSAXParserFactory.java | 18 +- .../org/apache/commons/xml/JaxpSetters.java | 25 +++ .../java/org/apache/commons/xml/Limits.java | 28 ++- .../org/apache/commons/xml/Resolvers.java | 3 +- .../apache/commons/xml/SAXParserHardener.java | 185 +++++++++++++++++ .../apache/commons/xml/StockJdkProvider.java | 32 +-- .../apache/commons/xml/XercesProvider.java | 45 +--- .../org/apache/commons/xml/XmlFactories.java | 39 +--- .../apache/commons/xml/AttackTestSupport.java | 4 +- .../commons/xml/SchemaLocationSaxTest.java | 128 ++++++++++++ .../xml/UnsupportedXmlImplementationTest.java | 35 ++++ 13 files changed, 489 insertions(+), 313 deletions(-) delete mode 100644 src/main/java/org/apache/commons/xml/AndroidProvider.java create mode 100644 src/main/java/org/apache/commons/xml/HardeningSAXParser.java create mode 100644 src/main/java/org/apache/commons/xml/SAXParserHardener.java create mode 100644 src/test/java/org/apache/commons/xml/SchemaLocationSaxTest.java diff --git a/src/main/java/org/apache/commons/xml/AndroidProvider.java b/src/main/java/org/apache/commons/xml/AndroidProvider.java deleted file mode 100644 index 8bdf20c8..00000000 --- a/src/main/java/org/apache/commons/xml/AndroidProvider.java +++ /dev/null @@ -1,195 +0,0 @@ -/* - * 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 java.util.Objects; - -import javax.xml.XMLConstants; -import javax.xml.parsers.DocumentBuilderFactory; -import javax.xml.parsers.ParserConfigurationException; -import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; - -import org.xml.sax.EntityResolver; -import org.xml.sax.InputSource; -import org.xml.sax.SAXException; -import org.xml.sax.SAXNotRecognizedException; -import org.xml.sax.SAXNotSupportedException; -import org.xml.sax.XMLReader; -import org.xml.sax.ext.DefaultHandler2; -import org.xml.sax.ext.LexicalHandler; - -/** - * Hardening recipes for Android's Apache Harmony based DOM and SAX implementation. - * - *

Factory classes live in the {@code org.apache.harmony.xml.parsers.*} package, but DOM and SAX are backed by two different engines:

- * - * - *

What the SAX/Expat surface exposes:

- * - * - *

What the DOM/KXmlParser surface exposes:

- * - * - *

The SAX path installs a {@link DtdAwareDenyResolver} as both {@link EntityResolver} and {@link LexicalHandler}: it allows the external subset to load - * silently (so a DOCTYPE that names an external DTD but does not use it parses) and throws on every external general or parameter entity reference.

- */ -final class AndroidProvider { - - /** - * Resolver that denies every external resource lookup, except the external DTD subset declared by the DOCTYPE - * - *

Merely declaring an external subset does not cause the parse to throw.

- */ - private static final class DtdAwareDenyResolver extends DefaultHandler2 { - - private static String forbiddenMessage(final String publicId, final String systemId) { - return String.format("External Entity: failed to read external entity (publicId='%s', systemId='%s'); external entity access is denied.", - publicId, systemId); - } - - private String dtdPublicId; - private String dtdSystemId; - private boolean inDtd; - - @Override - public void endDTD() { - inDtd = false; - } - - @Override - public InputSource resolveEntity(final String publicId, final String systemId) throws SAXException { - if (inDtd && Objects.equals(publicId, dtdPublicId) && Objects.equals(systemId, dtdSystemId)) { - return null; - } - throw new SAXException(forbiddenMessage(publicId, systemId)); - } - - @Override - public void startDTD(final String name, final String publicId, final String systemId) { - inDtd = true; - dtdPublicId = publicId; - dtdSystemId = systemId; - } - } - - /** - * {@link SAXParser} wrapper whose {@link #getXMLReader()} returns a {@link GuardedXMLReader}. - */ - private static final class GuardedSAXParser extends DelegatingSAXParser { - - private final XMLReader guardedReader; - - GuardedSAXParser(final SAXParser delegate, final XMLReader guardedReader) { - super(delegate); - this.guardedReader = guardedReader; - } - - @Override - public XMLReader getXMLReader() { - return guardedReader; - } - } - - /** - * {@link SAXParserFactory} wrapper that produces {@link GuardedSAXParser}s. - */ - private static final class GuardedSAXParserFactory extends DelegatingSAXParserFactory { - - GuardedSAXParserFactory(final SAXParserFactory delegate) { - super(delegate); - } - - @Override - public SAXParser newSAXParser() throws ParserConfigurationException, SAXException { - final SAXParser parser = super.newSAXParser(); - return new GuardedSAXParser(parser, configure(parser.getXMLReader())); - } - } - - /** - * {@link XMLReader} wrapper that surfaces ExpatReader's conflicting-feature error at {@code setFeature} time rather than at {@code parse} time. - * - *

Android's {@code ExpatReader.parse()} throws {@link SAXNotSupportedException} when {@code namespaces} and {@code namespace-prefixes} are both - * enabled. Reporting the error at configuration time lets consumers, such as Apache Xalan's identity transformer, catch the exception and still parse - * the document. Without the wrapper, parsing fails.

- */ - static final class GuardedXMLReader extends DelegatingXMLReader { - - GuardedXMLReader(final XMLReader delegate) { - super(delegate); - } - - @Override - public void setFeature(final String name, final boolean value) throws SAXNotRecognizedException, SAXNotSupportedException { - if (value - && (NAMESPACE_PREFIXES_FEATURE.equals(name) && super.getFeature(NAMESPACES_FEATURE) - || NAMESPACES_FEATURE.equals(name) && super.getFeature(NAMESPACE_PREFIXES_FEATURE))) { - throw new SAXNotSupportedException("ExpatReader cannot have both '" + NAMESPACES_FEATURE + "' and '" + NAMESPACE_PREFIXES_FEATURE + "' " + - "enabled simultaneously"); - } - super.setFeature(name, value); - } - } - - private static final String LEXICAL_HANDLER_PROPERTY = "http://xml.org/sax/properties/lexical-handler"; - - private static final String NAMESPACES_FEATURE = "http://xml.org/sax/features/namespaces"; - - private static final String NAMESPACE_PREFIXES_FEATURE = "http://xml.org/sax/features/namespace-prefixes"; - - static SAXParserFactory configure(final SAXParserFactory factory) { - return new GuardedSAXParserFactory(factory); - } - - static XMLReader configure(final XMLReader reader) { - if (reader instanceof GuardedXMLReader) { - return reader; - } - final DtdAwareDenyResolver resolver = new DtdAwareDenyResolver(); - reader.setEntityResolver(resolver); - try { - reader.setProperty(LEXICAL_HANDLER_PROPERTY, resolver); - } catch (final SAXException ignore) { - // ExpatReader recognizes the lexical-handler property; if a future replacement does not, fall through and lose subset-vs-entity discrimination. - } - return new GuardedXMLReader(reader); - } - - private AndroidProvider() { - } -} \ No newline at end of file diff --git a/src/main/java/org/apache/commons/xml/HardeningSAXParser.java b/src/main/java/org/apache/commons/xml/HardeningSAXParser.java new file mode 100644 index 00000000..766c399f --- /dev/null +++ b/src/main/java/org/apache/commons/xml/HardeningSAXParser.java @@ -0,0 +1,65 @@ +/* + * 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 javax.xml.parsers.SAXParser; + +import org.xml.sax.Parser; +import org.xml.sax.SAXException; +import org.xml.sax.XMLReader; +import org.xml.sax.helpers.XMLReaderAdapter; + +/** + * {@link SAXParser} that exposes a hardened {@link XMLReader} and a matching SAX 1 {@link Parser}. + * + *

Both views are produced from the same hardened reader, so a caller reaching the parser through either the SAX 2 ({@link #getXMLReader()}) or the legacy + * SAX 1 ({@link #getParser()}) path gets the same hardening. The SAX 1 view matters because some consumers, such as Xalan's identity transformer, still ask + * for a {@link Parser}.

+ * + *

The hardened reader is computed lazily on first access and cached: hardening an {@link XMLReader} can install a fresh wrapper (Android's Expat path), so + * every parse must run through the same instance. The {@code parse(...)} overloads inherited from {@link SAXParser} dispatch virtually to {@link #getXMLReader()} + * and {@link #getParser()}, so they too run through the hardened views without further overrides.

+ */ +final class HardeningSAXParser extends DelegatingSAXParser { + + private XMLReader hardenedReader; + private Parser hardenedParser; + + HardeningSAXParser(final SAXParser delegate) { + super(delegate); + } + + @Override + public XMLReader getXMLReader() throws SAXException { + if (hardenedReader == null) { + hardenedReader = SAXParserHardener.hardenReader(super.getXMLReader()); + } + return hardenedReader; + } + + @Override + @SuppressWarnings("deprecation") + public Parser getParser() throws SAXException { + if (hardenedParser == null) { + final XMLReader reader = getXMLReader(); + // Reuse the reader directly if it already is a SAX 1 parser; otherwise adapt it, so the SAX 1 path runs through the same hardened reader. + hardenedParser = reader instanceof Parser ? (Parser) reader : new XMLReaderAdapter(reader); + } + return hardenedParser; + } +} diff --git a/src/main/java/org/apache/commons/xml/HardeningSAXParserFactory.java b/src/main/java/org/apache/commons/xml/HardeningSAXParserFactory.java index a096c049..df78abef 100644 --- a/src/main/java/org/apache/commons/xml/HardeningSAXParserFactory.java +++ b/src/main/java/org/apache/commons/xml/HardeningSAXParserFactory.java @@ -17,8 +17,6 @@ package org.apache.commons.xml; -import java.util.function.UnaryOperator; - import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; @@ -27,24 +25,20 @@ import org.xml.sax.XMLReader; /** - * Universal SAX wrapper that defers per-parser hardening to a supplied {@link XMLReader} hardener. + * Universal SAX factory wrapper that funnels every produced parser through {@link SAXParserHardener#hardenReader(XMLReader)}. * - *

{@link SAXParserFactory} has no property API, only a feature API. Therefore, complex configuration must be performed on each new - * {@link XMLReader} parser.

+ *

{@link SAXParserFactory} exposes only a feature API and no property API, so the per-parse hardening (limits, entity blocking, implementation-specific + * fixups) has to run on each {@link XMLReader} the factory produces. This wrapper returns a {@link HardeningSAXParser}, which applies that hardening lazily to + * both the SAX 2 {@link XMLReader} and the SAX 1 {@link org.xml.sax.Parser} it exposes.

*/ final class HardeningSAXParserFactory extends DelegatingSAXParserFactory { - private final UnaryOperator hardener; - - HardeningSAXParserFactory(final SAXParserFactory delegate, final UnaryOperator hardener) { + HardeningSAXParserFactory(final SAXParserFactory delegate) { super(delegate); - this.hardener = hardener; } @Override public SAXParser newSAXParser() throws ParserConfigurationException, SAXException { - final SAXParser parser = super.newSAXParser(); - hardener.apply(parser.getXMLReader()); - return parser; + return new HardeningSAXParser(super.newSAXParser()); } } diff --git a/src/main/java/org/apache/commons/xml/JaxpSetters.java b/src/main/java/org/apache/commons/xml/JaxpSetters.java index 3d2ffecd..9d0ee32e 100644 --- a/src/main/java/org/apache/commons/xml/JaxpSetters.java +++ b/src/main/java/org/apache/commons/xml/JaxpSetters.java @@ -125,6 +125,14 @@ static void setFeature(final XMLReader reader, final String feature, final boole apply(reader, KIND_FEATURE, feature, () -> reader.setFeature(feature, value)); } + static void setOptionalFeature(final XMLReader reader, final String feature, final boolean value) { + try { + reader.setFeature(feature, value); + } catch (final Exception e) { + // Ignored: the implementation does not recognize this feature. + } + } + static void setProperty(final XMLInputFactory factory, final String property, final Object value) { apply(factory, KIND_PROPERTY, property, () -> factory.setProperty(property, value)); } @@ -137,6 +145,23 @@ static void setProperty(final XMLReader reader, final String property, final Obj apply(reader, KIND_PROPERTY, property, () -> reader.setProperty(property, value)); } + /** + * Sets a property on an {@link XMLReader} and returns whether the implementation accepted it. + * + * @param reader The target reader on which to set the property. + * @param property The name of the property to set. + * @param value The value of the property to set. + * @return {@code true} if the property was applied, {@code false} if the implementation rejected it. + */ + static boolean trySetProperty(final XMLReader reader, final String property, final Object value) { + try { + reader.setProperty(property, value); + return true; + } catch (final Exception e) { + return false; + } + } + static void setProperty(final SchemaFactory factory, final String property, final Object value) { apply(factory, KIND_PROPERTY, property, () -> factory.setProperty(property, value)); } diff --git a/src/main/java/org/apache/commons/xml/Limits.java b/src/main/java/org/apache/commons/xml/Limits.java index d60c655c..df971503 100644 --- a/src/main/java/org/apache/commons/xml/Limits.java +++ b/src/main/java/org/apache/commons/xml/Limits.java @@ -19,6 +19,7 @@ import static org.apache.commons.xml.JaxpSetters.setAttribute; import static org.apache.commons.xml.JaxpSetters.setOptionalAttribute; import static org.apache.commons.xml.JaxpSetters.setProperty; +import static org.apache.commons.xml.JaxpSetters.trySetProperty; import java.util.Collections; import java.util.LinkedHashMap; @@ -30,6 +31,8 @@ import javax.xml.transform.TransformerFactory; import javax.xml.validation.SchemaFactory; +import org.xml.sax.SAXNotRecognizedException; +import org.xml.sax.SAXNotSupportedException; import org.xml.sax.XMLReader; /** @@ -221,6 +224,10 @@ final class Limits { * Class name of the external Apache Xerces {@link DocumentBuilderFactory}, whose limits live on a {@code SecurityManager} rather than JDK attributes. */ private static final String EXTERNAL_XERCES_DOCUMENT_BUILDER_FACTORY = "org.apache.xerces.jaxp.DocumentBuilderFactoryImpl"; + /** + * Class name of the external Apache Xerces {@link XMLReader}, whose limits live on a {@code SecurityManager} rather than JDK properties. + */ + private static final String EXTERNAL_XERCES_SAX_READER = "org.apache.xerces.jaxp.SAXParserImpl$JAXPSAXParser"; static { final Map map = new LinkedHashMap<>(); @@ -283,12 +290,27 @@ static void applyToJdkTransformer(final TransformerFactory factory) { } /** - * Sets every JDK-supported limit on a stock JDK {@link XMLReader}. + * Best-effort application of the processing limits to an {@link XMLReader}, dispatched on the implementation. + * + *

External Xerces carries its limits on an {@code org.apache.xerces.util.SecurityManager} instance reachable through the + * {@value XercesProvider#XERCES_SECURITY_MANAGER_PROPERTY} property. The stock JDK reader is itself a Xerces fork that exposes the same property, so a probe + * cannot tell the two apart; the external distribution is therefore matched by class name, and every other reader (the stock JDK, a Saxon-picked JDK reader, + * any future attribute-based parser) takes the JDK limit properties. Neither path throws if the reader declines a limit.

* * @param reader The target reader to modify. */ - static void applyToJdkXmlReader(final XMLReader reader) { - JDK_LIMITS.forEach((name, supplier) -> setProperty(reader, name, Integer.toString(supplier.getAsInt()))); + static void tryApply(final XMLReader reader) { + if (EXTERNAL_XERCES_SAX_READER.equals(reader.getClass().getName())) { + try { + // External Xerces: tighten the SecurityManager it already installed under FSP to JDK 25 limits. + applyToXerces(reader.getProperty(XercesProvider.XERCES_SECURITY_MANAGER_PROPERTY)); + } catch (final SAXNotRecognizedException | SAXNotSupportedException e) { + throw new HardeningException("Failed to read Xerces security manager from XMLReader", e); + } + return; + } + // Pin the JDK limit properties to JDK 25 secure values; skip silently any property the reader does not recognize. + JDK_LIMITS.forEach((name, supplier) -> trySetProperty(reader, name, Integer.toString(supplier.getAsInt()))); } /** diff --git a/src/main/java/org/apache/commons/xml/Resolvers.java b/src/main/java/org/apache/commons/xml/Resolvers.java index 33867df9..59772cea 100644 --- a/src/main/java/org/apache/commons/xml/Resolvers.java +++ b/src/main/java/org/apache/commons/xml/Resolvers.java @@ -113,8 +113,7 @@ private DenyAll() { /** * Returns an empty input for every external resource lookup so the parse can continue without replacement content. * - *

Only an {@link XMLResolver} flavour is exposed: schema and XSLT compile paths must always deny imports, and SAX/DOM use the deny-all hooks plus - * {@link AndroidProvider}'s subset-aware resolver where needed.

+ *

Only an {@link XMLResolver} flavour is exposed: schema and XSLT compile paths must always deny imports, and SAX/DOM use the deny-all hooks.

*/ static final class IgnoreAll { diff --git a/src/main/java/org/apache/commons/xml/SAXParserHardener.java b/src/main/java/org/apache/commons/xml/SAXParserHardener.java new file mode 100644 index 00000000..fb18d5f8 --- /dev/null +++ b/src/main/java/org/apache/commons/xml/SAXParserHardener.java @@ -0,0 +1,185 @@ +/* + * 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.apache.commons.xml.JaxpSetters.setFeature; +import static org.apache.commons.xml.JaxpSetters.setOptionalFeature; +import static org.apache.commons.xml.JaxpSetters.trySetProperty; + +import java.util.Objects; + +import javax.xml.XMLConstants; +import javax.xml.parsers.SAXParserFactory; + +import org.xml.sax.EntityResolver; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; +import org.xml.sax.SAXNotRecognizedException; +import org.xml.sax.SAXNotSupportedException; +import org.xml.sax.XMLReader; +import org.xml.sax.ext.DefaultHandler2; +import org.xml.sax.ext.LexicalHandler; + +/** + * Capability-driven hardening for any {@link SAXParserFactory} on the classpath. + * + *

Rather than branching on the implementation class, {@link #harden(SAXParserFactory)} probes what the parser supports and adapts. Because + * {@link SAXParserFactory} exposes only a feature API and no property API, the per-parse configuration runs on each {@link XMLReader} the factory produces, + * funnelled through {@link HardeningSAXParserFactory} into {@link #hardenReader(XMLReader)}:

+ *
    + *
  • Android (Harmony / Expat): {@link XMLConstants#FEATURE_SECURE_PROCESSING FSP} and the JAXP 1.5 {@code ACCESS_EXTERNAL_*} properties + * are not recognized, and libexpat enforces its own Billion Laughs check, so neither is applied. Two fixups are still needed: a subset-aware deny-all + * resolver (Expat ignores external fetches silently when no resolver is set, so an explicit one is required to fail on external entities while + * still letting an unused external subset load), and an {@link ExpatReaderWrapper} so the unsupported {@code namespace-prefixes} feature is rejected at + * configuration time rather than mid-parse.
    • + *
  • FSP: required on every other reader. It switches on the implementation's built-in security manager, which is what carries the + * processing limits.
    • + *
  • {@code XERCES_LOAD_EXTERNAL_DTD}: optional. Where supported, it skips the external DTD subset on non-validating parsers so a + * DOCTYPE-only document parses without a fetch attempt. If not supported, the fetch will throw instead, due to the following settings.
    • + *
  • Limits: applied best-effort by {@link Limits#tryApply(XMLReader)}, which adapts to the JDK limit properties or Xerces' + * {@code SecurityManager} as appropriate.
    • + *
  • {@code ACCESS_EXTERNAL_*}: the dividing capability. Readers that honour it (the JDK-internal Xerces) block external fetches through + * the JAXP 1.5 properties and are returned as-is. Readers that reject it (the external Xerces distribution) get a deny-all {@link EntityResolver} + * installed instead.
    • + *
+ */ +final class SAXParserHardener { + + /** + * Resolver that denies every external resource lookup an {@link XMLReader} attempts, except the external DTD subset declared by the DOCTYPE. + * + *

Android's Expat routes every external fetch (subset, DOCTYPE {@code SYSTEM}, general/parameter entity) through the 2-arg + * {@link EntityResolver#resolveEntity(String, String)}; a deny-all resolver there would also reject a DOCTYPE that merely names an unused external + * subset. Tracking the subset's identifiers through the {@link LexicalHandler} DTD events lets this resolver allow exactly that one lookup and throw on + * everything else. It is stateful, so a fresh instance is installed per reader.

+ */ + private static final class DtdAwareDenyResolver extends DefaultHandler2 { + + private static String forbiddenMessage(final String publicId, final String systemId) { + return String.format("External Entity: failed to read external entity (publicId='%s', systemId='%s'); external entity access is denied.", + publicId, systemId); + } + + private String dtdPublicId; + private String dtdSystemId; + private boolean inDtd; + + @Override + public void endDTD() { + inDtd = false; + } + + @Override + public InputSource resolveEntity(final String publicId, final String systemId) throws SAXException { + if (inDtd && Objects.equals(publicId, dtdPublicId) && Objects.equals(systemId, dtdSystemId)) { + return null; + } + throw new SAXException(forbiddenMessage(publicId, systemId)); + } + + @Override + public void startDTD(final String name, final String publicId, final String systemId) { + inDtd = true; + dtdPublicId = publicId; + dtdSystemId = systemId; + } + } + + /** + * Wrapper around Android's {@code org.apache.harmony.xml.ExpatReader} that surfaces its {@code namespace-prefixes} limitation at configuration time. + * + *

ExpatReader does not actually support the {@code namespace-prefixes} feature: enabling it is accepted by {@code setFeature} but fails later, during + * {@code parse}, with a {@link SAXNotSupportedException}. Reporting the rejection eagerly from {@link #setFeature(String, boolean)} lets consumers that probe + * the feature, such as Xalan's identity transformer, catch the exception and fall back instead of failing the whole parse.

+ */ + static final class ExpatReaderWrapper extends DelegatingXMLReader { + + private static final String NAMESPACE_PREFIXES_FEATURE = "http://xml.org/sax/features/namespace-prefixes"; + + ExpatReaderWrapper(final XMLReader delegate) { + super(delegate); + } + + @Override + public void setFeature(final String name, final boolean value) throws SAXNotRecognizedException, SAXNotSupportedException { + if (value && NAMESPACE_PREFIXES_FEATURE.equals(name)) { + throw new SAXNotSupportedException("ExpatReader does not support enabling the '" + NAMESPACE_PREFIXES_FEATURE + "' feature"); + } + super.setFeature(name, value); + } + } + + /** Class name of Android's Harmony-based {@link SAXParserFactory}, backed by the native Expat parser. */ + private static final String ANDROID_SAX_PARSER_FACTORY = "org.apache.harmony.xml.parsers.SAXParserFactoryImpl"; + + /** Class name of Android's Expat-backed {@link XMLReader}. */ + private static final String ANDROID_EXPAT_READER = "org.apache.harmony.xml.ExpatReader"; + + /** SAX property carrying the {@link LexicalHandler}; used to observe the DTD boundary on Android's Expat. */ + private static final String LEXICAL_HANDLER_PROPERTY = "http://xml.org/sax/properties/lexical-handler"; + + /** Xerces feature: load the external DTD subset for non-validating parsers. */ + private static final String XERCES_LOAD_EXTERNAL_DTD = "http://apache.org/xml/features/nonvalidating/load-external-dtd"; + + static SAXParserFactory harden(final SAXParserFactory factory) { + // Required: enables the implementation's security manager, which carries the limits. Android's Expat rejects FSP, so it is skipped there. + if (!ANDROID_SAX_PARSER_FACTORY.equals(factory.getClass().getName())) { + setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true); + } + // The per-parse hardening (limits, entity blocking, Android fixups) lives in hardenReader() because SAXParserFactory has no property API. + return new HardeningSAXParserFactory(factory); + } + + /** + * Capability-driven hardening for a single {@link XMLReader}, whatever produced it (a {@link HardeningSAXParser} product, a Saxon-picked reader or a raw + * reader handed to {@link XmlFactories#harden(XMLReader)}). + */ + static XMLReader hardenReader(final XMLReader reader) { + if (reader instanceof ExpatReaderWrapper) { + // Already hardened (e.g. handed back through XmlFactories.harden(XMLReader)); applying the Expat fixups again would be redundant. + return reader; + } + if (ANDROID_EXPAT_READER.equals(reader.getClass().getName())) { + // Expat ignores external fetches when no resolver is set; install one that fails on external entities but lets an unused external subset load. + final DtdAwareDenyResolver resolver = new DtdAwareDenyResolver(); + reader.setEntityResolver(resolver); + // The resolver needs the DTD-boundary events to tell the subset apart from entities; Expat recognizes the lexical-handler property. + trySetProperty(reader, LEXICAL_HANDLER_PROPERTY, resolver); + // Reject the unsupported namespace-prefixes feature eagerly rather than mid-parse. + return new ExpatReaderWrapper(reader); + } + // Required: enables the JDK XMLSecurityManager / Xerces SecurityManager limits. + setFeature(reader, XMLConstants.FEATURE_SECURE_PROCESSING, true); + // Optional: skip the external DTD subset on non-validating parsers so DOCTYPE-only documents parse without a blocked fetch attempt. + setOptionalFeature(reader, XERCES_LOAD_EXTERNAL_DTD, false); + // Optional, implementation-based: JDK limit properties or Xerces' SecurityManager. + Limits.tryApply(reader); + // ACCESS_EXTERNAL_* support is the dividing capability between JAXP 1.5 implementations and older ones. + if (trySetProperty(reader, XMLConstants.ACCESS_EXTERNAL_DTD, "") + && trySetProperty(reader, XMLConstants.ACCESS_EXTERNAL_SCHEMA, "")) { + // Honoured (stock JDK): the JAXP 1.5 properties block external fetches, so the bare reader is already hardened. + return reader; + } + // Rejected: external Xerces ignores ACCESS_EXTERNAL_*; install a deny-all resolver, the only block. + reader.setEntityResolver(Resolvers.DenyAll.ENTITY2); + return reader; + } + + private SAXParserHardener() { + } +} diff --git a/src/main/java/org/apache/commons/xml/StockJdkProvider.java b/src/main/java/org/apache/commons/xml/StockJdkProvider.java index 99a7d8d2..751c60fe 100644 --- a/src/main/java/org/apache/commons/xml/StockJdkProvider.java +++ b/src/main/java/org/apache/commons/xml/StockJdkProvider.java @@ -51,8 +51,7 @@ * when the subset that would have declared the entity was skipped, so no extra hook is needed. * * - *

SAX hardening lives in {@link #configure(XMLReader)}: {@link SAXParserFactory} has no property API, so the {@link HardeningSAXParserFactory} wrapper - * funnels each produced parser's {@link XMLReader} through that method.

+ *

SAX hardening is not handled here: it is capability-driven across all implementations and lives in {@link SAXParserHardener}.

*/ final class StockJdkProvider { @@ -62,37 +61,10 @@ final class StockJdkProvider { private static final String FEATURE_OVERRIDE_DEFAULT_PARSER = "jdk.xml.overrideDefaultParser"; /** - * Xerces feature: load the external DTD subset for non-validating parsers. - */ - private static final String XERCES_LOAD_EXTERNAL_DTD = "http://apache.org/xml/features/nonvalidating/load-external-dtd"; - - /** - * Zephyr property: skip external DTD subset loading entirely (StAX equivalent of {@link #XERCES_LOAD_EXTERNAL_DTD} {@code = false}). + * Zephyr property: skip external DTD subset loading entirely (StAX equivalent of the Xerces {@code load-external-dtd} feature {@code = false}). */ private static final String ZEPHYR_IGNORE_EXTERNAL_DTD = "http://java.sun.com/xml/stream/properties/ignore-external-dtd"; - static SAXParserFactory configure(final SAXParserFactory factory) { - // Required: enables the JDK XMLSecurityManager limits. - setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true); - // Useful: namespaces should be recognized by default - factory.setNamespaceAware(true); - // The remaining hardening (limits, ACCESS_EXTERNAL_*) lives in the XMLReader configure() because SAXParserFactory has no property API. - return new HardeningSAXParserFactory(factory, StockJdkProvider::configure); - } - - static XMLReader configure(final XMLReader reader) { - // Required: enables the JDK XMLSecurityManager limits on a raw reader (e.g. one Saxon picked). - setFeature(reader, XMLConstants.FEATURE_SECURE_PROCESSING, true); - // Let DOCTYPE-only documents parse silently without SSRF: skip the external DTD subset on non-validating parsers. - setFeature(reader, XERCES_LOAD_EXTERNAL_DTD, false); - // Defense-in-depth: pin to JDK 25 limits so older JDKs do not fall back to looser secure values. - Limits.applyToJdkXmlReader(reader); - // Defense-in-depth: already FSP-secure defaults, set explicitly so they are not relaxed via system property. - setProperty(reader, XMLConstants.ACCESS_EXTERNAL_DTD, ""); - setProperty(reader, XMLConstants.ACCESS_EXTERNAL_SCHEMA, ""); - return reader; - } - static XMLInputFactory configure(final XMLInputFactory factory) { // Required: XMLInputFactory rejects FSP, so the limits below are the only way to enable JDK XMLSecurityManager caps on the StAX path. Limits.applyToJdkStax(factory); diff --git a/src/main/java/org/apache/commons/xml/XercesProvider.java b/src/main/java/org/apache/commons/xml/XercesProvider.java index 4634c9f9..f5ad9c36 100644 --- a/src/main/java/org/apache/commons/xml/XercesProvider.java +++ b/src/main/java/org/apache/commons/xml/XercesProvider.java @@ -20,8 +20,6 @@ import static org.apache.commons.xml.JaxpSetters.setFeature; import javax.xml.XMLConstants; -import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; import javax.xml.validation.Schema; import javax.xml.validation.SchemaFactory; import javax.xml.validation.Validator; @@ -29,13 +27,13 @@ import org.xml.sax.SAXNotRecognizedException; import org.xml.sax.SAXNotSupportedException; -import org.xml.sax.XMLReader; /** * Hardening recipes for the external Apache Xerces distribution (the {@code xerces:xercesImpl} artifact). * *

Factory classes live in the {@code org.apache.xerces.*} package. External Xerces does not ship a {@code TransformerFactory}, {@code XMLInputFactory} or - * {@code XPathFactory}, so this class only handles SAX and Schema factories. DOM hardening lives in {@link DocumentBuilderHardener}.

+ * {@code XPathFactory}, so this class only handles Schema factories; SAX hardening lives in {@link SAXParserHardener} and DOM hardening in + * {@link DocumentBuilderHardener}.

* *

Hardening recipe applied to every factory below uses the same building blocks:

*
    @@ -46,13 +44,10 @@ * API exposes setters for). *
  • *

    {@code HardeningXxx} wrappers + {@link Resolvers.DenyAll}: required. Xerces does not implement the JAXP 1.5 - * {@code ACCESS_EXTERNAL_*} properties, so an explicit resolver installed on every parser/validator is the best way to block external - * entity, DTD and schema fetching, without disabling those features altogether. The wrappers exist for two reasons:

    - *
      - *
    1. {@link SAXParserFactory} carries no resolver, so it has to be set on each {@link SAXParser} produced.
      2. - *
    2. Xerces' {@link Schema} does not propagate the {@link SchemaFactory}'s resolver or security manager to its - * {@link Validator} / {@link ValidatorHandler} products, so the wrapper re-installs both on every product.
      3. - *
    + * {@code ACCESS_EXTERNAL_*} properties, so an explicit resolver installed on every validator is the best way to block external + * schema fetching, without disabling those features altogether. The wrapper exists because Xerces' {@link Schema} does not propagate the + * {@link SchemaFactory}'s resolver or security manager to its {@link Validator} / {@link ValidatorHandler} products, so it re-installs both on + * every product.

    *
    • *
*/ @@ -83,34 +78,6 @@ private static ValidatorHandler hardenValidatorHandler(final ValidatorHandler ha */ static final String XERCES_SECURITY_MANAGER_PROPERTY = "http://apache.org/xml/properties/security-manager"; - /** Xerces feature: load the external DTD subset for non-validating parsers. */ - private static final String XERCES_LOAD_EXTERNAL_DTD = "http://apache.org/xml/features/nonvalidating/load-external-dtd"; - - static SAXParserFactory configure(final SAXParserFactory factory) { - // Required: enables Xerces' built-in SecurityManager (which is what carries the limits). - setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true); - // Useful: namespaces should be recognized by default - factory.setNamespaceAware(true); - // The remaining hardening (limits, entity resolver) lives in the XMLReader configure() because SAXParserFactory has no property API. - return new HardeningSAXParserFactory(factory, XercesProvider::configure); - } - - static XMLReader configure(final XMLReader reader) { - // Required: enables the JDK XMLSecurityManager limits on a raw reader (e.g. one Saxon picked). - setFeature(reader, XMLConstants.FEATURE_SECURE_PROCESSING, true); - // Let DOCTYPE-only documents parse silently without SSRF: skip the external DTD subset on non-validating parsers. - setFeature(reader, XERCES_LOAD_EXTERNAL_DTD, false); - try { - // Defense-in-depth: tighten the SecurityManager Xerces already installed on the reader to JDK 25 limits. - Limits.applyToXerces(reader.getProperty(XERCES_SECURITY_MANAGER_PROPERTY)); - } catch (final SAXNotRecognizedException | SAXNotSupportedException e) { - throw new HardeningException("Failed to read Xerces security manager from XMLReader", e); - } - // Required: Xerces does not honour JAXP 1.5 ACCESS_EXTERNAL_*; the deny-all resolver is the only block. - reader.setEntityResolver(Resolvers.DenyAll.ENTITY2); - return reader; - } - static SchemaFactory configure(final SchemaFactory factory) { // Required: enables Xerces' built-in SecurityManager. setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true); diff --git a/src/main/java/org/apache/commons/xml/XmlFactories.java b/src/main/java/org/apache/commons/xml/XmlFactories.java index e7a79531..6a557387 100644 --- a/src/main/java/org/apache/commons/xml/XmlFactories.java +++ b/src/main/java/org/apache/commons/xml/XmlFactories.java @@ -73,19 +73,6 @@ */ public final class XmlFactories { - private static SAXParserFactory dispatch(final SAXParserFactory factory) { - switch (factory.getClass().getName()) { - case "com.sun.org.apache.xerces.internal.jaxp.SAXParserFactoryImpl": - return StockJdkProvider.configure(factory); - case "org.apache.harmony.xml.parsers.SAXParserFactoryImpl": - return AndroidProvider.configure(factory); - case "org.apache.xerces.jaxp.SAXParserFactoryImpl": - return XercesProvider.configure(factory); - default: - throw noProvider(factory); - } - } - private static XMLInputFactory dispatch(final XMLInputFactory factory) { switch (factory.getClass().getName()) { case "com.sun.xml.internal.stream.XMLInputFactoryImpl": @@ -142,6 +129,8 @@ private static SchemaFactory dispatch(final SchemaFactory factory) { * *

Only {@link StreamSource} and {@link SAXSource} without a reader are enriched with a hardened reader. Other kinds of sources are returned as-is.

* + *

The reader is namespace-aware.

+ * * @param source the source to harden; never {@code null}. * @return a hardened source. * @throws TransformerConfigurationException if a hardened reader cannot be obtained. @@ -149,7 +138,9 @@ private static SchemaFactory dispatch(final SchemaFactory factory) { public static Source harden(final Source source) throws TransformerConfigurationException { if (source instanceof StreamSource || source instanceof SAXSource && ((SAXSource) source).getXMLReader() == null) { try { - final XMLReader reader = newSAXParserFactory().newSAXParser().getXMLReader(); + final SAXParserFactory factory = newSAXParserFactory(); + factory.setNamespaceAware(true); + final XMLReader reader = factory.newSAXParser().getXMLReader(); final InputSource inputSource = SAXSource.sourceToInputSource(source); return inputSource == null ? source : new SAXSource(reader, inputSource); } catch (final ParserConfigurationException | SAXException e) { @@ -164,21 +155,10 @@ public static Source harden(final Source source) throws TransformerConfiguration * * @param reader the reader to harden; never {@code null}. * @return a hardened reader. - * @throws IllegalStateException if the reader's concrete class is not recognized by any bundled hardening recipe, or if the matching recipe cannot apply - * its settings to it. + * @throws IllegalStateException if a required hardening setting cannot be applied to the underlying implementation. */ public static XMLReader harden(final XMLReader reader) { - switch (reader.getClass().getName()) { - case "com.sun.org.apache.xerces.internal.jaxp.SAXParserImpl$JAXPSAXParser": - return StockJdkProvider.configure(reader); - case "org.apache.harmony.xml.ExpatReader": - case "org.apache.commons.xml.AndroidProvider$GuardedXMLReader": - return AndroidProvider.configure(reader); - case "org.apache.xerces.jaxp.SAXParserImpl$JAXPSAXParser": - return XercesProvider.configure(reader); - default: - throw noProvider(reader); - } + return SAXParserHardener.hardenReader(reader); } /** @@ -203,11 +183,10 @@ public static DocumentBuilderFactory newDocumentBuilderFactory() { * an {@code xi:include} element fails.

* * @return a hardened factory. - * @throws IllegalStateException if the underlying JAXP implementation is not recognized by any bundled hardening recipe, or if the matching recipe cannot - * apply its settings to it. + * @throws IllegalStateException if a required hardening setting cannot be applied to the underlying implementation. */ public static SAXParserFactory newSAXParserFactory() { - return dispatch(SAXParserFactory.newInstance()); + return SAXParserHardener.harden(SAXParserFactory.newInstance()); } /** diff --git a/src/test/java/org/apache/commons/xml/AttackTestSupport.java b/src/test/java/org/apache/commons/xml/AttackTestSupport.java index b02dc069..1afc49eb 100644 --- a/src/test/java/org/apache/commons/xml/AttackTestSupport.java +++ b/src/test/java/org/apache/commons/xml/AttackTestSupport.java @@ -732,7 +732,7 @@ private static SAXSource permissiveSaxSource(final String xml) { factory.setNamespaceAware(true); final XMLReader reader = strictXMLReader(factory); suppressException(() -> reader.setProperty(JDK_ENTITY_EXPANSION_LIMIT, "0")); - return new SAXSource(IS_ANDROID ? new AndroidProvider.GuardedXMLReader(reader) : reader, new InputSource(new StringReader(xml))); + return new SAXSource(IS_ANDROID ? new SAXParserHardener.ExpatReaderWrapper(reader) : reader, new InputSource(new StringReader(xml))); } private static boolean probeAndroid() { @@ -854,7 +854,7 @@ private static XMLReader strictXMLReader(final SAXParserFactory factory) { /** * Installs {@link #STRICT_REPORTER} as the error handler on {@code reader} and returns it; for raw-reader paths. */ - private static XMLReader strictXMLReader(final XMLReader reader) { + static XMLReader strictXMLReader(final XMLReader reader) { reader.setErrorHandler(STRICT_REPORTER); return reader; } diff --git a/src/test/java/org/apache/commons/xml/SchemaLocationSaxTest.java b/src/test/java/org/apache/commons/xml/SchemaLocationSaxTest.java new file mode 100644 index 00000000..6939817f --- /dev/null +++ b/src/test/java/org/apache/commons/xml/SchemaLocationSaxTest.java @@ -0,0 +1,128 @@ +/* + * 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.apache.commons.xml.AttackTestSupport.LEAKED_MARKER; +import static org.apache.commons.xml.AttackTestSupport.resourceUrl; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assumptions.assumeTrue; + +import javax.xml.XMLConstants; +import javax.xml.parsers.SAXParser; +import javax.xml.parsers.SAXParserFactory; + +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.xml.sax.Attributes; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; +import org.xml.sax.XMLReader; +import org.xml.sax.helpers.DefaultHandler; + +/** + * Checks that a hardened {@link SAXParserFactory} performing JAXP 1.2 XSD validation does not fetch an external schema named by an + * {@code xsi:noNamespaceSchemaLocation} hint in the instance document. + * + *

This is the SAX counterpart of {@link SchemaLocationDomTest}. The instance is empty {@code }; the referenced schema declares a default {@code leak} + * attribute carrying {@link AttackTestSupport#LEAKED_MARKER}. A parser that fetches the schema augments the element's attributes with that default (the + * permissive control observes it in {@link DefaultHandler#startElement}), while a hardened parser refuses the fetch and fails the parse. The attribution differs + * by implementation (the stock JDK reports an {@code accessExternalSchema} / {@code schema_reference} error; external Xerces' deny-all resolver reports a + * forbidden-fetch error), so the test only asserts the fetch was blocked, not how.

+ * + *

The test runs only where the implementation supports JAXP 1.2 schema-language XSD validation (the stock JDK and external Xerces do; Android does not), so it + * skips on parsers without it.

+ */ +@Tag("sax") +class SchemaLocationSaxTest { + + /** Captures the root element's schema-defaulted {@code leak} attribute, the SAX-visible signal that the external schema was fetched. */ + private static final class LeakCapturingHandler extends DefaultHandler { + private String leak; + + @Override + public void startElement(final String uri, final String localName, final String qName, final Attributes attributes) { + if ("root".equals(localName) || "root".equals(qName)) { + leak = attributes.getValue("leak"); + } + } + } + + /** JAXP 1.2 property selecting the schema language used by {@link SAXParserFactory#setValidating(boolean)}. */ + private static final String SCHEMA_LANGUAGE = "http://java.sun.com/xml/jaxp/properties/schemaLanguage"; + + private static final String INSTANCE = "schema-location-instance.xml"; + + /** Name of the external schema the instance points at; both block mechanisms name it in the failure message. */ + private static final String SCHEMA = "schema-location.xsd"; + + @Test + void hardenedBlocksExternalSchemaFetch() throws Exception { + assumeTrue(supportsSchemaLanguage(), "parser does not support JAXP 1.2 schema-language XSD validation"); + final SAXParser parser = newValidatingParser(XmlFactories.newSAXParserFactory()); + // The schemaLocation fetch is denied and surfaced as an error rather than a silent fetch. The attribution is implementation-specific, so assert only + // that the failure names the external schema, not the mechanism (accessExternalSchema on the JDK, the deny-all resolver on external Xerces). + final SAXException thrown = assertThrows(SAXException.class, () -> parse(parser, new DefaultHandler())); + assertTrue(thrown.getMessage() != null && thrown.getMessage().contains(SCHEMA), + "Block must reference the external schema, got: " + thrown.getMessage()); + } + + @Test + void unconfiguredFetchesExternalSchema() throws Exception { + assumeTrue(supportsSchemaLanguage(), "parser does not support JAXP 1.2 schema-language XSD validation"); + final SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, false); + final SAXParser parser = newValidatingParser(factory); + // Positive control: without hardening the external schema is fetched and its default attribute is augmented onto the root element. + final LeakCapturingHandler handler = new LeakCapturingHandler(); + parse(parser, handler); + assertEquals(LEAKED_MARKER, handler.leak, + "Permissive parse should have fetched the external schema and augmented its default attribute onto the element."); + } + + private static SAXParser newValidatingParser(final SAXParserFactory factory) throws Exception { + factory.setNamespaceAware(true); + factory.setValidating(true); + final SAXParser parser = factory.newSAXParser(); + parser.setProperty(SCHEMA_LANGUAGE, XMLConstants.W3C_XML_SCHEMA_NS_URI); + return parser; + } + + private static void parse(final SAXParser parser, final DefaultHandler handler) throws Exception { + // Drive the XMLReader directly rather than SAXParser.parse(InputSource, DefaultHandler): the latter calls reader.setEntityResolver(handler), which would + // clobber the hardened deny-all resolver that external Xerces relies on to block the schemaLocation fetch. Reuse AttackTestSupport's shared strict + // reporter as the error handler so a blocked fetch surfaces as a thrown exception rather than a silent recovery. + final XMLReader reader = parser.getXMLReader(); + reader.setContentHandler(handler); + AttackTestSupport.strictXMLReader(reader); + reader.parse(new InputSource(resourceUrl(INSTANCE).toString())); + } + + private static boolean supportsSchemaLanguage() { + try { + final SAXParserFactory factory = SAXParserFactory.newInstance(); + factory.setNamespaceAware(true); + factory.setValidating(true); + factory.newSAXParser().setProperty(SCHEMA_LANGUAGE, XMLConstants.W3C_XML_SCHEMA_NS_URI); + return true; + } catch (final Exception e) { + return false; + } + } +} diff --git a/src/test/java/org/apache/commons/xml/UnsupportedXmlImplementationTest.java b/src/test/java/org/apache/commons/xml/UnsupportedXmlImplementationTest.java index ff8d80a2..a6f7aa6f 100644 --- a/src/test/java/org/apache/commons/xml/UnsupportedXmlImplementationTest.java +++ b/src/test/java/org/apache/commons/xml/UnsupportedXmlImplementationTest.java @@ -24,8 +24,12 @@ import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; +import javax.xml.parsers.SAXParser; +import javax.xml.parsers.SAXParserFactory; import org.junit.jupiter.api.Test; +import org.xml.sax.SAXNotRecognizedException; +import org.xml.sax.SAXNotSupportedException; /** * Verifies that an implementation which does not honour the required secure-processing feature surfaces {@link IllegalStateException} with a message naming the @@ -64,6 +68,27 @@ public boolean getFeature(final String name) { } } + /** + * A stand-in SAX factory that rejects the secure-processing feature, like a JAXP implementation that does not recognize it. + */ + public static final class FakeSAXParserFactory extends SAXParserFactory { + + @Override + public SAXParser newSAXParser() { + throw new UnsupportedOperationException(); + } + + @Override + public void setFeature(final String name, final boolean value) throws SAXNotRecognizedException { + throw new SAXNotRecognizedException("feature not recognized: " + name); + } + + @Override + public boolean getFeature(final String name) throws SAXNotSupportedException { + throw new SAXNotSupportedException("feature not recognized: " + name); + } + } + @Test void hardenRejectsUnsecurableFactory() { final IllegalStateException thrown = assertThrows( @@ -73,4 +98,14 @@ void hardenRejectsUnsecurableFactory() { assertTrue(thrown.getMessage().contains(FakeDocumentBuilderFactory.class.getName()), "Exception message must name the unsupported class: " + thrown.getMessage()); } + + @Test + void hardenRejectsUnsecurableSaxFactory() { + final IllegalStateException thrown = assertThrows( + IllegalStateException.class, + () -> SAXParserHardener.harden(new FakeSAXParserFactory())); + assertNotNull(thrown.getMessage()); + assertTrue(thrown.getMessage().contains(FakeSAXParserFactory.class.getName()), + "Exception message must name the unsupported class: " + thrown.getMessage()); + } } From 749b96fab657d6984b3937c62487f4f0f58fddaa Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Sun, 28 Jun 2026 19:08:08 +0200 Subject: [PATCH 2/2] Fix checkstyle errors --- .../java/org/apache/commons/xml/SAXParserHardener.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/apache/commons/xml/SAXParserHardener.java b/src/main/java/org/apache/commons/xml/SAXParserHardener.java index fb18d5f8..9e99ed50 100644 --- a/src/main/java/org/apache/commons/xml/SAXParserHardener.java +++ b/src/main/java/org/apache/commons/xml/SAXParserHardener.java @@ -146,8 +146,11 @@ static SAXParserFactory harden(final SAXParserFactory factory) { } /** - * Capability-driven hardening for a single {@link XMLReader}, whatever produced it (a {@link HardeningSAXParser} product, a Saxon-picked reader or a raw - * reader handed to {@link XmlFactories#harden(XMLReader)}). + * Hardens an existing {@link XMLReader}. + * + * @param reader the reader to harden; never {@code null}. + * @return a hardened reader. + * @throws IllegalStateException if a required hardening setting cannot be applied to the underlying implementation. */ static XMLReader hardenReader(final XMLReader reader) { if (reader instanceof ExpatReaderWrapper) {