From 4a693c5aeeef2be6c7ecf80e7b5ec79f6ab59437 Mon Sep 17 00:00:00 2001
From: Jesse Wilson <jwilson@squareup.com>
Date: Tue, 17 Apr 2018 13:55:19 -0400
Subject: [PATCH] Lock down JAXB. Don't load remote entities.

XML is subject to XXE attacks.
https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Prevention_Cheat_Sheet#JAXB_Unmarshaller
---
 .../converter/jaxb/JaxbResponseConverter.java |  4 ++
 .../jaxb/JaxbConverterFactoryTest.java        | 51 +++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/retrofit-converters/jaxb/src/main/java/retrofit2/converter/jaxb/JaxbResponseConverter.java b/retrofit-converters/jaxb/src/main/java/retrofit2/converter/jaxb/JaxbResponseConverter.java
index 1b0666b52d..469b863d1a 100644
--- a/retrofit-converters/jaxb/src/main/java/retrofit2/converter/jaxb/JaxbResponseConverter.java
+++ b/retrofit-converters/jaxb/src/main/java/retrofit2/converter/jaxb/JaxbResponseConverter.java
@@ -33,6 +33,10 @@ final class JaxbResponseConverter<T> implements Converter<ResponseBody, T> {
   JaxbResponseConverter(JAXBContext context, Class<T> type) {
     this.context = context;
     this.type = type;
+
+    // Prevent XML External Entity attacks (XXE).
+    xmlInputFactory.setProperty(XMLInputFactory.IS_SUPPORTING_EXTERNAL_ENTITIES, false);
+    xmlInputFactory.setProperty(XMLInputFactory.SUPPORT_DTD, false);
   }
 
   @Override public T convert(ResponseBody value) throws IOException {
diff --git a/retrofit-converters/jaxb/src/test/java/retrofit2/converter/jaxb/JaxbConverterFactoryTest.java b/retrofit-converters/jaxb/src/test/java/retrofit2/converter/jaxb/JaxbConverterFactoryTest.java
index 1067f8af56..186d506fbb 100644
--- a/retrofit-converters/jaxb/src/test/java/retrofit2/converter/jaxb/JaxbConverterFactoryTest.java
+++ b/retrofit-converters/jaxb/src/test/java/retrofit2/converter/jaxb/JaxbConverterFactoryTest.java
@@ -146,4 +146,55 @@ interface Service {
     Response<Contact> response = call.execute();
     assertThat(response.body().name).isEqualTo("Jenny");
   }
+
+  @Test public void externalEntity() throws Exception {
+    server.enqueue(new MockResponse()
+        .setBody(""
+            + "<?xml version=\"1.0\" ?>"
+            + "<!DOCTYPE contact["
+            + "  <!ENTITY secret SYSTEM \"" + server.url("/secret.txt") + "\">"
+            + "]>"
+            + "<contact>"
+            + "<name>&secret;</name>"
+            + "</contact>"));
+    server.enqueue(new MockResponse()
+        .setBody("hello"));
+
+    Call<Contact> call = service.getXml();
+    try {
+      Response<Contact> response = call.execute();
+      response.body();
+      fail();
+    } catch (RuntimeException expected) {
+      assertThat(expected).hasMessageContaining("ParseError");
+    }
+
+    assertThat(server.getRequestCount()).isEqualTo(1);
+  }
+
+  @Test public void externalDtd() throws Exception {
+    server.enqueue(new MockResponse()
+        .setBody(""
+            + "<?xml version=\"1.0\" ?>"
+            + "<!DOCTYPE contact SYSTEM \"" + server.url("/contact.dtd") + "\">"
+            + "<contact>"
+            + "<name>&secret;</name>"
+            + "</contact>"));
+    server.enqueue(new MockResponse()
+        .setBody(""
+            + "<!ELEMENT contact (name)>\n"
+            + "<!ELEMENT name (#PCDATA)>\n"
+            + "<!ENTITY secret \"hello\">"));
+
+    Call<Contact> call = service.getXml();
+    try {
+      Response<Contact> response = call.execute();
+      response.body();
+      fail();
+    } catch (RuntimeException expected) {
+      assertThat(expected).hasMessageContaining("ParseError");
+    }
+
+    assertThat(server.getRequestCount()).isEqualTo(1);
+  }
 }