Harden XML parsing via commons-secure-xml

pom.xml

@@ -87,6 +87,11 @@
       <artifactId>commons-lang3</artifactId>
       <version>${commons.lang3.version}</version>
     </dependency>
+    <dependency>
+      <groupId>org.apache.commons</groupId>
+      <artifactId>commons-secure-xml</artifactId>
+      <version>1.0.0</version>
+    </dependency>
     <!-- testing -->
     <dependency>
       <groupId>org.junit.jupiter</groupId>

src/changes/changes.xml

@@ -72,7 +72,10 @@ The <action> type attribute can be add,update,fix,remove.
       <action type="fix" dev="ggregory" due-to="Javid Khan, Gary Gregory">Fix ArrayIndexOutOfBoundsException in DnsStringLookup for '|' key (#765).</action>
       <action type="fix" dev="ggregory" due-to="Weiki, Gary Gregory">Compare characters by value in DamerauLevenshteinDistance (#770).</action>
       <action type="fix" dev="ggregory" due-to="Jeff Lenamon, Gary Gregory">StringMatcher.isMatch(CharSequence, int, int, int) forwards bufferEnd as bufferStart (#768).</action>
+      <action type="fix" dev="pkarwasz" due-to="Piotr P. Karwasz, Gary Gregory">XmlStringLookup resolves relative system identifiers against the XML document, instead of the current working directory.</action>
       <!-- ADD -->
+      <action type="add" dev="pkarwasz" due-to="Piotr P. Karwasz, Gary Gregory">XmlStringLookup creates its XML parser and XPath factories through Commons Secure XML.</action>
+      <action type="add" dev="pkarwasz" due-to="Piotr P. Karwasz, Gary Gregory">XmlStringLookup resolves external DTD subsets and entities from its Path fences.</action>
       <!-- UPDATE -->
       <action type="update" dev="ggregory" due-to="Gary Gregory">Bump org.apache.commons:commons-parent from 93 to 105.</action>
       <action type="update" dev="ggregory" due-to="Gary Gregory">Bump the level of test coverage checks.</action>

src/main/java/org/apache/commons/text/lookup/PathFence.java

@@ -94,7 +94,7 @@ private PathFence(final Builder builder) {
      */
     Path apply(final String fileName) {
         final Path path = Paths.get(fileName);
-        if (roots.isEmpty()) {
+        if (isEmpty()) {
             return path;
         }
         final Path pathAbs = normalize(path);
@@ -105,6 +105,15 @@ Path apply(final String fileName) {
         throw new IllegalArgumentException(String.format("[%s] -> [%s] not in the fence %s", fileName, pathAbs, roots));
     }
 
+    /**
+     * Tests whether this fence has no roots, in which case {@link #apply(String)} lets every path through.
+     *
+     * @return whether this fence has no roots.
+     */
+    boolean isEmpty() {
+        return roots.isEmpty();
+    }
+
     private Path normalize(final Path path) {
         return path.toAbsolutePath().normalize();
     }

src/main/java/org/apache/commons/text/lookup/PathFenceResolver.java

@@ -0,0 +1,107 @@
+/*
+ * 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
+ *
+ *      https://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.commons.text.lookup;
+
+import java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Objects;
+
+import org.apache.commons.lang3.StringUtils;
+import org.xml.sax.EntityResolver;
+import org.xml.sax.InputSource;
+import org.xml.sax.SAXException;
+import org.xml.sax.SAXParseException;
+
+/**
+ * Resolves the external resources of an XML document from within a {@link PathFence}.
+ * <p>
+ * A document fetched from a fence may legitimately reference a follow-up resource, an external DTD subset or an external entity, living in the same roots.
+ * </p>
+ *
+ * Keep package-private.
+ */
+final class PathFenceResolver implements EntityResolver {
+
+    /**
+     * Converts a system identifier to a local Path.
+     *
+     * @param systemId An absolute 'file:' URI, may be null.
+     * @return A Path, or null if the system identifier is empty.
+     * @throws SAXException If the system identifier is not a valid `file:` URI.
+     */
+    private static Path toPath(final String systemId) throws SAXException {
+        if (StringUtils.isEmpty(systemId)) {
+            return null;
+        }
+        try {
+            final URI uri = new URI(systemId);
+            if (!"file".equals(uri.getScheme())) {
+                throw new SAXParseException("Failed to read external document '" + systemId + "', because only 'file' access is allowed.", null, systemId, -1,
+                        -1);
+            }
+            return Paths.get(uri);
+        } catch (final URISyntaxException | IllegalArgumentException e) {
+            throw new SAXParseException("Failed to read external document '" + systemId + "'.", null, systemId, -1, -1, e);
+        }
+    }
+
+    /**
+     * A fence is made of Paths guarding Path resolution.
+     */
+    private final PathFence fence;
+
+    /**
+     * Constructs a new instance.
+     *
+     * @param fence The fence guarding Path resolution.
+     */
+    PathFenceResolver(final PathFence fence) {
+        this.fence = Objects.requireNonNull(fence, "fence");
+    }
+
+    /**
+     * Resolves an external resource, opting it in when it resolves within our fence.
+     *
+     * @param publicId The public identifier, may be null.
+     * @param systemId The system identifier, already absolutized by the caller, may be null.
+     * @return An InputSource on the resource.
+     * @throws SAXException if the system identifier names a file outside our fence.
+     * @throws IOException  if the resource cannot be read.
+     */
+    @Override
+    public InputSource resolveEntity(final String publicId, final String systemId) throws SAXException, IOException {
+        final Path path = toPath(systemId);
+        if (path == null) {
+            return null;
+        }
+        final Path fenced;
+        try {
+            fenced = fence.apply(path.toString());
+        } catch (final IllegalArgumentException e) {
+            throw new SAXException(e);
+        }
+        final InputSource inputSource = new InputSource(Files.newInputStream(fenced));
+        inputSource.setPublicId(publicId);
+        inputSource.setSystemId(systemId);
+        return inputSource;
+    }
+}

src/main/java/org/apache/commons/text/lookup/StringLookupFactory.java

@@ -30,6 +30,7 @@
 import java.util.function.Function;
 import java.util.function.Supplier;
 
+import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.xpath.XPathFactory;
 
@@ -1614,8 +1615,19 @@ public StringLookup xmlEncoderStringLookup() {
     /**
      * Returns an XML StringLookup instance.
      * <p>
-     * If this factory was built using {@link Builder#setFences(Path...)}, then the string lookup is fenced and will throw an {@link IllegalArgumentException}
-     * if a lookup causes a path to resolve outside of these fences. Otherwise, the result is unfenced to preserved behavior from previous versions.
+     * XML files are parsed using Commons Secure XML, which enables the {@link XMLConstants#FEATURE_SECURE_PROCESSING} processing limits and ignores external
+     * DTD subsets and entities by default.
+     * </p>
+     * <p>
+     * If this factory was built using {@link Builder#setFences}, then the string lookup is <strong>fenced</strong> and additional features are available:
+     * </p>
+     * <ul>
+     * <li>External DTD subsets and entities are enabled.</li>
+     * <li>The document and any external DTD subset or entity it references are read from within those fences, and a path resolving outside them throws an
+     * {@link IllegalArgumentException}.</li>
+     * </ul>
+     * <p>
+     * Every resource inside the fences is considered trusted, so fence only directories whose contents you control.
      * </p>
      * <p>
      * We looks up values in an XML document in the format {@code "DocumentPath:XPath"}.
@@ -1627,10 +1639,6 @@ public StringLookup xmlEncoderStringLookup() {
      * <li>{@code "com/domain/document.xml:/path/to/node"}</li>
      * </ul>
      * <p>
-     * Secure processing is enabled by default and can be overridden with the system property {@code "XmlStringLookup.secure"} set to {@code false}. The secure
-     * boolean String parsing follows the syntax defined by {@link Boolean#parseBoolean(String)}.
-     * </p>
-     * <p>
      * Using a {@link StringLookup} from the {@link StringLookupFactory}:
      * </p>
      *
@@ -1652,14 +1660,25 @@ public StringLookup xmlEncoderStringLookup() {
      * @since 1.5
      */
     public StringLookup xmlStringLookup() {
-        return fences != null ? xmlStringLookup(XmlStringLookup.DEFAULT_XPATH_FEATURES, fences) : XmlStringLookup.INSTANCE;
+        return fences != null ? xmlStringLookup(Collections.emptyMap(), fences) : XmlStringLookup.INSTANCE;
     }
 
     /**
      * Returns an XML StringLookup instance.
      * <p>
-     * If this factory was built using {@link Builder#setFences(Path...)}, then the string lookup is fenced and will throw an {@link IllegalArgumentException}
-     * if a lookup causes a path to resolve outside of these fences. Otherwise, the result is unfenced to preserved behavior from previous versions.
+     * XML files are parsed using Commons Secure XML, which enables the {@link XMLConstants#FEATURE_SECURE_PROCESSING} processing limits, which
+     * {@code factoryFeatures} can turn back off, and ignores external DTD subsets and entities by default.
+     * </p>
+     * <p>
+     * If this factory was built using {@link Builder#setFences}, then the string lookup is <strong>fenced</strong> and additional features are available:
+     * </p>
+     * <ul>
+     * <li>External DTD subsets and entities are enabled.</li>
+     * <li>The document and any external DTD subset or entity it references are read from within those fences, and a path resolving outside them throws an
+     * {@link IllegalArgumentException}.</li>
+     * </ul>
+     * <p>
+     * Every resource inside the fences is considered trusted, so fence only directories whose contents you control.
      * </p>
      * <p>
      * We looks up values in an XML document in the format {@code "]DocumentPath:XPath"}.
@@ -1671,10 +1690,6 @@ public StringLookup xmlStringLookup() {
      * <li>{@code "com/domain/document.xml:/path/to/node"}</li>
      * </ul>
      * <p>
-     * Secure processing is enabled by default and can be overridden with the system property {@code "XmlStringLookup.secure"} set to {@code false}. The secure
-     * boolean String parsing follows the syntax defined by {@link Boolean#parseBoolean(String)}.
-     * </p>
-     * <p>
      * Using a {@link StringLookup} from the {@link StringLookupFactory}:
      * </p>
      *
@@ -1705,8 +1720,19 @@ public StringLookup xmlStringLookup(final Map<String, Boolean> factoryFeatures)
     /**
      * Returns a fenced XML StringLookup instance.
      * <p>
-     * If this factory was built using {@link Builder#setFences(Path...)}, then the string lookup is fenced and will throw an {@link IllegalArgumentException}
-     * if a lookup causes a path to resolve outside of these fences. Otherwise, the result is unfenced to preserved behavior from previous versions.
+     * XML files are parsed using Commons Secure XML, which enables the {@link XMLConstants#FEATURE_SECURE_PROCESSING} processing limits, which
+     * {@code factoryFeatures} can turn back off, and ignores external DTD subsets and entities by default.
+     * </p>
+     * <p>
+     * If the {@code fences} argument is not empty, then the string lookup is <strong>fenced</strong> and additional features are available:
+     * </p>
+     * <ul>
+     * <li>External DTD subsets and entities are enabled.</li>
+     * <li>The document and any external DTD subset or entity it references are read from within those fences, and a path resolving outside them throws an
+     * {@link IllegalArgumentException}.</li>
+     * </ul>
+     * <p>
+     * Every resource inside the fences is considered trusted, so fence only directories whose contents you control.
      * </p>
      * <p>
      * We looks up values in an XML document in the format {@code "DocumentPath:XPath"}.
@@ -1718,9 +1744,6 @@ public StringLookup xmlStringLookup(final Map<String, Boolean> factoryFeatures)
      * <li>{@code "com/domain/document.xml:/path/to/node"}</li>
      * </ul>
      * <p>
-     * Secure processing is enabled by default and can be overridden with this constructor.
-     * </p>
-     * <p>
      * Using a {@link StringLookup} from the {@link StringLookupFactory} fenced by the current directory ({@code Paths.get("")}):
      * </p>
      *

src/main/java/org/apache/commons/text/lookup/XmlStringLookup.java

@@ -20,16 +20,19 @@
 import java.io.InputStream;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.util.HashMap;
+import java.util.Collections;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Objects;
 
 import javax.xml.XMLConstants;
+import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.xpath.XPathFactory;
 
 import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.xml.secure.SecureDocumentBuilderFactory;
+import org.apache.commons.xml.secure.SecureXPathFactory;
 import org.w3c.dom.Document;
 
 /**
@@ -41,7 +44,19 @@
  * <li>{@code "com/domain/document.xml:/path/to/node"}</li>
  * </ul>
  * <p>
- * Secure processing is enabled by default and can be overridden with {@link StringLookupFactory#xmlStringLookup(Map, Path...)}.
+ * DOM parser and XPath factory features can be set with {@link StringLookupFactory#xmlStringLookup(Map)}.
+ * </p>
+ * <p>
+ * Documents are parsed through Apache Commons Secure XML, which secures two separate aspects:
+ * </p>
+ * <ul>
+ * <li>Processing limits, such as the number of entity expansions, come from {@link XMLConstants#FEATURE_SECURE_PROCESSING}. That feature is enabled by
+ * default, and the feature maps above can turn it off.</li>
+ * <li>External resource fetching, that is external DTD subsets and external entities, is ignored by default and can <strong>not</strong> be enabled via the
+ * feature map. To enable external resource fetching, provide a non-empty list of <strong>fences</strong>.</li>
+ * </ul>
+ * <p>
+ * Every resource inside the fences is considered trusted, so fence only directories whose contents you control.
  * </p>
  *
  * @since 1.5
@@ -54,28 +69,12 @@ final class XmlStringLookup extends AbstractPathFencedLookup {
     private static final int KEY_PARTS_LEN = 2;
 
     /**
-     * Defines default XPath factory features.
-     */
-    static final Map<String, Boolean> DEFAULT_XPATH_FEATURES;
-
-    /**
-     * Defines default XML factory features.
-     */
-    static final Map<String, Boolean> DEFAULT_XML_FEATURES;
-    static {
-        DEFAULT_XPATH_FEATURES = new HashMap<>(1);
-        DEFAULT_XPATH_FEATURES.put(XMLConstants.FEATURE_SECURE_PROCESSING, Boolean.TRUE);
-        DEFAULT_XML_FEATURES = new HashMap<>(1);
-        DEFAULT_XML_FEATURES.put(XMLConstants.FEATURE_SECURE_PROCESSING, Boolean.TRUE);
-    }
-
-    /**
-     * Defines the singleton for this class with secure processing enabled by default.
+     * Defines the singleton for this class, which sets no parser or XPath factory feature and has no fence.
      * <p>
-     * Secure processing is enabled by default and can be overridden with {@link StringLookupFactory#xmlStringLookup(Map, Path...)}.
+     * Use {@link StringLookupFactory#xmlStringLookup(Map, Path...)} to set features and fences; without any fence, external resources are ignored.
      * </p>
      */
-    static final XmlStringLookup INSTANCE = new XmlStringLookup(DEFAULT_XML_FEATURES, DEFAULT_XPATH_FEATURES, (Path[]) null);
+    static final XmlStringLookup INSTANCE = new XmlStringLookup(Collections.emptyMap(), Collections.emptyMap(), (Path[]) null);
 
     /**
      * Defines XPath factory features.
@@ -98,7 +97,7 @@ final class XmlStringLookup extends AbstractPathFencedLookup {
     XmlStringLookup(final Map<String, Boolean> xmlFactoryFeatures, final Map<String, Boolean> xPathFactoryFeatures, final Path... fences) {
         super(fences);
         this.xmlFactoryFeatures = Objects.requireNonNull(xmlFactoryFeatures, "xmlFactoryFeatures");
-        this.xPathFactoryFeatures = Objects.requireNonNull(xPathFactoryFeatures, "xPathFfactoryFeatures");
+        this.xPathFactoryFeatures = Objects.requireNonNull(xPathFactoryFeatures, "xPathFactoryFeatures");
     }
 
     /**
@@ -110,7 +109,14 @@ final class XmlStringLookup extends AbstractPathFencedLookup {
      * <li>{@code "com/domain/document.xml:/path/to/node"}</li>
      * </ul>
      * <p>
-     * Secure processing is enabled by default and can be overridden with {@link StringLookupFactory#xmlStringLookup(Map, Path...)}.
+     * The document is parsed through Apache Commons Secure XML:
+     * </p>
+     * <ul>
+     * <li>Processing limits are governed by {@link XMLConstants#FEATURE_SECURE_PROCESSING}, which is enabled by default.</li>
+     * <li>External DTD subsets and external entities are resolved only from the fences guarding this lookup if these are not empty.</li>
+     * </ul>
+     * <p>
+     * Every resource inside the fences is considered trusted, so fence only directories whose contents you control.
      * </p>
      *
      * @param key The key to be looked up, may be null.
@@ -128,14 +134,21 @@ public String lookup(final String key) {
         }
         final String documentPath = keys[0];
         final String xpath = StringUtils.substringAfterLast(key, SPLIT_CH);
-        final DocumentBuilderFactory dbFactory = DocumentBuilderFactory.newInstance();
+        final DocumentBuilderFactory dbFactory = SecureDocumentBuilderFactory.newInstance();
         try {
             for (final Entry<String, Boolean> p : xmlFactoryFeatures.entrySet()) {
                 dbFactory.setFeature(p.getKey(), p.getValue());
             }
-            try (InputStream inputStream = Files.newInputStream(getPath(documentPath))) {
-                final Document doc = dbFactory.newDocumentBuilder().parse(inputStream);
-                final XPathFactory xpFactory = XPathFactory.newInstance();
+            final Path documentFile = getPath(documentPath);
+            try (InputStream inputStream = Files.newInputStream(documentFile)) {
+                final DocumentBuilder documentBuilder = dbFactory.newDocumentBuilder();
+                // If the fence is not empty, opt-in follow-up resources fetched from the fence.
+                if (!fence.isEmpty()) {
+                    documentBuilder.setEntityResolver(new PathFenceResolver(fence));
+                }
+                // Parsing with the document's own URI gives relative system identifiers a base URI to resolve against, as XML requires.
+                final Document doc = documentBuilder.parse(inputStream, documentFile.toUri().toString());
+                final XPathFactory xpFactory = SecureXPathFactory.newInstance();
                 for (final Entry<String, Boolean> p : xPathFactoryFeatures.entrySet()) {
                     xpFactory.setFeature(p.getKey(), p.getValue());
                 }