JCR-5195: Utilities for 'safe' creation of XML document builders

jackrabbit-core/src/main/java/org/apache/jackrabbit/core/query/lucene/SearchIndex.java

@@ -92,7 +92,6 @@
 import org.apache.lucene.document.Fieldable;
 import org.apache.lucene.index.IndexReader;
 import org.apache.lucene.index.MultiReader;
-import org.apache.lucene.index.Payload;
 import org.apache.lucene.index.Term;
 import org.apache.lucene.index.TermDocs;
 import org.apache.lucene.search.IndexSearcher;

jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/DOMBuilder.java

@@ -16,6 +16,7 @@
  */
 package org.apache.jackrabbit.core.util;
 
+import org.apache.jackrabbit.commons.xml.XMLFactories;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
 
@@ -39,7 +40,7 @@ public final class DOMBuilder {
 
     /** Static factory for creating DOM DocumentBuilder instances. */
     private static final DocumentBuilderFactory BUILDER_FACTORY =
-        DocumentBuilderFactory.newInstance();
+            XMLFactories.safeDocumentBuilderFactory();
 
     /** Static factory for creating document to output stream transformers. */
     private static final TransformerFactory TRANSFORMER_FACTORY =

jackrabbit-core/src/main/java/org/apache/jackrabbit/core/util/DOMWalker.java

@@ -16,22 +16,19 @@
  */
 package org.apache.jackrabbit.core.util;
 
+import org.apache.jackrabbit.commons.xml.XMLFactories;
 import org.w3c.dom.Attr;
 import org.w3c.dom.CharacterData;
 import org.w3c.dom.Document;
 import org.w3c.dom.Element;
 import org.w3c.dom.NamedNodeMap;
 import org.w3c.dom.Node;
 import org.w3c.dom.NodeList;
-import org.xml.sax.EntityResolver;
-import org.xml.sax.InputSource;
 
-import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
 import java.io.IOException;
 import java.io.InputStream;
-import java.io.StringReader;
 import java.util.Properties;
 
 /**
@@ -41,36 +38,7 @@
 public final class DOMWalker {
 
     /** Static factory for creating stream to DOM transformers. */
-    private static final DocumentBuilderFactory factory = createFactory();
-
-    private static DocumentBuilderFactory createFactory() {
-        DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
-        factory.setIgnoringComments(false);
-        factory.setIgnoringElementContentWhitespace(true);
-        factory.setXIncludeAware(false);
-
-        // Prevent XXE attacks by disabling external entity processing
-        factory.setExpandEntityReferences(false);
-
-        String feature = null;
-
-        try {
-            feature = XMLConstants.FEATURE_SECURE_PROCESSING;
-            factory.setFeature(feature, true);
-            feature = "http://apache.org/xml/features/disallow-doctype-decl";
-            factory.setFeature(feature, true);
-            feature = "http://apache.org/xml/features/nonvalidating/load-external-dtd";
-            factory.setFeature(feature, false);
-            feature = "http://xml.org/sax/features/external-general-entities";
-            factory.setFeature(feature, false);
-            feature = "http://xml.org/sax/features/external-parameter-entities";
-            factory.setFeature(feature, false);
-        } catch (Exception ex) {
-            // abort if secure processing is not supported
-            throw new IllegalStateException("Secure processing feature '" + feature + "' not supported by the DocumentBuilderFactory: " + factory.getClass().getName(), ex);
-        }
-        return factory;
-    }
+    private static final DocumentBuilderFactory factory = XMLFactories.safeDocumentBuilderFactory();
 
     /** The DOM document being traversed by this walker. */
     private final Document document;
@@ -90,9 +58,7 @@ public DOMWalker(InputStream xml) throws IOException {
         try {
             DocumentBuilder builder = factory.newDocumentBuilder();
             // defense in depth: entity resolver that will break any document on purpose
-            EntityResolver stopMe = (publicId, systemId) -> new InputSource(
-                    new StringReader("<preventing read of: " + publicId + " " + systemId + ">"));
-            builder.setEntityResolver(stopMe);
+            builder.setEntityResolver(XMLFactories.nonResolvingEntityResolver());
             document = builder.parse(xml);
             current = document.getDocumentElement();
         } catch (IOException e) {

jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/xml/XMLFactories.java

@@ -0,0 +1,77 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.commons.xml;
+
+import org.xml.sax.EntityResolver;
+
+import javax.xml.XMLConstants;
+import javax.xml.parsers.DocumentBuilderFactory;
+import java.io.IOException;
+
+/**
+ * Factory for "safe" instances of XML parsers (wrt XXE etc.).
+ */
+public class XMLFactories {
+
+    private XMLFactories() {
+    }
+
+    /**
+     * @return a "safe" {@link DocumentBuilderFactory}
+     */
+    public static DocumentBuilderFactory safeDocumentBuilderFactory() {
+        // see https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html
+
+        javax.xml.parsers.DocumentBuilderFactory factory = javax.xml.parsers.DocumentBuilderFactory.newInstance();
+
+        factory.setIgnoringComments(false);
+        factory.setIgnoringElementContentWhitespace(true);
+        factory.setXIncludeAware(false);
+
+        // Prevent XXE attacks by disabling external entity processing
+        factory.setExpandEntityReferences(false);
+
+        String feature = null;
+
+        try {
+            feature = XMLConstants.FEATURE_SECURE_PROCESSING;
+            factory.setFeature(feature, true);
+            feature = "http://apache.org/xml/features/disallow-doctype-decl";
+            factory.setFeature(feature, true);
+            feature = "http://apache.org/xml/features/nonvalidating/load-external-dtd";
+            factory.setFeature(feature, false);
+            feature = "http://xml.org/sax/features/external-general-entities";
+            factory.setFeature(feature, false);
+            feature = "http://xml.org/sax/features/external-parameter-entities";
+            factory.setFeature(feature, false);
+        } catch (Exception ex) {
+            // abort if secure processing is not supported
+            throw new IllegalStateException("Secure processing feature '" + feature + "' not supported by the DocumentBuilderFactory: " + factory.getClass().getName(), ex);
+        }
+        return factory;
+    }
+
+    /**
+     * @return an {@link EntityResolver} which will always cause a parse exception.
+     */
+    public static EntityResolver nonResolvingEntityResolver() {
+        return (publicId, systemId) -> {
+            throw new IOException("This parser does not support resolution of external entities (publicId: " + publicId
+                    + ", systemId: " + systemId + ")");
+        };
+    }
+}

jackrabbit-jcr-commons/src/main/java/org/apache/jackrabbit/commons/xml/package-info.java

@@ -14,5 +14,5 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-@org.osgi.annotation.versioning.Version("2.2")
+@org.osgi.annotation.versioning.Version("2.3")
 package org.apache.jackrabbit.commons.xml;

jackrabbit-jcr-commons/src/test/java/org/apache/jackrabbit/xml/XMLFactoriesTest.java

@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.jackrabbit.xml;
+
+import org.apache.jackrabbit.commons.xml.XMLFactories;
+import org.junit.Test;
+import org.w3c.dom.Document;
+import org.xml.sax.EntityResolver;
+import org.xml.sax.SAXParseException;
+
+import javax.xml.parsers.DocumentBuilder;
+import javax.xml.parsers.DocumentBuilderFactory;
+
+import java.io.ByteArrayInputStream;
+import java.io.FileNotFoundException;
+import java.util.UUID;
+
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThrows;
+import static org.junit.Assert.assertTrue;
+
+public class XMLFactoriesTest {
+
+    String randomName = UUID.randomUUID().toString();
+
+    String xxeXml =
+            "<!DOCTYPE root [\n" +
+                    "  <!ELEMENT root ANY >\n" +
+                    "  <!ENTITY xxe SYSTEM \"file:///tmp/" + randomName + "\" >]>\n" +
+                    "<root>&xxe;</root>";
+
+    @Test
+    // verify that default builder is vulnerable
+    public void testDefaultParserDoesResolveExternalEntities() throws Exception {
+        DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
+
+        DocumentBuilder db = dbf.newDocumentBuilder();
+        Document doc = null;
+
+        try (ByteArrayInputStream input = new ByteArrayInputStream(xxeXml.getBytes())) {
+            doc = db.parse(input);
+        } catch (FileNotFoundException expected) {
+            assertTrue("FileNotFoundException message should contain file name '"
+                    + randomName + "', but was: '" + expected.getMessage() + "'", expected.getMessage().contains(randomName));
+        }
+
+        assertNull("There should be no doc: " + doc, doc);
+    }
+
+    @Test
+    // verify that utility builder is not vulnerable
+    public void testParserDoesNotResolveExternalEntities() throws Exception {
+        DocumentBuilderFactory dbf = XMLFactories.safeDocumentBuilderFactory();
+
+        DocumentBuilder db = dbf.newDocumentBuilder();
+
+        try (ByteArrayInputStream input = new ByteArrayInputStream(xxeXml.getBytes())) {
+            db.parse(input);
+        } catch (SAXParseException expected) {
+            // all good
+        }
+    }
+
+    @Test
+    public void testSafeEntityResolver() {
+        EntityResolver test = XMLFactories.nonResolvingEntityResolver();
+        assertThrows(Exception.class, () -> test.resolveEntity("-//The Apache Software Foundation//DTD Jackrabbit 1.6//EN" ,
+                "http://jackrabbit.apache.org/dtd/repository-1.6.dtd"));
+    }
+}

jackrabbit-spi-commons/src/main/java/org/apache/jackrabbit/spi/commons/privilege/PrivilegeXmlHandler.java

@@ -16,6 +16,7 @@
  */
 package org.apache.jackrabbit.spi.commons.privilege;
 
+import org.apache.jackrabbit.commons.xml.XMLFactories;
 import org.apache.jackrabbit.spi.Name;
 import org.apache.jackrabbit.spi.NameFactory;
 import org.apache.jackrabbit.spi.PrivilegeDefinition;
@@ -27,12 +28,10 @@
 import org.w3c.dom.NamedNodeMap;
 import org.w3c.dom.Node;
 import org.w3c.dom.NodeList;
-import org.xml.sax.EntityResolver;
 import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
 import org.xml.sax.helpers.DefaultHandler;
 
-import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
@@ -46,9 +45,7 @@
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.io.Reader;
-import java.io.StringReader;
 import java.io.Writer;
-import java.rmi.server.ExportException;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -109,37 +106,7 @@ private static String createLicenseHeader() {
     /**
      * Constant for <code>DocumentBuilderFactory</code>.
      */
-    private static DocumentBuilderFactory DOCUMENT_BUILDER_FACTORY = createFactory();
-
-    private static DocumentBuilderFactory createFactory() {
-        DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
-        factory.setNamespaceAware(true);
-        factory.setIgnoringComments(false);
-        factory.setIgnoringElementContentWhitespace(true);
-        factory.setXIncludeAware(false);
-
-        // Prevent XXE attacks by disabling external entity processing
-        factory.setExpandEntityReferences(false);
-
-        String feature = null;
-
-        try {
-            feature = XMLConstants.FEATURE_SECURE_PROCESSING;
-            factory.setFeature(feature, true);
-            feature = "http://apache.org/xml/features/disallow-doctype-decl";
-            factory.setFeature(feature, true);
-            feature = "http://apache.org/xml/features/nonvalidating/load-external-dtd";
-            factory.setFeature(feature, false);
-            feature = "http://xml.org/sax/features/external-general-entities";
-            factory.setFeature(feature, false);
-            feature = "http://xml.org/sax/features/external-parameter-entities";
-            factory.setFeature(feature, false);
-        } catch (Exception ex) {
-            // abort if secure processing is not supported
-            throw new IllegalStateException("Secure processing feature '" + feature + "' not supported by the DocumentBuilderFactory: " + factory.getClass().getName(), ex);
-        }
-        return factory;
-    }
+    private static DocumentBuilderFactory DOCUMENT_BUILDER_FACTORY = XMLFactories.safeDocumentBuilderFactory();
 
     /**
      * Constant for <code>TransformerFactory</code>
@@ -306,9 +273,7 @@ private PrivilegeDefinition parseDefinition(Node n, Map<String, String> namespac
     private static DocumentBuilder createDocumentBuilder() throws ParserConfigurationException {
         DocumentBuilder builder = DOCUMENT_BUILDER_FACTORY.newDocumentBuilder();
         // defense in depth: entity resolver that will break any document on purpose
-        EntityResolver stopMe = (publicId, systemId) -> new InputSource(
-                new StringReader("<preventing read of: " + publicId + " " + systemId + ">"));
-        builder.setEntityResolver(stopMe);
+        builder.setEntityResolver(XMLFactories.nonResolvingEntityResolver());
         builder.setErrorHandler(new DefaultHandler());
         return builder;
     }