diff --git a/server/src/main/java/com/mirth/connect/plugins/datatypes/hl7v2/ER7Serializer.java b/server/src/main/java/com/mirth/connect/plugins/datatypes/hl7v2/ER7Serializer.java index 71e9a49b54..05838b5df0 100644 --- a/server/src/main/java/com/mirth/connect/plugins/datatypes/hl7v2/ER7Serializer.java +++ b/server/src/main/java/com/mirth/connect/plugins/datatypes/hl7v2/ER7Serializer.java @@ -18,9 +18,12 @@ import java.util.Map; import java.util.regex.Pattern; +import javax.xml.parsers.DocumentBuilderFactory; + import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.w3c.dom.Document; import org.xml.sax.InputSource; import org.xml.sax.XMLReader; import org.xml.sax.helpers.XMLReaderFactory; @@ -522,6 +525,30 @@ protected Message instantiateMessage(String theName, String theVersion, boolean return message; } + + /* + * HAPI 2.3's XMLUtils.parse builds its DOM parser with no protection against external XML + * entities, so a strict-parsed inbound message carrying a DOCTYPE can trigger XXE (SSRF and + * local file disclosure), reachable unauthenticated over an MLLP/TCP listener. This is the + * only method that reaches that parser, so override it to reject any DOCTYPE up front, using + * the same disallow-doctype-decl hardening already applied to the fromXML path above. + * Legitimate HL7 v2.x XML never contains a DOCTYPE. + * + * This stays stronger than HAPI's own >= 2.4 fix, which permits a DOCTYPE and only disables + * entity resolution. Remove only once HAPI is upgraded to >= 2.4 AND that weaker posture is + * deliberately accepted. + */ + @Override + protected synchronized Document parseStringIntoDocument(String xml) throws HL7Exception { + try { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + factory.setNamespaceAware(true); + return factory.newDocumentBuilder().parse(new InputSource(new StringReader(xml))); + } catch (Exception e) { + throw new HL7Exception("Exception parsing XML", e); + } + } } } diff --git a/server/src/test/java/com/mirth/connect/plugins/datatypes/hl7v2/ER7SerializerTest.java b/server/src/test/java/com/mirth/connect/plugins/datatypes/hl7v2/ER7SerializerTest.java index a7319dca5a..67e77787f8 100644 --- a/server/src/test/java/com/mirth/connect/plugins/datatypes/hl7v2/ER7SerializerTest.java +++ b/server/src/test/java/com/mirth/connect/plugins/datatypes/hl7v2/ER7SerializerTest.java @@ -6,6 +6,7 @@ import java.io.File; import org.apache.commons.io.FileUtils; +import org.apache.commons.lang3.exception.ExceptionUtils; import org.junit.BeforeClass; import org.junit.Test; import org.xml.sax.SAXParseException; @@ -15,42 +16,89 @@ public class ER7SerializerTest { private static ER7Serializer serializer; - + // Strict parser with strict validation: XML input is parsed by HAPI (the XXE sink). + private static ER7Serializer strictValidatingSerializer; + @BeforeClass public static void setupClass() throws Exception { SerializerProperties serializerProperties = new SerializerProperties(new HL7v2SerializationProperties(), new HL7v2DeserializationProperties(), null); serializer = new ER7Serializer(serializerProperties); + + HL7v2SerializationProperties strictValidatingProperties = new HL7v2SerializationProperties(); + strictValidatingProperties.setUseStrictParser(true); + strictValidatingProperties.setUseStrictValidation(true); + strictValidatingSerializer = new ER7Serializer(new SerializerProperties(strictValidatingProperties, new HL7v2DeserializationProperties(), null)); } - + @Test public void testFromXMLWithExternalDTD() throws Exception { String xml = FileUtils.readFileToString(new File("tests/test-xxe-hl7-example.xml"), "UTF-8"); - + boolean exceptionThrown = false; try { serializer.fromXML(xml); } catch (MessageSerializerException e) { exceptionThrown = true; - + // See https://cheatsheetseries.owasp.org/cheatsheets/XML_External_Entity_Prevention_Cheat_Sheet.html#jaxp-documentbuilderfactory-saxparserfactory-and-dom4j assertTrue(e.getCause() instanceof SAXParseException); } - + assertTrue(exceptionThrown); } @Test public void testValidFromXMLWithExternalDTD() throws Exception { String xml = FileUtils.readFileToString(new File("tests/test-xxe-hl7-example-valid.xml"), "UTF-8"); - + boolean exceptionThrown = false; try { serializer.fromXML(xml); } catch (MessageSerializerException e) { exceptionThrown = true; - + + } + + assertFalse(exceptionThrown); + } + + @Test + public void testToXmlStrictValidatingRejectsExternalDTD() throws Exception { + // A DOCTYPE-bearing message on the strict-parser toXML path is the unauthenticated MLLP XXE + // vector (HAPI 2.3 resolved external entities). It must be rejected rather than have its + // external entity resolved. Note: this asserts the intended behavior; the discriminating + // before/after proof that the override (not incidental parser behavior) closes the XXE is the + // live MLLP reproduction documented in the PR. + String xml = FileUtils.readFileToString(new File("tests/test-xxe-hl7-strict-mllp.xml"), "UTF-8"); + + boolean exceptionThrown = false; + try { + strictValidatingSerializer.toXML(xml); + } catch (MessageSerializerException e) { + exceptionThrown = true; + + // The rejection must be the DOCTYPE being disallowed, not some incidental parse failure. + Throwable rootCause = ExceptionUtils.getRootCause(e); + assertTrue(rootCause instanceof SAXParseException); + assertTrue(rootCause.getMessage().contains("DOCTYPE")); + } + + assertTrue(exceptionThrown); + } + + @Test + public void testToXmlStrictValidatingAllowsValidXml() throws Exception { + // The same message without a DOCTYPE is legitimate HL7 v2.x XML and must still round-trip, so + // the hardening does not break the strict parser's XML support. + String xml = FileUtils.readFileToString(new File("tests/test-xxe-hl7-strict-mllp-valid.xml"), "UTF-8"); + + boolean exceptionThrown = false; + try { + strictValidatingSerializer.toXML(xml); + } catch (MessageSerializerException e) { + exceptionThrown = true; } - + assertFalse(exceptionThrown); } } diff --git a/server/tests/test-xxe-hl7-strict-mllp-valid.xml b/server/tests/test-xxe-hl7-strict-mllp-valid.xml new file mode 100644 index 0000000000..0622b17fb9 --- /dev/null +++ b/server/tests/test-xxe-hl7-strict-mllp-valid.xml @@ -0,0 +1,2 @@ + +|^~\&APPACK12.4AA1 diff --git a/server/tests/test-xxe-hl7-strict-mllp.xml b/server/tests/test-xxe-hl7-strict-mllp.xml new file mode 100644 index 0000000000..3433b023de --- /dev/null +++ b/server/tests/test-xxe-hl7-strict-mllp.xml @@ -0,0 +1,3 @@ + + ]> +|^~\&&xxe;ACK12.4AA1