From 3f6f5c485d03545f0d0631fdc95879f0c324cafc Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 29 Mar 2026 08:15:54 +0200 Subject: [PATCH] WW-5621 Harden XML parsers against Entity Expansion (Billion Laughs) attacks Backport of apache/struts#1642 from Struts 7 to Struts 6. Modern JDKs (7u45+) already protect against this attack with a built-in 64K entity expansion limit. These changes add defense-in-depth hardening and remove unnecessary attack surface. - Enable FEATURE_SECURE_PROCESSING in DomHelper SAX parser - Enable FEATURE_SECURE_PROCESSING in DigesterDefinitionsReader - Remove unused parseStringAsXML feature from StringAdapter to eliminate a theoretical XML Entity Expansion vector - Deprecate setParseStringAsXML() and getParseStringAsXML() for removal - Add Billion Laughs protection tests for DomHelper and DigesterDefinitionsReader Co-Authored-By: Claude Opus 4.6 --- .../opensymphony/xwork2/util/DomHelper.java | 2 + .../xwork2/util/DomHelperTest.java | 38 ++++++++++++-- .../digester/DigesterDefinitionsReader.java | 3 ++ .../TestDigesterDefinitionsReader.java | 23 ++++++++ .../struts2/result/xslt/StringAdapter.java | 52 +++++-------------- 5 files changed, 77 insertions(+), 41 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/util/DomHelper.java b/core/src/main/java/com/opensymphony/xwork2/util/DomHelper.java index b79c3c03a3..6c086a0770 100644 --- a/core/src/main/java/com/opensymphony/xwork2/util/DomHelper.java +++ b/core/src/main/java/com/opensymphony/xwork2/util/DomHelper.java @@ -38,6 +38,7 @@ import org.xml.sax.SAXParseException; import org.xml.sax.helpers.DefaultHandler; +import javax.xml.XMLConstants; import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; @@ -104,6 +105,7 @@ public static Document parse(InputSource inputSource, Map dtdMap try { factory.setFeature("http://xml.org/sax/features/external-general-entities", false); factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false); + factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); } catch (ParserConfigurationException | SAXNotRecognizedException | SAXNotSupportedException e) { throw new StrutsException("Unable to disable resolving external entities!", e); } diff --git a/core/src/test/java/com/opensymphony/xwork2/util/DomHelperTest.java b/core/src/test/java/com/opensymphony/xwork2/util/DomHelperTest.java index 4c56f57597..4aa2d797b6 100644 --- a/core/src/test/java/com/opensymphony/xwork2/util/DomHelperTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/util/DomHelperTest.java @@ -24,17 +24,18 @@ import org.w3c.dom.Element; import org.w3c.dom.NodeList; import org.xml.sax.InputSource; +import org.xml.sax.SAXParseException; import java.io.StringReader; +import java.util.Objects; /** * Test cases for {@link DomHelper}. */ public class DomHelperTest extends TestCase { - private final String xml = "]>\n\n\n\n"; - public void testParse() { + String xml = "]>\n\n\n\n"; InputSource in = new InputSource(new StringReader(xml)); in.setSystemId("foo://bar"); @@ -47,6 +48,7 @@ public void testParse() { } public void testGetLocationObject() { + String xml = "]>\n\n\n\n"; InputSource in = new InputSource(new StringReader(xml)); in.setSystemId("foo://bar"); @@ -61,7 +63,7 @@ public void testGetLocationObject() { } public void testExternalEntities() { - String dtdFile = getClass().getResource("/author.dtd").getPath(); + String dtdFile = Objects.requireNonNull(getClass().getResource("/author.dtd")).getPath(); String xml = "]>&writer;"; InputSource in = new InputSource(new StringReader(xml)); in.setSystemId("foo://bar"); @@ -74,4 +76,34 @@ public void testExternalEntities() { assertEquals(1, nl.getLength()); assertNull(nl.item(0).getNodeValue()); } + + /** + * Tests that the parser is protected against Billion Laughs (XML Entity Expansion) attack. + * The FEATURE_SECURE_PROCESSING flag and the JDK's built-in entity expansion limit (64K + * since JDK 7u45) both cap entity expansion to prevent DoS. + * See: Billion laughs attack + */ + public void testBillionLaughsProtection() { + String xml = "" + + "" + + "" + + "" + + "" + + "" + + "" + + "]>" + + "&lol5;"; + + InputSource in = new InputSource(new StringReader(xml)); + in.setSystemId("test://billion-laughs"); + + try { + DomHelper.parse(in); + fail("Parser should reject excessive entity expansion"); + } catch (Exception e) { + assertNotNull(e.getCause()); + assertTrue(e.getCause() instanceof SAXParseException); + } + } } diff --git a/plugins/tiles/src/main/java/org/apache/tiles/core/definition/digester/DigesterDefinitionsReader.java b/plugins/tiles/src/main/java/org/apache/tiles/core/definition/digester/DigesterDefinitionsReader.java index 6370b37030..47c076be47 100644 --- a/plugins/tiles/src/main/java/org/apache/tiles/core/definition/digester/DigesterDefinitionsReader.java +++ b/plugins/tiles/src/main/java/org/apache/tiles/core/definition/digester/DigesterDefinitionsReader.java @@ -35,6 +35,7 @@ import org.xml.sax.SAXNotSupportedException; import org.xml.sax.SAXParseException; +import javax.xml.XMLConstants; import javax.xml.parsers.ParserConfigurationException; import java.io.IOException; import java.io.InputStream; @@ -164,6 +165,8 @@ public DigesterDefinitionsReader() { // Disable external DTDs as well digester.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); digester.setXIncludeAware(false); + // Enable secure processing to limit entity expansion (prevents Billion Laughs attack) + digester.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true); } catch (ParserConfigurationException | SAXNotRecognizedException | SAXNotSupportedException e) { throw new StrutsException("Unable to disable external XML entity parsing", e); } diff --git a/plugins/tiles/src/test/java/org/apache/tiles/core/definition/digester/TestDigesterDefinitionsReader.java b/plugins/tiles/src/test/java/org/apache/tiles/core/definition/digester/TestDigesterDefinitionsReader.java index f8a5ee0a3e..54841462e2 100644 --- a/plugins/tiles/src/test/java/org/apache/tiles/core/definition/digester/TestDigesterDefinitionsReader.java +++ b/plugins/tiles/src/test/java/org/apache/tiles/core/definition/digester/TestDigesterDefinitionsReader.java @@ -26,9 +26,11 @@ import org.junit.Before; import org.junit.Test; +import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; import java.net.URL; +import java.nio.charset.StandardCharsets; import java.util.List; import java.util.Map; @@ -268,6 +270,27 @@ public void testReadNoSource() { assertNull(reader.read(null)); } + /** + * Tests that the Digester parser is protected against Billion Laughs (XML Entity Expansion) attack. + * FEATURE_SECURE_PROCESSING is enabled in DigesterDefinitionsReader to limit entity expansion. + */ + @Test(expected = DefinitionsFactoryException.class) + public void testBillionLaughsProtection() { + String xml = "" + + "" + + "" + + "" + + "" + + "" + + "" + + "]>" + + "&lol5;"; + + InputStream source = new ByteArrayInputStream(xml.getBytes(StandardCharsets.UTF_8)); + reader.read(source); + } + /** * Tests {@link DigesterDefinitionsReader#addDefinition(Definition)}. */ diff --git a/plugins/xslt/src/main/java/org/apache/struts2/result/xslt/StringAdapter.java b/plugins/xslt/src/main/java/org/apache/struts2/result/xslt/StringAdapter.java index 927019a5ad..d1fdfdae6e 100644 --- a/plugins/xslt/src/main/java/org/apache/struts2/result/xslt/StringAdapter.java +++ b/plugins/xslt/src/main/java/org/apache/struts2/result/xslt/StringAdapter.java @@ -18,13 +18,10 @@ */ package org.apache.struts2.result.xslt; -import com.opensymphony.xwork2.util.DomHelper; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.w3c.dom.Node; -import org.xml.sax.InputSource; -import java.io.StringReader; import java.util.ArrayList; import java.util.List; @@ -39,17 +36,13 @@ * *

* Subclasses may override the getStringValue() method in order to use StringAdapter - * as a simplified custom XML adapter for Java types. A subclass can enable XML - * parsing of the value string via the setParseStringAsXML() method and then - * override getStringValue() to return a String containing the custom formatted XML. + * as a simplified custom XML adapter for Java types. *

*/ public class StringAdapter extends AbstractAdapterElement { private static final Logger LOG = LogManager.getLogger(StringAdapter.class); - boolean parseStringAsXML; - public StringAdapter() { } @@ -58,16 +51,11 @@ public StringAdapter(AdapterFactory adapterFactory, AdapterNode parent, String p } /** - *

* Get the object to be adapted as a String value. - *

* *

* This method can be overridden by subclasses that wish to use StringAdapter - * as a simplified customizable XML adapter for Java types. A subclass can - * enable parsing of the value string as containing XML text via the - * setParseStringAsXML() method and then override getStringValue() to return a - * String containing the custom formatted XML. + * as a simplified customizable XML adapter for Java types. *

* * @return the string value @@ -78,17 +66,8 @@ protected String getStringValue() { @Override protected List buildChildAdapters() { - Node node; - if (getParseStringAsXML()) { - LOG.debug("parsing string as xml: {}", getStringValue()); - // Parse the String to a DOM, then proxy that as our child - node = DomHelper.parse(new InputSource(new StringReader(getStringValue()))); - node = getAdapterFactory().proxyNode(this, node); - } else { - LOG.debug("using string as is: {}", getStringValue()); - // Create a Text node as our child - node = new SimpleTextNode(getAdapterFactory(), this, "text", getStringValue()); - } + LOG.debug("using string as is: {}", getStringValue()); + Node node = new SimpleTextNode(getAdapterFactory(), this, "text", getStringValue()); List children = new ArrayList<>(); children.add(node); @@ -96,26 +75,23 @@ protected List buildChildAdapters() { } /** - * @return is this StringAdapter to interpret its string values as containing - * XML Text? - * - * @see #setParseStringAsXML(boolean) + * @return always returns false + * @deprecated This feature has been removed for security reasons (potential XML Entity Expansion attacks). + * This method now always returns false and will be removed in a future version. */ + @Deprecated public boolean getParseStringAsXML() { - return parseStringAsXML; + return false; } /** - * When set to true the StringAdapter will interpret its String value - * as containing XML text and parse it to a DOM Element. The new DOM - * Element will be a child of this String element. (i.e. wrapped in an - * element of the property name specified for this StringAdapter). - * - * @param parseStringAsXML when set to true the StringAdapter will interpret its String value as containing XML text - * @see #getParseStringAsXML() + * @param parseStringAsXML ignored + * @deprecated This feature has been removed for security reasons (potential XML Entity Expansion attacks). + * This method is now a no-op and will be removed in a future version. */ + @Deprecated public void setParseStringAsXML(boolean parseStringAsXML) { - this.parseStringAsXML = parseStringAsXML; + // no-op - feature removed for security reasons } }