fix(security): shell command injection in database builders (ssh_db_list/dump/import)#51
Merged
Merged
Conversation
…uilders Every ssh_db_* tool argument (database, table, collection, file path, user, host, port, password) was interpolated straight into a shell-evaluated command string in src/database-manager.js. A crafted value such as `$(...)` / backticks / `; cmd` yielded arbitrary command execution on the SSH target under the account the MCP server uses. The strongest path is ssh_db_list: it is classified read-only, so it stays allowed in the readonly and restricted per-server security modes while still reaching an injectable builder — reported privately by Ugur Ozer (AI Risk Management). The v3.6.5 heredoc fix (#44) only covered ssh_db_query's query *text*; the list/dump/import/restore builders and even the query builders' connection flags were untouched. Add a centralized shellQuote() (single-quote wrap + '\'' escaping) and route every interpolated value in all 15 builders through it. The MySQL list-tables database is now a shell-quoted positional argument instead of an interpolated `USE <db>` SQL clause, closing the SQL path too. Add tests/test-database-injection.js: drives every builder x every caller-controlled parameter x an injection-payload battery through a real /bin/sh with fake DB client binaries (648 combinations) and asserts no payload executes; unit-tests shellQuote(); checks benign values stay quoted. Add SECURITY.md documenting the private disclosure channel (issue #48).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a command-injection vulnerability across the database command builders in
src/database-manager.js. Reported privately by Ugur Ozer (AI Risk Management) through the newly-enabled private disclosure channel (see #48).Every
ssh_db_*tool argument —database,table,collection, output/input file paths, and evenuser/host/port/password— arrives from tool-call parameters and was interpolated straight into a shell-evaluated command string. A crafted value ($(...), backticks,; cmd,| cmd,&& cmd, bare> file) executed arbitrary commands on the configured SSH target, under the account the MCP server uses.Strongest path —
ssh_db_list: it is classified read-only, so it stays allowed in thereadonlyandrestrictedper-server security modes while still reaching an injectable builder. Confirmed by policy evaluation:[ { "mode": "readonly", "ssh_db_list": { "allowed": true } }, { "mode": "restricted", "ssh_db_list": { "allowed": true } }, { "mode": "unrestricted", "ssh_db_list": { "allowed": true } } ]The v3.6.5 heredoc fix (#44) only hardened
ssh_db_query's query text; thelist/dump/import/restorebuilders — and the query builders' connection flags — were untouched.Reproduction (local, no SSH)
Generating a command with
database = "$(echo PWNED > canary)"and running it under/bin/shwith fake no-op DB binaries created the canary in 5/5 builders before the fix, and 0/5 after.Fix
shellQuote()(single-quote wrap +'\''escaping) and route every interpolated caller value through it, across all 15 builders (list/dump/import/restore/query, MySQL/PostgreSQL/MongoDB).databasebecomes a shell-quoted positional argument instead of an interpolatedUSE <db>SQL clause, closing the SQL-injection path as well.Tests / verification
tests/test-database-injection.js(wired intonpm testastest:dbinjection): drives every builder × every caller-controlled parameter × an injection-payload battery through a real/bin/shwith fake DB client binaries — 648 combinations, asserting no payload ever executes. Also unit-testsshellQuote()round-tripping and checks benign values stay correctly quoted.npm test✅ ·./scripts/validate.sh✅ ·npx knip✅ ·npx eslint src/0 errors.Also in this PR
SECURITY.mddocumenting the private disclosure process (addresses Private security disclosure channel request #48): GitHub private vulnerability reporting + security email, 72h acknowledgement, advisory + reporter credit.Disclosure
A GitHub Security Advisory will be published crediting the reporter. No technical detail has been posted to the public issue thread. The
ssh_db_queryMongoDBcollection/queryvalues remain interpreted as Mongo JS inside the single-quoted heredoc — that is shell-safe (no host RCE) and is the tool's intended query mechanism; DB-engine-level query semantics are out of scope for this shell-injection fix.