diff --git a/src/conf/spotbugs-exclude-filter.xml b/src/conf/spotbugs-exclude-filter.xml index 095ba0e..24038fd 100644 --- a/src/conf/spotbugs-exclude-filter.xml +++ b/src/conf/spotbugs-exclude-filter.xml @@ -19,9 +19,9 @@ xmlns="https://github.com/spotbugs/filter/3.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="https://github.com/spotbugs/filter/3.0.0 https://raw.githubusercontent.com/spotbugs/spotbugs/3.1.0/spotbugs/etc/findbugsfilter.xsd"> - + - + diff --git a/src/main/java/org/apache/commons/xml/JaxpSetters.java b/src/main/java/org/apache/commons/xml/JaxpSetters.java index 9d0ee32..6f24c5c 100644 --- a/src/main/java/org/apache/commons/xml/JaxpSetters.java +++ b/src/main/java/org/apache/commons/xml/JaxpSetters.java @@ -37,15 +37,14 @@ */ final class JaxpSetters { - private static final String KIND_PROPERTY = "property"; - private static final String KIND_FEATURE = "feature"; - private static final String KIND_ATTRIBUTE = "attribute"; - /** Action that may throw any exception; used to share a single try/catch around every JAXP setter. */ @FunctionalInterface private interface ThrowingAction { void run() throws Exception; } + private static final String KIND_ATTRIBUTE = "attribute"; + private static final String KIND_FEATURE = "feature"; + private static final String KIND_PROPERTY = "property"; private static void apply(final Object factory, final String kind, final String name, final ThrowingAction action) { try { @@ -59,28 +58,6 @@ static void setAttribute(final DocumentBuilderFactory factory, final String attr apply(factory, KIND_ATTRIBUTE, attribute, () -> factory.setAttribute(attribute, value)); } - /** - * Sets an attribute on a {@link DocumentBuilderFactory} and returns whether the implementation accepted it. Some implementations may reject certain - * attributes, in which case this method will return {@code false}. - * - * @param factory The target factory on which to set the attribute. - * @param attribute The name of the attribute to set. - * @param value The value of the attribute to set. - * @return {@code true} if the attribute was applied, {@code false} if the implementation rejected it. - */ - static boolean trySetAttribute(final DocumentBuilderFactory factory, final String attribute, final Object value) { - try { - factory.setAttribute(attribute, value); - return true; - } catch (final Exception e) { - return false; - } - } - - static void setOptionalAttribute(final DocumentBuilderFactory factory, final String attribute, final Object value) { - trySetAttribute(factory, attribute, value); - } - static void setAttribute(final TransformerFactory factory, final String attribute, final Object value) { apply(factory, KIND_ATTRIBUTE, attribute, () -> factory.setAttribute(attribute, value)); } @@ -89,14 +66,6 @@ static void setFeature(final DocumentBuilderFactory factory, final String featur apply(factory, KIND_FEATURE, feature, () -> factory.setFeature(feature, value)); } - 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. - } - } - static void setFeature(final SAXParserFactory factory, final String feature, final boolean value) { apply(factory, KIND_FEATURE, feature, () -> factory.setFeature(feature, value)); } @@ -125,6 +94,18 @@ static void setFeature(final XMLReader reader, final String feature, final boole apply(reader, KIND_FEATURE, feature, () -> reader.setFeature(feature, value)); } + static void setOptionalAttribute(final DocumentBuilderFactory factory, final String attribute, final Object value) { + trySetAttribute(factory, attribute, value); + } + + 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. + } + } + static void setOptionalFeature(final XMLReader reader, final String feature, final boolean value) { try { reader.setFeature(feature, value); @@ -133,6 +114,10 @@ static void setOptionalFeature(final XMLReader reader, final String feature, fin } } + static void setOptionalProperty(final XMLInputFactory factory, final String property, final Object value) { + trySetProperty(factory, property, value); + } + static void setProperty(final XMLInputFactory factory, final String property, final Object value) { apply(factory, KIND_PROPERTY, property, () -> factory.setProperty(property, value)); } @@ -174,6 +159,41 @@ static void setProperty(final ValidatorHandler handler, final String property, f apply(handler, KIND_PROPERTY, property, () -> handler.setProperty(property, value)); } + /** + * Sets an attribute on a {@link DocumentBuilderFactory} and returns whether the implementation accepted it. Some implementations may reject certain + * attributes, in which case this method will return {@code false}. + * + * @param factory The target factory on which to set the attribute. + * @param attribute The name of the attribute to set. + * @param value The value of the attribute to set. + * @return {@code true} if the attribute was applied, {@code false} if the implementation rejected it. + */ + static boolean trySetAttribute(final DocumentBuilderFactory factory, final String attribute, final Object value) { + try { + factory.setAttribute(attribute, value); + return true; + } catch (final Exception e) { + return false; + } + } + + /** + * Sets a property on an {@link XMLInputFactory} and returns whether the implementation accepted it. + * + * @param factory The target factory 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 XMLInputFactory factory, final String property, final Object value) { + try { + factory.setProperty(property, value); + return true; + } catch (final Exception e) { + return false; + } + } + private JaxpSetters() { } } diff --git a/src/main/java/org/apache/commons/xml/Limits.java b/src/main/java/org/apache/commons/xml/Limits.java index df97150..216f604 100644 --- a/src/main/java/org/apache/commons/xml/Limits.java +++ b/src/main/java/org/apache/commons/xml/Limits.java @@ -242,26 +242,6 @@ final class Limits { JDK_LIMITS = Collections.unmodifiableMap(map); } - /** - * Best-effort application of the processing limits to a {@link DocumentBuilderFactory}, dispatched on the implementation. - * - *

External Xerces carries its limits on an {@code org.apache.xerces.util.SecurityManager} instance. Every other implementation (the stock JDK and any - * future attribute-based parser) takes the JDK limit attributes. Neither path throws if the implementation declines a limit.

- * - * @param factory The target factory to modify. - */ - static void tryApply(final DocumentBuilderFactory factory) { - if (EXTERNAL_XERCES_DOCUMENT_BUILDER_FACTORY.equals(factory.getClass().getName())) { - // Install a fresh SecurityManager pinned to JDK 25 limits, replacing Xerces' built-in caps which are looser than even JDK 8. - final Object securityManager = newSecurityManager(); - applyToXerces(securityManager); - setAttribute(factory, XercesProvider.XERCES_SECURITY_MANAGER_PROPERTY, securityManager); - return; - } - // Pin the JDK attribute limits to JDK 25 secure values; skip silently any attribute the implementation does not recognize. - JDK_LIMITS.forEach((name, supplier) -> setOptionalAttribute(factory, name, Integer.toString(supplier.getAsInt()))); - } - /** * Sets every JDK-supported limit on a stock JDK {@link SchemaFactory}. * @@ -271,15 +251,6 @@ static void applyToJdkSchema(final SchemaFactory factory) { JDK_LIMITS.forEach((name, supplier) -> setProperty(factory, name, Integer.toString(supplier.getAsInt()))); } - /** - * Sets every JDK-supported limit on the stock JDK's {@link XMLInputFactory}. - * - * @param factory The target factory to modify. - */ - static void applyToJdkStax(final XMLInputFactory factory) { - JDK_LIMITS.forEach((name, supplier) -> setProperty(factory, name, Integer.toString(supplier.getAsInt()))); - } - /** * Sets every JDK-supported limit on a stock JDK {@link TransformerFactory}. * @@ -313,17 +284,6 @@ static void tryApply(final XMLReader reader) { JDK_LIMITS.forEach((name, supplier) -> trySetProperty(reader, name, Integer.toString(supplier.getAsInt()))); } - /** - * Sets every JDK-supported limit on a Woodstox {@link XMLInputFactory}. - * - * @param factory The target factory to modify. - */ - static void applyToWoodstox(final XMLInputFactory factory) { - setProperty(factory, WSTX_MAX_ENTITY_COUNT, getEntityExpansionLimit()); - setProperty(factory, WSTX_MAX_ATTRIBUTES_PER_ELEMENT, getElementAttributeLimit()); - setProperty(factory, WSTX_MAX_ELEMENT_DEPTH, getMaxElementDepth()); - } - /** * Sets every JDK-supported limit on a Xerces {@code org.apache.xerces.util.SecurityManager}. * @@ -398,6 +358,41 @@ private static int read(final String systemPropertyName, final int defaultValue) } } + /** + * Best-effort application of the processing limits to a {@link DocumentBuilderFactory}, dispatched on the implementation. + * + *

External Xerces carries its limits on an {@code org.apache.xerces.util.SecurityManager} instance. Every other implementation (the stock JDK and any + * future attribute-based parser) takes the JDK limit attributes. Neither path throws if the implementation declines a limit.

+ * + * @param factory The target factory to modify. + */ + static void tryApply(final DocumentBuilderFactory factory) { + if (EXTERNAL_XERCES_DOCUMENT_BUILDER_FACTORY.equals(factory.getClass().getName())) { + // Install a fresh SecurityManager pinned to JDK 25 limits, replacing Xerces' built-in caps which are looser than even JDK 8. + final Object securityManager = newSecurityManager(); + applyToXerces(securityManager); + setAttribute(factory, XercesProvider.XERCES_SECURITY_MANAGER_PROPERTY, securityManager); + return; + } + // Pin the JDK attribute limits to JDK 25 secure values; skip silently any attribute the implementation does not recognize. + JDK_LIMITS.forEach((name, supplier) -> setOptionalAttribute(factory, name, Integer.toString(supplier.getAsInt()))); + } + + /** + * Best-effort application of the processing limits to an {@link XMLInputFactory}, regardless of implementation. + * + *

The JDK's Zephyr honours the JDK URL limit properties; Woodstox honours its own {@code com.ctc.wstx.*} properties. Each implementation rejects the + * other's, so both sets are applied best-effort and the rejected ones are skipped silently.

+ * + * @param factory The target factory to modify. + */ + static void tryApply(final XMLInputFactory factory) { + JDK_LIMITS.forEach((name, supplier) -> trySetProperty(factory, name, Integer.toString(supplier.getAsInt()))); + trySetProperty(factory, WSTX_MAX_ENTITY_COUNT, getEntityExpansionLimit()); + trySetProperty(factory, WSTX_MAX_ATTRIBUTES_PER_ELEMENT, getElementAttributeLimit()); + trySetProperty(factory, WSTX_MAX_ELEMENT_DEPTH, getMaxElementDepth()); + } + private Limits() { } } diff --git a/src/main/java/org/apache/commons/xml/StaxHardener.java b/src/main/java/org/apache/commons/xml/StaxHardener.java new file mode 100644 index 0000000..378edc1 --- /dev/null +++ b/src/main/java/org/apache/commons/xml/StaxHardener.java @@ -0,0 +1,92 @@ +/* + * 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.setOptionalProperty; +import static org.apache.commons.xml.JaxpSetters.trySetProperty; + +import javax.xml.stream.XMLInputFactory; +import javax.xml.stream.XMLResolver; +import javax.xml.stream.XMLStreamException; + +/** + * Capability-driven hardening for any {@link XMLInputFactory} (StAX) on the classpath. + * + *

Rather than branching on the implementation class, {@link #harden(XMLInputFactory)} consolidates the JDK Zephyr and Woodstox recipes into one pass that + * probes which properties each factory accepts and adapts:

+ * + */ +final class StaxHardener { + + /** Zephyr property: skip external DTD subset loading entirely, so a DOCTYPE-only document parses without a fetch attempt. */ + private static final String ZEPHYR_IGNORE_EXTERNAL_DTD = "http://java.sun.com/xml/stream/properties/ignore-external-dtd"; + + /** Woodstox property: resolver consulted for the external DTD subset. */ + private static final String WSTX_DTD_RESOLVER = "com.ctc.wstx.dtdResolver"; + + /** Woodstox property: resolver consulted for declared external general entities. */ + private static final String WSTX_ENTITY_RESOLVER = "com.ctc.wstx.entityResolver"; + + /** Woodstox property: resolver consulted for undeclared entity references. */ + private static final String WSTX_UNDECLARED_ENTITY_RESOLVER = "com.ctc.wstx.undeclaredEntityResolver"; + + /** + * Hybrid Woodstox DTD resolver: returns the empty input for the external DTD subset, throws on external parameter entities. + * + *

Woodstox calls this hook with {@code entityName == null} for the subset and {@code entityName != null} for parameter-entity expansion; that + * discriminator is Woodstox-specific (the JDK Zephyr's {@code XMLResolver} always receives {@code null} as the 4th argument), so the resolver lives + * here and is applied best-effort, ignored by implementations that do not recognize the property.

+ */ + static final XMLResolver DTD_SUBSET_ONLY = (publicID, systemID, baseURI, entityName) -> { + if (entityName != null) { + throw new XMLStreamException("External parameter entity '" + entityName + "' refused (publicID=" + publicID + ", systemID=" + systemID + + ", baseURI=" + baseURI + ")"); + } + return Resolvers.IgnoreAll.XML.resolveEntity(publicID, systemID, baseURI, entityName); + }; + + static XMLInputFactory harden(final XMLInputFactory factory) { + // Optional, implementation-based: JDK limit properties or Woodstox limit properties. + Limits.tryApply(factory); + // Optional: Zephyr's StAX equivalent of XERCES_LOAD_EXTERNAL_DTD=false skips the external DTD subset entirely. + setOptionalProperty(factory, ZEPHYR_IGNORE_EXTERNAL_DTD, true); + + // Woodstox-specific fine-grained resolvers + if (!(trySetProperty(factory, WSTX_DTD_RESOLVER, DTD_SUBSET_ONLY) + && trySetProperty(factory, WSTX_ENTITY_RESOLVER, Resolvers.DenyAll.XML) + && trySetProperty(factory, WSTX_UNDECLARED_ENTITY_RESOLVER, Resolvers.IgnoreAll.XML))) { + // Fallback: use deny-all resolver + factory.setXMLResolver(Resolvers.DenyAll.XML); + } + return factory; + } + + private StaxHardener() { + } +} diff --git a/src/main/java/org/apache/commons/xml/StockJdkProvider.java b/src/main/java/org/apache/commons/xml/StockJdkProvider.java index 751c60f..7bd4ced 100644 --- a/src/main/java/org/apache/commons/xml/StockJdkProvider.java +++ b/src/main/java/org/apache/commons/xml/StockJdkProvider.java @@ -23,7 +23,6 @@ import javax.xml.XMLConstants; import javax.xml.parsers.SAXParserFactory; -import javax.xml.stream.XMLInputFactory; import javax.xml.transform.TransformerFactory; import javax.xml.transform.sax.SAXTransformerFactory; import javax.xml.validation.SchemaFactory; @@ -42,16 +41,11 @@ * bundled SAX parser instead of {@link SAXParserFactory#newInstance()}, blocking a sysprop swap to a third-party parser. Defense-in-depth. *
  • FSP ({@link XMLConstants#FEATURE_SECURE_PROCESSING}, set to {@code true}): switches the JDK's {@code XMLSecurityManager} into secure * mode, which is what enables the JDK-side processing limits in the first place. Required.
    • - *
  • {@code Limits.applyToJdk*}: required on {@link XMLInputFactory} (it rejects FSP); elsewhere defense-in-depth, pinning the limits to - * JDK 25 secure values so older JDKs do not fall back to looser defaults.
    • + *
  • {@code Limits.applyToJdk*}: defense-in-depth, pinning the limits to JDK 25 secure values so older JDKs do not fall back to looser + * defaults.
    • *
  • {@code ACCESS_EXTERNAL_*}: already the FSP-secure default but set to {@code ""} explicitly so a sysprop ({@code - * javax.xml.accessExternal*}) cannot loosen them. {@link XMLInputFactory} has no equivalent property, so the StAX path pairs Zephyr's - * {@value #ZEPHYR_IGNORE_EXTERNAL_DTD} property (skip the external DTD subset, lets DOCTYPE-only documents parse) with {@link Resolvers.DenyAll#XML} - * (throw on declared external entity references). Undeclared general-entity references are silently dropped: Zephyr does not raise a fatal error - * when the subset that would have declared the entity was skipped, so no extra hook is needed.
    • + * javax.xml.accessExternal*}) cannot loosen them. * - * - *

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

    */ final class StockJdkProvider { @@ -60,21 +54,6 @@ final class StockJdkProvider { */ private static final String FEATURE_OVERRIDE_DEFAULT_PARSER = "jdk.xml.overrideDefaultParser"; - /** - * 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 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); - // Let DOCTYPE-only documents parse silently: Zephyr's StAX equivalent of XERCES_LOAD_EXTERNAL_DTD=false skips the external DTD subset entirely. - factory.setProperty(ZEPHYR_IGNORE_EXTERNAL_DTD, true); - // Required: XMLInputFactory has no ACCESS_EXTERNAL_* either; an explicit deny-all resolver is the only way to block external entity fetching. - factory.setXMLResolver(Resolvers.DenyAll.XML); - return factory; - } - static TransformerFactory configure(final TransformerFactory factory) { // Required: enables XSLTC's runtime evaluator limits (entity expansion, attribute count, element/name depth). setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true); diff --git a/src/main/java/org/apache/commons/xml/WoodstoxProvider.java b/src/main/java/org/apache/commons/xml/WoodstoxProvider.java deleted file mode 100644 index 1e6624c..0000000 --- a/src/main/java/org/apache/commons/xml/WoodstoxProvider.java +++ /dev/null @@ -1,88 +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 javax.xml.stream.XMLInputFactory; -import javax.xml.stream.XMLResolver; -import javax.xml.stream.XMLStreamException; - -/** - * Hardening recipe for the FasterXML Woodstox StAX implementation ({@code com.ctc.wstx:woodstox-core}). - * - *

    Woodstox is a StAX-only library, so this class only handles {@link XMLInputFactory}.

    - * - *

    Hardening recipe used below:

    - * - */ -final class WoodstoxProvider { - - /** Woodstox property: resolver consulted for the external DTD subset and for external parameter entities. */ - private static final String WSTX_DTD_RESOLVER = "com.ctc.wstx.dtdResolver"; - - /** Woodstox property: resolver consulted for declared external general entities. */ - private static final String WSTX_ENTITY_RESOLVER = "com.ctc.wstx.entityResolver"; - - /** Woodstox property: resolver consulted for undeclared entity references. */ - private static final String WSTX_UNDECLARED_ENTITY_RESOLVER = "com.ctc.wstx.undeclaredEntityResolver"; - - /** - * Hybrid Woodstox DTD resolver: returns the empty input for the external DTD subset, throws on external parameter entities. - * - *

    Woodstox calls this hook with {@code entityName == null} for the subset and {@code entityName != null} for parameter-entity expansion; that - * discriminator is Woodstox-specific (the JDK Zephyr's {@code XMLResolver} always receives {@code null} as the 4th argument), so the resolver lives - * here rather than in {@link Resolvers}.

    - */ - static final XMLResolver DTD_SUBSET_ONLY = (publicID, systemID, baseURI, entityName) -> { - if (entityName != null) { - throw new XMLStreamException("External parameter entity '" + entityName + "' refused (publicID=" + publicID + ", systemID=" + systemID - + ", baseURI=" + baseURI + ")"); - } - return Resolvers.IgnoreAll.XML.resolveEntity(publicID, systemID, baseURI, entityName); - }; - - static XMLInputFactory configure(final XMLInputFactory factory) { - // Defense-in-depth: align Woodstox's built-in caps with the JDK 25 secure values; Woodstox's own defaults are functional but looser. - Limits.applyToWoodstox(factory); - // Required: empty external subset, throw on external parameter entities. - factory.setProperty(WSTX_DTD_RESOLVER, DTD_SUBSET_ONLY); - // Required: throw on declared external general entities. - factory.setProperty(WSTX_ENTITY_RESOLVER, Resolvers.DenyAll.XML); - // Required: silently drop undeclared entity references, matching the SAX path's tolerance. - factory.setProperty(WSTX_UNDECLARED_ENTITY_RESOLVER, Resolvers.IgnoreAll.XML); - return factory; - } - - private WoodstoxProvider() { - } -} diff --git a/src/main/java/org/apache/commons/xml/XmlFactories.java b/src/main/java/org/apache/commons/xml/XmlFactories.java index 6a55738..53bcfd2 100644 --- a/src/main/java/org/apache/commons/xml/XmlFactories.java +++ b/src/main/java/org/apache/commons/xml/XmlFactories.java @@ -73,17 +73,6 @@ */ public final class XmlFactories { - private static XMLInputFactory dispatch(final XMLInputFactory factory) { - switch (factory.getClass().getName()) { - case "com.sun.xml.internal.stream.XMLInputFactoryImpl": - return StockJdkProvider.configure(factory); - case "com.ctc.wstx.stax.WstxInputFactory": - return WoodstoxProvider.configure(factory); - default: - throw noProvider(factory); - } - } - private static TransformerFactory dispatch(final TransformerFactory factory) { switch (factory.getClass().getName()) { case "com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl": @@ -233,11 +222,10 @@ public static TransformerFactory newTransformerFactory() { *

    The three universal guarantees on {@link XmlFactories} apply; StAX exposes no additional vectors beyond them.

    * * @return a hardened factory. - * @throws IllegalStateException if the underlying StAX 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 XMLInputFactory newXMLInputFactory() { - return dispatch(XMLInputFactory.newInstance()); + return StaxHardener.harden(XMLInputFactory.newInstance()); } /**