Fix XXE issue in XML parser

hapi-base/src/main/java/ca/uhn/hl7v2/parser/XMLParser.java

@@ -509,7 +509,7 @@ private void parseComposite(Composite datatypeObject, Element datatypeElement)
 						compNum = Integer.parseInt(localName.substring(dotIndex + 1)) - 1;
 					} else {
 						log.debug(
-								"Datatype element {} doesn't have a valid numbered name, usgin default index of {}",
+								"Datatype element {} doesn't have a valid numbered name, using default index of {}",
 								datatypeElement.getLocalName(), compNum);
 					}
 					Type nextComponent = datatypeObject.getComponent(compNum);

hapi-base/src/main/java/ca/uhn/hl7v2/util/XMLUtils.java

@@ -25,10 +25,10 @@ GNU General Public License (the  "GPL"), in which case the provisions of the GPL
  */
 package ca.uhn.hl7v2.util;
 
-import java.io.InputStream;
-import java.io.StringWriter;
-import java.io.Writer;
+import java.io.*;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.w3c.dom.DOMConfiguration;
 import org.w3c.dom.DOMErrorHandler;
 import org.w3c.dom.DOMImplementation;
@@ -37,11 +37,17 @@ GNU General Public License (the  "GPL"), in which case the provisions of the GPL
 import org.w3c.dom.ls.DOMImplementationLS;
 import org.w3c.dom.ls.LSInput;
 import org.w3c.dom.ls.LSOutput;
-import org.w3c.dom.ls.LSParser;
 import org.w3c.dom.ls.LSResourceResolver;
 import org.w3c.dom.ls.LSSerializer;
+import org.xml.sax.InputSource;
+import org.xml.sax.SAXException;
+
+import javax.xml.parsers.DocumentBuilder;
+import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.ParserConfigurationException;
 
 public class XMLUtils {
+    private static final Logger ourLog = LoggerFactory.getLogger(XMLUtils.class);
 
     private static DOMImplementation IMPL;
 
@@ -59,43 +65,50 @@ public synchronized static <T> T getDOMImpl() {
     }
 
     @SuppressWarnings("unchecked")
-    public static <T> T getDOMImplUncached() {
-        try {
-            DOMImplementationRegistry registry = DOMImplementationRegistry.newInstance();
-            return (T) registry.getDOMImplementation("LS 3.0");
-        } catch (Exception e) {
-            throw new RuntimeException(e);
-        }
-    }
-
     public static Document parse(String s) {
-        return parse(s, false);
+        return parseDocument(new InputSource(new StringReader(s)), false);
     }
 
     public static Document parse(String s, boolean validateIfSchema) {
-        DOMImplementationLS impl = getDOMImpl();
-        LSInput input = impl.createLSInput();
-        input.setStringData(s);
-        return parse(input, validateIfSchema);
+        return parseDocument(new InputSource(new StringReader(s)), validateIfSchema);
     }
 
     public static Document parse(InputStream s, boolean validateIfSchema) {
-        DOMImplementationLS impl = getDOMImpl();
-        LSInput input = impl.createLSInput();
-        input.setByteStream(s);
-        return parse(input, validateIfSchema);
+        return parseDocument(new InputSource(s), validateIfSchema);
     }
 
-    private static Document parse(LSInput input, boolean validateIfSchema) {
-        DOMImplementationLS impl = getDOMImpl();
-        LSParser parser = impl.createLSParser(DOMImplementationLS.MODE_SYNCHRONOUS, null);
-        DOMConfiguration config = parser.getDomConfig();
-        config.setParameter("element-content-whitespace", false);
-        config.setParameter("namespaces", true);
-        config.setParameter("validate-if-schema", validateIfSchema);
-        return parser.parse(input);
+    public static Document parseDocument(InputSource theInputSource, boolean theValidating) {
+        DocumentBuilder builder;
+        try {
+            DocumentBuilderFactory docBuilderFactory = DocumentBuilderFactory.newInstance();
+            docBuilderFactory.setNamespaceAware(true);
+            docBuilderFactory.setXIncludeAware(false);
+            docBuilderFactory.setExpandEntityReferences(false);
+            docBuilderFactory.setValidating(theValidating);
+            try {
+                docBuilderFactory.setFeature(
+                        "http://apache.org/xml/features/disallow-doctype-decl", false);
+                docBuilderFactory.setFeature("http://xml.org/sax/features/external-general-entities", false);
+                docBuilderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-dtd-grammar", false);
+                docBuilderFactory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false);
+                docBuilderFactory.setFeature("http://xml.org/sax/features/external-parameter-entities", false);
+            } catch (Exception e) {
+                ourLog.warn("Failed to set feature on XML parser: " + e.toString());
+            }
+
+            builder = docBuilderFactory.newDocumentBuilder();
+        } catch (ParserConfigurationException e) {
+            throw new RuntimeException(e);
+        }
+
+        try {
+            return builder.parse(theInputSource);
+        } catch (SAXException | IOException e) {
+            throw new RuntimeException(e);
+        }
     }
 
+
     public static void validate(Document d, String schema, DOMErrorHandler handler) {
         DOMConfiguration config = d.getDomConfig();
         config.setParameter("schema-type", "http://www.w3.org/2001/XMLSchema");

hapi-test/pom.xml

@@ -195,9 +195,8 @@
 		</dependency>
 		<dependency>
 			<groupId>org.mockito</groupId>
-			<artifactId>mockito-all</artifactId>
-			<version>${mockito.version}</version>
-			<scope>test</scope>
+			<artifactId>mockito-core</artifactId>
+			<version>${mockito_version}</version>
 		</dependency>
 		<dependency>
 			<groupId>org.awaitility</groupId>

hapi-test/src/test/java/ca/uhn/hl7v2/app/ConnectionHubTest.java

@@ -36,11 +36,7 @@
 import static org.junit.Assert.assertNotSame;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.times;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.verifyNoMoreInteractions;
-import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.*;
 
 /**
  * JUnit test harmess for ConnectionHub connecting to a SimpleServer
@@ -96,7 +92,7 @@ public void testConnectWithTlsSocketFactory() throws HL7Exception, IOException {
 		HapiContext context = new DefaultHapiContext();
 		context.setValidationContext(ValidationContextFactory.noValidation());
 
-		SocketFactory socketFactory = mock(SocketFactory.class);
+		SocketFactory socketFactory = mock(SocketFactory.class, withSettings().verboseLogging());
 		context.setSocketFactory(socketFactory);
 		when(socketFactory.createTlsSocket()).thenReturn(
 				javax.net.SocketFactory.getDefault().createSocket());

hapi-test/src/test/java/ca/uhn/hl7v2/parser/XMLParserTest.java

@@ -12,16 +12,14 @@
 import static org.junit.Assert.assertTrue;
 // import hapi.on.olis.erp_z99.message.ERP_R09;
 
-import java.io.BufferedReader;
-import java.io.IOException;
-import java.io.InputStream;
-import java.io.InputStreamReader;
+import java.io.*;
 
 import ca.uhn.hl7v2.DefaultHapiContext;
 import ca.uhn.hl7v2.HapiContext;
 import ca.uhn.hl7v2.Version;
 import ca.uhn.hl7v2.model.GenericMessage;
 import ca.uhn.hl7v2.util.Hl7InputStreamMessageIterator;
+import ca.uhn.hl7v2.util.XMLUtils;
 import org.junit.Before;
 import org.junit.Ignore;
 import org.junit.Test;
@@ -39,6 +37,14 @@
 import ca.uhn.hl7v2.model.v25.message.ORU_R01;
 import ca.uhn.hl7v2.model.v25.segment.OBX;
 import ca.uhn.hl7v2.util.Terser;
+import org.w3c.dom.Node;
+
+import javax.xml.transform.OutputKeys;
+import javax.xml.transform.Transformer;
+import javax.xml.transform.TransformerException;
+import javax.xml.transform.TransformerFactory;
+import javax.xml.transform.dom.DOMSource;
+import javax.xml.transform.stream.StreamResult;
 
 /**
  * JUnit test harness for XMLParser
@@ -467,6 +473,20 @@ public void testParseEncodedXml() throws Exception {
 		}
 	}
 
+	@Test
+	public void testParseRejextXxe() throws Exception {
+		String msg = loadFile("/ca/uhn/hl7v2/parser/xml-with-xxe.xml");
+
+		XMLParser p = DefaultXMLParser.getInstanceWithNoValidation();
+		ADT_A01 a01 = (ADT_A01) p.parse(msg);
+
+		ST msh10MessageControlID = a01.getMSH().getMsh10_MessageControlID();
+		String value = msh10MessageControlID.getValue();
+		ourLog.info("Encoded: {}", value);
+		assertEquals("", value);
+
+	}
+
 	@Test
 	public void testEncodeGenericMessage() throws Exception {
 		DefaultXMLParser xmlParser = new DefaultXMLParser();

hapi-test/src/test/java/ca/uhn/hl7v2/protocol/impl/ApplicationRouterImplTest.java

@@ -26,6 +26,7 @@
 import ca.uhn.hl7v2.protocol.ReceivingApplicationExceptionHandler;
 import ca.uhn.hl7v2.protocol.Transportable;
 import ca.uhn.hl7v2.util.Terser;
+import org.mockito.MockSettings;
 
 /**
  * Unit tests for <code>ApplicationRouterImpl</code>.
@@ -74,17 +75,17 @@ public void testProcessMessage() throws Exception {
     @Test
     public void testExceptionHandlerWorksCorrectlyWithInvalidMessage() throws HL7Exception {
 
-        ReceivingApplicationExceptionHandler handler = mock(ReceivingApplicationExceptionHandler.class);
+        ReceivingApplicationExceptionHandler handler = mock(ReceivingApplicationExceptionHandler.class, withSettings().verboseLogging());
 
         String msg = "BAD MESSAGE";
         String respMsg = "BAD RESPONSE MESSAGE";
 
-        when(handler.processException(eq(msg), any(Map.class), any(String.class), any(Exception.class))).thenReturn(respMsg);
+        when(handler.processException(eq(msg), any(), any(), any())).thenReturn(respMsg);
 
         myRouter.setExceptionHandler(handler);
         myRouter.processMessage(new TransportableImpl(msg));
 
-        verify(handler).processException(eq(msg), any(Map.class), any(String.class), any(Exception.class));
+        verify(handler).processException(eq(msg), any(), any(), any());
     }
 
     @Test

hapi-test/src/test/java/ca/uhn/hl7v2/validation/MessageValidatorTest.java

@@ -3,7 +3,7 @@
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
-import static org.mockito.Matchers.any;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 

hapi-test/src/test/java/ca/uhn/hl7v2/validation/RespondingValidationExceptionHandlerTest.java

@@ -1,7 +1,7 @@
 package ca.uhn.hl7v2.validation;
 
 import static org.junit.Assert.assertEquals;
-import static org.mockito.Matchers.any;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 

hapi-test/src/test/resources/ca/uhn/hl7v2/parser/xml-with-xxe.xml

@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE foo [ <!ENTITY xxe SYSTEM "file:///etc/passwd"> ]>
+<ADT_A01 xmlns="urn:hl7-org:v2xml">
+    <MSH>
+        <MSH.1>|</MSH.1>
+        <MSH.2>^~\&amp;</MSH.2>
+        <MSH.9>
+            <MSG.1>ADT</MSG.1>
+            <MSG.2>A01</MSG.2>
+            <MSG.3>ADT_A01</MSG.3>
+        </MSH.9>
+        <MSH.11>
+            <PT.1>P</PT.1>
+        </MSH.11>
+        <MSH.10>&xxe;</MSH.10>
+        <MSH.12>
+            <VID.1>2.5</VID.1>
+        </MSH.12>
+    </MSH>
+</ADT_A01>

pom.xml

@@ -644,7 +644,7 @@
         <commons-lang.version>2.6</commons-lang.version>
 		<geronimo.jms.spec.version>1.1.1</geronimo.jms.spec.version>
         <hamcrest.version>2.2</hamcrest.version>
-        <mockito.version>1.10.19</mockito.version>
+		<mockito_version>4.8.1</mockito_version>
 		<jetty.version>6.1.26</jetty.version> <!-- !! -->
 		<junit.version>4.12</junit.version>
 		<log4j.version>1.2.17</log4j.version> <!-- !! -->
@@ -655,7 +655,6 @@
 		<log4j_version>${log4j.version}</log4j_version>
 		<slf4j_version>${slf4j.version}</slf4j_version>
 		<hapi_version_stable>${hapi.version.stable}</hapi_version_stable>
-		<project_parent_basedir>${project.parent.basedir}</project_parent_basedir>
 
 		<!-- Build-time Maven plugin versions -->
 		<maven.assembly.plugin.version>3.3.0</maven.assembly.plugin.version>

src/changes/changes.xml

@@ -6,6 +6,12 @@
 	</properties>
 	<body>
 		<release version="2.4" date="TBD">
+			<action type="fix" dev="James Agnew">
+				The XML parser was vulnerable to XXE injections (meaning that the XML
+				parser could be coerced into inadvertently loading the contents of files
+				on the local filesystem). It is highly recommended to upgrade to this
+				release in order to mitigate this issue.
+			</action>
 			<action type="change" dev="James Agnew">
 				As of HAPI 2.4, support for JDK 1.4 and JDK 1.5 has been withdrawn. It is
 				no longer feasible to produce these binaries, as the retrotranslator plugin