This is an automated email from the ASF dual-hosted git repository. ppkarwasz pushed a commit to branch feature/ignore-all-resolver-floors in repository https://gitbox.apache.org/repos/asf/commons-xml.git
commit 7e5b3d429159ae8372c8205c0fff867f8150d4c5 Author: Piotr P. Karwasz <[email protected]> AuthorDate: Sat Aug 1 19:24:24 2026 +0200 Drop XERCES_LOAD_EXTERNAL_DTD from the hardening recipes With the ignore-all floors as universal behavior, skipping the external DTD subset up front is redundant: when the parser requests it, the floor resolves it to empty content and the parse continues. Remove the feature from the DOM and SAX recipes together with the setOptionalFeature helpers it was the only user of. Actually requesting the subset exposed two gaps in the floors, fixed here: both now echo the requested identifiers on the empty source they return (Xerces derives the entity's base URI from the system id and fails on null), and the LSResourceResolver floor hands its empty content over as a character stream, because the JDK's DOMEntityResolverWrapper discards empty string data. The ignore outcome stays best-effort: Saxon's ALLOWED_PROTOCOLS restrictor sits ahead of the floor and rejects the subset lookup outright, so the affected TrAX tests accept either outcome through the assert*BlocksOrDoesNotLeak helpers and DoctypeOnlyTest documents the dual outcome. Assisted-By: Claude Fable 5 <[email protected]> --- .../org/apache/commons/xml/DocumentBuilderHardener.java | 15 --------------- .../apache/commons/xml/FallbackIgnoreEntityResolver2.java | 10 +++++++--- .../commons/xml/FallbackIgnoreLSResourceResolver.java | 10 +++++++++- .../java/org/apache/commons/xml/SAXParserHardener.java | 15 --------------- .../java/org/apache/commons/xml/AttackTestSupport.java | 14 ++++++-------- src/test/java/org/apache/commons/xml/DoctypeOnlyTest.java | 10 +++++++--- src/test/java/org/apache/commons/xml/ExternalDtdTest.java | 6 ++++-- 7 files changed, 33 insertions(+), 47 deletions(-) diff --git a/src/main/java/org/apache/commons/xml/DocumentBuilderHardener.java b/src/main/java/org/apache/commons/xml/DocumentBuilderHardener.java index 6cf9c6a..5f5e7d4 100644 --- a/src/main/java/org/apache/commons/xml/DocumentBuilderHardener.java +++ b/src/main/java/org/apache/commons/xml/DocumentBuilderHardener.java @@ -32,8 +32,6 @@ * FSP}, no JAXP 1.5 {@code ACCESS_EXTERNAL_*} and no attribute API at all, while KXmlParser silently drops user-defined entities, so there is nothing to * apply.</li> * <li><strong>FSP</strong>: required. It switches on the implementation's built-in security manager, which is what carries the processing limits.</li> - * <li><strong>{@code XERCES_LOAD_EXTERNAL_DTD}</strong>: 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.</li> * <li><strong>Ignore-all resolver floor</strong>: every produced {@link DocumentBuilder} is wrapped by a {@link HardeningDocumentBuilderFactory} that keeps an * ignore-all {@link EntityResolver} floor. That floor blocks external DTD, entity, schema and {@code xi:include} fetches in one place: the stock JDK's * XInclude processor ignores {@code ACCESS_EXTERNAL_*} and consults the {@link EntityResolver} instead, so no {@code ACCESS_EXTERNAL_*} attributes are @@ -45,9 +43,6 @@ final class DocumentBuilderHardener { /** Class name of Android's Harmony-based {@link DocumentBuilderFactory}, which exposes no hardening surface. */ private static final String ANDROID_DOCUMENT_BUILDER_FACTORY = "org.apache.harmony.xml.parsers.DocumentBuilderFactoryImpl"; - /** 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 DocumentBuilderFactory harden(final DocumentBuilderFactory factory) { // Android exposes no FSP, ACCESS_EXTERNAL_* or attribute API, and KXmlParser drops user-defined entities; nothing to apply. if (ANDROID_DOCUMENT_BUILDER_FACTORY.equals(factory.getClass().getName())) { @@ -55,8 +50,6 @@ static DocumentBuilderFactory harden(final DocumentBuilderFactory factory) { } // Required: enables the implementation's security manager, which carries the limits. setFeature(factory, 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(factory, XERCES_LOAD_EXTERNAL_DTD, false); // Required: HardeningDocumentBuilderFactory installs an ignore-all EntityResolver floor on every DocumentBuilder. // That floor blocks external DTD, entity, schema and xi:include fetches in one place: no ACCESS_EXTERNAL_* attributes are needed here. // Callers can chain their resolvers, but not override the floor. @@ -71,14 +64,6 @@ private static void setFeature(final DocumentBuilderFactory factory, final Strin } } - private static void setOptionalFeature(final DocumentBuilderFactory factory, final String feature, final boolean value) { - try { - factory.setFeature(feature, value); - } catch (final Exception e) { - // Ignored: the implementation does not recognize this feature. - } - } - private DocumentBuilderHardener() { } } diff --git a/src/main/java/org/apache/commons/xml/FallbackIgnoreEntityResolver2.java b/src/main/java/org/apache/commons/xml/FallbackIgnoreEntityResolver2.java index d2aa6e1..9c4d736 100644 --- a/src/main/java/org/apache/commons/xml/FallbackIgnoreEntityResolver2.java +++ b/src/main/java/org/apache/commons/xml/FallbackIgnoreEntityResolver2.java @@ -107,19 +107,23 @@ public final InputSource resolveEntity(final String name, final String publicId, /** * Outcome when neither the caller delegate nor this resolver provides the entity. Resolves to empty content by default, so the external resource is neither - * fetched nor leaked and the parse continues with no replacement text. + * fetched nor leaked and the parse continues with no replacement text. The returned source echoes the requested identifiers (with {@code systemId} + * absolutized): the parser reads the empty byte stream, but Xerces still derives the entity's base URI from the system id and fails on a {@code null} one. * * @param name The entity name, or {@code null} on the 2-arg resolution path. * @param publicId The public identifier, or {@code null} if none. * @param baseURI The base URI for relative resolution, or {@code null}. * @param systemId The system identifier of the unresolved entity. - * @return An empty {@link InputSource}. + * @return An empty {@link InputSource} carrying the requested identifiers. * @throws SAXException never by the default implementation. * @throws IOException never by the default implementation. */ protected InputSource onUnresolved(final String name, final String publicId, final String baseURI, final String systemId) throws SAXException, IOException { - return new InputSource(new ByteArrayInputStream(new byte[0])); + final InputSource empty = new InputSource(new ByteArrayInputStream(new byte[0])); + empty.setPublicId(publicId); + empty.setSystemId(absolutize(baseURI, systemId)); + return empty; } private InputSource resolveWithDelegate(final String name, final String publicId, final String baseURI, diff --git a/src/main/java/org/apache/commons/xml/FallbackIgnoreLSResourceResolver.java b/src/main/java/org/apache/commons/xml/FallbackIgnoreLSResourceResolver.java index ab2c030..58c6fa0 100644 --- a/src/main/java/org/apache/commons/xml/FallbackIgnoreLSResourceResolver.java +++ b/src/main/java/org/apache/commons/xml/FallbackIgnoreLSResourceResolver.java @@ -17,6 +17,8 @@ package org.apache.commons.xml; +import java.io.StringReader; + import org.w3c.dom.bootstrap.DOMImplementationRegistry; import org.w3c.dom.ls.DOMImplementationLS; import org.w3c.dom.ls.LSInput; @@ -63,8 +65,14 @@ public LSInput resolveResource(final String type, final String namespaceURI, fin if (resolved != null) { return resolved; } + // A character stream, not setStringData(""): the JDK's DOMEntityResolverWrapper discards empty string data, leaving a source with no content and a + // null system id that Xerces then fails to absolutize. The echoed identifiers give Xerces a valid base URI; the content still comes from this + // empty stream, so nothing is fetched. final LSInput empty = DOM_LS.createLSInput(); - empty.setStringData(""); + empty.setCharacterStream(new StringReader("")); + empty.setPublicId(publicId); + empty.setSystemId(systemId); + empty.setBaseURI(baseURI); return empty; } } diff --git a/src/main/java/org/apache/commons/xml/SAXParserHardener.java b/src/main/java/org/apache/commons/xml/SAXParserHardener.java index b609334..4cd784f 100644 --- a/src/main/java/org/apache/commons/xml/SAXParserHardener.java +++ b/src/main/java/org/apache/commons/xml/SAXParserHardener.java @@ -46,8 +46,6 @@ * configuration time rather than mid-parse.</li> * <li><strong>FSP</strong>: required on every other reader. It switches on the implementation's built-in security manager, which is what carries the * processing limits.</li> - * <li><strong>{@code XERCES_LOAD_EXTERNAL_DTD}</strong>: 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 resolver floor below resolves the subset to empty instead.</li> * <li><strong>Ignore-all resolver floor</strong>: every reader is wrapped in a {@link HardeningXMLReader} that keeps an ignore-all {@link EntityResolver} floor. * That floor blocks external DTD, entity, schema and {@code xi:include} fetches in one place: the stock JDK's XInclude processor ignores * {@code ACCESS_EXTERNAL_*} and consults the {@link EntityResolver} instead, so no {@code ACCESS_EXTERNAL_*} properties are needed here. A caller can @@ -87,9 +85,6 @@ public void setFeature(final String name, final boolean value) throws SAXNotReco /** Class name of Android's Expat-backed {@link XMLReader}. */ private static final String ANDROID_EXPAT_READER = "org.apache.harmony.xml.ExpatReader"; - /** 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())) { @@ -119,8 +114,6 @@ static XMLReader hardenReader(final XMLReader 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); // Required: HardeningXMLReader installs an ignore-all EntityResolver floor on the reader. // That floor blocks external DTD, entity, schema and xi:include fetches in one place: no ACCESS_EXTERNAL_* properties are needed here. // Callers can chain their resolvers, but not override the floor. @@ -168,14 +161,6 @@ private static void setFeature(final XMLReader reader, final String feature, fin } } - private 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. - } - } - private SAXParserHardener() { } } diff --git a/src/test/java/org/apache/commons/xml/AttackTestSupport.java b/src/test/java/org/apache/commons/xml/AttackTestSupport.java index 66a2f3a..a7f8413 100644 --- a/src/test/java/org/apache/commons/xml/AttackTestSupport.java +++ b/src/test/java/org/apache/commons/xml/AttackTestSupport.java @@ -65,11 +65,10 @@ * layer is expected to reject the attack outright.</li> * <li>{@code assert*DoesNotLeak(...)} runs the payload through a hardened factory and asserts the parse completes without throwing and without producing the * {@link #LEAKED_MARKER} string. Used when the hardening contract guarantees the parse succeeds but never resolves the external resource (for example, - * {@code XERCES_LOAD_EXTERNAL_DTD=false} silently skipping the external subset, with the body's undeclared entity reference dropped per XML 1.0 - * §4.1).</li> + * the ignore-all resolver floor resolving the external subset to empty content).</li> * <li>{@code assert*BlocksOrDoesNotLeak(...)} accepts either of the previous two outcomes. Used where the same hardening contract surfaces differently across - * providers (for example, stock-JDK XSLTC throws via {@code ACCESS_EXTERNAL_DTD} while Apache Xalan silently skips because its source-rewrite routes parsing - * through a {@code XERCES_LOAD_EXTERNAL_DTD=false} reader).</li> + * providers (for example, an entity declared in the emptied external subset is a fatal error on one implementation and a silently skipped reference on + * another).</li> * </ul> * * <p>DOM tests that depend on user-defined entity machinery should gate themselves with {@link org.junit.jupiter.api.Assumptions#assumeTrue} on @@ -228,7 +227,7 @@ static void assertDomBlocksOrDoesNotLeak(final String payload) { * Asserts a hardened DOM parse completes without throwing and without leaked content. * * <p>{@link DocumentBuilder#parse(InputSource)} via {@link XmlFactories#newDocumentBuilderFactory()}; use this when the hardening guarantee is "the parse - * succeeds but never resolves the external resource", for example, when {@code XERCES_LOAD_EXTERNAL_DTD=false} silently skips the external subset.</p> + * succeeds but never resolves the external resource", for example, when the ignore-all resolver floor resolves the external subset to empty content.</p> */ static void assertDomDoesNotLeak(final String payload) { assertNoLeakStrict(() -> domParseAndCaptureText(payload), "DOM"); @@ -462,7 +461,7 @@ static void assertSaxBlocksOrDoesNotLeak(final String payload) { * Asserts a hardened SAX parse completes without throwing and without leaked content. * * <p>{@link XMLReader#parse(InputSource)} on a parser from {@link XmlFactories#newSAXParserFactory()}; use this when the hardening guarantee is "the parse - * succeeds but never resolves the external resource", for example, when {@code XERCES_LOAD_EXTERNAL_DTD=false} silently skips the external subset.</p> + * succeeds but never resolves the external resource", for example, when the ignore-all resolver floor resolves the external subset to empty content.</p> */ static void assertSaxDoesNotLeak(final String payload) { assertNoLeakStrict(() -> captureCharacters(strictXMLReader(XmlFactories.newSAXParserFactory()), payload), "SAX"); @@ -510,8 +509,7 @@ static void assertSchemaCompiles(final Source xsd) { * Asserts a hardened Schema compilation completes without throwing. * * <p>{@link SchemaFactory#newSchema(Source)} via {@link XmlFactories#newSchemaFactory()}; use this when the hardening contract guarantees the compile - * succeeds but never resolves the external resource (for example, {@code XERCES_LOAD_EXTERNAL_DTD=false} silently skipping the external subset, with the body's - * undeclared entity reference dropped per XML 1.0 §4.1).</p> + * succeeds but never resolves the external resource (for example, the ignore-all resolver floor resolving the external subset to empty content).</p> */ static void assertSchemaDoesNotLeak(final Source xsd) { assertParseSucceeds(() -> strictSchema(XmlFactories.newSchemaFactory(), xsd), "Schema compile"); diff --git a/src/test/java/org/apache/commons/xml/DoctypeOnlyTest.java b/src/test/java/org/apache/commons/xml/DoctypeOnlyTest.java index 1c78212..18d86c0 100644 --- a/src/test/java/org/apache/commons/xml/DoctypeOnlyTest.java +++ b/src/test/java/org/apache/commons/xml/DoctypeOnlyTest.java @@ -27,7 +27,9 @@ * * <p>The SYSTEM identifier points at a deliberately bogus URL ({@code http://invalid.example.invalid/...}) under the IANA-reserved {@code .invalid} TLD: any * attempt to fetch it would raise a network error long before the test could complete, so a passing test proves the parser did not even try. The hardening - * contract being verified is "skip the external DTD silently when nothing in the body needs it" rather than "reject every DOCTYPE".</p> + * contract being verified is best-effort "skip the external DTD silently when nothing in the body needs it": most surfaces parse the document, but an + * implementation-prescribed rejection stays acceptable (Saxon's {@code ALLOWED_PROTOCOLS} restrictor refuses the subset lookup outright), so the TrAX cases + * accept either outcome.</p> */ class DoctypeOnlyTest { @@ -80,13 +82,15 @@ void hardenedStaxParses() { @Test @Tag("trax") void hardenedTemplatesCompiles() { - AttackTestSupport.assertTemplatesCompiles(AttackTestSupport.streamSource(xsltPayload())); + // Saxon prescribes a rejection here: its ALLOWED_PROTOCOLS restrictor refuses the subset lookup before the ignore-all floor can empty it. + AttackTestSupport.assertTemplatesBlocksOrDoesNotLeak(AttackTestSupport.streamSource(xsltPayload())); } @Test @Tag("trax") void hardenedTransformerTransforms() { - AttackTestSupport.assertTransformerTransforms(payload()); + // Saxon prescribes a rejection here: its ALLOWED_PROTOCOLS restrictor refuses the subset lookup before the ignore-all floor can empty it. + AttackTestSupport.assertTransformerBlocksOrDoesNotLeak(payload()); } @Test diff --git a/src/test/java/org/apache/commons/xml/ExternalDtdTest.java b/src/test/java/org/apache/commons/xml/ExternalDtdTest.java index da8184c..cfd014f 100644 --- a/src/test/java/org/apache/commons/xml/ExternalDtdTest.java +++ b/src/test/java/org/apache/commons/xml/ExternalDtdTest.java @@ -89,13 +89,15 @@ void hardenedStaxDoesNotLeak() { @Test @Tag("trax") void hardenedTemplatesDoesNotLeak() { - AttackTestSupport.assertTemplatesDoesNotLeak(AttackTestSupport.streamSource(xsltPayload())); + // Saxon rejects the subset lookup through ALLOWED_PROTOCOLS instead of resolving it to empty; both outcomes keep the resource unfetched. + AttackTestSupport.assertTemplatesBlocksOrDoesNotLeak(AttackTestSupport.streamSource(xsltPayload())); } @Test @Tag("trax") void hardenedTransformerDoesNotLeak() { - AttackTestSupport.assertTransformerDoesNotLeak(xmlPayload()); + // Saxon rejects the subset lookup through ALLOWED_PROTOCOLS instead of resolving it to empty; both outcomes keep the resource unfetched. + AttackTestSupport.assertTransformerBlocksOrDoesNotLeak(xmlPayload()); } @Test
