Spotbugs: XML External Entity (XXE) Injection Vulnerability

[alcastic]

Consider update dom4j dependency version to at least 2.1.3 (instead of 2.1.1).
Security vulnerability: https://snyk.io/vuln/SNYK-JAVA-ORGDOM4J-565810
dom4j:2.1.3 notes: https://github.com/dom4j/dom4j/blob/master/README.md

[welcome[bot]]

Thanks for opening your first issue here! :smiley:
Please check our contributing guideline. Especially when you report a problem, make sure you share a Minimal, Complete, and Verifiable example to reproduce it in this issue.

[KengoTODA]

ArrayIndexOutOfBoundsException is thrown in test cases. I checked the diff in dom4j but not sure about why...

Caused by:
org.dom4j.DocumentException: arraycopy: last destination index 1275 out of bounds for char[1024]
  at org.dom4j.io.SAXReader.read(SAXReader.java:513)
  at org.dom4j.io.SAXReader.read(SAXReader.java:408)
  at edu.umd.cs.findbugs.PluginLoader.addCollection(PluginLoader.java:1285)

Caused by:
java.lang.ArrayIndexOutOfBoundsException: arraycopy: last destination index 1275 out of bounds for char[1024]
  at java.base/java.lang.System.arraycopy(Native Method)
  at org.gjt.xpp.impl.tokenizer.Tokenizer.next(Tokenizer.java:1274)
  at org.gjt.xpp.impl.pullparser.PullParser.next(PullParser.java:392)
  at org.gjt.xpp.sax2.Driver.parseSubTree(Driver.java:415)
  at org.gjt.xpp.sax2.Driver.parse(Driver.java:310)
  at org.dom4j.io.SAXReader.read(SAXReader.java:494)

I also tried new factory method, but no luck.

diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/PluginLoader.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/PluginLoader.java
index c51e9dea7..f235d49bd 100644
--- a/spotbugs/src/main/java/edu/umd/cs/findbugs/PluginLoader.java
+++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/PluginLoader.java
@@ -1150,7 +1150,7 @@ public class PluginLoader {
             throw new PluginDoesntContainMetadataException((corePlugin ? "Core plugin" : "Plugin ") + jarName
                     + " doesn't contain findbugs.xml; got " + findbugsXML_URL + " from " + classloaderName);
         }
-        SAXReader reader = new SAXReader();
+        SAXReader reader = SAXReader.createDefault();

         try (InputStream input = IO.openNonCachedStream(findbugsXML_URL);
                 Reader r = UTF8.bufferedReader(input)) {
@@ -1278,7 +1278,7 @@ public class PluginLoader {
     private void addCollection(List<Document> messageCollectionList, String filename) throws PluginException {
         URL messageURL = getResource(filename);
         if (messageURL != null) {
-            SAXReader reader = new SAXReader();
+            SAXReader reader = SAXReader.createDefault();
             try (InputStream input = IO.openNonCachedStream(messageURL);
                     Reader stream = UTF8.bufferedReader(input)) {
                 Document messageCollection;
@@ -1581,7 +1581,7 @@ public class PluginLoader {
     private static Document parseDocument(@WillClose InputStream in) throws DocumentException {
         Reader r = UTF8.bufferedReader(in);
         try {
-            SAXReader reader = new SAXReader();
+            SAXReader reader = SAXReader.createDefault();
             Document d = reader.read(r);
             return d;
         } finally {
diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/xml/XPathFind.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/xml/XPathFind.java
index bc1b8afe1..f70b0a10a 100644
--- a/spotbugs/src/main/java/edu/umd/cs/findbugs/xml/XPathFind.java
+++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/xml/XPathFind.java
@@ -66,7 +66,7 @@ public abstract class XPathFind {
         String fileName = argv[0];
         String xpath = argv[1];

-        SAXReader reader = new SAXReader();
+        SAXReader reader = SAXReader.createDefault();
         Document document = reader.read(fileName);

         XPathFind finder = new XPathFind(document) {
diff --git a/spotbugs/src/tools/edu/umd/cs/findbugs/tools/ComparePerfomance.java b/spotbugs/src/tools/edu/umd/cs/findbugs/tools/ComparePerfomance.java
index 4f11004cd..0642fabae 100644
--- a/spotbugs/src/tools/edu/umd/cs/findbugs/tools/ComparePerfomance.java
+++ b/spotbugs/src/tools/edu/umd/cs/findbugs/tools/ComparePerfomance.java
@@ -64,7 +64,7 @@ public class ComparePerfomance {

     public  void foo(File f, int i) throws DocumentException, IOException {
         Document doc;
-        SAXReader reader = new SAXReader();
+        SAXReader reader = SAXReader.createDefault();

         String fName = f.getName();
         InputStream in = new FileInputStream(f);
diff --git a/spotbugs/src/tools/edu/umd/cs/findbugs/tools/xml/CheckMessages.java b/spotbugs/src/tools/edu/umd/cs/findbugs/tools/xml/CheckMessages.java
index 215ad9548..e5ae3ad6b 100644
--- a/spotbugs/src/tools/edu/umd/cs/findbugs/tools/xml/CheckMessages.java
+++ b/spotbugs/src/tools/edu/umd/cs/findbugs/tools/xml/CheckMessages.java
@@ -64,7 +64,7 @@ public class CheckMessages {
             this.filename = filename;

             File file = new File(filename);
-            SAXReader saxReader = new SAXReader();
+            SAXReader saxReader = SAXReader.createDefault();
             this.document = saxReader.read(file);
         }

[KengoTODA]

According to ./gradlew spotbugs:dependencies the dom4j v2.1.3 has transitive dependencies that v2.1.1 doesn't have. In my understanding, v2.1.1 removed these transitive dependencies intentionally so I guess it's a defect in v2.1.3.

I'll wait dom4j's next release which fixes the related issue https://github.com/dom4j/dom4j/issues/85

+--- org.dom4j:dom4j:2.1.3
|    +--- jaxen:jaxen:1.1.6
|    +--- javax.xml.stream:stax-api:1.0-2
|    +--- net.java.dev.msv:xsdlib:2013.6.1
|    |    \--- relaxngDatatype:relaxngDatatype:20020414
|    +--- javax.xml.bind:jaxb-api:2.2.12
|    +--- pull-parser:pull-parser:2
|    \--- xpp3:xpp3:1.1.4c

+--- org.dom4j:dom4j:2.1.1

[KengoTODA]

It seems that dom4j needs time to release their fix, so I'll use gradle's feature to exclude transitive dependencies.

Actual Dependencies in issue-1122 branch

$./gradlew spotbugs:dependencies

:
:

compileClasspath - Compile classpath for source set 'main'.
+--- org.ow2.asm:asm:8.0.1
+--- org.ow2.asm:asm-analysis:8.0.1
|    \--- org.ow2.asm:asm-tree:8.0.1
|         \--- org.ow2.asm:asm:8.0.1
+--- org.ow2.asm:asm-commons:8.0.1
|    +--- org.ow2.asm:asm:8.0.1
|    +--- org.ow2.asm:asm-tree:8.0.1 (*)
|    \--- org.ow2.asm:asm-analysis:8.0.1 (*)
+--- org.ow2.asm:asm-tree:8.0.1 (*)
+--- org.ow2.asm:asm-util:8.0.1
|    +--- org.ow2.asm:asm:8.0.1
|    +--- org.ow2.asm:asm-tree:8.0.1 (*)
|    \--- org.ow2.asm:asm-analysis:8.0.1 (*)
+--- org.apache.bcel:bcel:6.4.1
+--- net.jcip:jcip-annotations:1.0
+--- org.dom4j:dom4j:2.1.3
+--- org.apache.commons:commons-lang3:3.10
+--- org.apache.commons:commons-text:1.8
|    \--- org.apache.commons:commons-lang3:3.9 -> 3.10
+--- org.slf4j:slf4j-api:1.8.0-beta4
+--- project :spotbugs-annotations
|    \--- com.google.code.findbugs:jsr305:3.0.2
+--- jaxen:jaxen:1.1.6
\--- net.sf.saxon:Saxon-HE:9.9.1-2
     \--- com.ibm.icu:icu4j:63.1