From 2ea7bc57fac814bd10b36376edc2a1645ae1361c Mon Sep 17 00:00:00 2001 From: Kyle Thomson Date: Mon, 9 Oct 2017 11:24:46 -0700 Subject: [PATCH] Ensuring XML parsing does not resolve DTDs which can cause security issues --- .../awssdk/core/http/StaxResponseHandler.java | 14 ++- .../StaxResponseHandlerComponentTest.java | 92 +++++++++++++++++++ 2 files changed, 104 insertions(+), 2 deletions(-) create mode 100644 core/src/test/java/software/amazon/awssdk/core/http/StaxResponseHandlerComponentTest.java diff --git a/core/src/main/java/software/amazon/awssdk/core/http/StaxResponseHandler.java b/core/src/main/java/software/amazon/awssdk/core/http/StaxResponseHandler.java index ad72f8acb014..9aaf11e4bea2 100644 --- a/core/src/main/java/software/amazon/awssdk/core/http/StaxResponseHandler.java +++ b/core/src/main/java/software/amazon/awssdk/core/http/StaxResponseHandler.java @@ -50,8 +50,7 @@ public class StaxResponseHandler implements HttpResponseHandler { /** * Shared factory for creating XML event readers. */ - private static final XMLInputFactory XML_INPUT_FACTORY = XMLInputFactory.newInstance(); - + private static final XMLInputFactory XML_INPUT_FACTORY = createXmlInputFactory(); /** * The StAX unmarshaller to use when handling the response. */ @@ -169,6 +168,17 @@ 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}). diff --git a/core/src/test/java/software/amazon/awssdk/core/http/StaxResponseHandlerComponentTest.java b/core/src/test/java/software/amazon/awssdk/core/http/StaxResponseHandlerComponentTest.java new file mode 100644 index 000000000000..d6f4cf5cc6d0 --- /dev/null +++ b/core/src/test/java/software/amazon/awssdk/core/http/StaxResponseHandlerComponentTest.java @@ -0,0 +1,92 @@ +/* + * Copyright 2010-2017 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.core.http; + +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.get; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.stubFor; +import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.CoreMatchers.equalTo; +import static org.mockito.Mockito.*; + +import com.github.tomakehurst.wiremock.client.VerificationException; +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.junit.WireMockRule; +import java.io.ByteArrayInputStream; +import java.io.File; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import software.amazon.awssdk.core.interceptor.ExecutionAttributes; +import software.amazon.awssdk.core.runtime.transform.StaxUnmarshallerContext; +import software.amazon.awssdk.core.runtime.transform.Unmarshaller; + +public class StaxResponseHandlerComponentTest { + + @Rule + public WireMockRule wireMockServer = new WireMockRule(); + + @Rule + public TemporaryFolder temporaryFolder = new TemporaryFolder(); + + @Test(expected = VerificationException.class) + public void saxParserShouldNotExposeLocalFileSystem() throws Exception { + File tmpFile = temporaryFolder.newFile("contents.txt"); + Files.write(tmpFile.toPath(), "hello-world".getBytes(StandardCharsets.UTF_8)); + + String payload = " \n" + + " \n" + + "%asd; \n" + + "%c; \n" + + "]> \n" + + "&rrr;"; + + String entityString = " \n" + + "\">"; + + stubFor(get(urlPathEqualTo("/payload.dtd")).willReturn(aResponse().withBody(entityString))); + stubFor(get(urlPathEqualTo("/?hello-world")).willReturn(aResponse())); + + StaxResponseHandler responseHandler = new StaxResponseHandler<>(dummyUnmarshaller()); + + HttpResponse response = mock(HttpResponse.class); + when(response.getContent()).thenReturn(new ByteArrayInputStream(payload.getBytes(Charset.forName("UTF-8")))); + + try { + responseHandler.handle(response, mock(ExecutionAttributes.class)); + } catch (Exception e) { + //expected + } + + WireMock.verify(getRequestedFor(urlPathEqualTo("/?hello-world"))); //We expect this to fail, this call should not be made + } + + private Unmarshaller dummyUnmarshaller() { + return staxContext -> { + while(!staxContext.nextEvent().isEndDocument()) { + //read the whole document + } + return "Success"; + }; + } + +} \ No newline at end of file