Disable external entities by default to prevent XXE injection attacks…

…, re #6 XML Builder classes now explicitly enable or disable 'external-general-entities' and 'external-parameter-entities' features of the DocumentBuilderFactory when #create or #parse methods are used. To prevent XML External Entity (XXE) injection attacks, these features are disabled by default. They can only be enabled by passing a true boolean value to new versions of the #create and #parse methods that accept a flag for this feature.
jmurty · Jul 22, 2014 · e6fddca · e6fddca
1 parent 5d5caa5
commit e6fddca
Show file tree

Hide file tree

Showing 6 changed files with 376 additions and 15 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -1,6 +1,17 @@
 Release Notes for java-xmlbuilder
 =================================
 
+Version 1.2 - Pending
+---------------------
+
+Fixes:
+
+* Prevent XML External Entity (XXE) injection attacks by disabling parsing of
+  general and parameter external entities by default (#6). External entities
+  are now only parsed if this feature is explicitly enabled by passing a boolean
+  flag value to the #create and #parse methods.  
+  WARNING: This will break code that expects external entities to be parsed.
+
 Version 1.1 - 22 July 2014
 --------------------------
 

diff --git a/src/main/java/com/jamesmurty/utils/BaseXMLBuilder.java b/src/main/java/com/jamesmurty/utils/BaseXMLBuilder.java
@@ -76,6 +76,13 @@ public abstract class BaseXMLBuilder {
 
     private static boolean isNamespaceAware = true;
 
+    /**
+     * If true, the builder will raise an {@link XMLBuilderRuntimeException}
+     * if external general and parameter entities cannot be explicitly enabled
+     * or disabled.
+     */
+    public static boolean failIfExternalEntityParsingCannotBeConfigured = true;
+
     /**
      * Construct a new builder object that wraps the given XML document.
      * This constructor is for internal use only.
@@ -112,6 +119,78 @@ protected BaseXMLBuilder(Node myNode, Node parentNode) {
         }
     }
 
+    /**
+     * Explicitly enable or disable the 'external-general-entities' and
+     * 'external-parameter-entities' features of the underlying
+     * DocumentBuilderFactory.
+     *
+     * TODO This is a naive approach that simply tries to apply all known
+     * feature name/URL values in turn until one succeeds, or none do.
+     *
+     * @param factory
+     * factory which will have external general and parameter entities enabled
+     * or disabled.
+     * @param enableExternalEntities
+     * if true external entities will be explicitly enabled, otherwise they
+     * will be explicitly disabled.
+     */
+    protected static void enableOrDisableExternalEntityParsing(
+        DocumentBuilderFactory factory, boolean enableExternalEntities)
+    {
+        // Feature list drawn from:
+        // https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing
+
+        /* Enable or disable external general entities */
+        String[] externalGeneralEntitiesFeatures = {
+            // General
+            "http://xml.org/sax/features/external-general-entities",
+            // Xerces 1
+            "http://xerces.apache.org/xerces-j/features.html#external-general-entities",
+            // Xerces 2
+            "http://xerces.apache.org/xerces2-j/features.html#external-general-entities",
+        };
+        boolean success = false;
+        for (String feature: externalGeneralEntitiesFeatures) {
+            try {
+                factory.setFeature(feature, enableExternalEntities);
+                success = true;
+                break;
+            } catch (ParserConfigurationException e) {
+            }
+        }
+        if (!success && failIfExternalEntityParsingCannotBeConfigured) {
+            throw new XMLBuilderRuntimeException(
+                new ParserConfigurationException(
+                    "Failed to set 'external-general-entities' feature to "
+                    + enableExternalEntities));
+        }
+
+        /* Enable or disable external parameter entities */
+        String[] externalParameterEntitiesFeatures = {
+            // General
+            "http://xml.org/sax/features/external-parameter-entities",
+            // Xerces 1
+            "http://xerces.apache.org/xerces-j/features.html#external-parameter-entities",
+            // Xerces 2
+            "http://xerces.apache.org/xerces2-j/features.html#external-parameter-entities",
+        };
+        success = false;
+        for (String feature: externalParameterEntitiesFeatures) {
+            try {
+                factory.setFeature(feature, enableExternalEntities);
+                success = true;
+                break;
+            } catch (ParserConfigurationException e) {
+            }
+        }
+        if (!success && failIfExternalEntityParsingCannotBeConfigured) {
+            throw new XMLBuilderRuntimeException(
+                new ParserConfigurationException(
+                    "Failed to set 'external-parameter-entities' feature to "
+                    + enableExternalEntities));
+        }
+    }
+
     /**
      * Construct an XML Document with a default namespace with the given
      * root element.
@@ -126,11 +205,13 @@ protected BaseXMLBuilder(Node myNode, Node parentNode) {
      * @throws FactoryConfigurationError
      * @throws ParserConfigurationException
      */
-    protected static Document createDocumentImpl(String name, String namespaceURI)
+    protected static Document createDocumentImpl(
+        String name, String namespaceURI, boolean enableExternalEntities)
         throws ParserConfigurationException, FactoryConfigurationError
     {
         DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
         factory.setNamespaceAware(isNamespaceAware);
+        enableOrDisableExternalEntityParsing(factory, enableExternalEntities);
         DocumentBuilder builder = factory.newDocumentBuilder();
         Document document = builder.newDocument();
         Element rootElement = null;
@@ -157,11 +238,13 @@ protected static Document createDocumentImpl(String name, String namespaceURI)
      * @throws IOException
      * @throws SAXException
      */
-    protected static Document parseDocumentImpl(InputSource inputSource)
+    protected static Document parseDocumentImpl(
+        InputSource inputSource, boolean enableExternalEntities)
         throws ParserConfigurationException, SAXException, IOException
     {
         DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
         factory.setNamespaceAware(isNamespaceAware);
+        enableOrDisableExternalEntityParsing(factory, enableExternalEntities);
         DocumentBuilder builder = factory.newDocumentBuilder();
         Document document = builder.parse(inputSource);
         return document;

diff --git a/src/main/java/com/jamesmurty/utils/XMLBuilder.java b/src/main/java/com/jamesmurty/utils/XMLBuilder.java
@@ -89,6 +89,53 @@ protected XMLBuilder(Node myNode, Node parentNode) {
         super(myNode, parentNode);
     }
 
+    /**
+     * Construct a builder for new XML document with a default namespace.
+     * The document will be created with the given root element, and the builder
+     * returned by this method will serve as the starting-point for any further
+     * document additions.
+     *
+     * @param name
+     * the name of the document's root element.
+     * @param namespaceURI
+     * default namespace URI for document, ignored if null or empty.
+     * @param enableExternalEntities
+     * enable external entities; beware of XML External Entity (XXE) injection.
+     * @return
+     * a builder node that can be used to add more nodes to the XML document.
+     *
+     * @throws FactoryConfigurationError
+     * @throws ParserConfigurationException
+     */
+    public static XMLBuilder create(String name, String namespaceURI,
+        boolean enableExternalEntities)
+        throws ParserConfigurationException, FactoryConfigurationError
+    {
+        return new XMLBuilder(
+            createDocumentImpl(name, namespaceURI, enableExternalEntities));
+    }
+
+    /**
+     * Construct a builder for new XML document. The document will be created
+     * with the given root element, and the builder returned by this method
+     * will serve as the starting-point for any further document additions.
+     *
+     * @param name
+     * the name of the document's root element.
+     * @param enableExternalEntities
+     * enable external entities; beware of XML External Entity (XXE) injection.
+     * @return
+     * a builder node that can be used to add more nodes to the XML document.
+     *
+     * @throws FactoryConfigurationError
+     * @throws ParserConfigurationException
+     */
+    public static XMLBuilder create(String name, boolean enableExternalEntities)
+        throws ParserConfigurationException, FactoryConfigurationError
+    {
+        return create(name, null, enableExternalEntities);
+    }
+
     /**
      * Construct a builder for new XML document with a default namespace.
      * The document will be created with the given root element, and the builder
@@ -108,7 +155,7 @@ protected XMLBuilder(Node myNode, Node parentNode) {
     public static XMLBuilder create(String name, String namespaceURI)
         throws ParserConfigurationException, FactoryConfigurationError
     {
-        return new XMLBuilder(createDocumentImpl(name, namespaceURI));
+        return create(name, namespaceURI, false);
     }
 
     /**
@@ -130,6 +177,84 @@ public static XMLBuilder create(String name)
         return create(name, null);
     }
 
+    /**
+     * Construct a builder from an existing XML document. The provided XML
+     * document will be parsed and an XMLBuilder object referencing the
+     * document's root element will be returned.
+     *
+     * @param inputSource
+     * an XML document input source that will be parsed into a DOM.
+     * @param enableExternalEntities
+     * enable external entities; beware of XML External Entity (XXE) injection.
+     * @return
+     * a builder node that can be used to add more nodes to the XML document.
+     * @throws ParserConfigurationException
+     *
+     * @throws FactoryConfigurationError
+     * @throws ParserConfigurationException
+     * @throws IOException
+     * @throws SAXException
+     */
+    public static XMLBuilder parse(
+        InputSource inputSource, boolean enableExternalEntities)
+        throws ParserConfigurationException, SAXException, IOException
+    {
+        return new XMLBuilder(
+            parseDocumentImpl(inputSource, enableExternalEntities));
+    }
+
+    /**
+     * Construct a builder from an existing XML document string.
+     * The provided XML document will be parsed and an XMLBuilder
+     * object referencing the document's root element will be returned.
+     *
+     * @param xmlString
+     * an XML document string that will be parsed into a DOM.
+     * @param enableExternalEntities
+     * enable external entities; beware of XML External Entity (XXE) injection.
+     * @return
+     * a builder node that can be used to add more nodes to the XML document.
+     *
+     * @throws ParserConfigurationException
+     * @throws FactoryConfigurationError
+     * @throws ParserConfigurationException
+     * @throws IOException
+     * @throws SAXException
+     */
+    public static XMLBuilder parse(
+        String xmlString, boolean enableExternalEntities)
+        throws ParserConfigurationException, SAXException, IOException
+    {
+        return XMLBuilder.parse(
+            new InputSource(new StringReader(xmlString)),
+            enableExternalEntities);
+    }
+
+    /**
+     * Construct a builder from an existing XML document file.
+     * The provided XML document will be parsed and an XMLBuilder
+     * object referencing the document's root element will be returned.
+     *
+     * @param xmlFile
+     * an XML document file that will be parsed into a DOM.
+     * @param enableExternalEntities
+     * enable external entities; beware of XML External Entity (XXE) injection.
+     * @return
+     * a builder node that can be used to add more nodes to the XML document.
+     *
+     * @throws ParserConfigurationException
+     * @throws FactoryConfigurationError
+     * @throws ParserConfigurationException
+     * @throws IOException
+     * @throws SAXException
+     */
+    public static XMLBuilder parse(File xmlFile, boolean enableExternalEntities)
+        throws ParserConfigurationException, SAXException, IOException
+    {
+        return XMLBuilder.parse(
+            new InputSource(new FileReader(xmlFile)), enableExternalEntities);
+    }
+
     /**
      * Construct a builder from an existing XML document. The provided XML
      * document will be parsed and an XMLBuilder object referencing the
@@ -149,7 +274,7 @@ public static XMLBuilder create(String name)
     public static XMLBuilder parse(InputSource inputSource)
         throws ParserConfigurationException, SAXException, IOException
     {
-        return new XMLBuilder(parseDocumentImpl(inputSource));
+        return XMLBuilder.parse(inputSource, false);
     }
 
     /**
@@ -171,7 +296,7 @@ public static XMLBuilder parse(InputSource inputSource)
     public static XMLBuilder parse(String xmlString)
         throws ParserConfigurationException, SAXException, IOException
     {
-        return XMLBuilder.parse(new InputSource(new StringReader(xmlString)));
+        return XMLBuilder.parse(xmlString, false);
     }
 
     /**
@@ -193,7 +318,7 @@ public static XMLBuilder parse(String xmlString)
     public static XMLBuilder parse(File xmlFile)
         throws ParserConfigurationException, SAXException, IOException
     {
-        return XMLBuilder.parse(new InputSource(new FileReader(xmlFile)));
+        return XMLBuilder.parse(xmlFile, false);
     }
 
     @Override