From f796c5053b6f275db9a3907fa24207d233974e29 Mon Sep 17 00:00:00 2001
From: Roman Ivanov <ivanov-jr@mail.ru>
Date: Sat, 23 Feb 2019 09:10:41 -0800
Subject: [PATCH] Issue #6474: disable external dtd load by default

---
 config/pmd.xml                                |  4 ++--
 pom.xml                                       |  4 ++--
 .../tools/checkstyle/XmlLoader.java           | 23 +++++++++++++------
 .../checkstyle/ConfigurationLoaderTest.java   |  9 ++++++++
 .../tools/checkstyle/XmlLoaderTest.java       |  2 +-
 src/xdocs/config_reporting.xml                | 11 +++++++++
 6 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/config/pmd.xml b/config/pmd.xml
index 1ff824a22cd..3ad8ae385f0 100644
--- a/config/pmd.xml
+++ b/config/pmd.xml
@@ -96,13 +96,13 @@
   </rule>
   <rule ref="category/java/codestyle.xml/ClassNamingConventions">
     <properties>
-     <!-- Definitions and XmlLoader.FeaturesForVerySecureJavaInstallations aren't utility classes.
+     <!-- Definitions and XmlLoader.LoadExternalDtdFeatureProvider aren't utility classes.
           JavadocTokenTypes and TokenTypes aren't utility classes.
             They are token definition classes. Also, they are part of the API. -->
      <property name="violationSuppressXPath"
                value="//ClassOrInterfaceDeclaration[@Image='Definitions'
                    or @Image='JavadocTokenTypes' or @Image='TokenTypes'
-                   or @Image='FeaturesForVerySecureJavaInstallations']"/>
+                   or @Image='LoadExternalDtdFeatureProvider']"/>
     </properties>
   </rule>
   <rule ref="category/java/codestyle.xml/ConfusingTernary">
diff --git a/pom.xml b/pom.xml
index c1bcbac7940..039e39f06a6 100644
--- a/pom.xml
+++ b/pom.xml
@@ -2678,12 +2678,12 @@
               </targetTests>
               <excludedMethods>
                 <!--cause of https://github.com/checkstyle/checkstyle/issues/3605-->
-                <param>addFeaturesForVerySecureJavaInstallations</param>
+                <param>setFeaturesBySystemProperty</param>
               </excludedMethods>
               <avoidCallsTo>
                 <!--cause of https://github.com/checkstyle/checkstyle/issues/3605-->
                 <avoidCallsTo>
-                  com.puppycrawl.tools.checkstyle.XmlLoader$FeaturesForVerySecureJavaInstallations
+                  com.puppycrawl.tools.checkstyle.XmlLoader$LoadExternalDtdFeatureProvider
                 </avoidCallsTo>
               </avoidCallsTo>
               <coverageThreshold>100</coverageThreshold>
diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/XmlLoader.java b/src/main/java/com/puppycrawl/tools/checkstyle/XmlLoader.java
index 792a9da1c29..786652d9c34 100644
--- a/src/main/java/com/puppycrawl/tools/checkstyle/XmlLoader.java
+++ b/src/main/java/com/puppycrawl/tools/checkstyle/XmlLoader.java
@@ -65,7 +65,7 @@ protected XmlLoader(Map<String, String> publicIdToResourceNameMap)
             throws SAXException, ParserConfigurationException {
         this.publicIdToResourceNameMap = new HashMap<>(publicIdToResourceNameMap);
         final SAXParserFactory factory = SAXParserFactory.newInstance();
-        FeaturesForVerySecureJavaInstallations.addFeaturesForVerySecureJavaInstallations(factory);
+        LoadExternalDtdFeatureProvider.setFeaturesBySystemProperty(factory);
         factory.setValidating(true);
         parser = factory.newSAXParser().getXMLReader();
         parser.setContentHandler(this);
@@ -113,7 +113,10 @@ public void error(SAXParseException exception) throws SAXException {
      * Used for setting specific for secure java installations features to SAXParserFactory.
      * Pulled out as a separate class in order to suppress Pitest mutations.
      */
-    public static final class FeaturesForVerySecureJavaInstallations {
+    public static final class LoadExternalDtdFeatureProvider {
+        
+        /** System property name to enable external DTD load. */
+        public static final String ENABLE_EXTERNAL_DTD_LOAD = "checkstyle.enableExternalDtdLoad";
 
         /** Feature that enables loading external DTD when loading XML files. */
         private static final String LOAD_EXTERNAL_DTD =
@@ -123,20 +126,26 @@ public static final class FeaturesForVerySecureJavaInstallations {
                 "http://xml.org/sax/features/external-general-entities";
 
         /** Stop instances being created. **/
-        private FeaturesForVerySecureJavaInstallations() {
+        private LoadExternalDtdFeatureProvider() {
         }
 
         /**
          * Configures SAXParserFactory with features required
-         * for execution on very secured environments.
+         * to use external DTD file loading, this is not activated by default to no allow
+         * usage of schema files that checkstyle do not know
+         * it is even security problem to allow files from outside.
          * @param factory factory to be configured with special features
          * @throws SAXException if an error occurs
          * @throws ParserConfigurationException if an error occurs
          */
-        public static void addFeaturesForVerySecureJavaInstallations(SAXParserFactory factory)
+        public static void setFeaturesBySystemProperty(SAXParserFactory factory)
                 throws SAXException, ParserConfigurationException {
-            factory.setFeature(LOAD_EXTERNAL_DTD, true);
-            factory.setFeature(EXTERNAL_GENERAL_ENTITIES, true);
+
+            final boolean enableExternalDtdLoad = Boolean.valueOf(
+                System.getProperty(ENABLE_EXTERNAL_DTD_LOAD, "false"));
+
+            factory.setFeature(LOAD_EXTERNAL_DTD, enableExternalDtdLoad);
+            factory.setFeature(EXTERNAL_GENERAL_ENTITIES, enableExternalDtdLoad);
         }
 
     }
diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/ConfigurationLoaderTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/ConfigurationLoaderTest.java
index 3b2d2086494..50dde992964 100644
--- a/src/test/java/com/puppycrawl/tools/checkstyle/ConfigurationLoaderTest.java
+++ b/src/test/java/com/puppycrawl/tools/checkstyle/ConfigurationLoaderTest.java
@@ -406,6 +406,9 @@ public void testExternalEntity() throws Exception {
         final Properties props = new Properties();
         props.setProperty("checkstyle.basedir", "basedir");
 
+        System.setProperty(
+                XmlLoader.LoadExternalDtdFeatureProvider.ENABLE_EXTERNAL_DTD_LOAD, "true");
+
         final DefaultConfiguration config =
             (DefaultConfiguration) loadConfiguration(
                 "InputConfigurationLoaderExternalEntity.xml", props);
@@ -421,6 +424,9 @@ public void testExternalEntitySubdirectory() throws Exception {
         final Properties props = new Properties();
         props.setProperty("checkstyle.basedir", "basedir");
 
+        System.setProperty(
+                XmlLoader.LoadExternalDtdFeatureProvider.ENABLE_EXTERNAL_DTD_LOAD, "true");
+
         final DefaultConfiguration config =
             (DefaultConfiguration) loadConfiguration(
                 "subdir/InputConfigurationLoaderExternalEntitySubDir.xml", props);
@@ -436,6 +442,9 @@ public void testExternalEntityFromUri() throws Exception {
         final Properties props = new Properties();
         props.setProperty("checkstyle.basedir", "basedir");
 
+        System.setProperty(
+                XmlLoader.LoadExternalDtdFeatureProvider.ENABLE_EXTERNAL_DTD_LOAD, "true");
+
         final File file = new File(
                 getPath("subdir/InputConfigurationLoaderExternalEntitySubDir.xml"));
         final DefaultConfiguration config =
diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/XmlLoaderTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/XmlLoaderTest.java
index 8ee873f6218..026d9856dc2 100644
--- a/src/test/java/com/puppycrawl/tools/checkstyle/XmlLoaderTest.java
+++ b/src/test/java/com/puppycrawl/tools/checkstyle/XmlLoaderTest.java
@@ -47,7 +47,7 @@ public void testParserConfiguredSuccessfully() throws Exception {
     @Test
     public void testIsProperUtilsClass() throws ReflectiveOperationException {
         assertTrue("Constructor is not private", isUtilsClassHasPrivateConstructor(
-                XmlLoader.FeaturesForVerySecureJavaInstallations.class, true));
+                XmlLoader.LoadExternalDtdFeatureProvider.class, true));
     }
 
     @Test
diff --git a/src/xdocs/config_reporting.xml b/src/xdocs/config_reporting.xml
index a2c25848d99..c5f868ad988 100644
--- a/src/xdocs/config_reporting.xml
+++ b/src/xdocs/config_reporting.xml
@@ -69,5 +69,16 @@
         to an empty string.
       </p>
     </section>
+
+    <section name="Enable External DTD load">
+      <p>
+        The property <code>checkstyle.enableExternalDtdLoad</code>
+        defines ability use custom DTD files inconfig and load them from some location.
+        The property type
+        is <a href="property_types.html#boolean">boolean</a> and defaults
+        to <code>false</code>.
+      </p>
+    </section>
+
   </body>
 </document>