Skip to content

Fix SQLi in DatabaseConnectorServlet (CVE-2026-82583) - #451

Open
mgaffigan wants to merge 4 commits into
OpenIntegrationEngine:mainfrom
mgaffigan:maint/fix-cve-2026-82583
Open

mgaffigan wants to merge 4 commits into
OpenIntegrationEngine:mainfrom
mgaffigan:maint/fix-cve-2026-82583

Conversation

@mgaffigan

Copy link
Copy Markdown
Contributor

Prior code was attempting to retrieve 0-1 rows in a per-driver manner,
which is hard. Switched to using the standard WHERE 1 = 0 approach.

Left the now-pointless parameter in place for compatibility with existing callers.

Review notes:

  • See the Fix SQLi in DatabaseConnectorServlet commit for the actual fix
  • Most of the PR is tests. The functionality is now tested across all supported engines.
  • Drive-by fix for improper quoting of table names

Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
Prior code was attempting to retrieve 0-1 rows in a per-driver manner,
which is hard.  Switched to using the standard `WHERE 1 = 0` approach.

Left the now-pointless parameter in place.  Added tests.

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

Copy link
Copy Markdown

Test Results

124 files  + 1  124 suites  +1   2m 49s ⏱️ + 1m 0s
703 tests +13  703 ✅ +13  0 💤 ±0  0 ❌ ±0 
733 runs  +31  727 ✅ +25  6 💤 +6  0 ❌ ±0 

Results for commit 04b18f9. ± Comparison against base commit 9359d9a.

@pacmano1

pacmano1 commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Nothing exercises the servlet with a non-null selectLimit, so deleting the selectLimit = null; line puts the CVE back with every test still green.

Can you add a regression test? #452 and #453 each carry one, this is the only one of the three without.

The fix works. I verified it against the smoke harness on Derby: a caller-supplied selectLimit that aliases a column no longer reaches executeQuery, and with that line removed as a control, the same check fails and returns the injected alias. Aliasing a column and checking whether the alias comes back in the metadata works at whatever layer you prefer to test.

@mgaffigan

Copy link
Copy Markdown
Contributor Author

Nothing exercises the servlet with a non-null selectLimit, so deleting the selectLimit = null; line puts the CVE back with every test still green.

The selectLimit parameter is not referenced - so there is nothing to exercise. The selectLimit = null is just for clarity and to avoid a mental phi node.

Can you add a regression test? #452 and #453 each carry one, this is the only one of the three without.

It's challenging to write any meaningful test for this - the unit tests do not run with a database, and since the parameter is not referenced it is a bit pointless to assert that it is not present in the generated SQL.

The correct fix is the absence of the code. We don't have to test the code since it does not exist. We test the feature, though, but we can't meaningfully test for the absence of user input that might end up in the query. (To extreme, we can remove the parameter, which would guarantee that it is unused, but cause minimal compat issues).

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.

3 participants