diff --git a/.jules/sentinel.md b/.jules/sentinel.md new file mode 100644 index 00000000000..3243691143e --- /dev/null +++ b/.jules/sentinel.md @@ -0,0 +1,6 @@ +# Sentinel Learnings + +## Command Injection in Windows `taskkill` +**Vulnerability:** Use of `child_process.exec` with user-controlled (or improperly validated) input for `taskkill` on Windows allowed potential command injection. +**Learning:** `child_process.exec` spawns a shell, making it susceptible to shell injection. `Number.isNaN()` does not perform type coercion on strings, so `Number.isNaN("abc")` is `false`, failing as a numeric validator. +**Prevention:** Always use `child_process.spawn` with an arguments array to avoid shell interpretation. Use strict regex like `/^\d+$/` for numeric validation of strings. Always handle the `'error'` event on `spawn` to prevent process crashes. diff --git a/packages/cli-kit/src/public/node/tree-kill.test.ts b/packages/cli-kit/src/public/node/tree-kill.test.ts new file mode 100644 index 00000000000..a5a3b32f0fb --- /dev/null +++ b/packages/cli-kit/src/public/node/tree-kill.test.ts @@ -0,0 +1,130 @@ +/* eslint-disable no-restricted-imports */ +import {treeKill} from './tree-kill.js' +import * as output from './output.js' +import {describe, expect, test, vi, afterEach} from 'vitest' +import {spawn} from 'child_process' + +vi.mock('./output.js') +vi.mock('child_process', () => ({ + spawn: vi.fn(() => ({ + stdout: {on: vi.fn()}, + on: vi.fn(), + })), +})) + +describe('treeKill', () => { + const originalPlatform = process.platform + + afterEach(() => { + Object.defineProperty(process, 'platform', { + value: originalPlatform, + }) + }) + + test('logs an error if pid is not a number and no callback is provided', () => { + // When + treeKill('abc') + + // Then + expect(output.outputDebug).toHaveBeenCalledWith( + expect.stringContaining('Failed to kill process abc: Error: pid must be a number'), + ) + }) + + test('calls callback with error if pid is not a number and callback is provided', () => { + // Given + const callback = vi.fn() + + // When + treeKill('abc', 'SIGTERM', true, callback) + + // Then + expect(callback).toHaveBeenCalledWith(expect.any(Error)) + expect(callback.mock.calls[0]![0].message).toBe('pid must be a number') + }) + + test('calls taskkill on Windows', () => { + // Given + Object.defineProperty(process, 'platform', { + value: 'win32', + }) + const pid = 1234 + const callback = vi.fn() + const mockOn = vi.fn() + vi.mocked(spawn).mockReturnValue({ + on: mockOn.mockReturnValue({ + on: mockOn, + }), + } as any) + + // When + treeKill(pid, 'SIGTERM', true, callback) + + // Then + expect(spawn).toHaveBeenCalledWith('taskkill', ['/pid', '1234', '/T', '/F']) + expect(mockOn).toHaveBeenCalledWith('error', expect.any(Function)) + expect(mockOn).toHaveBeenCalledWith('close', expect.any(Function)) + }) + + test('calls callback with error if taskkill fails to spawn', () => { + // Given + Object.defineProperty(process, 'platform', { + value: 'win32', + }) + const pid = 1234 + const callback = vi.fn() + const mockOn = vi.fn() + vi.mocked(spawn).mockReturnValue({ + on: mockOn.mockImplementation((event, cb) => { + if (event === 'error') { + cb(new Error('spawn ENOENT')) + } + return {on: mockOn} + }), + } as any) + + // When + treeKill(pid, 'SIGTERM', true, callback) + + // Then + expect(callback).toHaveBeenCalledWith(expect.objectContaining({message: 'spawn ENOENT'})) + }) + + test('calls pgrep on Darwin', () => { + // Given + Object.defineProperty(process, 'platform', { + value: 'darwin', + }) + const pid = 1234 + const mockSpawn = vi.mocked(spawn) + mockSpawn.mockReturnValue({ + stdout: {on: vi.fn()}, + on: vi.fn(), + } as any) + + // When + treeKill(pid) + + // Then + expect(spawn).toHaveBeenCalledWith('pgrep', ['-lfP', '1234']) + }) + + test('calls ps on other platforms', () => { + // Given + Object.defineProperty(process, 'platform', { + value: 'linux', + }) + const pid = 1234 + const mockSpawn = vi.mocked(spawn) + mockSpawn.mockReturnValue({ + stdout: {on: vi.fn()}, + on: vi.fn(), + } as any) + + // When + treeKill(pid) + + // Then + expect(spawn).toHaveBeenCalledWith('ps', ['-o', 'pid command', '--no-headers', '--ppid', '1234']) + }) +}) diff --git a/packages/cli-kit/src/public/node/tree-kill.ts b/packages/cli-kit/src/public/node/tree-kill.ts index 1bd7bdee30d..4a22fa336f5 100644 --- a/packages/cli-kit/src/public/node/tree-kill.ts +++ b/packages/cli-kit/src/public/node/tree-kill.ts @@ -3,7 +3,7 @@ /* eslint-disable no-restricted-imports */ import {outputDebug} from './output.js' -import {exec, spawn} from 'child_process' +import {spawn} from 'child_process' type ProcessTree = Record @@ -52,13 +52,10 @@ function adaptedTreeKill( ): void { const rootPid = typeof pid === 'number' ? pid.toString() : pid - if (Number.isNaN(rootPid)) { - if (callback) { - callback(new Error('pid must be a number')) - return - } else { - throw new Error('pid must be a number') - } + // PID must be strictly numeric to prevent command injection + if (!/^\d+$/.test(rootPid)) { + callback(new Error('pid must be a number')) + return } // A map from parent pid to an array of children pids @@ -72,8 +69,19 @@ function adaptedTreeKill( // eslint-disable-next-line @typescript-eslint/switch-exhaustiveness-check -- default handles all Unix-like platforms switch (process.platform) { case 'win32': + // We use spawn instead of exec to avoid shell injection vulnerabilities. // @ts-ignore - exec(`taskkill /pid ${pid} /T /F`, callback) + spawn('taskkill', ['/pid', rootPid, '/T', '/F']) + .on('error', (err) => { + callback(err) + }) + .on('close', (code) => { + if (code !== 0 && code !== null) { + callback(new Error(`taskkill exited with code ${code}`)) + } else { + callback() + } + }) break case 'darwin': buildProcessTree(