HPCC4J-463 Eliminate External Entity XML parsing (hpcc-systems#567)

- Provides centralized safer Docbuilder helper method - FileSpray and BaseHPCCWsClient to use new safer doc builder - Adds settings verifying Junit tests Signed-off-by: Rodrigo Pastrana <[email protected]>
rpastrana · Jul 1, 2022 · c760129 · c760129
1 parent 4510ad7
commit c760129
Show file tree

Hide file tree

Showing 4 changed files with 97 additions and 25 deletions.
diff --git a/wsclient/src/main/java/org/hpccsystems/ws/client/BaseHPCCWsClient.java b/wsclient/src/main/java/org/hpccsystems/ws/client/BaseHPCCWsClient.java
@@ -5,9 +5,7 @@
 import java.net.URL;
 import java.nio.charset.StandardCharsets;
 
-import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilder;
-import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.xpath.XPath;
 import javax.xml.xpath.XPathConstants;
@@ -617,29 +615,15 @@ protected void setUpversionParser() throws ParserConfigurationException, XPathEx
         if(m_versionParser != null && m_versionXpathExpression != null)
             return;
 
-	    DocumentBuilderFactory docBuilderFactory = DocumentBuilderFactory.newInstance();
-	    // Settings for secure XML parsing
-        docBuilderFactory.setAttribute(XMLConstants.FEATURE_SECURE_PROCESSING, true);
-        docBuilderFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
-        docBuilderFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
-
-        docBuilderFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
-        docBuilderFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
-        docBuilderFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
-        // Disable external DTDs as well
-        docBuilderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
-        // and these as well, per Timothy Morgan's 2014 paper: "XML Schema, DTD, and Entity Attacks"
-        docBuilderFactory.setXIncludeAware(false);
-        docBuilderFactory.setExpandEntityReferences(false);
+        m_versionParser = Utils.newSafeXMLDocBuilder();
+        if (m_versionParser == null)
+            throw new XPathExpressionException ("Could not create new version parser");
 
         XPath versionXpath = XPathFactory.newInstance().newXPath();
         m_versionXpathExpression = versionXpath.compile("string(/VersionInfo/Version)");
         if (m_versionXpathExpression == null)
             throw new XPathExpressionException ("Could not Compile versionXpathExpression");
 
-        m_versionParser = docBuilderFactory.newDocumentBuilder();
-        if (m_versionParser == null)
-            throw new XPathExpressionException ("Could not create new version parser");
     }
     /**
      * Attempts to retrieve the default WSDL version of the target runtime ESP service

diff --git a/wsclient/src/main/java/org/hpccsystems/ws/client/HPCCFileSprayClient.java b/wsclient/src/main/java/org/hpccsystems/ws/client/HPCCFileSprayClient.java
@@ -27,8 +27,10 @@
 
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.xpath.XPath;
 import javax.xml.xpath.XPathExpression;
+import javax.xml.xpath.XPathExpressionException;
 import javax.xml.xpath.XPathFactory;
 
 import org.apache.axis2.AxisFault;
@@ -1438,6 +1440,26 @@ public boolean uploadFileLocalDropZone(File file) throws Exception, ArrayOfEspEx
         return uploadFile(file, fetchLocalDropZones.get(0));
     }
 
+    static DocumentBuilder m_safeXMLDocBuilder = null;
+    static XPathExpression m_uploadResultExpression = null;
+
+    static protected void setupUploadResultParser() throws XPathExpressionException, ParserConfigurationException
+    {
+        if(m_uploadResultExpression != null && m_safeXMLDocBuilder != null)
+            return;
+
+        m_safeXMLDocBuilder = Utils.newSafeXMLDocBuilder();
+
+        if (m_safeXMLDocBuilder == null)
+            throw new XPathExpressionException ("Could not create new result XML parser");
+
+        XPath xpath = XPathFactory.newInstance().newXPath();
+        m_uploadResultExpression= xpath.compile("string(/UploadFilesResponse/UploadFileResults/DFUActionResult/Result)");
+
+        if (m_uploadResultExpression == null)
+            throw new XPathExpressionException ("Could not Compile versionXpathExpression");
+    }
+
     /**
      * UPLOADS A FILE( UP TO 2GB FILE SIZES) TO THE SPECIFIED LANDING ZONE.
      *
@@ -1553,14 +1575,11 @@ public boolean uploadLargeFile(File uploadFile, DropZoneWrapper dropZone)
         {
             try
             {
-                DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
-                DocumentBuilder parser = factory.newDocumentBuilder();
-                Document document = parser.parse(new ByteArrayInputStream(response.toString().getBytes(StandardCharsets.UTF_8)));
+                setupUploadResultParser(); // throws if expression or docbuilder == null
 
-                XPath xpath = XPathFactory.newInstance().newXPath();
-                XPathExpression expr = xpath.compile("string(/UploadFilesResponse/UploadFileResults/DFUActionResult/Result)");
+                Document document = m_safeXMLDocBuilder.parse(new ByteArrayInputStream(response.toString().getBytes(StandardCharsets.UTF_8)));
 
-                String result = expr.evaluate(document);
+                String result = m_uploadResultExpression.evaluate(document);
                 log.info("uploadLargeFile ( " + uploadFile + ") result: '" + result + "'");
 
                 if (result.isEmpty() || !result.equalsIgnoreCase("Success"))

diff --git a/wsclient/src/main/java/org/hpccsystems/ws/client/utils/Utils.java b/wsclient/src/main/java/org/hpccsystems/ws/client/utils/Utils.java
@@ -18,6 +18,7 @@
 import java.util.Date;
 import java.util.List;
 
+import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
@@ -27,6 +28,7 @@
 import javax.xml.transform.TransformerFactory;
 import javax.xml.transform.dom.DOMSource;
 import javax.xml.transform.stream.StreamResult;
+import javax.xml.xpath.XPathExpressionException;
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -1029,4 +1031,33 @@ public static Date UTCStringToDate(String utc) throws ParseException
         return df.parse(utc);
     }
 
+    public static DocumentBuilderFactory newSafeXMLDocBuilderFactory() throws ParserConfigurationException
+    {
+	    DocumentBuilderFactory docBuilderFactory = DocumentBuilderFactory.newInstance();
+	    // Settings for secure XML parsing
+        docBuilderFactory.setAttribute(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+        docBuilderFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_DTD, "");
+        docBuilderFactory.setAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
+
+        docBuilderFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true);
+        docBuilderFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
+        docBuilderFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+        // Disable external DTDs as well
+        docBuilderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+        // and these as well, per Timothy Morgan's 2014 paper: "XML Schema, DTD, and Entity Attacks"
+        docBuilderFactory.setXIncludeAware(false);
+        docBuilderFactory.setExpandEntityReferences(false);
+
+        return docBuilderFactory;
+    }
+
+    public static DocumentBuilder newSafeXMLDocBuilder() throws ParserConfigurationException, XPathExpressionException
+    {
+        DocumentBuilderFactory docBuilderFactory = newSafeXMLDocBuilderFactory();
+        DocumentBuilder safeXMLDocBuilder = docBuilderFactory.newDocumentBuilder();
+        if (safeXMLDocBuilder == null)
+            throw new XPathExpressionException ("Could not create new safe XML doc builder");
+
+        return safeXMLDocBuilder;
+    }
 }
diff --git a/wsclient/src/test/java/org/hpccsystems/ws/client/utils/Security.java b/wsclient/src/test/java/org/hpccsystems/ws/client/utils/Security.java
@@ -0,0 +1,38 @@
+package org.hpccsystems.ws.client.utils;
+
+import static org.junit.Assert.*;
+
+import javax.xml.XMLConstants;
+import javax.xml.parsers.DocumentBuilder;
+import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.ParserConfigurationException;
+import javax.xml.xpath.XPathExpressionException;
+
+import org.junit.Test;
+
+public class Security
+{
+    @Test
+    public void testXMLExternalEntityDocBuilderFactorySuppressionSettings() throws XPathExpressionException, ParserConfigurationException 
+    {
+        DocumentBuilderFactory safefactory = Utils.newSafeXMLDocBuilderFactory();
+        assertTrue("XML DocBuilder Factory attribute 'XMLConstants.FEATURE_SECURE_PROCESSING' not set to TRUE!", safefactory.getAttribute(XMLConstants.FEATURE_SECURE_PROCESSING) == Boolean.TRUE);
+        Object accessExternalDTDAttribute = safefactory.getAttribute(XMLConstants.ACCESS_EXTERNAL_DTD);
+        assertTrue("XML DocBuilder Factory attribute 'XMLConstants.ACCESS_EXTERNAL_DTD' not set to \"\"!", accessExternalDTDAttribute != null && accessExternalDTDAttribute.equals(""));
+        Object accessExternalSchemaAttribute = safefactory.getAttribute(XMLConstants.ACCESS_EXTERNAL_SCHEMA);
+        assertTrue("XML DocBuilder Factory attribute 'XMLConstants.ACCESS_EXTERNAL_SCHEMA' not set to \"\"!", accessExternalSchemaAttribute != null && accessExternalSchemaAttribute.equals(""));
+        assertTrue("XML DocBuilder Factory 'http://apache.org/xml/features/disallow-doctype-decl' not set to TRUE!", safefactory.getFeature("http://apache.org/xml/features/disallow-doctype-decl"));
+        assertFalse("XML DocBuilder Factory 'http://xml.org/sax/features/external-general-entities' not set to FALSE!", safefactory.getFeature("http://xml.org/sax/features/external-general-entities"));
+        assertFalse("XML DocBuilder Factory 'http://xml.org/sax/features/external-parameter-entities' not set to FALSE!", safefactory.getFeature( "http://xml.org/sax/features/external-parameter-entities"));
+        assertFalse("XML DocBuilder Factory 'http://apache.org/xml/features/nonvalidating/load-external-dtd' not set to FALSE!", safefactory.getFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd"));
+        assertFalse("XML DocBuilder Factory 'ExpandEntityReferences' not set to FALSE!", safefactory.isExpandEntityReferences());
+        assertFalse("XML DocBuilder Factory 'XIncludeAware' not set to FALSE!", safefactory.isXIncludeAware());
+    }
+
+    @Test
+    public void testXMLExternalEntityDocBuilderSuppressionSettings() throws XPathExpressionException, ParserConfigurationException 
+    {
+        DocumentBuilder xmldocparser = Utils.newSafeXMLDocBuilder();
+        assertFalse("XML DocBuilder 'XIncludeAware' not set to FALSE!", xmldocparser.isXIncludeAware());
+    }
+}