Improper Restriction of XML External Entity Reference in hazelcast/hazelcast

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

@@ -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;
@@ -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 {
 
@@ -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() {
@@ -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;
     }
 
@@ -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
@@ -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;
         }
     }
 

@@ -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;
@@ -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]", ""));
@@ -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));
@@ -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;
+                }
+            }
+        }
+    }
 }