Repository: crawler-commons/crawler-commons
Pull request #371 (2022-02-25 11:47:32)

Not allow doctype when parsing sitemap
### Description of Changes
- Added feature http://apache.org/xml/features/disallow-doctype-decl when creating SAXParser for site maps.
- Allow exempt if particular property has been set.

### Reference
- https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#java

### Risks & Impacts
- Added option `allowDocTypeDefinitions `to allow DTD in `SiteMapParser `(disabled by default)
- Added new feature in site map parser if DTD is not allowed
- Will not parse any sitemaps with DOCTYPE definition, if `allowDocTypeDefinitions `isn't set as true.

### Security
- Prevent possible DTD vulnerability when parsing site maps


Patch:
diff --git a/src/main/java/crawlercommons/sitemaps/SiteMapParser.java b/src/main/java/crawlercommons/sitemaps/SiteMapParser.java
index b89b66e..6ddcdde 100644
--- a/src/main/java/crawlercommons/sitemaps/SiteMapParser.java
+++ b/src/main/java/crawlercommons/sitemaps/SiteMapParser.java
@@ -101,6 +101,11 @@ public class SiteMapParser {
     protected Map<String, Extension> extensionNamespaces = new HashMap<>();
 
     private MimeTypeDetector mimeTypeDetector;
+    
+    /**
+     * Option to allow DTD when parsing site map
+     */
+    private boolean allowDocTypeDefinitions = false;
 
     /* Function to normalize or filter URLs. Does nothing by default. */
     private Function<String, String> urlFilter = (String url) -> url;
@@ -595,6 +600,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());
         }
@@ -661,4 +669,12 @@ public class SiteMapParser {
     public static boolean urlIsValid(String sitemapBaseUrl, String testUrl) {
         return testUrl.startsWith(sitemapBaseUrl);
     }
+    
+    /**
+     * Set if the parser allow DTD
+     * @param allowDocTypeDefinitions true if allowed. Default is false.
+     */
+    public void setAllowDocTypeDefinitions(boolean allowDocTypeDefinitions) {
+        this.allowDocTypeDefinitions = allowDocTypeDefinitions;
+    }
 }


Test patch:
diff --git a/src/test/java/crawlercommons/sitemaps/SiteMapParserTest.java b/src/test/java/crawlercommons/sitemaps/SiteMapParserTest.java
index 3856e17..c7f8be2 100644
--- a/src/test/java/crawlercommons/sitemaps/SiteMapParserTest.java
+++ b/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,18 +137,49 @@ 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());
         assertEquals(true, asm instanceof SiteMap);
         assertEquals(true, asm.isProcessed());
         SiteMap sm = (SiteMap) asm;
-
+    
         // Should only return a single valid URL and ignore the external entity
         assertEquals(1, sm.getSiteMapUrls().size());
         assertEquals(new URL("http://www.example.com/visit-here"), sm.getSiteMapUrls().iterator().next().getUrl());
     }
-
+    
     @Test
     public void testSitemapXIncludeDisabled() throws UnknownFormatException, IOException {
         // A file on disk that would be read if we were vulnerable to XInclude