diff --git a/ci/tests/200-xslt-step-xxe/channels/01-xslt-xxe/channel.xml b/ci/tests/200-xslt-step-xxe/channels/01-xslt-xxe/channel.xml new file mode 100644 index 0000000000..5175d66711 --- /dev/null +++ b/ci/tests/200-xslt-step-xxe/channels/01-xslt-xxe/channel.xml @@ -0,0 +1,195 @@ + + 5ec00001-0000-4000-8000-00000000c1a5 + 2 + XSLT Step XXE + + 1 + + 0 + sourceConnector + + + + None + true + false + false + 1 + + + Default Resource + [Default Resource] + + + 1000 + + + + + + XSLT XXE + 0 + true + connectorMessage.getRawData() + xsltResult + + false + + + + RAW + RAW + + + JavaScript + + + + + + JavaScript + + + + + + + + Channel Reader + SOURCE + true + true + + + + 1 + Destination 1 + + + + false + false + 10000 + false + 0 + false + false + 1 + + false + + + Default Resource + [Default Resource] + + + 1000 + true + + none + ${message.encodedData} + + + + + RAW + RAW + + + JavaScript + + + + + + JavaScript + + + + + + + RAW + RAW + + + JavaScript + + + + + + JavaScript + + + + + + + + Channel Writer + DESTINATION + true + true + + + // Modify the message variable below to pre process data +return message; + // This script executes once after a message has been processed +// Responses returned from here will be stored as "Postprocessor" in the response map +return; + // This script executes once when the channel is deployed +// You only have access to the globalMap and globalChannelMap here to persist data +return; + // This script executes once when the channel is undeployed +// You only have access to the globalMap and globalChannelMap here to persist data +return; + + true + DEVELOPMENT + false + false + false + false + false + false + STARTED + true + + + SOURCE + STRING + mirth_source + + + TYPE + STRING + mirth_type + + + + None + + + + + Default Resource + [Default Resource] + + + + + + true + + + America/Chicago + + + true + false + + 1 + + + \ No newline at end of file diff --git a/ci/tests/200-xslt-step-xxe/channels/01-xslt-xxe/messages/01-external-entity/source b/ci/tests/200-xslt-step-xxe/channels/01-xslt-xxe/messages/01-external-entity/source new file mode 100644 index 0000000000..7d3c53932b --- /dev/null +++ b/ci/tests/200-xslt-step-xxe/channels/01-xslt-xxe/messages/01-external-entity/source @@ -0,0 +1 @@ +]>&x; \ No newline at end of file diff --git a/ci/tests/200-xslt-step-xxe/channels/01-xslt-xxe/messages/01-external-entity/source_status b/ci/tests/200-xslt-step-xxe/channels/01-xslt-xxe/messages/01-external-entity/source_status new file mode 100644 index 0000000000..17416ae33e --- /dev/null +++ b/ci/tests/200-xslt-step-xxe/channels/01-xslt-xxe/messages/01-external-entity/source_status @@ -0,0 +1 @@ +ERROR \ No newline at end of file diff --git a/ci/tests/200-xslt-step-xxe/channels/01-xslt-xxe/messages/02-benign-control/source b/ci/tests/200-xslt-step-xxe/channels/01-xslt-xxe/messages/02-benign-control/source new file mode 100644 index 0000000000..b277c80b54 --- /dev/null +++ b/ci/tests/200-xslt-step-xxe/channels/01-xslt-xxe/messages/02-benign-control/source @@ -0,0 +1 @@ +hello \ No newline at end of file diff --git a/ci/tests/200-xslt-step-xxe/channels/01-xslt-xxe/messages/02-benign-control/source_status b/ci/tests/200-xslt-step-xxe/channels/01-xslt-xxe/messages/02-benign-control/source_status new file mode 100644 index 0000000000..1316c06a69 --- /dev/null +++ b/ci/tests/200-xslt-step-xxe/channels/01-xslt-xxe/messages/02-benign-control/source_status @@ -0,0 +1 @@ +TRANSFORMED \ No newline at end of file diff --git a/server/src/main/java/com/mirth/connect/plugins/xsltstep/XsltStep.java b/server/src/main/java/com/mirth/connect/plugins/xsltstep/XsltStep.java index 1facca8b46..1b67b52829 100644 --- a/server/src/main/java/com/mirth/connect/plugins/xsltstep/XsltStep.java +++ b/server/src/main/java/com/mirth/connect/plugins/xsltstep/XsltStep.java @@ -70,7 +70,7 @@ private String getTransformationScript() { script.append("transformer = tFactory.newTransformer(new Packages.javax.xml.transform.stream.StreamSource(xsltTemplate));\n"); script.append("sourceVar = new Packages.java.io.StringReader(" + sourceXml + ");\n"); script.append("resultVar = new Packages.java.io.StringWriter();\n"); - script.append("transformer.transform(new Packages.javax.xml.transform.stream.StreamSource(sourceVar), new Packages.javax.xml.transform.stream.StreamResult(resultVar));\n"); + script.append("transformer.transform(Packages.com.mirth.connect.util.MirthXmlUtil.getSecureSource(sourceVar), new Packages.javax.xml.transform.stream.StreamResult(resultVar));\n"); return script.toString(); } diff --git a/server/src/main/java/com/mirth/connect/util/MirthXmlUtil.java b/server/src/main/java/com/mirth/connect/util/MirthXmlUtil.java index 10804100e7..b6406acf10 100644 --- a/server/src/main/java/com/mirth/connect/util/MirthXmlUtil.java +++ b/server/src/main/java/com/mirth/connect/util/MirthXmlUtil.java @@ -9,12 +9,14 @@ package com.mirth.connect.util; +import java.io.Reader; import java.io.StringReader; import java.io.StringWriter; import java.io.Writer; import java.util.Hashtable; import javax.xml.XMLConstants; +import javax.xml.parsers.SAXParserFactory; import javax.xml.transform.OutputKeys; import javax.xml.transform.Source; import javax.xml.transform.Templates; @@ -24,10 +26,12 @@ import javax.xml.transform.TransformerFactory; import javax.xml.transform.dom.DOMResult; import javax.xml.transform.dom.DOMSource; +import javax.xml.transform.sax.SAXSource; import javax.xml.transform.stream.StreamResult; import javax.xml.transform.stream.StreamSource; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.xml.sax.InputSource; public class MirthXmlUtil { @@ -114,6 +118,18 @@ public static String prettyPrint(String input) { return input; } + /** Returns a {@link Source} for XML from an untrusted origin. */ + public static Source getSecureSource(Reader reader) throws Exception { + // Use newDefaultInstance to avoid whatever is on the classpath that might + // be poisoned from the channel classloader. + SAXParserFactory factory = SAXParserFactory.newDefaultInstance(); + // False by default + factory.setNamespaceAware(true); + factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); + + return new SAXSource(factory.newSAXParser().getXMLReader(), new InputSource(reader)); + } + public static String decode(String entity) { if (entity.charAt(entity.length() - 1) == ';') // remove trailing // semicolon diff --git a/server/src/test/java/com/mirth/connect/plugins/xsltstep/XsltStepTest.java b/server/src/test/java/com/mirth/connect/plugins/xsltstep/XsltStepTest.java new file mode 100644 index 0000000000..b09d620867 --- /dev/null +++ b/server/src/test/java/com/mirth/connect/plugins/xsltstep/XsltStepTest.java @@ -0,0 +1,220 @@ +// SPDX-License-Identifier: MPL-2.0 +// SPDX-FileCopyrightText: Mitch Gaffigan +package com.mirth.connect.plugins.xsltstep; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import java.io.File; +import java.net.URL; +import java.net.URLClassLoader; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.HashMap; + +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.parsers.SAXParser; +import javax.xml.parsers.SAXParserFactory; + +import org.apache.commons.io.FileUtils; +import org.apache.commons.text.StringEscapeUtils; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.xml.sax.SAXException; +import org.xml.sax.SAXNotRecognizedException; +import org.xml.sax.SAXNotSupportedException; +import org.xml.sax.XMLReader; + +import com.mirth.connect.donkey.model.message.ConnectorMessage; +import com.mirth.connect.util.JavaScriptTestUtil; + +/** + * Covers the script {@link XsltStep} generates for a plain transformer step. The iterator form is + * covered by FilterTransformerIterableTest#testIteratorXsltStep. + */ +public class XsltStepTest { + + /** Copies the whole source document to the result, so anything leaked shows up in the output. */ + private static final String TEXT_XSLT = "" + + ""; + + private static File secret; + + @BeforeClass + public static void setup() throws Exception { + JavaScriptTestUtil.setup(); + + secret = File.createTempFile("xxe", ".txt"); + secret.deleteOnExit(); + FileUtils.write(secret, "canary", UTF_8); + } + + /** Dropping the entity and rejecting the message are both fine; leaking the file is not. */ + @Test + public void externalEntityInSourceIsNotResolved() throws Exception { + String xml = " ]>&xxe;"; + String outcome; + + try { + outcome = transform(xml, TEXT_XSLT); + } catch (Exception e) { + outcome = String.valueOf(e); + } + + assertFalse(outcome, outcome.contains("canary")); + } + + /** + * A DOCTYPE is refused outright, as everywhere else in the codebase, so the payload above is + * rejected before any entity is looked at. + */ + @Test + public void doctypeIsRejected() throws Exception { + try { + transform(" ]>&ok;", TEXT_XSLT); + fail("expected the DOCTYPE to be refused"); + } catch (Exception e) { + assertTrue(String.valueOf(e), String.valueOf(e).contains("DOCTYPE is disallowed")); + } + } + + @Test + public void benignDocumentIsTransformed() throws Exception { + assertEquals("hello", transform("hello", TEXT_XSLT)); + } + + /** The step restricts the message, not the stylesheet. */ + @Test + public void stylesheetMayImportFromAFileUri() throws Exception { + File imported = File.createTempFile("imported", ".xsl"); + imported.deleteOnExit(); + FileUtils.write(imported, "" + + "imported", UTF_8); + + String xslt = "" + + ""; + assertEquals("imported", transform("hello", xslt)); + } + + @Test + public void customFactoryIsUsed() throws Exception { + XsltStep step = step("hello", TEXT_XSLT); + step.setUseCustomFactory(true); + step.setCustomFactory("com.sun.org.apache.xalan.internal.xsltc.trax.TransformerFactoryImpl"); + assertEquals("hello", transform(step)); + } + + private static String transform(String sourceXml, String xslt) throws Exception { + return transform(step(sourceXml, xslt)); + } + + private static XsltStep step(String sourceXml, String xslt) { + XsltStep step = new XsltStep(); + step.setSourceXml("'" + StringEscapeUtils.escapeEcmaScript(sourceXml) + "'"); + step.setTemplate("'" + StringEscapeUtils.escapeEcmaScript(xslt) + "'"); + step.setResultVariable("xsltResult"); + return step; + } + + private static String transform(XsltStep step) throws Exception { + ConnectorMessage connectorMessage = new ConnectorMessage(); + connectorMessage.setMetaDataId(0); + connectorMessage.setChannelMap(new HashMap()); + + JavaScriptTestUtil.testTransformerStep(step, connectorMessage); + + return String.valueOf(connectorMessage.getChannelMap().get("xsltResult")); + } + + /** + * A channel's resource library is on the context classloader while the step runs, so a plain + * JAXP lookup resolves against it. Xerces 2.12.2 is the reported case: it parses normally but + * does not recognize accessExternalDTD, which failed every transformation in such a channel. + */ + @Test + public void parserFromChannelResourceLibraryIsNotUsed() throws Exception { + Thread currentThread = Thread.currentThread(); + ClassLoader contextClassLoader = currentThread.getContextClassLoader(); + + try (URLClassLoader resourceLibrary = resourceLibraryProviding(RejectsSecurityProperties.class)) { + currentThread.setContextClassLoader(resourceLibrary); + + // Guards the fixture: a plain lookup really would pick the channel's parser up. + assertEquals(RejectsSecurityProperties.class, SAXParserFactory.newInstance().getClass()); + + assertEquals("hello", transform("hello", TEXT_XSLT)); + } finally { + currentThread.setContextClassLoader(contextClassLoader); + } + } + + /** Builds a classloader that advertises the factory the way a jar in a library would. */ + private static URLClassLoader resourceLibraryProviding(Class factory) throws Exception { + Path root = Files.createTempDirectory("resource-library"); + root.toFile().deleteOnExit(); + + Path services = Files.createDirectories(root.resolve("META-INF").resolve("services")); + FileUtils.write(services.resolve(SAXParserFactory.class.getName()).toFile(), factory.getName(), UTF_8); + + return new URLClassLoader(new URL[] { root.toUri().toURL() }, XsltStepTest.class.getClassLoader()); + } + + /** Stands in for Xerces 2.12.2: parses normally, but rejects the security property. */ + public static class RejectsSecurityProperties extends SAXParserFactory { + + private final SAXParserFactory delegate = SAXParserFactory.newInstance("com.sun.org.apache.xerces.internal.jaxp.SAXParserFactoryImpl", null); + + @Override + public SAXParser newSAXParser() throws ParserConfigurationException, SAXException { + delegate.setNamespaceAware(isNamespaceAware()); + SAXParser parser = delegate.newSAXParser(); + + return new SAXParser() { + @Override + public void setProperty(String name, Object value) throws SAXNotRecognizedException { + throw new SAXNotRecognizedException("Property '" + name + "' is not recognized."); + } + + @Override + public Object getProperty(String name) throws SAXNotRecognizedException { + throw new SAXNotRecognizedException("Property '" + name + "' is not recognized."); + } + + @Override + @SuppressWarnings("deprecation") + public org.xml.sax.Parser getParser() throws SAXException { + return parser.getParser(); + } + + @Override + public XMLReader getXMLReader() throws SAXException { + return parser.getXMLReader(); + } + + @Override + public boolean isNamespaceAware() { + return parser.isNamespaceAware(); + } + + @Override + public boolean isValidating() { + return parser.isValidating(); + } + }; + } + + @Override + public void setFeature(String name, boolean value) throws ParserConfigurationException, SAXNotRecognizedException, SAXNotSupportedException { + delegate.setFeature(name, value); + } + + @Override + public boolean getFeature(String name) throws ParserConfigurationException, SAXNotRecognizedException, SAXNotSupportedException { + return delegate.getFeature(name); + } + } +}