disable dtd support (not needed by schema based parser)

geotools · Sep 19, 2016 · 7bb399c · 7bb399c
1 parent ecf1499
commit 7bb399c
Show file tree

Hide file tree

Showing 5 changed files with 27 additions and 15 deletions.
diff --git a/modules/library/xml/src/main/java/org/geotools/xml/DocumentFactory.java b/modules/library/xml/src/main/java/org/geotools/xml/DocumentFactory.java
@@ -48,6 +48,7 @@
  * <li>{@link XMLHandlerHints#NAMESPACE_MAPPING} - Map&lt;String,URL&gt; namespace mapping</li>
  * <li>{@link XMLHandlerHints#ENTITY_RESOLVER} - control entry resolution<li>
  * <li>{@link #DISABLE_EXTERNAL_ENTITIES} - Boolean.TRUE to disable entity resolution<li>
+ * <li>{@link XMLHandlerHints#SAX_PARSER_FACTORY} - supply factory used by {@link #getParser(Map)}</li>
  * </ul>
  * 
  * @author dzwiers, Refractions Research, Inc. http://www.refractions.net
@@ -173,16 +174,24 @@ private static SAXParser getParser(Map<String,Object> hints) throws SAXException
         }
         spf.setNamespaceAware(true);
         spf.setValidating(false);
+
         try {
+            // Extra precaution to reduce/prevent XXE attacks
+            //
+            // For more info: https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing
+            // https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Prevention_Cheat_Sheet
+
+
+            // Step 1 distable DTD support - not needed for schema driven parser
+            // 
+            // Note: XMLSaxHandler will reject all DTD references
+            spf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+            spf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+
+            // Step 2 optionally disable external entities
+            //
             if (hints != null && hints.containsKey(DISABLE_EXTERNAL_ENTITIES)
                     && Boolean.TRUE.equals(hints.get(DISABLE_EXTERNAL_ENTITIES))) {
-                // This is an XML Schema driven parser, no DTD required (XMLSaxHandler will reject all dtd references)
-                spf.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
-                spf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
-
-                // The following configuration prevents XML External Entity Injection (XXE) attacks
-                // See for more information:
-                // https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing
                 spf.setFeature("http://xml.org/sax/features/external-general-entities", false);
                 spf.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
             }

diff --git a/modules/library/xml/src/main/java/org/geotools/xml/XMLSAXHandler.java b/modules/library/xml/src/main/java/org/geotools/xml/XMLSAXHandler.java
@@ -172,7 +172,7 @@ public XMLSAXHandler(Map hints) {
     }
     protected void init(Map hints){
         if( hints == null ){
-            hints = Collections.EMPTY_MAP;
+            hints = new HashMap<String,Object>();
         }
         this.hints = hints;
         if( hints.containsKey(XMLHandlerHints.ENTITY_RESOLVER)){

diff --git a/modules/library/xml/src/main/java/org/geotools/xml/handlers/ComplexElementHandler.java b/modules/library/xml/src/main/java/org/geotools/xml/handlers/ComplexElementHandler.java
@@ -139,7 +139,7 @@ public void endElement(URI namespaceURI, String localName, Map hints)
         text = (text == null) ? null : text.trim();
 
         if(hints == null){
-            hints = new HashMap();
+            hints = new HashMap<String,Object>();
             hints.put(ElementHandlerFactory.KEY,ehf);
         }else{
             if(!hints.containsKey(ElementHandlerFactory.KEY))

diff --git a/modules/library/xml/src/main/java/org/geotools/xml/handlers/ElementHandlerFactory.java b/modules/library/xml/src/main/java/org/geotools/xml/handlers/ElementHandlerFactory.java
@@ -49,6 +49,7 @@
  * @source $URL$
  */
 public class ElementHandlerFactory {
+    /** Used to store ElementHandlerFactory */
     public static final String KEY = "org.geotools.xml.handlers.ElementHandlerFactory_KEY";
     private Logger logger;
     private Map targSchemas = new HashMap(); // maps prefix -->> Schema

diff --git a/modules/library/xml/src/test/java/org/geotools/xml/DocumentFactoryTest.java b/modules/library/xml/src/test/java/org/geotools/xml/DocumentFactoryTest.java
@@ -185,16 +185,18 @@ public void testGetInstanceFromInputStreamWithSpecifiedLevelParseFailureSAXExcep
         DocumentFactory.getInstance(inputStream, hints, Level.WARNING);
     }
 
-    void verifyDisableExternalEntities(boolean disabled) throws SAXNotRecognizedException,
+    void verifyDisableExternalEntities(boolean disabledExternalEntities) throws SAXNotRecognizedException,
             SAXNotSupportedException, ParserConfigurationException {
-        if (disabled) {
-            verify(mockSaxParserFactory).setFeature(DISALLOW_DOCTYPE_DECLAIRATION, true);
-            verify(mockSaxParserFactory).setFeature(LOAD_EXTERNAL_DTD, false);
+
+        // double check DTD support disabled
+        verify(mockSaxParserFactory).setFeature(DISALLOW_DOCTYPE_DECLAIRATION, true);
+        verify(mockSaxParserFactory).setFeature(LOAD_EXTERNAL_DTD, false);
+
+        // check optional eternal entity disabled
+        if (disabledExternalEntities) {
             verify(mockSaxParserFactory).setFeature(EXTERNAL_GENERAL_ENTITIES_FEATURE, false);
             verify(mockSaxParserFactory).setFeature(EXTERNAL_PARAMETER_ENTITIES_FEATURE, false);
         } else {
-            verify(mockSaxParserFactory, never()).setFeature(DISALLOW_DOCTYPE_DECLAIRATION, true);
-            verify(mockSaxParserFactory, never()).setFeature(LOAD_EXTERNAL_DTD, false);
             verify(mockSaxParserFactory, never()).setFeature(EXTERNAL_GENERAL_ENTITIES_FEATURE,
                     false);
             verify(mockSaxParserFactory, never()).setFeature(EXTERNAL_PARAMETER_ENTITIES_FEATURE,