From a9cd7d2c2fb5d9ef36b50907ad0cb46bdeac084f Mon Sep 17 00:00:00 2001
From: Binary Wang <binarywang@gmail.com>
Date: Sat, 28 Mar 2020 22:16:53 +0800
Subject: [PATCH] :art: #1427 fix XmlUtils.xml2Map() method which was
 vulnerable to XXE vulnerability

---
 .../chanjar/weixin/common/util/XmlUtils.java  | 27 +++++++++++--------
 .../weixin/common/util/XmlUtilsTest.java      | 15 +++++++++--
 2 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/weixin-java-common/src/main/java/me/chanjar/weixin/common/util/XmlUtils.java b/weixin-java-common/src/main/java/me/chanjar/weixin/common/util/XmlUtils.java
index cd3a7a984..c2ffdb001 100644
--- a/weixin-java-common/src/main/java/me/chanjar/weixin/common/util/XmlUtils.java
+++ b/weixin-java-common/src/main/java/me/chanjar/weixin/common/util/XmlUtils.java
@@ -1,21 +1,21 @@
 package me.chanjar.weixin.common.util;
 
-import java.io.StringReader;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+import com.google.common.collect.Sets;
 import org.dom4j.Document;
 import org.dom4j.DocumentException;
 import org.dom4j.Element;
 import org.dom4j.Node;
 import org.dom4j.io.SAXReader;
 import org.dom4j.tree.DefaultText;
+import org.xml.sax.SAXException;
 
-import com.google.common.collect.Lists;
-import com.google.common.collect.Maps;
-import com.google.common.collect.Sets;
+import java.io.StringReader;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
 
 /**
  * <pre>
@@ -31,13 +31,18 @@ public class XmlUtils {
     Map<String, Object> map = new HashMap<>(16);
     try {
       SAXReader saxReader = new SAXReader();
+      saxReader.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+      saxReader.setFeature("http://javax.xml.XMLConstants/feature/secure-processing", true);
+      saxReader.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+      saxReader.setFeature("http://xml.org/sax/features/external-general-entities", false);
+      saxReader.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
       Document doc = saxReader.read(new StringReader(xmlString));
       Element root = doc.getRootElement();
       List<Element> elements = root.elements();
       for (Element element : elements) {
-          map.put(element.getName(), element2MapOrString(element));
+        map.put(element.getName(), element2MapOrString(element));
       }
-    } catch (DocumentException e) {
+    } catch (DocumentException | SAXException e) {
       throw new RuntimeException(e);
     }
 
diff --git a/weixin-java-common/src/test/java/me/chanjar/weixin/common/util/XmlUtilsTest.java b/weixin-java-common/src/test/java/me/chanjar/weixin/common/util/XmlUtilsTest.java
index 1afd1c1d9..7b6bb536f 100644
--- a/weixin-java-common/src/test/java/me/chanjar/weixin/common/util/XmlUtilsTest.java
+++ b/weixin-java-common/src/test/java/me/chanjar/weixin/common/util/XmlUtilsTest.java
@@ -1,10 +1,10 @@
 package me.chanjar.weixin.common.util;
 
+import org.testng.annotations.Test;
+
 import java.util.List;
 import java.util.Map;
 
-import org.testng.annotations.*;
-
 import static org.assertj.core.api.Assertions.assertThat;
 
 /**
@@ -17,6 +17,17 @@ import static org.assertj.core.api.Assertions.assertThat;
  */
 public class XmlUtilsTest {
 
+  @Test(expectedExceptions = {RuntimeException.class})
+  public void testXml2Map_xxe() {
+    String xml = "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n" +
+      "<!DOCTYPE test [\n" +
+      "<!ENTITY xxe SYSTEM \"file:///etc/passwd\">\n" +
+      "<!ENTITY xxe2 SYSTEM \"http://localhost/test.php\">\n" +
+      "]>\n" +
+      "<xml></xml>";
+    XmlUtils.xml2Map(xml);
+  }
+
   @Test
   public void testXml2Map() {
     String xml = "<xml>\n" +