Repository: crawler-commons/crawler-commons
Pull request #323 (2021-09-29 17:02:40)

Fix XXE vulnerability in Sitemap parser
We've been using the Crawler Commons library in our product for a while now and recently received a vulnerability report from a security researcher. Upon investigation, we have discovered that the issue was caused by an insufficiently secure SAX parser configuration within the SitemapParser class.

In this PR I have added a test case to demonstrate an XXE attack by reading a local file via the SitemapParser. I've made a change in the parser to disable external entity resolution according to the [recommendations from OWASP](https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#java) and now the tests work as expected, the invalid URL with an external entity is getting ignored during parsing. I've tried to enable the "http://apache.org/xml/features/disallow-doctype-decl" feature, but then sitemap parsing completely blows up when any DOCTYPE definitions are present and I wasn't sure we want to do that. Let me know if we actually want to go that far and completely disable doctype resolution (which IMO would be reasonable for sitemap parsing, but I'll leave it up to you).

Let me know if I could improve the pull request in any way.


Patch:
diff --git a/src/main/java/crawlercommons/sitemaps/SiteMapParser.java b/src/main/java/crawlercommons/sitemaps/SiteMapParser.java
index aea86df..2a26af4 100644
--- a/src/main/java/crawlercommons/sitemaps/SiteMapParser.java
+++ b/src/main/java/crawlercommons/sitemaps/SiteMapParser.java
@@ -589,7 +589,11 @@ public class SiteMapParser {
         // support the use of an explicit namespace.
         factory.setNamespaceAware(true);
 
+        // Configure underlying parser features to reduce the risk of XXE attacks
+        // See https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#java
         try {
+            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);
         } catch (Exception e) {
             throw new RuntimeException("Failed to configure XML parser: " + e.toString());


Test patch:
diff --git a/src/test/java/crawlercommons/sitemaps/SiteMapParserTest.java b/src/test/java/crawlercommons/sitemaps/SiteMapParserTest.java
index 9a1eeb5..056dad9 100644
--- a/src/test/java/crawlercommons/sitemaps/SiteMapParserTest.java
+++ b/src/test/java/crawlercommons/sitemaps/SiteMapParserTest.java
@@ -31,6 +31,8 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.net.URL;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.time.ZonedDateTime;
 import java.time.temporal.ChronoField;
 import java.util.ArrayList;
@@ -112,6 +114,43 @@ public class SiteMapParserTest {
         assertEquals("http://www.example.com/dynsitemap?date=lastyear&all=false", currentSiteMap.getUrl().toString());
     }
 
+    @Test
+    public void testSitemapXXE() 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 temporary file
+        SiteMapParser parser = new SiteMapParser();
+        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.toPath().toAbsolutePath() + "\">\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 one
+        assertEquals(1, sm.getSiteMapUrls().size());
+        assertEquals(new URL("http://www.example.com/visit-here"), sm.getSiteMapUrls().iterator().next().getUrl());
+    }
+
     @Test
     public void testSitemapWithNamespace() throws UnknownFormatException, IOException {
         SiteMapParser parser = new SiteMapParser();
diff --git a/src/test/resources/sitemaps/do-not-visit.txt b/src/test/resources/sitemaps/do-not-visit.txt
new file mode 100644
index 0000000..1e6af59
--- /dev/null
+++ b/src/test/resources/sitemaps/do-not-visit.txt
@@ -0,0 +1,1 @@
+http://www.example.com/do-not-visit-here
\ No newline at end of file