diff --git a/hazelcast/src/main/java/com/hazelcast/internal/config/AbstractXmlConfigRootTagRecognizer.java b/hazelcast/src/main/java/com/hazelcast/internal/config/AbstractXmlConfigRootTagRecognizer.java index eeb7cf9492b4..92ade5f12a77 100644 --- a/hazelcast/src/main/java/com/hazelcast/internal/config/AbstractXmlConfigRootTagRecognizer.java +++ b/hazelcast/src/main/java/com/hazelcast/internal/config/AbstractXmlConfigRootTagRecognizer.java @@ -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(); } diff --git a/hazelcast/src/main/java/com/hazelcast/internal/util/XmlUtil.java b/hazelcast/src/main/java/com/hazelcast/internal/util/XmlUtil.java index 556cb213cf34..1d23a7d5f004 100644 --- a/hazelcast/src/main/java/com/hazelcast/internal/util/XmlUtil.java +++ b/hazelcast/src/main/java/com/hazelcast/internal/util/XmlUtil.java @@ -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 + * OWASP XXE prevention + * cheat-sheet). */ 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,20 +190,7 @@ 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); } } @@ -187,20 +198,18 @@ static void setFeature(DocumentBuilderFactory dbf, String featureName) throws Pa 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); } } @@ -208,20 +217,36 @@ static void setProperty(SchemaFactory schemaFactory, String propertyName) throws 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 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; } } diff --git a/hazelcast/src/test/java/com/hazelcast/internal/util/XmlUtilTest.java b/hazelcast/src/test/java/com/hazelcast/internal/util/XmlUtilTest.java index 351ff457d3c0..1502490f81e9 100644 --- a/hazelcast/src/test/java/com/hazelcast/internal/util/XmlUtilTest.java +++ b/hazelcast/src/test/java/com/hazelcast/internal/util/XmlUtilTest.java @@ -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(" c", format("c", 1).replaceAll("[\r\n]", "")); @@ -54,9 +98,9 @@ public void testFormat() throws Exception { assertThrows(IllegalArgumentException.class, () -> format("c", 0)); // check if the XXE protection is enabled - String xxeAttack = "\n" + " \n" + " ]>" + "&xxe;"; - assertEquals(xxeAttack, format(xxeAttack, 1)); + String xml = server.getTestXml(); + assertEquals(xml, format(xml, 1)); + assertEquals(0, server.getHits()); // wrongly formatted XML assertEquals("c", format("c", 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 = "\n" + + " \n" + " ]>" + "&xxe;"; + + 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; + } + } + } + } }