Merge pull request #508 from guusdk/SPARK-2147_guard-against-dom4j-ba…

…sed-xxe-attacks SPARK-2147 Guard against CVE-2020-10683 (dom4j reading external entities)
igniterealtime · Aug 17, 2020 · 084b199 · 084b199
2 parents 58560ea + 634865d
commit 084b199
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 32 deletions.
diff --git a/core/src/main/java/org/jivesoftware/LoginDialog.java b/core/src/main/java/org/jivesoftware/LoginDialog.java
@@ -1578,6 +1578,11 @@ private void checkForOldSettings() throws Exception {
             SAXReader saxReader = new SAXReader();
             Document pluginXML;
             try {
+                // SPARK-2147: Disable certain features for security purposes (CVE-2020-10683)
+                saxReader.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+                saxReader.setFeature("http://xml.org/sax/features/external-general-entities", false);
+                saxReader.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+
                 pluginXML = saxReader.read(settingsXML);
             }
             catch (DocumentException e) {

diff --git a/core/src/main/java/org/jivesoftware/spark/PluginManager.java b/core/src/main/java/org/jivesoftware/spark/PluginManager.java
@@ -33,6 +33,7 @@
 import org.jivesoftware.spark.util.log.Log;
 import org.jivesoftware.sparkimpl.settings.JiveInfo;
 import org.jivesoftware.sparkimpl.settings.local.SettingsManager;
+import org.xml.sax.SAXException;
 
 import java.awt.*;
 import java.io.*;
@@ -278,11 +279,16 @@ private boolean hasDependencies( File pluginFile )
         SAXReader saxReader = new SAXReader();
         try
         {
+            // SPARK-2147: Disable certain features for security purposes (CVE-2020-10683)
+            saxReader.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+            saxReader.setFeature("http://xml.org/sax/features/external-general-entities", false);
+            saxReader.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+
             final Document pluginXML = saxReader.read( pluginFile );
             final List dependencies = pluginXML.selectNodes( "plugin/depends/plugin" );
             return dependencies != null && dependencies.size() > 0;
         }
-        catch ( DocumentException e )
+        catch ( DocumentException | SAXException e )
         {
             Log.error( "Unable to read plugin dependencies from " + pluginFile, e );
             return false;
@@ -307,9 +313,14 @@ private Plugin loadPublicPlugin( File pluginDir )
         Document pluginXML = null;
         try
         {
+            // SPARK-2147: Disable certain features for security purposes (CVE-2020-10683)
+            saxReader.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+            saxReader.setFeature("http://xml.org/sax/features/external-general-entities", false);
+            saxReader.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+
             pluginXML = saxReader.read( pluginFile );
         }
-        catch ( DocumentException e )
+        catch ( DocumentException | SAXException e )
         {
             Log.error( "Unable to read plugin XML file from " + pluginDir, e );
         }
@@ -479,9 +490,14 @@ private void loadInternalPlugins( InputStreamReader reader )
         Document pluginXML = null;
         try
         {
+            // SPARK-2147: Disable certain features for security purposes (CVE-2020-10683)
+            saxReader.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+            saxReader.setFeature("http://xml.org/sax/features/external-general-entities", false);
+            saxReader.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+
             pluginXML = saxReader.read( reader );
         }
-        catch ( DocumentException e )
+        catch ( DocumentException | SAXException e )
         {
             Log.error( e );
         }

diff --git a/core/src/main/java/org/jivesoftware/sparkimpl/plugin/emoticons/EmoticonManager.java b/core/src/main/java/org/jivesoftware/sparkimpl/plugin/emoticons/EmoticonManager.java
@@ -291,34 +291,19 @@ public void addEmoticonPack(String packName) {
 		final SAXReader saxParser = new SAXReader();
 		saxParser.setValidation(false);
 		try {
-			saxParser.setFeature("http://xml.org/sax/features/validation",
-					false);
-			saxParser.setFeature("http://xml.org/sax/features/namespaces",
-					false);
-			saxParser.setFeature(
-					"http://apache.org/xml/features/validation/schema", false);
-			saxParser
-					.setFeature(
-							"http://apache.org/xml/features/validation/schema-full-checking",
-							false);
-			saxParser.setFeature(
-					"http://apache.org/xml/features/validation/dynamic", false);
-			saxParser
-					.setFeature(
-							"http://apache.org/xml/features/allow-java-encodings",
-							true);
-			saxParser
-					.setFeature(
-							"http://apache.org/xml/features/continue-after-fatal-error",
-							true);
-			saxParser
-					.setFeature(
-							"http://apache.org/xml/features/nonvalidating/load-dtd-grammar",
-							false);
-			saxParser
-					.setFeature(
-							"http://apache.org/xml/features/nonvalidating/load-external-dtd",
-							false);
+			saxParser.setFeature("http://xml.org/sax/features/validation", false);
+			saxParser.setFeature("http://xml.org/sax/features/namespaces", false);
+			saxParser.setFeature("http://apache.org/xml/features/validation/schema", false);
+			saxParser.setFeature("http://apache.org/xml/features/validation/schema-full-checking", false);
+			saxParser.setFeature("http://apache.org/xml/features/validation/dynamic", false);
+			saxParser.setFeature("http://apache.org/xml/features/allow-java-encodings", true);
+			saxParser.setFeature("http://apache.org/xml/features/continue-after-fatal-error", true);
+			saxParser.setFeature("http://apache.org/xml/features/nonvalidating/load-dtd-grammar", false);
+
+            // SPARK-2147: Disable certain features for security purposes (CVE-2020-10683)
+            saxParser.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+            saxParser.setFeature("http://xml.org/sax/features/external-general-entities", false);
+            saxParser.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
 		} catch (SAXException e) {
 			e.printStackTrace();
 		}

diff --git a/core/src/main/java/org/jivesoftware/sparkimpl/plugin/viewer/PluginViewer.java b/core/src/main/java/org/jivesoftware/sparkimpl/plugin/viewer/PluginViewer.java
@@ -43,6 +43,7 @@
 import org.jivesoftware.sparkimpl.settings.local.LocalPreferences;
 import org.jivesoftware.sparkimpl.settings.local.SettingsManager;
 import org.jivesoftware.sparkimpl.updater.EasySSLProtocolSocketFactory;
+import org.xml.sax.SAXException;
 
 import javax.swing.*;
 import java.awt.*;
@@ -515,9 +516,14 @@ public Collection<PublicPlugin> getPluginList( InputStream response )
 
         try
         {
+            // SPARK-2147: Disable certain features for security purposes (CVE-2020-10683)
+            saxReader.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+            saxReader.setFeature("http://xml.org/sax/features/external-general-entities", false);
+            saxReader.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+
             pluginXML = saxReader.read( response );
         }
-        catch ( DocumentException e )
+        catch ( DocumentException | SAXException e )
         {
             Log.error( e );
         }