Skip to content
Permalink
Browse files Browse the repository at this point in the history
Add helper method to XmlUtil to enable XXE protection in the SAXParse…
…rFactory and XMLInputFactory (#20407)
  • Loading branch information
kwart committed Jan 19, 2022
1 parent 7e7f00b commit 4d6b666
Show file tree
Hide file tree
Showing 3 changed files with 213 additions and 48 deletions.
Expand Up @@ -18,6 +18,7 @@

import com.hazelcast.config.ConfigRecognizer;
import com.hazelcast.config.ConfigStream;
import com.hazelcast.internal.util.XmlUtil;
import com.hazelcast.logging.ILogger;
import com.hazelcast.logging.Logger;
import org.xml.sax.Attributes;
Expand Down Expand Up @@ -54,7 +55,7 @@ public class AbstractXmlConfigRootTagRecognizer implements ConfigRecognizer {

public AbstractXmlConfigRootTagRecognizer(String expectedRootNode) throws Exception {
this.expectedRootNode = expectedRootNode;
SAXParserFactory factory = SAXParserFactory.newInstance();
SAXParserFactory factory = XmlUtil.getSAXParserFactory();
saxParser = factory.newSAXParser();
}

Expand Down
113 changes: 69 additions & 44 deletions hazelcast/src/main/java/com/hazelcast/internal/util/XmlUtil.java
Expand Up @@ -25,6 +25,8 @@
import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.parsers.SAXParserFactory;
import javax.xml.stream.XMLInputFactory;
import javax.xml.transform.ErrorListener;
import javax.xml.transform.OutputKeys;
import javax.xml.transform.Source;
Expand All @@ -41,7 +43,10 @@
import com.hazelcast.logging.Logger;

/**
* Utility class for XML processing.
* Utility class for XML processing. It contains several methods to retrieve XML processing factories with XXE protection
* enabled (based on recommendation in the
* <a href="https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html">OWASP XXE prevention
* cheat-sheet</a>).
*/
public final class XmlUtil {

Expand All @@ -55,6 +60,7 @@ public final class XmlUtil {
*/
public static final String SYSTEM_PROPERTY_IGNORE_XXE_PROTECTION_FAILURES = "hazelcast.ignoreXxeProtectionFailures";

private static final String FEATURES_DISALLOW_DOCTYPE = "http://apache.org/xml/features/disallow-doctype-decl";
private static final ILogger LOGGER = Logger.getLogger(XmlUtil.class);

private XmlUtil() {
Expand All @@ -68,7 +74,7 @@ private XmlUtil() {
public static DocumentBuilderFactory getNsAwareDocumentBuilderFactory() throws ParserConfigurationException {
DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
dbf.setNamespaceAware(true);
setFeature(dbf, "http://apache.org/xml/features/disallow-doctype-decl");
setFeature(dbf, FEATURES_DISALLOW_DOCTYPE);
return dbf;
}

Expand All @@ -92,6 +98,24 @@ public static SchemaFactory getSchemaFactory() throws SAXException {
return schemaFactory;
}

/**
* Returns {@link SAXParserFactory} with XXE protection enabled.
*/
public static SAXParserFactory getSAXParserFactory() throws ParserConfigurationException, SAXException {
SAXParserFactory factory = SAXParserFactory.newInstance();
setFeature(factory, FEATURES_DISALLOW_DOCTYPE);
return factory;
}

/**
* Returns {@link XMLInputFactory} with XXE protection enabled.
*/
public static XMLInputFactory getXMLInputFactory() {
XMLInputFactory xmlInputFactory = XMLInputFactory.newInstance();
setProperty(xmlInputFactory, XMLInputFactory.SUPPORT_DTD, false);
return xmlInputFactory;
}

/**
* Formats given XML String with the given indentation used. If the {@code input} XML string is {@code null}, or
* {@code indent} parameter is negative, or XML transformation fails, then the original value is returned unchanged. The
Expand Down Expand Up @@ -166,62 +190,63 @@ static void setAttribute(TransformerFactory transformerFactory, String attribute
try {
transformerFactory.setAttribute(attributeName, "");
} catch (IllegalArgumentException iae) {
if (Boolean.getBoolean(SYSTEM_PROPERTY_IGNORE_XXE_PROTECTION_FAILURES)) {
LOGGER.warning("Enabling XXE protection failed. The attribute " + attributeName
+ " is not supported by the TransformerFactory. The " + SYSTEM_PROPERTY_IGNORE_XXE_PROTECTION_FAILURES
+ " system property is used so the XML processing continues in the UNSECURE mode"
+ " with XXE protection disabled!!!");
} else {
LOGGER.severe("Enabling XXE protection failed. The attribute " + attributeName
+ " is not supported by the TransformerFactory. This usually mean an outdated XML processor"
+ " is present on the classpath (e.g. Xerces, Xalan). If you are not able to resolve the issue by"
+ " fixing the classpath, the " + SYSTEM_PROPERTY_IGNORE_XXE_PROTECTION_FAILURES
+ " system property can be used to disable XML External Entity protections."
+ " We don't recommend disabling the XXE as such the XML processor configuration is unsecure!!!", iae);
throw iae;
}
printWarningAndRethrowEventually(iae, TransformerFactory.class, "attribute " + attributeName);
}
}

static void setFeature(DocumentBuilderFactory dbf, String featureName) throws ParserConfigurationException {
try {
dbf.setFeature(featureName, true);
} catch (ParserConfigurationException e) {
if (Boolean.getBoolean(SYSTEM_PROPERTY_IGNORE_XXE_PROTECTION_FAILURES)) {
LOGGER.warning("Enabling XXE protection failed. The feature " + featureName
+ " is not supported by the DocumentBuilderFactory. The " + SYSTEM_PROPERTY_IGNORE_XXE_PROTECTION_FAILURES
+ " system property is used so the XML processing continues in the UNSECURE mode"
+ " with XXE protection disabled!!!");
} else {
LOGGER.severe("Enabling XXE protection failed. The feature " + featureName
+ " is not supported by the DocumentBuilderFactory. This usually mean an outdated XML processor"
+ " is present on the classpath (e.g. Xerces, Xalan). If you are not able to resolve the issue by"
+ " fixing the classpath, the " + SYSTEM_PROPERTY_IGNORE_XXE_PROTECTION_FAILURES
+ " system property can be used to disable XML External Entity protections."
+ " We don't recommend disabling the XXE as such the XML processor configuration is unsecure!!!", e);
throw e;
}
printWarningAndRethrowEventually(e, DocumentBuilderFactory.class, "feature " + featureName);
}
}

static void setFeature(SAXParserFactory saxParserFactory, String featureName)
throws ParserConfigurationException, SAXException {
try {
saxParserFactory.setFeature(featureName, true);
} catch (SAXException e) {
printWarningAndRethrowEventually(e, SAXParserFactory.class, "feature " + featureName);
} catch (ParserConfigurationException e) {
printWarningAndRethrowEventually(e, SAXParserFactory.class, "feature " + featureName);
}
}

static void setProperty(SchemaFactory schemaFactory, String propertyName) throws SAXException {
try {
schemaFactory.setProperty(propertyName, "");
} catch (SAXException e) {
if (Boolean.getBoolean(SYSTEM_PROPERTY_IGNORE_XXE_PROTECTION_FAILURES)) {
LOGGER.warning("Enabling XXE protection failed. The property " + propertyName
+ " is not supported by the SchemaFactory. The " + SYSTEM_PROPERTY_IGNORE_XXE_PROTECTION_FAILURES
+ " system property is used so the XML processing continues in the UNSECURE mode"
+ " with XXE protection disabled!!!");
} else {
LOGGER.severe("Enabling XXE protection failed. The property " + propertyName
+ " is not supported by the SchemaFactory. This usually mean an outdated XML processor"
+ " is present on the classpath (e.g. Xerces, Xalan). If you are not able to resolve the issue by"
+ " fixing the classpath, the " + SYSTEM_PROPERTY_IGNORE_XXE_PROTECTION_FAILURES
+ " system property can be used to disable XML External Entity protections."
+ " We don't recommend disabling the XXE as such the XML processor configuration is unsecure!!!", e);
throw e;
}
printWarningAndRethrowEventually(e, SchemaFactory.class, "property " + propertyName);
}
}

static void setProperty(XMLInputFactory xmlInputFactory, String propertyName, Object value) {
try {
xmlInputFactory.setProperty(propertyName, value);
} catch (IllegalArgumentException e) {
printWarningAndRethrowEventually(e, XMLInputFactory.class, "property " + propertyName);
}
}

private static <T extends Exception> void printWarningAndRethrowEventually(T cause, Class<?> clazz, String objective)
throws T {
String className = clazz.getSimpleName();
if (Boolean.getBoolean(SYSTEM_PROPERTY_IGNORE_XXE_PROTECTION_FAILURES)) {
LOGGER.warning("Enabling XXE protection failed. The " + objective + " is not supported by the " + className
+ ". The " + SYSTEM_PROPERTY_IGNORE_XXE_PROTECTION_FAILURES
+ " system property is used so the XML processing continues in the UNSECURE mode"
+ " with XXE protection disabled!!!");
} else {
LOGGER.severe(
"Enabling XXE protection failed. The " + objective + " is not supported by the " + className
+ ". This usually mean an outdated XML processor"
+ " is present on the classpath (e.g. Xerces, Xalan). If you are not able to resolve the issue by"
+ " fixing the classpath, the " + SYSTEM_PROPERTY_IGNORE_XXE_PROTECTION_FAILURES
+ " system property can be used to disable XML External Entity protections."
+ " We don't recommend disabling the XXE as such the XML processor configuration is unsecure!",
cause);
throw cause;
}
}

Expand Down
145 changes: 142 additions & 3 deletions hazelcast/src/test/java/com/hazelcast/internal/util/XmlUtilTest.java
Expand Up @@ -18,19 +18,39 @@

import static com.hazelcast.internal.util.XmlUtil.SYSTEM_PROPERTY_IGNORE_XXE_PROTECTION_FAILURES;
import static com.hazelcast.internal.util.XmlUtil.format;
import static java.nio.charset.StandardCharsets.UTF_8;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertThrows;

import java.io.IOException;
import java.io.StringReader;
import java.net.InetAddress;
import java.net.ServerSocket;
import java.net.Socket;
import java.util.concurrent.atomic.AtomicInteger;

import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.parsers.SAXParser;
import javax.xml.parsers.SAXParserFactory;
import javax.xml.stream.XMLEventReader;
import javax.xml.stream.XMLInputFactory;
import javax.xml.stream.XMLStreamException;
import javax.xml.transform.TransformerFactory;
import javax.xml.validation.SchemaFactory;

import org.fusesource.hawtbuf.ByteArrayInputStream;
import org.hamcrest.Matchers;
import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.experimental.categories.Category;
import org.junit.runner.RunWith;
import org.xml.sax.HandlerBase;
import org.xml.sax.SAXException;

import com.hazelcast.test.HazelcastSerialClassRunner;
Expand All @@ -45,6 +65,30 @@ public class XmlUtilTest {
public OverridePropertyRule ignoreXxeFailureProp = OverridePropertyRule
.clear(SYSTEM_PROPERTY_IGNORE_XXE_PROTECTION_FAILURES);

private DummyServer server;

@Before
public void before() throws IOException {
server = new DummyServer();
server.start();
}

@After
public void after() {
server.stop();
}

@Test
public void testUnprotectedXxe() throws Exception {
DocumentBuilder db = DocumentBuilderFactory.newInstance().newDocumentBuilder();
try {
db.parse(new ByteArrayInputStream(server.getTestXml().getBytes(UTF_8)));
} catch (Exception e) {
// not important if it fails
}
assertThat(server.getHits(), Matchers.greaterThan(0));
}

@Test
public void testFormat() throws Exception {
assertEquals("<a> <b>c</b></a>", format("<a><b>c</b></a>", 1).replaceAll("[\r\n]", ""));
Expand All @@ -54,9 +98,9 @@ public void testFormat() throws Exception {
assertThrows(IllegalArgumentException.class, () -> format("<a><b>c</b></a>", 0));

// check if the XXE protection is enabled
String xxeAttack = "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n" + " <!DOCTYPE test [\n"
+ " <!ENTITY xxe SYSTEM \"file:///etc/passwd\">\n" + " ]>" + "<a><b>&xxe;</b></a>";
assertEquals(xxeAttack, format(xxeAttack, 1));
String xml = server.getTestXml();
assertEquals(xml, format(xml, 1));
assertEquals(0, server.getHits());

// wrongly formatted XML
assertEquals("<a><b>c</b><a>", format("<a><b>c</b><a>", 1));
Expand Down Expand Up @@ -94,4 +138,99 @@ public void testGetDocumentBuilderFactory() throws Exception {
ignoreXxeFailureProp.setOrClearProperty("true");
XmlUtil.setFeature(dbf, "test://no-such-feature");
}

@SuppressWarnings("deprecation")
@Test
public void testGetSAXParserFactory() throws Exception {
SAXParserFactory saxParserFactory = XmlUtil.getSAXParserFactory();
assertNotNull(saxParserFactory);
// check if the XXE protection is enabled
SAXParser saxParser = saxParserFactory.newSAXParser();
assertThrows(SAXException.class,
() -> saxParser.parse(new ByteArrayInputStream(server.getTestXml().getBytes(UTF_8)), new HandlerBase()));
assertEquals(0, server.getHits());

assertThrows(SAXException.class, () -> XmlUtil.setFeature(saxParserFactory, "test://no-such-feature"));
ignoreXxeFailureProp.setOrClearProperty("false");
assertThrows(SAXException.class, () -> XmlUtil.setFeature(saxParserFactory, "test://no-such-feature"));
ignoreXxeFailureProp.setOrClearProperty("true");
XmlUtil.setFeature(saxParserFactory, "test://no-such-feature");
}

@Test
public void testGetXmlInputFactory() throws Exception {
XMLInputFactory xmlInputFactory = XmlUtil.getXMLInputFactory();
assertNotNull(xmlInputFactory);
// check if the XXE protection is enabled
assertThrows(XMLStreamException.class,
() -> staxReadEvents(xmlInputFactory.createXMLEventReader(new StringReader(server.getTestXml()))));
assertEquals(0, server.getHits());

assertThrows(IllegalArgumentException.class,
() -> XmlUtil.setProperty(xmlInputFactory, "test://no-such-property", false));
ignoreXxeFailureProp.setOrClearProperty("false");
assertThrows(IllegalArgumentException.class,
() -> XmlUtil.setProperty(xmlInputFactory, "test://no-such-property", false));
ignoreXxeFailureProp.setOrClearProperty("true");
XmlUtil.setProperty(xmlInputFactory, "test://no-such-feature", false);
}

private void staxReadEvents(XMLEventReader reader) throws XMLStreamException {
try {
while (reader.hasNext()) {
reader.nextEvent();
}
} finally {
reader.close();
}
}

static class DummyServer implements Runnable {
private static final String XXE_TEST_STR_TEMPLATE = "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n"
+ " <!DOCTYPE test [\n" + " <!ENTITY xxe SYSTEM \"%s\">\n" + " ]>" + "<a><b>&xxe;</b></a>";

private final ServerSocket serverSocket;
private final AtomicInteger counter = new AtomicInteger();

DummyServer() throws IOException {
serverSocket = new ServerSocket(0, 5, InetAddress.getLoopbackAddress());
}

public void start() {
new Thread(this, "DummyServer-acceptor").start();
}

public String getUrlString() {
return "http://127.0.0.1:" + serverSocket.getLocalPort();
}

public String getTestXml() {
return String.format(XXE_TEST_STR_TEMPLATE, getUrlString());
}

public int getHits() {
return counter.get();
}

public void stop() {
try {
serverSocket.close();
} catch (IOException e) {
e.printStackTrace();
}
}

@Override
public void run() {
while (true) {
try (Socket socket = serverSocket.accept()) {
System.out.println(">> connection accepted: " + socket);
counter.incrementAndGet();
} catch (Exception e) {
System.out.println(">> stopping the server. Cause: " + e.getClass().getName());
return;
}
}
}
}
}

0 comments on commit 4d6b666

Please sign in to comment.