This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5621-harden-xml-parsers-billion-laughs-s6 in repository https://gitbox.apache.org/repos/asf/struts.git
commit 3f6f5c485d03545f0d0631fdc95879f0c324cafc Author: Lukasz Lenart <[email protected]> AuthorDate: Sun Mar 29 08:15:54 2026 +0200 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 <[email protected]> --- .../com/opensymphony/xwork2/util/DomHelper.java | 2 + .../opensymphony/xwork2/util/DomHelperTest.java | 38 ++++++++++++++-- .../digester/DigesterDefinitionsReader.java | 3 ++ .../digester/TestDigesterDefinitionsReader.java | 23 ++++++++++ .../apache/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 b79c3c03a..6c086a077 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.SAXNotSupportedException; 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 class DomHelper { 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 4c56f5759..4aa2d797b 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.Document; 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 = "<!DOCTYPE foo [<!ELEMENT foo (bar)><!ELEMENT bar (#PCDATA)>]>\n<foo>\n<bar/>\n</foo>\n"; - public void testParse() { + String xml = "<!DOCTYPE foo [<!ELEMENT foo (bar)><!ELEMENT bar (#PCDATA)>]>\n<foo>\n<bar/>\n</foo>\n"; InputSource in = new InputSource(new StringReader(xml)); in.setSystemId("foo://bar"); @@ -47,6 +48,7 @@ public class DomHelperTest extends TestCase { } public void testGetLocationObject() { + String xml = "<!DOCTYPE foo [<!ELEMENT foo (bar)><!ELEMENT bar (#PCDATA)>]>\n<foo>\n<bar/>\n</foo>\n"; InputSource in = new InputSource(new StringReader(xml)); in.setSystemId("foo://bar"); @@ -61,7 +63,7 @@ public class DomHelperTest extends TestCase { } public void testExternalEntities() { - String dtdFile = getClass().getResource("/author.dtd").getPath(); + String dtdFile = Objects.requireNonNull(getClass().getResource("/author.dtd")).getPath(); String xml = "<!DOCTYPE foo [<!ELEMENT foo (bar)><!ELEMENT bar (#PCDATA)><!ENTITY writer SYSTEM \"file://" + dtdFile + "\">]><foo><bar>&writer;</bar></foo>"; InputSource in = new InputSource(new StringReader(xml)); in.setSystemId("foo://bar"); @@ -74,4 +76,34 @@ public class DomHelperTest extends TestCase { 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: <a href="https://en.wikipedia.org/wiki/Billion_laughs_attack";>Billion laughs attack</a> + */ + public void testBillionLaughsProtection() { + String xml = "<?xml version=\"1.0\"?>" + + "<!DOCTYPE root [" + + "<!ENTITY lol0 \"lol\">" + + "<!ENTITY lol1 \"&lol0;&lol0;&lol0;&lol0;&lol0;&lol0;&lol0;&lol0;&lol0;&lol0;\">" + + "<!ENTITY lol2 \"&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;\">" + + "<!ENTITY lol3 \"&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;\">" + + "<!ENTITY lol4 \"&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;\">" + + "<!ENTITY lol5 \"&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;\">" + + "]>" + + "<root>&lol5;</root>"; + + 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 6370b3703..47c076be4 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.SAXNotRecognizedException; 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 class DigesterDefinitionsReader implements DefinitionsReader { // 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 f8a5ee0a3..54841462e 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.apache.tiles.core.definition.DefinitionsFactoryException; 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 class TestDigesterDefinitionsReader { 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 = "<?xml version=\"1.0\"?>" + + "<!DOCTYPE root [" + + "<!ENTITY lol0 \"lol\">" + + "<!ENTITY lol1 \"&lol0;&lol0;&lol0;&lol0;&lol0;&lol0;&lol0;&lol0;&lol0;&lol0;\">" + + "<!ENTITY lol2 \"&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;\">" + + "<!ENTITY lol3 \"&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;\">" + + "<!ENTITY lol4 \"&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;\">" + + "<!ENTITY lol5 \"&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;\">" + + "]>" + + "<root>&lol5;</root>"; + + 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 927019a5a..d1fdfdae6 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 @@ import java.util.List; * * <p> * 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. * </p> */ public class StringAdapter extends AbstractAdapterElement { private static final Logger LOG = LogManager.getLogger(StringAdapter.class); - boolean parseStringAsXML; - public StringAdapter() { } @@ -58,16 +51,11 @@ public class StringAdapter extends AbstractAdapterElement { } /** - * <p> * Get the object to be adapted as a String value. - * </p> * * <p> * 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. * </p> * * @return the string value @@ -78,17 +66,8 @@ public class StringAdapter extends AbstractAdapterElement { @Override protected List<Node> 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<Node> children = new ArrayList<>(); children.add(node); @@ -96,26 +75,23 @@ public class StringAdapter extends AbstractAdapterElement { } /** - * @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 } }
