From 00723e3f9a2086a0decb83045e15ea53ae229ad3 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 9 Apr 2026 17:34:19 +0000 Subject: [PATCH] fix: use absolute paths for credential helper binaries to prevent PATH hijacking Resolve secret-tool (Linux) and powershell.exe (Windows) to absolute paths before execution, preventing local PATH hijacking attacks that could steal Resend API keys. Linux: - Check trusted paths (/usr/bin, /usr/local/bin, /bin) first - Fall back to /usr/bin/which for resolution - Reject non-absolute paths from which output - Cache resolved paths for subsequent calls - Use absolute /usr/bin/which instead of bare 'which' in isAvailable() Windows: - Construct absolute PowerShell path from %SystemRoot% env var - Default to C:\Windows if SystemRoot is not set Closes BU-627 Co-authored-by: Bu Kinoshita --- src/lib/credential-backends/linux.ts | 92 ++++++- src/lib/credential-backends/windows.ts | 40 +-- tests/lib/credential-backends/linux.test.ts | 251 ++++++++++++++++++ tests/lib/credential-backends/windows.test.ts | 54 +++- 4 files changed, 392 insertions(+), 45 deletions(-) create mode 100644 tests/lib/credential-backends/linux.test.ts diff --git a/src/lib/credential-backends/linux.ts b/src/lib/credential-backends/linux.ts index c0f0bcd3..ffe0d0df 100644 --- a/src/lib/credential-backends/linux.ts +++ b/src/lib/credential-backends/linux.ts @@ -1,6 +1,17 @@ import { execFile, spawn } from 'node:child_process'; +import { access, constants } from 'node:fs/promises'; import type { CredentialBackend } from '../credential-store'; +const TRUSTED_SECRET_TOOL_PATHS = [ + '/usr/bin/secret-tool', + '/usr/local/bin/secret-tool', + '/bin/secret-tool', +] as const; + +const WHICH_PATH = '/usr/bin/which'; + +const resolvedPaths = new Map(); + function run( cmd: string, args: string[], @@ -29,28 +40,78 @@ function runWithStdin( stdio: ['pipe', 'ignore', 'pipe'], timeout: 5000, }); - let stderr = ''; + const chunks: string[] = []; child.stderr?.on('data', (data: Buffer) => { - stderr += data.toString(); + chunks.push(data.toString()); }); child.on('close', (code) => { - resolve({ code, stderr }); + resolve({ code, stderr: chunks.join('') }); }); child.on('error', () => { resolve({ code: 1, stderr: 'Failed to spawn process' }); }); - child.stdin?.on('error', () => {}); // Prevent EPIPE crash + child.stdin?.on('error', () => {}); child.stdin?.write(stdin); child.stdin?.end(); }); } +const findExecutableInTrustedPaths = async ( + candidates: readonly string[], +): Promise => { + for (const candidate of candidates) { + try { + await access(candidate, constants.X_OK); + return candidate; + } catch {} + } + return null; +}; + +const resolveSecretTool = async (): Promise => { + const cached = resolvedPaths.get('secret-tool'); + if (cached) { + return cached; + } + + const trusted = await findExecutableInTrustedPaths(TRUSTED_SECRET_TOOL_PATHS); + if (trusted) { + resolvedPaths.set('secret-tool', trusted); + return trusted; + } + + const result = await run(WHICH_PATH, ['secret-tool']); + const resolved = result.stdout.trim(); + if (result.code === 0 && resolved.startsWith('/')) { + try { + await access(resolved, constants.X_OK); + resolvedPaths.set('secret-tool', resolved); + return resolved; + } catch { + return null; + } + } + + return null; +}; + +const requireSecretTool = async (): Promise => { + const resolved = await resolveSecretTool(); + if (!resolved) { + throw new Error( + 'secret-tool not found in trusted paths (/usr/bin, /usr/local/bin, /bin)', + ); + } + return resolved; +}; + export class LinuxBackend implements CredentialBackend { name = 'Secret Service (libsecret)'; readonly isSecure = true; async get(service: string, account: string): Promise { - const { stdout, code } = await run('secret-tool', [ + const cmd = await requireSecretTool(); + const { stdout, code } = await run(cmd, [ 'lookup', 'service', service, @@ -64,9 +125,9 @@ export class LinuxBackend implements CredentialBackend { } async set(service: string, account: string, secret: string): Promise { - // Pass secret via stdin to avoid exposing in process list + const cmd = await requireSecretTool(); const { code, stderr } = await runWithStdin( - 'secret-tool', + cmd, [ 'store', `--label=Resend CLI (${account})`, @@ -85,7 +146,8 @@ export class LinuxBackend implements CredentialBackend { } async delete(service: string, account: string): Promise { - const { code } = await run('secret-tool', [ + const cmd = await requireSecretTool(); + const { code } = await run(cmd, [ 'clear', 'service', service, @@ -99,18 +161,20 @@ export class LinuxBackend implements CredentialBackend { if (process.platform !== 'linux') { return false; } - // Check if secret-tool is installed - const which = await run('which', ['secret-tool']); - if (which.code !== 0) { + const secretTool = await resolveSecretTool(); + if (!secretTool) { return false; } - // Probe the daemon with a harmless lookup (3s timeout) const probe = await run( - 'secret-tool', + secretTool, ['lookup', 'service', '__resend_cli_probe__'], { timeout: 3000 }, ); - // exit code 0 or 1 means daemon is responding; timeout or other errors mean it's not return probe.code === 0 || probe.code === 1; } } + +export { + resolvedPaths as _resolvedPaths, + resolveSecretTool as _resolveSecretTool, +}; diff --git a/src/lib/credential-backends/windows.ts b/src/lib/credential-backends/windows.ts index 57114451..6acb3989 100644 --- a/src/lib/credential-backends/windows.ts +++ b/src/lib/credential-backends/windows.ts @@ -1,12 +1,19 @@ import { execFile, spawn } from 'node:child_process'; import type { CredentialBackend } from '../credential-store'; +const resolvePowershellPath = (): string => { + const systemRoot = process.env.SystemRoot ?? 'C:\\Windows'; + return `${systemRoot}\\System32\\WindowsPowerShell\\v1.0\\powershell.exe`; +}; + +const POWERSHELL_PATH = resolvePowershellPath(); + function runPowershell( script: string, ): Promise<{ stdout: string; stderr: string; code: number | null }> { return new Promise((resolve) => { execFile( - 'powershell.exe', + POWERSHELL_PATH, ['-NoProfile', '-NonInteractive', '-Command', script], { timeout: 10000 }, (err, stdout, stderr) => { @@ -23,38 +30,36 @@ function runPowershellWithStdin( ): Promise<{ stdout: string; stderr: string; code: number | null }> { return new Promise((resolve) => { const child = spawn( - 'powershell.exe', + POWERSHELL_PATH, ['-NoProfile', '-NonInteractive', '-Command', script], { stdio: ['pipe', 'pipe', 'pipe'], timeout: 10000 }, ); - let stdout = ''; - let stderr = ''; + const stdoutChunks: string[] = []; + const stderrChunks: string[] = []; child.stdout?.on('data', (data: Buffer) => { - stdout += data.toString(); + stdoutChunks.push(data.toString()); }); child.stderr?.on('data', (data: Buffer) => { - stderr += data.toString(); + stderrChunks.push(data.toString()); }); child.on('close', (code) => { - resolve({ stdout, stderr, code }); + resolve({ + stdout: stdoutChunks.join(''), + stderr: stderrChunks.join(''), + code, + }); }); child.on('error', () => { resolve({ stdout: '', stderr: 'Failed to spawn process', code: 1 }); }); - child.stdin?.on('error', () => {}); // Prevent EPIPE crash + child.stdin?.on('error', () => {}); child.stdin?.write(stdin); child.stdin?.end(); }); } -// Escape single quotes for PowerShell string literals -function psEscape(s: string): string { - return s.replace(/'/g, "''"); -} +const psEscape = (s: string): string => s.replace(/'/g, "''"); -// Snippet that ensures the WinRT PasswordVault type is loaded. -// On some environments (e.g. GitHub Actions) the type isn't available by -// default and must be explicitly loaded via its assembly-qualified name. const LOAD_VAULT = ` try { $null = [Windows.Security.Credentials.PasswordVault] } catch { [void][Windows.Security.Credentials.PasswordVault,Windows.Security.Credentials,ContentType=WindowsRuntime] @@ -84,7 +89,6 @@ export class WindowsBackend implements CredentialBackend { } async set(service: string, account: string, secret: string): Promise { - // Remove existing credential first (PasswordVault throws on duplicate) const removeScript = `${LOAD_VAULT} $v = New-Object Windows.Security.Credentials.PasswordVault try { @@ -94,7 +98,6 @@ export class WindowsBackend implements CredentialBackend { `; await runPowershell(removeScript); - // Read secret from stdin to avoid exposing it in process args const addScript = `${LOAD_VAULT} $secret = [Console]::In.ReadLine() $v = New-Object Windows.Security.Credentials.PasswordVault @@ -127,10 +130,11 @@ export class WindowsBackend implements CredentialBackend { if (process.platform !== 'win32') { return false; } - // Test that PowerShell and PasswordVault are accessible const { code } = await runPowershell( `${LOAD_VAULT} $null = New-Object Windows.Security.Credentials.PasswordVault`, ); return code === 0; } } + +export { POWERSHELL_PATH as _powershellPath }; diff --git a/tests/lib/credential-backends/linux.test.ts b/tests/lib/credential-backends/linux.test.ts new file mode 100644 index 00000000..3cb46ae7 --- /dev/null +++ b/tests/lib/credential-backends/linux.test.ts @@ -0,0 +1,251 @@ +import { access, constants } from 'node:fs/promises'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('node:fs/promises', () => ({ + access: vi.fn(), + constants: { X_OK: 1 }, +})); + +vi.mock('node:child_process', () => { + const mockExecFile = vi.fn( + ( + _cmd: string, + _args: string[], + _opts: unknown, + cb: (err: null, stdout: string, stderr: string) => void, + ) => { + cb(null, '', ''); + }, + ); + + const mockStdin = { + on: vi.fn(), + write: vi.fn(), + end: vi.fn(), + }; + + const mockSpawn = vi.fn(() => ({ + stdin: mockStdin, + stdout: { on: vi.fn() }, + stderr: { + on: vi.fn(), + }, + on: vi.fn((event: string, cb: (code: number) => void) => { + if (event === 'close') { + setTimeout(() => cb(0), 0); + } + }), + })); + + return { execFile: mockExecFile, spawn: mockSpawn }; +}); + +describe('LinuxBackend', () => { + beforeEach(async () => { + vi.clearAllMocks(); + const mod = await import('../../../src/lib/credential-backends/linux'); + mod._resolvedPaths.clear(); + }); + + it('resolves secret-tool from trusted paths before falling back to which', async () => { + vi.mocked(access).mockResolvedValueOnce(undefined); + + const { _resolveSecretTool } = await import( + '../../../src/lib/credential-backends/linux' + ); + const result = await _resolveSecretTool(); + + expect(result).toBe('/usr/bin/secret-tool'); + expect(access).toHaveBeenCalledWith('/usr/bin/secret-tool', constants.X_OK); + }); + + it('falls back to second trusted path when first is unavailable', async () => { + vi.mocked(access) + .mockRejectedValueOnce(new Error('ENOENT')) + .mockResolvedValueOnce(undefined); + + const { _resolveSecretTool } = await import( + '../../../src/lib/credential-backends/linux' + ); + const result = await _resolveSecretTool(); + + expect(result).toBe('/usr/local/bin/secret-tool'); + }); + + it('falls back to /usr/bin/which when no trusted path exists', async () => { + vi.mocked(access) + .mockRejectedValueOnce(new Error('ENOENT')) + .mockRejectedValueOnce(new Error('ENOENT')) + .mockRejectedValueOnce(new Error('ENOENT')) + .mockResolvedValueOnce(undefined); + + const { execFile } = await import('node:child_process'); + vi.mocked(execFile).mockImplementation( + (_cmd: unknown, _args: unknown, _opts: unknown, cb: unknown) => { + (cb as (err: null, stdout: string, stderr: string) => void)( + null, + '/some/other/path/secret-tool\n', + '', + ); + return undefined as never; + }, + ); + + const { _resolveSecretTool } = await import( + '../../../src/lib/credential-backends/linux' + ); + const result = await _resolveSecretTool(); + + expect(result).toBe('/some/other/path/secret-tool'); + expect(execFile).toHaveBeenCalledWith( + '/usr/bin/which', + ['secret-tool'], + expect.any(Object), + expect.any(Function), + ); + }); + + it('returns null when secret-tool is not found anywhere', async () => { + vi.mocked(access).mockRejectedValue(new Error('ENOENT')); + + const { execFile } = await import('node:child_process'); + vi.mocked(execFile).mockImplementation( + (_cmd: unknown, _args: unknown, _opts: unknown, cb: unknown) => { + const err = Object.assign(new Error('not found'), { code: 1 }); + (cb as (err: Error, stdout: string, stderr: string) => void)( + err, + '', + '', + ); + return undefined as never; + }, + ); + + const { _resolveSecretTool } = await import( + '../../../src/lib/credential-backends/linux' + ); + const result = await _resolveSecretTool(); + + expect(result).toBeNull(); + }); + + it('caches the resolved path across calls', async () => { + vi.mocked(access).mockResolvedValue(undefined); + + const { _resolveSecretTool } = await import( + '../../../src/lib/credential-backends/linux' + ); + const first = await _resolveSecretTool(); + const second = await _resolveSecretTool(); + + expect(first).toBe('/usr/bin/secret-tool'); + expect(second).toBe('/usr/bin/secret-tool'); + expect(access).toHaveBeenCalledTimes(1); + }); + + it('get() uses absolute path for secret-tool', async () => { + vi.mocked(access).mockResolvedValue(undefined); + + const { execFile } = await import('node:child_process'); + vi.mocked(execFile).mockImplementation( + (_cmd: unknown, _args: unknown, _opts: unknown, cb: unknown) => { + (cb as (err: null, stdout: string, stderr: string) => void)( + null, + 'the-secret\n', + '', + ); + return undefined as never; + }, + ); + + const { LinuxBackend } = await import( + '../../../src/lib/credential-backends/linux' + ); + const backend = new LinuxBackend(); + await backend.get('resend-cli', 'default'); + + const calls = vi.mocked(execFile).mock.calls; + const getCall = calls.find( + (c) => Array.isArray(c[1]) && (c[1] as string[])[0] === 'lookup', + ); + expect(getCall).toBeDefined(); + expect(getCall?.[0]).toBe('/usr/bin/secret-tool'); + }); + + it('set() uses absolute path for secret-tool', async () => { + vi.mocked(access).mockResolvedValue(undefined); + + const { spawn } = await import('node:child_process'); + + const { LinuxBackend } = await import( + '../../../src/lib/credential-backends/linux' + ); + const backend = new LinuxBackend(); + await backend.set('resend-cli', 'default', 're_test_key'); + + expect(spawn).toHaveBeenCalled(); + const spawnCall = vi.mocked(spawn).mock.calls[0]; + expect(spawnCall[0]).toBe('/usr/bin/secret-tool'); + }); + + it('isAvailable() uses absolute path for which', async () => { + vi.mocked(access).mockResolvedValue(undefined); + + const { execFile } = await import('node:child_process'); + vi.mocked(execFile).mockImplementation( + (_cmd: unknown, _args: unknown, _opts: unknown, cb: unknown) => { + (cb as (err: null, stdout: string, stderr: string) => void)( + null, + '', + '', + ); + return undefined as never; + }, + ); + + const originalPlatform = process.platform; + Object.defineProperty(process, 'platform', { value: 'linux' }); + + try { + const { LinuxBackend } = await import( + '../../../src/lib/credential-backends/linux' + ); + const backend = new LinuxBackend(); + await backend.isAvailable(); + + const execCalls = vi.mocked(execFile).mock.calls; + const allCmds = execCalls.map((c) => c[0]); + expect(allCmds.every((cmd) => (cmd as string).startsWith('/'))).toBe( + true, + ); + } finally { + Object.defineProperty(process, 'platform', { value: originalPlatform }); + } + }); + + it('rejects relative paths from which output', async () => { + vi.mocked(access) + .mockRejectedValueOnce(new Error('ENOENT')) + .mockRejectedValueOnce(new Error('ENOENT')) + .mockRejectedValueOnce(new Error('ENOENT')); + + const { execFile } = await import('node:child_process'); + vi.mocked(execFile).mockImplementation( + (_cmd: unknown, _args: unknown, _opts: unknown, cb: unknown) => { + (cb as (err: null, stdout: string, stderr: string) => void)( + null, + 'secret-tool\n', + '', + ); + return undefined as never; + }, + ); + + const { _resolveSecretTool } = await import( + '../../../src/lib/credential-backends/linux' + ); + const result = await _resolveSecretTool(); + + expect(result).toBeNull(); + }); +}); diff --git a/tests/lib/credential-backends/windows.test.ts b/tests/lib/credential-backends/windows.test.ts index 5d5c210e..219e5859 100644 --- a/tests/lib/credential-backends/windows.test.ts +++ b/tests/lib/credential-backends/windows.test.ts @@ -1,6 +1,5 @@ -import { beforeEach, describe, expect, test, vi } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; -// We mock child_process to verify stdin usage without requiring Windows vi.mock('node:child_process', () => { const mockExecFile = vi.fn( ( @@ -22,16 +21,13 @@ vi.mock('node:child_process', () => { const mockSpawn = vi.fn(() => ({ stdin: mockStdin, stdout: { - on: vi.fn((_event: string, _cb: (data: Buffer) => void) => { - // no output - }), + on: vi.fn((_event: string, _cb: (data: Buffer) => void) => {}), }, stderr: { on: vi.fn(), }, on: vi.fn((event: string, cb: (code: number) => void) => { if (event === 'close') { - // Defer to allow stdin writes to be tracked first setTimeout(() => cb(0), 0); } }), @@ -45,7 +41,7 @@ describe('WindowsBackend', () => { vi.clearAllMocks(); }); - test('set() passes secret via stdin, NOT in PowerShell script args', async () => { + it('passes secret via stdin, NOT in PowerShell script args', async () => { const { spawn } = await import('node:child_process'); const { WindowsBackend } = await import( '../../../src/lib/credential-backends/windows' @@ -54,24 +50,20 @@ describe('WindowsBackend', () => { await backend.set('resend-cli', 'default', 're_secret_key_1234'); - // spawn is called for the add operation (second call - first is execFile for remove) expect(spawn).toHaveBeenCalled(); const spawnCall = vi.mocked(spawn).mock.calls[0]; const script = spawnCall[1]?.[spawnCall[1].length - 1] as string; - // The script should NOT contain the secret directly expect(script).not.toContain('re_secret_key_1234'); - // The script should read from stdin expect(script).toContain('[Console]::In.ReadLine()'); - // The secret should have been written to stdin const mockChild = vi.mocked(spawn).mock.results[0].value as { stdin: { write: ReturnType }; }; expect(mockChild.stdin.write).toHaveBeenCalledWith('re_secret_key_1234'); }); - test('get() does not use stdin', async () => { + it('get() does not use stdin', async () => { const { execFile } = await import('node:child_process'); const { WindowsBackend } = await import( '../../../src/lib/credential-backends/windows' @@ -80,10 +72,46 @@ describe('WindowsBackend', () => { await backend.get('resend-cli', 'default'); - // get() uses execFile, not spawn expect(execFile).toHaveBeenCalled(); const args = vi.mocked(execFile).mock.calls[0][1] as string[]; const script = args[args.length - 1]; expect(script).toContain('Retrieve'); }); + + it('uses absolute path for powershell.exe based on SystemRoot', async () => { + const { _powershellPath } = await import( + '../../../src/lib/credential-backends/windows' + ); + + expect(_powershellPath).toMatch( + /\\System32\\WindowsPowerShell\\v1\.0\\powershell\.exe$/, + ); + expect(_powershellPath).not.toBe('powershell.exe'); + }); + + it('execFile receives absolute powershell path', async () => { + const { execFile } = await import('node:child_process'); + const { WindowsBackend, _powershellPath } = await import( + '../../../src/lib/credential-backends/windows' + ); + const backend = new WindowsBackend(); + + await backend.get('resend-cli', 'default'); + + const cmd = vi.mocked(execFile).mock.calls[0][0]; + expect(cmd).toBe(_powershellPath); + }); + + it('spawn receives absolute powershell path', async () => { + const { spawn } = await import('node:child_process'); + const { WindowsBackend, _powershellPath } = await import( + '../../../src/lib/credential-backends/windows' + ); + const backend = new WindowsBackend(); + + await backend.set('resend-cli', 'default', 're_test'); + + const cmd = vi.mocked(spawn).mock.calls[0][0]; + expect(cmd).toBe(_powershellPath); + }); });