Set secure processing property to XML factory and port XmlUtils changes from v1

Author: zoewangg

core/src/main/java/software/amazon/awssdk/core/http/StaxResponseHandler.java

@@ -15,13 +15,10 @@
 
 package software.amazon.awssdk.core.http;
 
-import static software.amazon.awssdk.utils.FunctionalUtils.invokeSafely;
-
 import java.io.ByteArrayInputStream;
 import java.io.InputStream;
 import java.util.Map;
 import javax.xml.stream.XMLEventReader;
-import javax.xml.stream.XMLInputFactory;
 import javax.xml.stream.XMLStreamException;
 import software.amazon.awssdk.annotations.ReviewBeforeRelease;
 import software.amazon.awssdk.annotations.SdkProtectedApi;
@@ -34,6 +31,7 @@
 import software.amazon.awssdk.core.util.StringUtils;
 import software.amazon.awssdk.utils.FunctionalUtils.UnsafeFunction;
 import software.amazon.awssdk.utils.Logger;
+import software.amazon.awssdk.utils.XmlUtils;
 
 /**
  * Default implementation of HttpResponseHandler that handles a successful
@@ -47,10 +45,6 @@
 public class StaxResponseHandler<T> implements HttpResponseHandler<T> {
     private static final Logger log = Logger.loggerFor(StaxResponseHandler.class);
 
-    /**
-     * Shared factory for creating XML event readers.
-     */
-    private static final XMLInputFactory XML_INPUT_FACTORY = createXmlInputFactory();
     /**
      * The StAX unmarshaller to use when handling the response.
      */
@@ -90,10 +84,7 @@ public T handle(HttpResponse response, ExecutionAttributes executionAttributes)
             content = new ByteArrayInputStream("<eof/>".getBytes(StringUtils.UTF8));
         }
 
-        XMLEventReader eventReader;
-        synchronized (XML_INPUT_FACTORY) {
-            eventReader = XML_INPUT_FACTORY.createXMLEventReader(content);
-        }
+        XMLEventReader eventReader = XmlUtils.xmlInputFactory().createXMLEventReader(content);
 
         try {
             StaxUnmarshallerContext unmarshallerContext = new StaxUnmarshallerContext(eventReader, response.getHeaders());
@@ -128,7 +119,7 @@ protected ResponseMetadata getResponseMetadata(Map<String, String> metadata) {
      * from service responses.
      *
      * @param unmarshallerContext The unmarshaller context used to configure a service's response
-     *                            data.
+     * data.
      */
     protected void registerAdditionalMetadataExpressions(StaxUnmarshallerContext unmarshallerContext) {
     }
@@ -150,10 +141,10 @@ public boolean needsConnectionLeftOpen() {
      * for example).
      *
      * @param unmarshaller Unmarshaller for response POJO.
-     * @param <ResponseT>  Response POJO type.
+     * @param <ResponseT> Response POJO type.
      */
     public static <ResponseT> HttpResponseHandler<ResponseT> createStreamingResponseHandler(
-            Unmarshaller<ResponseT, StaxUnmarshallerContext> unmarshaller) {
+        Unmarshaller<ResponseT, StaxUnmarshallerContext> unmarshaller) {
         UnsafeFunction<HttpResponse, ResponseT> unmarshallFunction = response -> unmarshallStreaming(unmarshaller, response);
         return new HttpResponseHandler<ResponseT>() {
             @Override
@@ -168,35 +159,21 @@ public boolean needsConnectionLeftOpen() {
         };
     }
 
-    /**
-     * Create an {@link XMLInputFactory} with features that may cause security problems disabled
-     * @return an instance of {@link XMLInputFactory}
-     */
-    private static XMLInputFactory createXmlInputFactory() {
-        XMLInputFactory factory = XMLInputFactory.newInstance();
-        factory.setProperty(XMLInputFactory.SUPPORT_DTD, false);
-        factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
-        return factory;
-    }
-
     /**
      * Unmarshalls a streaming HTTP response into a POJO. Does not touch the content since that's consumed by the response
      * handler (either {@link StreamingResponseHandler} or {@link AsyncResponseHandler}).
      *
      * @param unmarshaller Unmarshaller for resposne type.
-     * @param response     HTTP response
-     * @param <ResponseT>  Response POJO Type.
+     * @param response HTTP response
+     * @param <ResponseT> Response POJO Type.
      * @return Unmarshalled response type.
      * @throws Exception if error occurs during unmarshalling.
      */
     private static <ResponseT> ResponseT unmarshallStreaming(Unmarshaller<ResponseT, StaxUnmarshallerContext> unmarshaller,
                                                              HttpResponse response) throws Exception {
         // Create a dummy event reader to make unmarshallers happy
-        XMLEventReader eventReader;
-        synchronized (XML_INPUT_FACTORY) {
-            eventReader = invokeSafely(() -> XML_INPUT_FACTORY
-                    .createXMLEventReader(new ByteArrayInputStream("<eof/>".getBytes(StringUtils.UTF8))));
-        }
+        XMLEventReader eventReader = XmlUtils.xmlInputFactory().createXMLEventReader(
+            new ByteArrayInputStream("<eof/>".getBytes(StringUtils.UTF8)));
 
         StaxUnmarshallerContext unmarshallerContext = new StaxUnmarshallerContext(eventReader, response.getHeaders());
         unmarshallerContext.registerMetadataExpression("ResponseMetadata/RequestId", 2, ResponseMetadata.AWS_REQUEST_ID);

core/src/main/java/software/amazon/awssdk/core/util/XpathUtils.java

@@ -38,6 +38,7 @@
 import org.xml.sax.SAXException;
 import org.xml.sax.SAXParseException;
 import software.amazon.awssdk.utils.Base64Utils;
+import software.amazon.awssdk.utils.XmlUtils;
 
 /**
  * Utility methods for extracting data from XML documents using Xpath
@@ -164,7 +165,7 @@ public static Document documentFrom(InputStream is)
             throws SAXException, IOException, ParserConfigurationException {
         is = new NamespaceRemovingInputStream(is);
         // DocumentBuilderFactory is not thread safe
-        DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
+        DocumentBuilderFactory factory = XmlUtils.documentBuilderFactory();
         DocumentBuilder builder = factory.newDocumentBuilder();
         // ensure that parser writes error/warning messages to the logger
         // rather than stderr

services/simpledb/src/test/java/software/amazon/awssdk/services/simpledb/transform/DomainMetadataResultUnmarshallerTest.java

@@ -22,6 +22,7 @@
 import org.junit.Test;
 import software.amazon.awssdk.core.runtime.transform.StaxUnmarshallerContext;
 import software.amazon.awssdk.services.simpledb.model.DomainMetadataResponse;
+import software.amazon.awssdk.utils.XmlUtils;
 
 public class DomainMetadataResultUnmarshallerTest {
 
@@ -30,7 +31,7 @@ public class DomainMetadataResultUnmarshallerTest {
      */
     @Test
     public final void testXpathUnmarshaller() throws Exception {
-        XMLInputFactory xmlInputFactory = XMLInputFactory.newInstance();
+        XMLInputFactory xmlInputFactory = XmlUtils.xmlInputFactory();
         XMLEventReader eventReader = xmlInputFactory.createXMLEventReader(DomainMetadataResultUnmarshallerTest.class
                                                                                   .getResourceAsStream("DomainMetadataResponse.xml"));
         StaxUnmarshallerContext unmarshallerContext = new StaxUnmarshallerContext(eventReader);

services/simpledb/src/test/java/software/amazon/awssdk/services/simpledb/transform/GetAttributesResultUnmarshallerTest.java

@@ -22,6 +22,7 @@
 import org.junit.Test;
 import software.amazon.awssdk.core.runtime.transform.StaxUnmarshallerContext;
 import software.amazon.awssdk.services.simpledb.model.GetAttributesResponse;
+import software.amazon.awssdk.utils.XmlUtils;
 
 public class GetAttributesResultUnmarshallerTest {
 
@@ -30,7 +31,7 @@ public class GetAttributesResultUnmarshallerTest {
      */
     @Test
     public final void testUnmarshall() throws Exception {
-        XMLInputFactory xmlInputFactory = XMLInputFactory.newInstance();
+        XMLInputFactory xmlInputFactory = XmlUtils.xmlInputFactory();
         XMLEventReader eventReader = xmlInputFactory.createXMLEventReader(DomainMetadataResultUnmarshallerTest.class
                                                                                   .getResourceAsStream("GetAttributesResponse.xml"));
         StaxUnmarshallerContext unmarshallerContext = new StaxUnmarshallerContext(eventReader);

services/simpledb/src/test/java/software/amazon/awssdk/services/simpledb/transform/ListDomainsResultUnmarshallerTest.java

@@ -22,6 +22,7 @@
 import org.junit.Test;
 import software.amazon.awssdk.core.runtime.transform.StaxUnmarshallerContext;
 import software.amazon.awssdk.services.simpledb.model.ListDomainsResponse;
+import software.amazon.awssdk.utils.XmlUtils;
 
 public class ListDomainsResultUnmarshallerTest {
 
@@ -30,7 +31,7 @@ public class ListDomainsResultUnmarshallerTest {
      */
     @Test
     public final void testUnmarshall() throws Exception {
-        XMLInputFactory xmlInputFactory = XMLInputFactory.newInstance();
+        XMLInputFactory xmlInputFactory = XmlUtils.xmlInputFactory();
         XMLEventReader eventReader = xmlInputFactory.createXMLEventReader(DomainMetadataResultUnmarshallerTest.class
                                                                                   .getResourceAsStream("ListDomainsResponse.xml"));
         StaxUnmarshallerContext unmarshallerContext = new StaxUnmarshallerContext(eventReader);

services/simpledb/src/test/java/software/amazon/awssdk/services/simpledb/transform/SelectResultUnmarshallerTest.java

@@ -23,6 +23,7 @@
 import software.amazon.awssdk.core.runtime.transform.StaxUnmarshallerContext;
 import software.amazon.awssdk.services.simpledb.model.Item;
 import software.amazon.awssdk.services.simpledb.model.SelectResponse;
+import software.amazon.awssdk.utils.XmlUtils;
 
 public class SelectResultUnmarshallerTest {
 
@@ -31,7 +32,7 @@ public class SelectResultUnmarshallerTest {
      */
     @Test
     public final void testUnmarshall() throws Exception {
-        XMLInputFactory xmlInputFactory = XMLInputFactory.newInstance();
+        XMLInputFactory xmlInputFactory = XmlUtils.xmlInputFactory();
         XMLEventReader eventReader = xmlInputFactory.createXMLEventReader(DomainMetadataResultUnmarshallerTest.class
                                                                                   .getResourceAsStream("SelectResponse.xml"));
         StaxUnmarshallerContext unmarshallerContext = new StaxUnmarshallerContext(eventReader);

test/protocol-tests-core/src/main/java/software/amazon/awssdk/protocol/asserts/marshalling/XmlAsserts.java

@@ -18,11 +18,13 @@
 import static org.junit.Assert.fail;
 
 import java.io.StringWriter;
+import javax.xml.XMLConstants;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
 import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.transform.OutputKeys;
 import javax.xml.transform.Transformer;
+import javax.xml.transform.TransformerConfigurationException;
 import javax.xml.transform.TransformerFactory;
 import javax.xml.transform.dom.DOMSource;
 import javax.xml.transform.stream.StreamResult;
@@ -71,11 +73,18 @@ private static String formatXml(String xmlDocumentString) throws Exception {
     }
 
     private static String formatXml(Document xmlDocument) throws Exception {
-        Transformer transformer = TransformerFactory.newInstance().newTransformer();
+        Transformer transformer = transformerFactory().newTransformer();
         transformer.setOutputProperty(OutputKeys.INDENT, "yes");
         StreamResult result = new StreamResult(new StringWriter());
         DOMSource source = new DOMSource(xmlDocument);
         transformer.transform(source, result);
         return result.getWriter().toString();
     }
+
+    private static TransformerFactory transformerFactory() throws TransformerConfigurationException {
+        TransformerFactory factory = TransformerFactory.newInstance();
+        factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+        factory.setFeature(XMLConstants.ACCESS_EXTERNAL_DTD, false);
+        return factory;
+    }
 }

utils/src/main/java/software/amazon/awssdk/utils/XmlUtils.java

@@ -0,0 +1,65 @@
+/*
+ * Copyright 2010-2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License").
+ * You may not use this file except in compliance with the License.
+ * A copy of the License is located at
+ *
+ *  http://aws.amazon.com/apache2.0
+ *
+ * or in the "license" file accompanying this file. This file is distributed
+ * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
+ * permissions and limitations under the License.
+ */
+
+package software.amazon.awssdk.utils;
+
+import javax.xml.XMLConstants;
+import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.ParserConfigurationException;
+import javax.xml.stream.XMLInputFactory;
+
+public final class XmlUtils {
+
+    /**
+     * Shared factory for creating XML event readers
+     */
+    private static final ThreadLocal<XMLInputFactory> XML_INPUT_FACTORY =
+        ThreadLocal.withInitial(XmlUtils::createXmlInputFactory);
+
+    private XmlUtils() {
+    }
+
+    /**
+     * @return A {@link ThreadLocal} copy of {@link XMLInputFactory}.
+     */
+    public static XMLInputFactory xmlInputFactory() {
+        return XML_INPUT_FACTORY.get();
+    }
+
+    /**
+     * Disables certain dangerous features that attempt to automatically fetch DTDs
+     *
+     * See <a href="https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Prevention_Cheat_Sheet">OWASP XXE Cheat Sheet</a>
+     */
+    public static DocumentBuilderFactory documentBuilderFactory() throws ParserConfigurationException {
+        DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance();
+        factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+        factory.setFeature("http://xml.org/sax/features/external-general-entities", false);
+        factory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+        return factory;
+    }
+
+    /**
+     * Disables certain dangerous features that attempt to automatically fetch DTDs
+     *
+     * See <a href="https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Prevention_Cheat_Sheet">OWASP XXE Cheat Sheet</a>
+     */
+    private static XMLInputFactory createXmlInputFactory() {
+        XMLInputFactory factory = XMLInputFactory.newInstance();
+        factory.setProperty(XMLInputFactory.SUPPORT_DTD, false);
+        factory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
+        return factory;
+    }
+}