From f79d5e059c4c06ebe6c48ac3b41206f3d443bed1 Mon Sep 17 00:00:00 2001 From: PROTHAM <155388736+ProthamD@users.noreply.github.com> Date: Thu, 21 May 2026 02:13:14 +0530 Subject: [PATCH] fix(core): prevent SIGHUP kills in PTY environments (WSL2/Kitty/Alacritty) (#27267) --- .../core/src/sandbox/utils/commandUtils.ts | 6 +- .../services/shellExecutionService.test.ts | 8 +-- .../src/services/shellExecutionService.ts | 38 ++++++++++++- packages/core/src/utils/shell-utils.test.ts | 57 +++++++++++++++++++ packages/core/src/utils/shell-utils.ts | 47 +++++++++++++-- 5 files changed, 145 insertions(+), 11 deletions(-) diff --git a/packages/core/src/sandbox/utils/commandUtils.ts b/packages/core/src/sandbox/utils/commandUtils.ts index 772df65afa..143ad86ed7 100644 --- a/packages/core/src/sandbox/utils/commandUtils.ts +++ b/packages/core/src/sandbox/utils/commandUtils.ts @@ -56,7 +56,11 @@ export async function getCommandName(req: SandboxRequest): Promise { const roots = getCommandRoots(stripped).filter( (r) => r !== 'shopt' && r !== 'set', ); - if (roots.length > 0) { + // Single-root enforcement: only grant named-command permissions when the + // command is unambiguous. Multi-root chains fall back to basename so that + // a chained command like `git; malicious_cmd` never inherits `git`'s + // sandbox policy for the entire chain. + if (roots.length === 1) { return roots[0]; } return path.basename(req.command); diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index bc448e57a7..30c7dee28b 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -289,7 +289,7 @@ describe('ShellExecutionService', () => { 'bash', [ '-c', - 'shopt -u promptvars nullglob extglob nocaseglob dotglob; ls -l', + "trap '' HUP; shopt -u promptvars nullglob extglob nocaseglob dotglob; ls -l", ], expect.any(Object), ); @@ -1049,7 +1049,7 @@ describe('ShellExecutionService', () => { 'bash', [ '-c', - 'shopt -u promptvars nullglob extglob nocaseglob dotglob; ls "foo bar"', + 'trap \'\' HUP; shopt -u promptvars nullglob extglob nocaseglob dotglob; ls "foo bar"', ], expect.any(Object), ); @@ -1305,7 +1305,7 @@ describe('ShellExecutionService child_process fallback', () => { 'bash', [ '-c', - 'shopt -u promptvars nullglob extglob nocaseglob dotglob; ls -l', + "trap '' HUP; shopt -u promptvars nullglob extglob nocaseglob dotglob; ls -l", ], expect.objectContaining({ shell: false, detached: true }), ); @@ -1664,7 +1664,7 @@ describe('ShellExecutionService child_process fallback', () => { 'bash', [ '-c', - 'shopt -u promptvars nullglob extglob nocaseglob dotglob; ls "foo bar"', + 'trap \'\' HUP; shopt -u promptvars nullglob extglob nocaseglob dotglob; ls "foo bar"', ], expect.objectContaining({ shell: false, diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 26d2538f8d..c35669f2f0 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -17,6 +17,7 @@ import { getShellConfiguration, resolveExecutable, type ShellType, + BASH_HUP_GUARD, } from '../utils/shell-utils.js'; import { isBinary, truncateString } from '../utils/textUtils.js'; import pkg from '@xterm/headless'; @@ -114,6 +115,32 @@ function injectUtf8CodepageForPty( return command; } +/** + * Prepends a POSIX SIGHUP-ignore guard to bash commands on non-Windows platforms. + * + * PTY environments such as WSL2, Kitty, and Alacritty aggressively send SIGHUP + * to process groups that lose their controlling terminal. By prepending + * `trap '' HUP;` we apply the same mechanism as the POSIX `nohup` utility: + * SIG_IGN is inherited across exec(), so every child spawned by the command + * also ignores SIGHUP — making the guard genuinely effective even in subshells. + * + * The guard is bash-only and idempotent (won't be doubled if already present). + * It is stripped back out by stripShellWrapper() / stripHupGuard() before any + * sandbox or permission-check logic sees the command, so there is no + * privilege-escalation surface from the preamble itself. + */ +function ensureHupIgnored(command: string, shell: ShellType): string { + if (shell !== 'bash') { + return command; + } + const trimmed = command.trimStart(); + const prefix = `${BASH_HUP_GUARD} `; + if (trimmed.startsWith(prefix) || trimmed === BASH_HUP_GUARD) { + return command; // Already guarded — idempotent + } + return `${BASH_HUP_GUARD} ${command}`; +} + /** A structured result from a shell command execution. */ export type ShellExecutionResult = ExecutionResult; @@ -450,9 +477,16 @@ export class ShellExecutionService { const resolvedExecutable = resolveExecutable(executable) ?? executable; - const guardedCommand = ensurePromptvarsDisabled(commandToExecute, shell); + const promptGuarded = ensurePromptvarsDisabled(commandToExecute, shell); + // Prepend the SIGHUP-ignore guard for bash on non-Windows. This uses the + // same mechanism as POSIX `nohup`: SIG_IGN is inherited across exec(), so + // child processes spawned by the command also ignore SIGHUP. The guard is + // stripped by stripShellWrapper() before any sandbox permission checks. + const hupGuarded = !isWindows + ? ensureHupIgnored(promptGuarded, shell) + : promptGuarded; const finalCommand = injectUtf8CodepageForPty( - guardedCommand, + hupGuarded, shell, isWindows, usingPty, diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts index bc96d0716c..fc0a056acc 100644 --- a/packages/core/src/utils/shell-utils.test.ts +++ b/packages/core/src/utils/shell-utils.test.ts @@ -21,6 +21,8 @@ import { parseCommandDetails, splitCommands, stripShellWrapper, + stripHupGuard, + BASH_HUP_GUARD, normalizeCommand, hasRedirection, resolveExecutable, @@ -646,3 +648,58 @@ describe('resolveExecutable', () => { expect(resolveExecutable('anything')).toBeUndefined(); }); }); + +describe('stripHupGuard', () => { + it('should remove our own HUP guard prefix from the start of a command', () => { + expect(stripHupGuard(`${BASH_HUP_GUARD} git status`)).toBe('git status'); + }); + + it('should be idempotent: if no guard present, return command unchanged', () => { + expect(stripHupGuard('git status')).toBe('git status'); + }); + + it('should return empty string when command is exactly the guard itself', () => { + expect(stripHupGuard(BASH_HUP_GUARD)).toBe(''); + }); + + it('should handle leading whitespace before the guard', () => { + expect(stripHupGuard(` ${BASH_HUP_GUARD} git status`)).toBe('git status'); + }); + + // Security regression: must NOT strip user-supplied trap commands + it('should NOT strip a user-supplied trap command (only strips our exact preamble)', () => { + // A user attempting to use trap as a sandbox bypass + const maliciousCmd = `trap 'rm -rf /' EXIT; git status`; + // stripHupGuard looks for the exact BASH_HUP_GUARD prefix (trap '' HUP;), not 'trap' + expect(stripHupGuard(maliciousCmd)).toBe(maliciousCmd); + }); + + it('should NOT strip a trap command with different arguments', () => { + const cmd = `trap '' TERM; git status`; + expect(stripHupGuard(cmd)).toBe(cmd); + }); +}); + +describe('stripShellWrapper (with HUP guard integration)', () => { + it('should strip bash -c wrapper AND then the HUP guard preamble', () => { + const wrapped = `bash -c "${BASH_HUP_GUARD} git status"`; + expect(stripShellWrapper(wrapped)).toBe('git status'); + }); + + it('should strip the HUP guard alone when no shell wrapper', () => { + expect(stripShellWrapper(`${BASH_HUP_GUARD} git status`)).toBe( + 'git status', + ); + }); + + it('should leave user-supplied trap commands intact (security check)', () => { + // This ensures that a user command starting with a different trap + // is not incorrectly stripped by the HUP guard removal logic + const maliciousCmd = `trap 'rm -rf /' EXIT; git status`; + expect(stripShellWrapper(maliciousCmd)).toBe(maliciousCmd); + }); + + it('should still strip a plain shell wrapper without HUP guard', () => { + expect(stripShellWrapper('sh -c "ls -l"')).toEqual('ls -l'); + }); +}); diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 2c2d2aca7e..f918ebe20e 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -14,9 +14,40 @@ import { type SpawnOptionsWithoutStdio, } from 'node:child_process'; +/** + * The exact HUP-signal guard preamble injected by ensureHupIgnored(). + * Exported so shellExecutionService and stripShellWrapper stay in sync. + */ +export const BASH_HUP_GUARD = `trap '' HUP;`; + +/** + * Strips the SIGHUP guard prepended by ensureHupIgnored() from a command. + * + * This is intentionally narrow: it only removes the exact literal string + * `trap '' HUP; ` from the very start of a command. It does NOT + * skip or ignore arbitrary user-supplied `trap` commands, which would be a + * sandbox-bypass vector (e.g., `trap 'rm -rf /' EXIT; git status`). + */ +export function stripHupGuard(command: string): string { + const trimmed = command.trimStart(); + const prefix = `${BASH_HUP_GUARD} `; + if (trimmed.startsWith(prefix)) { + return trimmed.slice(prefix.length); + } + // Handle case where there's no trailing space (e.g., guard is the whole command) + if (trimmed === BASH_HUP_GUARD) { + return ''; + } + return command; +} + /** * Extracts the primary command name from a potentially wrapped shell command. - * Strips shell wrappers and handles shopt/set/etc. + * Strips shell wrappers (including our own HUP guard) and handles shopt/set/etc. + * + * Returns the command name only when there is exactly ONE non-builtin root so + * that chained commands (e.g. `git; malicious_cmd`) never silently inherit the + * first command's sandbox permissions. * * @param command - The full command string. * @param args - The arguments for the command. @@ -32,7 +63,10 @@ export async function getCommandName( const roots = getCommandRoots(stripped).filter( (r) => r !== 'shopt' && r !== 'set', ); - if (roots.length > 0) { + // Single-root enforcement: only grant named-command permissions when the + // command is unambiguous. Multi-root chains fall back to basename so that + // `git; malicious_cmd` never inherits `git`'s sandbox policy. + if (roots.length === 1) { return roots[0]; } return path.basename(command); @@ -838,6 +872,7 @@ export function stripShellWrapper(command: string): string { const pattern = /^\s*(?:(?:(?:\S+\/)?(?:sh|bash|zsh))\s+-c|cmd\.exe\s+\/c|powershell(?:\.exe)?\s+(?:-NoProfile\s+)?-Command|pwsh(?:\.exe)?\s+(?:-NoProfile\s+)?-Command)\s+/i; const match = command.match(pattern); + let result: string; if (match) { let newCommand = command.substring(match[0].length).trim(); if ( @@ -846,9 +881,13 @@ export function stripShellWrapper(command: string): string { ) { newCommand = newCommand.substring(1, newCommand.length - 1); } - return newCommand; + result = newCommand; + } else { + result = command.trim(); } - return command.trim(); + // Peel off the SIGHUP guard that ensureHupIgnored() prepends so that + // sandbox managers see the actual user command, not our preamble. + return stripHupGuard(result); } /**