From 6e5acc54a4db6ad6b7b57131f63743bba378572a Mon Sep 17 00:00:00 2001 From: Rodrigo Date: Fri, 1 Jul 2022 17:22:39 -0400 Subject: [PATCH] HPCC4J-463 Eliminate External Entity XML parsing (#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 --- .../ws/client/BaseHPCCWsClient.java | 22 ++--------- .../ws/client/HPCCFileSprayClient.java | 31 ++++++++++++--- .../hpccsystems/ws/client/utils/Utils.java | 31 +++++++++++++++ .../hpccsystems/ws/client/utils/Security.java | 38 +++++++++++++++++++ 4 files changed, 97 insertions(+), 25 deletions(-) create mode 100644 wsclient/src/test/java/org/hpccsystems/ws/client/utils/Security.java diff --git a/wsclient/src/main/java/org/hpccsystems/ws/client/BaseHPCCWsClient.java b/wsclient/src/main/java/org/hpccsystems/ws/client/BaseHPCCWsClient.java index c84d0755e..d268bcb7e 100644 --- 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 index 2381e2a01..0e153b0f4 100644 --- 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 index 41b7214c8..3176c485c 100644 --- 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 new file mode 100644 index 000000000..f0be48301 --- /dev/null +++ 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()); + } +}