Fix SQLi + XXE CVEs (CVE-2026-82583, -78224, -82578) with regression tests by jonbartels · Pull Request #441 · OpenIntegrationEngine/engine

jonbartels · 2026-09-14T15:27:19Z

Summary

Fixes three publicly-disclosed vulnerabilities (advisory) that are present in this tree (4.6.0 is below every upstream fix version). Each fix uses an idiom already established in this codebase, and each is covered by a regression test in the ci/smoketest harness that is RED on the vulnerable code and GREEN with the fix.

CVE	Component	Fix
CVE-2026-82583 (SQLi, CWE-89)	`DatabaseConnectorServlet`	Allowlist `selectLimit` against configured drivers
CVE-2026-78224 (XSLT XXE, CWE-611)	`XsltStep`	Emit secure-processing + external-access-off into the generated script
CVE-2026-82578 (XML batch XXE, CWE-611)	`XMLBatchAdaptor`	Parse with a hardened `DocumentBuilderFactory`, then evaluate XPath

The fixes

CVE-2026-82583 — SQL injection

DatabaseConnectorServlet.getTables concatenated the caller-supplied selectLimit query parameter into SQL and ran it via Statement.executeQuery. The Database connector metadata dialog only ever sends a selectLimit drawn from the configured driver list (DatabaseReader/DatabaseWriter → DriverInfo.getSelectLimit()), so selectLimit is now validated against that list (getDatabaseDrivers() plus the built-in DriverInfo defaults, always included so a cleared list cannot disable the check) before any SQL runs. Non-allowlisted values are rejected with a generic exception that does not reflect the input; a blank value still routes to the safe DatabaseMetaData.getColumns() path. This mirrors the allowlist-at-a-single-choke-point approach of #361.

Deliberately out of scope (follow-ups): the endpoint's @MirthOperation has no permission and auditable = false, and driver/url are unconstrained (SSRF / arbitrary class load). The same no-permission/auditable=false gap exists on every other connector test servlet (file/tcp/http/smtp/ws/jms) and is better handled as its own change.

CVE-2026-78224 — XSLT step XXE

XsltStep builds a TransformerFactory inside generated JavaScript, so it was never reached by the Java-side XML hardening elsewhere in the tree. The generated script now enables FEATURE_SECURE_PROCESSING and sets ACCESS_EXTERNAL_DTD / ACCESS_EXTERNAL_STYLESHEET to "" (the setAttribute calls guarded for implementations that reject them), on both the normal and iterator code paths — blocking external entity resolution in both the stylesheet and the source XML.

CVE-2026-82578 — XML batch adaptor XXE

XMLBatchAdaptor evaluated XPath directly over an InputSource, letting the XPath engine build its own DOCTYPE-resolving parser. It now parses with DocumentSerializer.getSecureDocumentBuilderFactory() (disallow-doctype-decl, external entities/DTD off, no entity expansion) and evaluates against the parsed Document. Note the output path in this same file was already hardened; only the reader side was missed.

Tests

XsltStepSecurityTest (server unit test, runs in ./gradlew :server:test) — asserts the hardening is emitted into the generated script.
DatabaseConnectorSqlInjectionTest, XsltStepXxeTest, XmlBatchXxeTest (smoketest harness) — drive the live server through the Client SDK:
- SQLi: calls _getTables with an injected selectLimit whose alias would surface as a column; asserts it never does. Runs on DB-backed configurations (oie.db.* coordinates wired for postgres/mysql via run-configuration.sh + harness.compose.yml); skips embedded-Derby.
- XSLT: feeds a file:///etc/hostname external entity; asserts the source message reaches ERROR.
- XML batch: feeds a DOCTYPE entity into a batch-split channel; asserts no split message contains the marker.
- New SecurityChannels builds these channels from a base VM no-op fixture; new OieServer helpers add the connector call, a Channel-object deploy, and a message-list accessor.

Verification

./gradlew :server:test — XsltStepSecurityTest passes; existing datatypes.xml, DocumentSerializer, and jdbc tests still pass.
Integration harness (ci/runtests.sh alpine-temurin21-postgres) exercises all three integration tests against the real server. To observe the RED (CVE-present) baseline, revert the three server/src/main fixes and re-run.

🤖 Generated with Claude Code

…tests Three vulnerabilities reported against NextGen/Mirth Connect are present in this 4.6.0 tree (below every upstream fix version). Each is fixed using the project's own established idioms, and each is covered by an integration test in the ci/smoketest harness that is RED on the vulnerable code and GREEN once the fix is applied. CVE-2026-82583 (SQL injection): DatabaseConnectorServlet.getTables executed the caller-supplied selectLimit query parameter verbatim via Statement.executeQuery. The Database connector metadata dialog only ever sends a selectLimit taken from the configured driver list, so selectLimit is now validated against that list (getDatabaseDrivers plus the built-in DriverInfo defaults, which are always included so a cleared list cannot disable the check) before any SQL runs. A non-allowlisted value is rejected with a generic exception that does not reflect the input. A blank value still routes to the safe DatabaseMetaData.getColumns path. Not fixed here (noted as follow-ups): the endpoint's missing @MirthOperation permission and unconstrained driver/url, a gap shared by every connector test servlet. CVE-2026-78224 (XSLT step XXE): XsltStep builds a TransformerFactory inside generated JavaScript, so it was never reached by the Java-side XML hardening. The generated script now enables FEATURE_SECURE_PROCESSING and sets ACCESS_EXTERNAL_DTD/ACCESS_EXTERNAL_STYLESHEET to "" (setAttribute guarded for implementations that reject it), on both the normal and iterator paths, blocking external entity resolution in the stylesheet and the source XML. CVE-2026-82578 (XML batch XXE): XMLBatchAdaptor evaluated XPath directly over an InputSource, letting XPath build its own DOCTYPE-resolving parser. It now parses with DocumentSerializer.getSecureDocumentBuilderFactory() (disallow-doctype-decl, external entities/DTD off, no entity expansion) and evaluates against the parsed Document. The output path in this file was already hardened; only the reader side was missed. Tests: - XsltStepSecurityTest (unit): asserts the hardening is emitted into the script. - DatabaseConnectorSqlInjectionTest, XsltStepXxeTest, XmlBatchXxeTest (smoketest): drive the live server; SQLi runs on DB-backed configurations (DB coordinates passed as oie.db.* via run-configuration.sh + harness.compose.yml) and skips embedded-Derby, the XXE tests build channels from a base fixture via new OieServer/SecurityChannels helpers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jon Bartels <jonathan.bartels@gmail.com>

jonbartels · 2026-09-14T15:36:19Z

@abhinavagarwal07 - OpenIntegrationEngine is a fork of Mirth Connect. OIE is often affected by the same historical security risks as OIE. We learned about your security findings at https://abhinavagarwal07.github.io/posts/nextgen-mirth-connect-sqli-xxe/

Would you be willing to evaluate our fixes against your findings please?

Copilot

🟡 Changes recommended

XML namespace behavior regresses, and the SQL injection test can falsely pass on MariaDB.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Hardens JDBC metadata, XSLT, and XML batch processing against SQL injection and XXE vulnerabilities, with regression coverage.

Changes:

Adds allowlist validation for JDBC selectLimit.
Secures XSLT and XML batch parsing.
Adds unit/smoke tests and CI database configuration.

File summaries

File	Description
`smoketest/.../base-vm-noop.xml`	Adds security-test channel fixture.
`smoketest/.../XsltStepXxeTest.java`	Tests XSLT XXE rejection.
`smoketest/.../XmlBatchXxeTest.java`	Tests XML batch entity rejection.
`smoketest/.../SecurityChannels.java`	Builds security-test channels.
`smoketest/.../OieServer.java`	Adds security-test server helpers.
`smoketest/.../DatabaseConnectorSqlInjectionTest.java`	Tests JDBC SQL injection blocking.
`smoketest/build.gradle`	Adds test compile dependencies.
`server/.../XsltStepSecurityTest.java`	Verifies generated XSLT hardening.
`server/.../XsltStep.java`	Secures generated transformer factories.
`server/.../XMLBatchAdaptor.java`	Uses hardened XML parsing.
`server/.../DatabaseConnectorServlet.java`	Validates `selectLimit`.
`ci/run-configuration.sh`	Supplies database test parameters.
`ci/harness.compose.yml`	Forwards harness JVM options.

Review details

Files reviewed: 13/13 changed files
Comments generated: 2
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

github-actions · 2026-09-14T15:39:41Z

Test Results

124 files + 1 124 suites +1 4m 32s ⏱️ + 2m 43s
696 tests + 6 696 ✅ + 6 0 💤 ±0 0 ❌ ±0
732 runs +30 724 ✅ +22 8 💤 +8 0 ❌ ±0

Results for commit 42db3da. ± Comparison against base commit 9359d9a.

♻️ This comment has been updated with latest results.

The client SDK deserializes model objects with XStream, whose reflective converters need the same module access the server's own test task already opens (server/build.gradle). The harness JVM launched plain `java`, so deserializing a SortedSet response -- the JDBC connector's getTables result, exercised by the new SQL-injection test -- failed on JDK 17 with InaccessibleObjectException on TreeSet.m. Add the java.base/java.util (and related) opens to run-harness.sh. The extra per-configuration -D flags are now read into an array so they reach the java command as separate arguments without relying on unquoted word-splitting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Jon Bartels <jonathan.bartels@gmail.com>

abhinavagarwal07 · 2026-09-14T15:44:24Z

@jonbartels Sure. I will review it.

mgaffigan

I'm not sure about the database connector. XML and XsltStep look legitimate. The tests need to be substantially simplified:

The SQL test should be a unit test - does not need a live server to confirm that it validates
The Xxe repros should be fixture tests. See https://github.com/OpenIntegrationEngine/engine/blob/main/ci/README.md#add-a-fixture-test or https://github.com/OpenIntegrationEngine/engine/tree/main/ci/tests/110-hl7-no-op/channels/01-hl7-no-op

mgaffigan · 2026-09-14T16:29:36Z

+        export OIE_HARNESS_OPTS="-Doie.db.driver=org.postgresql.Driver -Doie.db.url=jdbc:postgresql://db:5432/mirthdb -Doie.db.user=mirthdb -Doie.db.password=mirthdb"
+        ;;
+    *-mysql)
+        export OIE_HARNESS_OPTS="-Doie.db.driver=com.mysql.cj.jdbc.Driver -Doie.db.url=jdbc:mysql://db:3306/mirthdb -Doie.db.user=mirthdb -Doie.db.password=mirthdb"
+        ;;
+    *)
+        export OIE_HARNESS_OPTS=""
+        ;;


These should come from the configurations/ directory, not hard-coded in this file.

mgaffigan · 2026-09-14T16:30:37Z

+# 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
+)


They should be the same as the runtime oieserver - not the other tests, I would presume.

mgaffigan · 2026-09-14T16:31:05Z

+# 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:-}"


Can we avoid? If the options come from a file, I should assume this is not required.

mgaffigan · 2026-09-14T16:40:02Z

    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);


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?

mgaffigan · 2026-09-14T16:41:30Z

+        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");
+        script.append("try { tFactory.setAttribute(Packages.javax.xml.XMLConstants.ACCESS_EXTERNAL_STYLESHEET, ''); } catch (e) {}\n");


If we expect these to succeed, we should not be swallowing all errors.

mgaffigan · 2026-09-14T16:42:22Z

+// SPDX-License-Identifier: MPL-2.0
+// SPDX-FileCopyrightText: 2026 Open Integration Engine
+
+package org.openintegrationengine.smoketest;


This does not seem to require any detail of a particular online database - presumably we can run this as a unit test.

abhinavagarwal07

Reviewed at 42db3da5 — notes inline.

The XXE fixes look right for the default JDK factory, and the batch fix covers Element_Name and Level as well as XPath_Query, which is broader than what was reported.

Three things I wanted to check: the selectLimit allowlist is populated from an API-writable source, the XSLT hardening is dropped silently on Saxon 9.x, and the batch parser is no longer namespace-aware. Measurements are in the inline comments — all run standalone on JDK 21, none against a live OIE server.

abhinavagarwal07 · 2026-09-14T17:37:26Z

+        addSelectLimits(allowedSelectLimits, DriverInfo.getDefaultDrivers());
+
+        try {
+            addSelectLimits(allowedSelectLimits, configurationController.getDatabaseDrivers());


The allowlist is populated from configuration the same caller can write.

PUT /api/server/databaseDrivers accepts a caller-supplied selectLimit. It is annotated DATABASE_DRIVERS_EDIT, but DefaultAuthorizationController.isUserAuthorized() returns true unconditionally (:40-45); it is the only AuthorizationController in the tree and ControllerFactory.getFactory() hardcodes it (:21). The value persists through saveProperty (DefaultConfigurationController.java:753), and getDatabaseDrivers() reads that property before dbdrivers.xml and before the defaults (:685).

So a caller can PUT a driver whose selectLimit is their payload, then send the same string here. It passes validation and reaches executeQuery at :178.

Am I reading the stock authorization path correctly? If so, deriving selectLimit server-side from driver, or dropping it and always using getColumns(), would avoid depending on it.

abhinavagarwal07 · 2026-09-14T17:37:26Z

+     * disable the check (fail closed).
+     */
+    private void validateSelectLimit(String selectLimit) {
+        if (StringUtils.isBlank(selectLimit)) {


isBlank here and isEmpty at :160 differ. A whitespace-only selectLimit skips validation and then takes the query branch, trimming to "" and erroring into the fallback, so nothing executes. Is the difference deliberate?

abhinavagarwal07 · 2026-09-14T17:37:26Z

+        // 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");


These two catch blocks drop the restrictions with no signal when a factory rejects them. I ran the emitted sequence on JDK 21:

Saxon FEATURE_SECURE_PROCESSING ACCESS_EXTERNAL_* source-document XXE

9.5.1-5 accepted both throw IllegalArgumentException file read succeeds

9.7.0-21 accepted both throw IllegalArgumentException file read succeeds

9.9.1-8 accepted both throw IllegalArgumentException file read succeeds

10.9, 11.6, 12.5 accepted accepted blocked

Secure processing succeeding on 9.x means there is nothing to indicate the other two failed. Saxon's secure processing covers extension functions rather than external document access.

useCustomFactory is a supported option and both tests set it to false. Was the custom-factory path considered in scope here? Failing closed when either attribute cannot be set would cover it; parsing the source into a hardened SAXSource with a deny-all URIResolver would remove the dependency on JAXP attributes.

abhinavagarwal07 · 2026-09-14T17:37:26Z

+                // 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();


DocumentBuilderFactory defaults namespaceAware to false, and the previous XPath.evaluate(InputSource, ...) path parsed namespace-aware. On JDK 21:

//*[local-name()='message' and namespace-uri()='urn:test'] — 1 match before, 0 after

<batch xmlns="urn:test"> splits to <message>hello</message>

prefixed input splits to <p:message>hi</p:message> with no xmlns:p

Element_Name and Level still select nodes but serialize the same way, so it is not limited to XPath_Query.

dbf.setNamespaceAware(true) restored all three here and keeps the hardening. Was namespace handling checked against the old path?

abhinavagarwal07 · 2026-09-14T17:37:26Z

+                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));


Separately: disallow-doctype-decl rejects every DOCTYPE, including internal-only DTDs that previously parsed. Worth a release note?

abhinavagarwal07 · 2026-09-14T17:37:26Z

+    @Test
+    void selectLimitDoesNotExecuteArbitrarySql() throws Exception {
+        Db db = Db.fromSystemProperties();
+        assumeTrue(db != null, "oie.db.* coordinates not provided; skipping (embedded-database configuration)");


This skips whenever oie.db.* is unset, which is the embedded-Derby configurations. That is the default deployment and the one the reported impact used — SYSCS_EXPORT_QUERY writing the channel table to a path retrievable without authentication. Was leaving Derby uncovered deliberate?

abhinavagarwal07 · 2026-09-14T17:37:26Z

+            long messageId = server.submitMessage(channelId, payload, new LinkedHashMap<>());
+
+            Status sourceStatus = awaitSourceStatus(server, channelId, messageId);
+            assertEquals(Status.ERROR, sourceStatus, "XSLT step resolved an external entity instead of denying "


Status.ERROR is also reached on a deploy or template failure, so it does not separate "external access denied" from "step threw for another reason". Would a benign control plus an assertion that the file contents are absent be workable?

abhinavagarwal07 · 2026-09-14T17:37:26Z

+        String channelId = server.deployChannel(channel, "xml-batch-xxe");
+        try {
+            String payload = "<?xml version=\"1.0\"?>"
+                    + "<!DOCTYPE batch [<!ENTITY x \"" + MARKER + "\">]>"


This is an internal entity, so it covers expansion rather than external resolution. It passes because disallow-doctype-decl blocks both; under external-general-entities=false alone it would fail while the file-read path stayed closed. Is an external canary case worth adding?

abhinavagarwal07 · 2026-09-14T17:37:26Z

+                    + "<batch><message>&x;</message></batch>";
+            try {
+                server.submitMessage(channelId, payload, new LinkedHashMap<>());
+            } catch (Exception batchRejected) {


Same as the SQLi test — a failure before XMLBatchAdaptor runs is indistinguishable from the fix working.

abhinavagarwal07 · 2026-09-14T17:37:27Z

+
+        String script = step.getScript(false);
+
+        assertTrue("secure processing should be enabled on the transformer factory",


This asserts the constant names appear in the generated string, so it passes on the Saxon 9.x configuration noted in XsltStep.java. Would stubbing a factory that accepts FEATURE_SECURE_PROCESSING and rejects both attributes be a better fit?

jonbartels force-pushed the fix/mirth-sqli-xxe-cves branch from f263cd7 to 5b8b1bd Compare September 14, 2026 15:28

jonbartels requested a balanced review from Copilot September 14, 2026 15:29

Copilot started reviewing on behalf of jonbartels September 14, 2026 15:30 View session

Copilot AI reviewed Sep 14, 2026

View reviewed changes

Comment thread server/src/main/java/com/mirth/connect/plugins/datatypes/xml/XMLBatchAdaptor.java

Comment thread ...est/src/test/java/org/openintegrationengine/smoketest/DatabaseConnectorSqlInjectionTest.java

jonbartels marked this pull request as ready for review September 14, 2026 16:28

jonbartels requested review from a team, gibson9583, kayyagari, kpalang, mgaffigan, pacmano1, ssrowe and tonygermano September 14, 2026 16:28

mgaffigan requested changes Sep 14, 2026

View reviewed changes

abhinavagarwal07 reviewed Sep 14, 2026

View reviewed changes

Saxon	`FEATURE_SECURE_PROCESSING`	`ACCESS_EXTERNAL_*`	source-document XXE
9.5.1-5	accepted	both throw `IllegalArgumentException`	file read succeeds
9.7.0-21	accepted	both throw `IllegalArgumentException`	file read succeeds
9.9.1-8	accepted	both throw `IllegalArgumentException`	file read succeeds
10.9, 11.6, 12.5	accepted	accepted	blocked


		String script = step.getScript(false);

		assertTrue("secure processing should be enabled on the transformer factory",

Uh oh!

Conversation

jonbartels commented Sep 14, 2026

Summary

The fixes

CVE-2026-82583 — SQL injection

CVE-2026-78224 — XSLT step XXE

CVE-2026-82578 — XML batch adaptor XXE

Tests

Verification

Uh oh!

jonbartels commented Sep 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

abhinavagarwal07 commented Sep 14, 2026

Uh oh!

mgaffigan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavagarwal07 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Sep 14, 2026 •

edited

Loading