Merge pull request #543 from scireum/feature/ymo/SIRI-1037-xxe

SIRI-1037 Fix potential XXE vulnerability

src/main/java/sirius/kernel/xml/XMLGenerator.java

@@ -16,6 +16,7 @@
 
 import javax.annotation.Nullable;
 import javax.annotation.ParametersAreNonnullByDefault;
+import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
@@ -146,6 +147,7 @@ public static Document createDocument(@Nullable String namespaceURI,
                                           String qualifiedName,
                                           @Nullable DocumentType docType) throws ParserConfigurationException {
         DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
+        factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
         DocumentBuilder builder = factory.newDocumentBuilder();
         DOMImplementation impl = builder.getDOMImplementation();
         return impl.createDocument(namespaceURI, qualifiedName, docType);

src/main/java/sirius/kernel/xml/XMLReader.java

@@ -17,6 +17,7 @@
 import sirius.kernel.commons.Strings;
 import sirius.kernel.health.Exceptions;
 
+import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
@@ -37,11 +38,11 @@
 import java.util.function.Function;
 
 /**
- * A combination of DOM and SAX parser which permits to parse very large XML files while conveniently handling sub tree
+ * A combination of DOM and SAX parser which permits to parse very large XML files while conveniently handling subtree
  * using a DOM and xpath api.
  * <p>
  * Used SAX to parse a given XML file. A set of {@link NodeHandler} objects can be given, which get notified if
- * a sub-tree below a given tag was parsed. This sub-tree is available as DOM and can conveniently be processed
+ * a subtree below a given tag was parsed. This subtree is available as DOM and can conveniently be processed
  * using xpath.
  */
 public class XMLReader extends DefaultHandler {
@@ -56,14 +57,16 @@ public class XMLReader extends DefaultHandler {
     /**
      * Creates a new XMLReader.
      * <p>
-     * Use {@link #addHandler(String, NodeHandler)} tobind handlers to tags and then call one of the <tt>parse</tt>
+     * Use {@link #addHandler(String, NodeHandler)} to bind handlers to tags and then call one of the <tt>parse</tt>
      * methods to process the XML file.
      * <p>
      * To interrupt processing use {@link TaskContext#cancel()}.
      */
     public XMLReader() {
         try {
-            documentBuilder = DocumentBuilderFactory.newInstance().newDocumentBuilder();
+            DocumentBuilderFactory documentBuilderFactory = DocumentBuilderFactory.newInstance();
+            documentBuilderFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+            documentBuilder = documentBuilderFactory.newDocumentBuilder();
             taskContext = TaskContext.get();
         } catch (ParserConfigurationException exception) {
             throw Exceptions.handle(exception);
@@ -133,15 +136,15 @@ public void startElement(String uri, String localName, String name, Attributes a
      * Registers a new handler for a qualified name of a node.
      * <p>
      * Note that this can be either the node name itself or it can be the path to the node separated by
-     * "/". Therefore &lt;foo&gt;&lt;bar&gt; would be matched by <tt>bar</tt> and by <tt>foo/bar</tt>, where
+     * "/". Therefore, &lt;foo&gt;&lt;bar&gt; would be matched by <tt>bar</tt> and by <tt>foo/bar</tt>, where
      * the path always has precedence over the single node name.
      * <p>
      * Handlers are invoked after the complete node was read. Namespaces are ignored for now which eases
      * the processing a lot (especially for xpath related tasks). Namespaces however
      * could be easily added by replacing String with QName here.
      *
      * @param name    the qualified name of the tag which should be parsed and processed
-     * @param handler the NodeHandler used to process the parsed DOM sub-tree
+     * @param handler the NodeHandler used to process the parsed DOM subtree
      */
     public void addHandler(String name, NodeHandler handler) {
         handlers.put(name, handler);
@@ -159,7 +162,7 @@ public void parse(InputStream stream) throws IOException {
     }
 
     /**
-     * Used to handle the an abort via {@link TaskContext}
+     * Used to handle an abort via {@link TaskContext}
      */
     static class UserInterruptException extends RuntimeException {
 
@@ -178,6 +181,9 @@ static class UserInterruptException extends RuntimeException {
     public void parse(InputStream stream, Function<String, InputStream> resourceLocator) throws IOException {
         try (stream) {
             SAXParserFactory factory = SAXParserFactory.newInstance();
+            factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+            factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+            factory.setXIncludeAware(false);
             SAXParser saxParser = factory.newSAXParser();
             org.xml.sax.XMLReader reader = saxParser.getXMLReader();
             reader.setEntityResolver(new EntityResolver() {

src/main/java/sirius/kernel/xml/XMLStructuredInput.java

@@ -14,6 +14,7 @@
 
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
+import javax.xml.XMLConstants;
 import javax.xml.namespace.NamespaceContext;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
@@ -38,6 +39,7 @@ public class XMLStructuredInput implements StructuredInput {
     public XMLStructuredInput(InputStream inputStream, @Nullable NamespaceContext namespaceContext) throws IOException {
         try {
             DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
+            factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
             if (namespaceContext != null) {
                 factory.setNamespaceAware(true);
             }

src/test/kotlin/sirius/kernel/xml/XmlReaderTest.kt

@@ -9,11 +9,13 @@
 package sirius.kernel.xml
 
 import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.assertThrows
 import org.junit.jupiter.api.extension.ExtendWith
 import sirius.kernel.SiriusExtension
 import sirius.kernel.commons.ValueHolder
 import sirius.kernel.health.Counter
 import java.io.ByteArrayInputStream
+import java.io.IOException
 import kotlin.test.assertEquals
 import kotlin.test.assertFalse
 import kotlin.test.assertTrue
@@ -138,4 +140,24 @@ internal class XmlReaderTest {
         assertEquals(0, attributes.size)
         assertEquals("", attribute.get())
     }
+
+    @Test
+    fun `Reading an external entity is not allowed`() {
+        val readString = ValueHolder.of<String?>(null)
+        val reader = XMLReader()
+        reader.addHandler("root") { node: StructuredNode ->
+            readString.set(node.queryString("."))
+        }
+        assertThrows<IOException> {
+            reader.parse(
+                ByteArrayInputStream(//language=xml
+                    """
+                            <?xml version="1.0" encoding="UTF-8"?> 
+                            <!DOCTYPE root [<!ENTITY xxe SYSTEM "file:///etc/hosts">]> 
+                            <root>&xxe;</root>
+                        """.trimIndent().toByteArray()
+                )
+            )
+        }
+    }
 }