Skip to content

Prevent XML xxe on Strict HL7 parsing - #406

Draft
mgaffigan wants to merge 1 commit into
OpenIntegrationEngine:mainfrom
mgaffigan:fix/hl7-permits-xml-by-default
Draft

Prevent XML xxe on Strict HL7 parsing#406
mgaffigan wants to merge 1 commit into
OpenIntegrationEngine:mainfrom
mgaffigan:fix/hl7-permits-xml-by-default

Conversation

@mgaffigan

Copy link
Copy Markdown
Contributor

Avoids any possibility of XXE by not accepting XML input on a channel to receive HL7. Turns the message into an error - just as it would be for any other invalid format.

ER7Serializer error
ERROR MESSAGE: Error converting ER7 to XML
ca.uhn.hl7v2.parser.EncodingNotSupportedException: Determine encoding for message. The following is the first 50 chars of the message for reference, although this may not be where the issue is: <?xml version="1.0"?>
...

Breaking change for anyone relying on being able to send XML to a channel configured to receive HL7. Opt-out of the new behavior by toggling "Allow XML" in the datatype properties.

image

Thanks to Samuel Paschuan for reporting the issue.

Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
@github-actions

Copy link
Copy Markdown

Test Results

677 tests   677 ✅  2m 20s ⏱️
114 suites    0 💤
114 files      0 ❌

Results for commit dcb2781.

boolean exceptionThrown = false;
try {
strictSerializer.toXML(xmlDisguisedAsHl7);
} catch (MessageSerializerException e) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this specifically inspect the exception message and cause? That way it validates it threw because of the XML parsing and not because of some other serialization fault.

What is the output of this test before and after the other code changes?


@Test
public void testToXmlWithStrictParserAllowsXmlInputWhenOptedIn() throws Exception {
String xmlDisguisedAsHl7 = "<foo><bar>notreallyhl7</bar></foo>";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor this to a constant since its used in multiple tests

Message message = null;
source = source.trim();

if (source.length() > 0 && source.charAt(0) == '<') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking - How can this be documented in the release notes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants