Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -104,6 +105,7 @@ public static Document parse(InputSource inputSource, Map<String, String> 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);
}
Expand Down
38 changes: 35 additions & 3 deletions core/src/test/java/com/opensymphony/xwork2/util/DomHelperTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "<!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");

Expand All @@ -47,6 +48,7 @@ public void testParse() {
}

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");

Expand All @@ -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 = "<!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");
Expand All @@ -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: <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);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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 = "<?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)}.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -39,17 +36,13 @@
*
* <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() {
}

Expand All @@ -58,16 +51,11 @@ public StringAdapter(AdapterFactory adapterFactory, AdapterNode parent, String p
}

/**
* <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
Expand All @@ -78,44 +66,32 @@ protected String getStringValue() {

@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);
return children;
}

/**
* @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
}

}
Loading