Fix #339 - Disable XXE XML parsing vulnerability

hapi-fhir-base/src/main/java/ca/uhn/fhir/util/XmlUtil.java

@@ -1562,6 +1562,17 @@ public class XmlUtil {
 				logStaxImplementation(inputFactory.getClass());
 			}
 
+			/*
+			 * These two properties disable external entity processing, which can 
+			 * be a security vulnerability.
+			 * 
+			 * See https://github.com/jamesagnew/hapi-fhir/issues/339
+			 * https://www.owasp.org/index.php/XML_External_Entity_%28XXE%29_Processing
+			 */
+			inputFactory.setProperty(XMLInputFactory.SUPPORT_DTD, false); // This disables DTDs entirely for that factory
+			inputFactory.setProperty("javax.xml.stream.isSupportingExternalEntities", false); // disable external entities
+
+			
 			/*
 			 * In the following few lines, you can uncomment the first and comment the second to disable automatic
 			 * parsing of extended entities, e.g. §

hapi-fhir-base/src/main/java/ca/uhn/fhir/validation/SchemaBaseValidator.java

@@ -93,11 +93,26 @@ public class SchemaBaseValidator implements IValidatorModule {
 				encodedResource = theContext.getFhirContext().newXmlParser().encodeResourceToString((IBaseResource) theContext.getResource());
 			}
 
+			/*
+			 * See https://github.com/jamesagnew/hapi-fhir/issues/339
+			 * https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing
+			 */
+			validator.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, "");
+			validator.setProperty(XMLConstants.ACCESS_EXTERNAL_SCHEMA, "");
+
 			validator.validate(new StreamSource(new StringReader(encodedResource)));
+		} catch (SAXParseException e) {
+			SingleValidationMessage message = new SingleValidationMessage();
+			message.setLocationLine(e.getLineNumber());
+			message.setLocationCol(e.getColumnNumber());
+			message.setMessage(e.getLocalizedMessage());
+			message.setSeverity(ResultSeverityEnum.FATAL);
+			theContext.addValidationMessage(message);
 		} catch (SAXException e) {
-			throw new ConfigurationException("Could not apply schema file", e);
+			// Catch all
+			throw new ConfigurationException("Could not load/parse schema file", e);
 		} catch (IOException e) {
-			// This shouldn't happen since we're using a string source
+			// Catch all
 			throw new ConfigurationException("Could not load/parse schema file", e);
 		}
 	}
@@ -117,6 +132,13 @@ public class SchemaBaseValidator implements IValidatorModule {
 			schemaFactory.setResourceResolver(new MyResourceResolver());
 
 			try {
+				
+				/*
+				 * See https://github.com/jamesagnew/hapi-fhir/issues/339
+				 * https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing
+				 */
+				schemaFactory.setProperty(XMLConstants.ACCESS_EXTERNAL_DTD, "");
+				
 				schema = schemaFactory.newSchema(new Source[] { baseSource });
 			} catch (SAXException e) {
 				throw new ConfigurationException("Could not load/parse schema file: " + theSchemaName, e);

hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/parser/XmlParserDstu3Test.java

@@ -22,9 +22,9 @@ import java.io.IOException;
 import java.io.StringReader;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Date;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Locale;
 import java.util.UUID;
 
 import org.apache.commons.io.IOUtils;
@@ -146,7 +146,7 @@ public class XmlParserDstu3Test {
 		assertArrayEquals(new byte[] { 1, 2, 3, 4 }, bin.getContent());
 
 	}
-
+	
 	@Test
 	public void testContainedResourceInExtensionUndeclared() {
 		Patient p = new Patient();
@@ -1139,8 +1139,6 @@ public class XmlParserDstu3Test {
 		assertThat(encoded, containsString("maritalStatus"));
 	}
 
-	
-
 	@Test
 	public void testEncodeNonContained() {
 		// Create an organization
@@ -1181,6 +1179,8 @@ public class XmlParserDstu3Test {
 		
 	}
 
+	
+
 	/**
 	 * See #312
 	 */
@@ -1236,7 +1236,7 @@ public class XmlParserDstu3Test {
 		assertThat(str, containsString("<reference value=\"Patient/phitcc_pat_normal\"/>"));
 		assertThat(str, containsString("<reference value=\"Observation/phitcc_obs_bp_dia\"/>"));
 	}
-	
+
 	@Test
 	public void testEncodeSummary() {
 		Patient patient = new Patient();
@@ -1254,7 +1254,7 @@ public class XmlParserDstu3Test {
 		assertThat(encoded, containsString("family"));
 		assertThat(encoded, not(containsString("maritalStatus")));
 	}
-
+	
 	@Test
 	public void testEncodeSummary2() {
 		Patient patient = new Patient();
@@ -2536,6 +2536,38 @@ public class XmlParserDstu3Test {
 		assertEquals("Patient", reincarnatedPatient.getIdElement().getResourceType());
 	}
 
+	/**
+	 * See #339
+	 * 
+	 * https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing
+	 */
+	@Test
+	public void testXxe() {
+		//@formatter:off
+		String input =
+			"<?xml version=\"1.0\" encoding=\"ISO-8859-1\"?>\n" + 
+			"<!DOCTYPE foo [  \n" + 
+			"<!ELEMENT foo ANY >\n" + 
+			"<!ENTITY xxe SYSTEM \"file:///etc/passwd\" >]>" +
+			"<Patient xmlns=\"http://hl7.org/fhir\">" +
+				"<text>" + 
+					"<div xmlns=\"http://www.w3.org/1999/xhtml\">TEXT &xxe; TEXT</div>\n" + 
+				"</text>" +
+				"<address>" + 
+					"<line value=\"FOO\"/>" + 
+				"</address>" +
+			"</Patient>";
+		//@formatter:on
+		
+		try {
+			ourCtx.newXmlParser().parseResource(Patient.class, input);
+			fail();
+		} catch (DataFormatException e) {
+			assertThat(e.toString(), containsString("Undeclared general entity"));
+		}
+		
+	}
+
 	@AfterClass
 	public static void afterClassClearContext() {
 		TestUtil.clearAllStaticFieldsForUnitTest();

hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/validation/SchemaValidationTestDstu3.java

@@ -0,0 +1,63 @@
+package ca.uhn.fhir.validation;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.junit.Assert.*;
+
+import java.util.Locale;
+
+import org.hl7.fhir.dstu3.model.Patient;
+import org.junit.AfterClass;
+import org.junit.Test;
+
+import ca.uhn.fhir.context.FhirContext;
+import ca.uhn.fhir.parser.DataFormatException;
+import ca.uhn.fhir.util.TestUtil;
+
+public class SchemaValidationTestDstu3 {
+
+	private static FhirContext ourCtx = FhirContext.forDstu3();
+
+	private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(SchemaValidationTestDstu3.class);
+
+	/**
+	 * See #339
+	 * 
+	 * https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing
+	 */
+	@Test
+	public void testXxe() {
+		//@formatter:off
+		String input =
+			"<?xml version=\"1.0\" encoding=\"ISO-8859-1\"?>\n" + 
+			"<!DOCTYPE foo [  \n" + 
+			"<!ELEMENT foo ANY >\n" + 
+			"<!ENTITY xxe SYSTEM \"file:///etc/passwd\" >]>" +
+			"<Patient xmlns=\"http://hl7.org/fhir\">" +
+				"<text>" + 
+					"<status value=\"generated\"/>" +
+					"<div xmlns=\"http://www.w3.org/1999/xhtml\">TEXT &xxe; TEXT</div>\n" + 
+				"</text>" +
+				"<address>" + 
+					"<line value=\"FOO\"/>" + 
+				"</address>" +
+			"</Patient>";
+		//@formatter:on
+
+		FhirValidator val = ourCtx.newValidator();
+		val.setValidateAgainstStandardSchema(true);
+		val.setValidateAgainstStandardSchematron(false);
+		ValidationResult result = val.validateWithResult(input);
+
+		String encoded = ourCtx.newXmlParser().setPrettyPrint(true).encodeResourceToString(result.toOperationOutcome());
+		ourLog.info(encoded);
+
+		assertFalse(result.isSuccessful());
+		assertThat(encoded, containsString("passwd"));
+		assertThat(encoded, containsString("accessExternalDTD"));
+	}
+
+	@AfterClass
+	public static void afterClassClearContext() {
+		TestUtil.clearAllStaticFieldsForUnitTest();
+	}
+}

src/changes/changes.xml

@@ -7,6 +7,13 @@
 	</properties>
 	<body>
 		<release version="1.5" date="TBD">
+			<action type="fix" issue="339">
+				Security Fix: XML parser was vulnerable to XXE (XML External Entity) 
+				processing, which could result in local files on disk being disclosed.
+				See <![CDATA[<a href="https://www.owasp.org/index.php/XML_External_Entity_(XXE)_Processing">this page</a>]]>
+				for more information.
+				Thanks to Jim Steel for reporting!
+			</action>
 			<action type="add">
 				Bump the version of a few dependencies to the
 				latest versions (dependent HAPI modules listed in brackets):