-
Notifications
You must be signed in to change notification settings - Fork 75
Fix SQLi + XXE CVEs (CVE-2026-82583, -78224, -82578) with regression tests #441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,13 +13,32 @@ classpath=$(find "$ENGINE_HOME/server-lib" "$ENGINE_HOME/extensions" "$HARNESS_D | |
| results="$RESULTS_DIR/$OIE_CONFIGURATION" | ||
| mkdir -p "$results" | ||
|
|
||
| # The client SDK deserializes model objects with XStream, whose reflective converters need the | ||
| # same module access the server's own test task opens (server/build.gradle). Without at least | ||
| # java.base/java.util opened, deserializing a SortedSet response (e.g. the JDBC connector's | ||
| # getTables result) fails with InaccessibleObjectException on JDK 17. | ||
| jvm_opts=( | ||
| --add-exports=java.base/com.sun.crypto.provider=ALL-UNNAMED | ||
| --add-opens=java.base/java.util=ALL-UNNAMED | ||
| --add-opens=java.base/java.lang=ALL-UNNAMED | ||
| --add-opens=java.base/java.lang.reflect=ALL-UNNAMED | ||
| --add-opens=java.base/java.text=ALL-UNNAMED | ||
| --add-opens=java.sql/java.sql=ALL-UNNAMED | ||
| --add-opens=java.xml/com.sun.org.apache.xalan.internal.xsltc.trax=ALL-UNNAMED | ||
| ) | ||
|
Comment on lines
+16
to
+28
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They should be the same as the runtime oieserver - not the other tests, I would presume. |
||
|
|
||
| # Split the extra -D flags (set per configuration in run-configuration.sh) into an array so they | ||
| # reach the java command as separate arguments without unquoted globbing/word-splitting. | ||
| read -ra harness_opts <<< "${OIE_HARNESS_OPTS:-}" | ||
|
Comment on lines
+30
to
+32
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we avoid? If the options come from a file, I should assume this is not required. |
||
|
|
||
| # Lock engine to junit-jupiter to prevent false-pass results. | ||
| status=0 | ||
| java \ | ||
| "${jvm_opts[@]}" \ | ||
| -Doie.baseUrl="$OIE_BASE_URL" \ | ||
| -Doie.configuration="$OIE_CONFIGURATION" \ | ||
| -Doie.password="$OIE_PASSWORD" \ | ||
| ${OIE_HARNESS_OPTS:-} \ | ||
| "${harness_opts[@]}" \ | ||
| -cp "$classpath" \ | ||
| org.junit.platform.console.ConsoleLauncher execute \ | ||
| --select-package=org.openintegrationengine.smoketest \ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,7 +33,9 @@ | |
| import org.apache.logging.log4j.Logger; | ||
|
|
||
| import com.mirth.connect.client.core.api.MirthApiException; | ||
| import com.mirth.connect.model.DriverInfo; | ||
| import com.mirth.connect.server.api.MirthServlet; | ||
| import com.mirth.connect.server.controllers.ConfigurationController; | ||
| import com.mirth.connect.server.controllers.ContextFactoryController; | ||
| import com.mirth.connect.server.controllers.ControllerFactory; | ||
| import com.mirth.connect.server.util.TemplateValueReplacer; | ||
|
|
@@ -45,13 +47,18 @@ public class DatabaseConnectorServlet extends MirthServlet implements DatabaseCo | |
| private static final Logger logger = LogManager.getLogger(DatabaseConnectorServlet.class); | ||
| private static final TemplateValueReplacer replacer = new TemplateValueReplacer(); | ||
| private static final ContextFactoryController contextFactoryController = ControllerFactory.getFactory().createContextFactoryController(); | ||
| private static final ConfigurationController configurationController = ControllerFactory.getFactory().createConfigurationController(); | ||
|
|
||
| public DatabaseConnectorServlet(@Context HttpServletRequest request, @Context SecurityContext sc) { | ||
| super(request, sc, PLUGIN_POINT); | ||
| } | ||
|
|
||
| @Override | ||
| public SortedSet<Table> getTables(String channelId, String channelName, String driver, String url, String username, String password, Set<String> tableNamePatterns, String selectLimit, Set<String> resourceIds) { | ||
| // Reject any selectLimit that is not one the server itself configured, before it can be | ||
| // executed as SQL (CVE-2026-82583). Done outside the try below so it is not re-wrapped. | ||
| validateSelectLimit(selectLimit); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this actually an issue? The whole point of this connector is to run arbitrary queries against a database, and I've not heard anything of an auth bypass. If we're always validating against the constant from the driver, why is it a parameter? What's the intended use of the parameter? |
||
|
|
||
| CustomDriver customDriver = null; | ||
| Connection connection = null; | ||
| try { | ||
|
|
@@ -229,6 +236,49 @@ public SortedSet<Table> getTables(String channelId, String channelName, String d | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Validates the caller-supplied {@code selectLimit} against the server's configured driver list | ||
| * before it is ever executed as SQL. The Database connector metadata dialog only ever sends a | ||
| * {@code selectLimit} taken from the configured drivers (dbdrivers.xml / {@link DriverInfo}), so | ||
| * any other value is a SQL-injection attempt (CVE-2026-82583) and is rejected. A blank value is | ||
| * allowed: it routes to the safe {@link DatabaseMetaData#getColumns} path. The exception message | ||
| * is deliberately generic so the rejected value is not reflected back to the caller, and the | ||
| * built-in default drivers are always included so an empty/cleared configured list cannot | ||
| * disable the check (fail closed). | ||
| */ | ||
| private void validateSelectLimit(String selectLimit) { | ||
| if (StringUtils.isBlank(selectLimit)) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| return; | ||
| } | ||
|
|
||
| Set<String> allowedSelectLimits = new HashSet<String>(); | ||
| addSelectLimits(allowedSelectLimits, DriverInfo.getDefaultDrivers()); | ||
|
|
||
| try { | ||
| addSelectLimits(allowedSelectLimits, configurationController.getDatabaseDrivers()); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The allowlist is populated from configuration the same caller can write.
So: PUT the payload as a driver's Am I reading the stock authorization path right? If so, deriving |
||
| } catch (Exception e) { | ||
| // Fall back to the built-in driver list rather than failing open if the configured | ||
| // list cannot be read. | ||
| logger.warn("Could not load configured database drivers for selectLimit validation; using built-in defaults.", e); | ||
| } | ||
|
|
||
| if (!allowedSelectLimits.contains(selectLimit.trim())) { | ||
| logger.warn("Rejected database metadata request with a selectLimit that is not in the configured driver list."); | ||
| throw new MirthApiException("The provided selectLimit is not permitted."); | ||
| } | ||
| } | ||
|
|
||
| private void addSelectLimits(Set<String> allowedSelectLimits, List<DriverInfo> drivers) { | ||
| if (drivers == null) { | ||
| return; | ||
| } | ||
| for (DriverInfo driver : drivers) { | ||
| if (driver != null && driver.getSelectLimit() != null) { | ||
| allowedSelectLimits.add(driver.getSelectLimit().trim()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Translate the given pattern expression so that it can be used properly for searching tables | ||
| * in the database. Multiple table name patterns are delimited by comma (,) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| import java.util.Map; | ||
|
|
||
| import javax.xml.XMLConstants; | ||
| import javax.xml.parsers.DocumentBuilderFactory; | ||
| import javax.xml.transform.OutputKeys; | ||
| import javax.xml.transform.Transformer; | ||
| import javax.xml.transform.TransformerFactory; | ||
|
|
@@ -33,6 +34,7 @@ | |
| import org.mozilla.javascript.Context; | ||
| import org.mozilla.javascript.Script; | ||
| import org.mozilla.javascript.Scriptable; | ||
| import org.w3c.dom.Document; | ||
| import org.w3c.dom.Node; | ||
| import org.w3c.dom.NodeList; | ||
| import org.xml.sax.InputSource; | ||
|
|
@@ -42,6 +44,7 @@ | |
| import com.mirth.connect.donkey.server.message.batch.BatchMessageException; | ||
| import com.mirth.connect.donkey.server.message.batch.BatchMessageReader; | ||
| import com.mirth.connect.donkey.server.message.batch.BatchMessageReceiver; | ||
| import com.mirth.connect.model.converters.DocumentSerializer; | ||
| import com.mirth.connect.plugins.datatypes.xml.XMLBatchProperties.SplitType; | ||
| import com.mirth.connect.server.controllers.ContextFactoryController; | ||
| import com.mirth.connect.server.controllers.ControllerFactory; | ||
|
|
@@ -127,7 +130,19 @@ private String getMessageFromReader() throws Exception { | |
|
|
||
| XPath xpath = xPathFactory.newXPath(); | ||
|
|
||
| nodeList = (NodeList) xpath.evaluate(query.toString(), new InputSource(bufferedReader), XPathConstants.NODESET); | ||
| // Parse the untrusted batch with a hardened parser before evaluating XPath, rather than | ||
| // letting XPath.evaluate(InputSource) build its own DOCTYPE-resolving parser (XXE, | ||
| // CVE-2026-82578). getSecureDocumentBuilderFactory() already sets disallow-doctype-decl; | ||
| // the extra features below block external entities/DTDs and entity expansion outright. | ||
| DocumentBuilderFactory dbf = DocumentSerializer.getSecureDocumentBuilderFactory(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| dbf.setFeature("http://xml.org/sax/features/external-general-entities", false); | ||
| dbf.setFeature("http://xml.org/sax/features/external-parameter-entities", false); | ||
| dbf.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); | ||
| dbf.setXIncludeAware(false); | ||
| dbf.setExpandEntityReferences(false); | ||
| Document document = dbf.newDocumentBuilder().parse(new InputSource(bufferedReader)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Separately: |
||
|
|
||
| nodeList = (NodeList) xpath.evaluate(query.toString(), document, XPathConstants.NODESET); | ||
| } | ||
|
|
||
| if (currentNode < nodeList.getLength()) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -66,6 +66,14 @@ private String getTransformationScript() { | |||||||||||||
| script.append("tFactory = Packages.javax.xml.transform.TransformerFactory.newInstance();\n"); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Harden the factory against XXE (CVE-2026-78224): enable secure processing and forbid | ||||||||||||||
| // external DTD/stylesheet access so external entities in the stylesheet or the source XML | ||||||||||||||
| // are not resolved. setAttribute is guarded because some implementations (e.g. Saxon) reject | ||||||||||||||
| // these attributes; secure processing alone still applies. Mirrors XmlProcessor.configureSecureTF. | ||||||||||||||
| script.append("tFactory.setFeature(Packages.javax.xml.XMLConstants.FEATURE_SECURE_PROCESSING, true);\n"); | ||||||||||||||
| script.append("try { tFactory.setAttribute(Packages.javax.xml.XMLConstants.ACCESS_EXTERNAL_DTD, ''); } catch (e) {}\n"); | ||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These catch blocks drop the restrictions with no signal when a factory rejects them. Emitted sequence on JDK 21:
Secure processing succeeding on 9.x means nothing indicates the other two failed — it covers extension functions, not external document access.
|
||||||||||||||
| script.append("try { tFactory.setAttribute(Packages.javax.xml.XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ''); } catch (e) {}\n"); | ||||||||||||||
|
Comment on lines
+73
to
+75
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we expect these to succeed, we should not be swallowing all errors. |
||||||||||||||
|
|
||||||||||||||
| script.append("xsltTemplate = new Packages.java.io.StringReader(" + template + ");\n"); | ||||||||||||||
| script.append("transformer = tFactory.newTransformer(new Packages.javax.xml.transform.stream.StreamSource(xsltTemplate));\n"); | ||||||||||||||
| script.append("sourceVar = new Packages.java.io.StringReader(" + sourceXml + ");\n"); | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| /* | ||
| * Copyright (c) Open Integration Engine. All rights reserved. | ||
| * | ||
| * The software in this package is published under the terms of the MPL license a copy of which has | ||
| * been included with this distribution in the LICENSE.txt file. | ||
| */ | ||
|
|
||
| package com.mirth.connect.plugins.xsltstep; | ||
|
|
||
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| import org.junit.Test; | ||
|
|
||
| /** | ||
| * Guards the XXE hardening of the XSLT transformer step (CVE-2026-78224). The step emits JavaScript | ||
| * that builds a TransformerFactory at runtime, so the fix lives in the generated script text. | ||
| */ | ||
| public class XsltStepSecurityTest { | ||
|
|
||
| @Test | ||
| public void generatedScriptHardensTransformerFactoryAgainstXxe() { | ||
| XsltStep step = new XsltStep(); | ||
| step.setSourceXml("connectorMessage.getRawData()"); | ||
| step.setResultVariable("xsltResult"); | ||
| step.setTemplate("'<xsl:stylesheet version=\"1.0\"/>'"); | ||
|
|
||
| String script = step.getScript(false); | ||
|
|
||
| assertTrue("secure processing should be enabled on the transformer factory", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This asserts the constant names appear in the generated string, so it passes on the Saxon 9.x configuration noted in |
||
| script.contains("FEATURE_SECURE_PROCESSING")); | ||
| assertTrue("external DTD access should be disabled", script.contains("ACCESS_EXTERNAL_DTD")); | ||
| assertTrue("external stylesheet access should be disabled", script.contains("ACCESS_EXTERNAL_STYLESHEET")); | ||
| } | ||
|
|
||
| @Test | ||
| public void hardeningAlsoAppliedOnTheIteratorPath() throws Exception { | ||
| XsltStep step = new XsltStep(); | ||
| step.setSourceXml("connectorMessage.getRawData()"); | ||
| step.setResultVariable("xsltResult"); | ||
| step.setTemplate("'<xsl:stylesheet version=\"1.0\"/>'"); | ||
|
|
||
| String script = step.getIterationScript(false, new java.util.LinkedList<>()); | ||
|
|
||
| assertTrue("secure processing should be enabled on the iterator path", | ||
| script.contains("FEATURE_SECURE_PROCESSING")); | ||
| assertTrue("external DTD access should be disabled on the iterator path", | ||
| script.contains("ACCESS_EXTERNAL_DTD")); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should come from the
configurations/directory, not hard-coded in this file.