Merge pull request #371 from ebx/1.3-SNAPSHOT-EBX

CHANGES.txt

@@ -1,6 +1,7 @@
 Crawler-Commons Change Log
 
 Current Development 1.3-SNAPSHOT (yyyy-mm-dd)
+- [Sitemaps] Disable support for DTDs in XML sitemaps and feeds by default (Kenneth Wong) #371
 - Migrate Continuous Integration from Travis to GitHub Actions (Valery Yatsynovich) #333
 - Upgrade dependencies (dependabot, Richard Zowalla) #334, #339, #345, #346, #347, #350, #369
 - Upgrade Maven plugins (dependabot, Richard Zowalla, sebastian-nagel) #328, #329, #330, #331, #335, #336, #337, #338, #340, #341, #343, #356, #363. #364, #366

src/main/java/crawlercommons/sitemaps/SiteMapParser.java

@@ -102,6 +102,11 @@ public class SiteMapParser {
 
     private MimeTypeDetector mimeTypeDetector;
 
+    /**
+     * Option to allow DTDs in sitemaps.
+     */
+    private boolean allowDocTypeDefinitions = false;
+
     /* Function to normalize or filter URLs. Does nothing by default. */
     private Function<String, String> urlFilter = (String url) -> url;
 
@@ -140,6 +145,16 @@ public class SiteMapParser {
         this.mimeTypeDetector = new MimeTypeDetector();
     }
 
+    /**
+     * Sets if the parser allows a DTD in sitemaps or feeds.
+     *
+     * @param allowDocTypeDefinitions
+     *            true if allowed. Default is false.
+     */
+    public void setAllowDocTypeDefinitions(boolean allowDocTypeDefinitions) {
+        this.allowDocTypeDefinitions = allowDocTypeDefinitions;
+    }
+
     /**
      * @return whether invalid URLs will be rejected (where invalid means that
      *         the URL is not under the base URL, see <a href=
@@ -164,7 +179,7 @@ public class SiteMapParser {
      * specification, or any accepted namespace (see
      * {@link #addAcceptedNamespace(String)}). Note enabling strict namespace
      * checking always adds the namespace defined by the current sitemap
-     * specificiation ({@link Namespace#SITEMAP}) to the list of accepted
+     * specification ({@link Namespace#SITEMAP}) to the list of accepted
      * namespaces.
      * 
      * @param s
@@ -242,6 +257,7 @@ public class SiteMapParser {
     public void setURLFilter(URLFilter filter) {
         urlFilter = filter::filter;
     }
+
     /**
      * Returns a SiteMap or SiteMapIndex given an online sitemap URL
      *
@@ -595,6 +611,9 @@ public class SiteMapParser {
             factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
             factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
             factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+            if (!this.allowDocTypeDefinitions) {
+                factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+            }
         } catch (Exception e) {
             throw new RuntimeException("Failed to configure XML parser: " + e.toString());
         }

src/main/java/crawlercommons/sitemaps/SiteMapTester.java

@@ -46,6 +46,8 @@ public class SiteMapTester {
             LOG.error("Java properties:");
             LOG.error("  sitemap.strictNamespace");
             LOG.error("                  if true sitemaps are required to use the standard namespace URI");
+            LOG.error("  sitemap.allow.dtd");
+            LOG.error("                  if true sitemaps are allowed to include a DTD");
             LOG.error("  sitemap.extensions");
             LOG.error("                  if true enable sitemap extension parsing");
             LOG.error("  sitemap.filter.urls");
@@ -71,6 +73,9 @@ public class SiteMapTester {
         boolean strictNamespace = Boolean.getBoolean("sitemap.strictNamespace");
         saxParser.setStrictNamespace(strictNamespace);
 
+        boolean allowDTD = Boolean.getBoolean("sitemap.allow.dtd");
+        saxParser.setAllowDocTypeDefinitions(allowDTD);
+
         boolean enableExtensions = Boolean.getBoolean("sitemap.extensions");
         if (enableExtensions) {
             saxParser.enableExtensions();

src/test/java/crawlercommons/sitemaps/SiteMapParserTest.java

@@ -113,7 +113,7 @@ public class SiteMapParserTest {
     }
 
     @Test
-    public void testSitemapXXE() throws UnknownFormatException, IOException {
+    public void testSitemapXXE() throws IOException {
         // A file on disk that would be read if we were vulnerable to XXE
         File doNotVisit = new File("src/test/resources/sitemaps/do-not-visit.txt");
 
@@ -137,6 +137,37 @@ public class SiteMapParserTest {
                         .append("</urlset>");
         byte[] content = scontent.toString().getBytes(UTF_8);
 
+        URL url = new URL("http://www.example.com/sitemap.xxe.xml");
+        Assertions.assertThrows(UnknownFormatException.class,
+            () -> parser.parseSiteMap(contentType, content, url));
+    }
+
+    @Test
+    public void testSitemapXXEWithDocTypeAllowed() throws UnknownFormatException, IOException {
+        // A file on disk that would be read if we were vulnerable to XXE
+        File doNotVisit = new File("src/test/resources/sitemaps/do-not-visit.txt");
+
+        // Create a sitemap with an external entity referring to the local file
+        SiteMapParser parser = new SiteMapParser();
+        parser.setAllowDocTypeDefinitions(true);
+        String contentType = "text/xml";
+        StringBuilder scontent = new StringBuilder(1024);
+        scontent.append("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n") //
+                        .append("<!DOCTYPE urlset [\n") //
+                        .append("  <!ENTITY test SYSTEM \"file://" + doNotVisit.getAbsolutePath() + "\">\n") //
+                        .append("]>\n") //
+                        .append("<urlset xmlns=\"http://www.sitemaps.org/schemas/sitemap/0.9\">\n") //
+                        .append("  <url>\n") //
+                        .append("    <loc>http://www.example.com/visit-here</loc>\n") //
+                        .append("    <lastmod>2019-06-19</lastmod>\n") //
+                        .append("   </url>\n") //
+                        .append("  <url>\n") //
+                        .append("    <loc>&test;</loc>\n") //
+                        .append("    <lastmod>2019-06-19</lastmod>\n") //
+                        .append("  </url>\n") //
+                        .append("</urlset>");
+        byte[] content = scontent.toString().getBytes(UTF_8);
+
         URL url = new URL("http://www.example.com/sitemap.xxe.xml");
         AbstractSiteMap asm = parser.parseSiteMap(contentType, content, url);
         assertEquals(SitemapType.XML, asm.getType());