fix(core): remove detached spawn flag to prevent SIGHUP in PTY enviro… - #27248
fix(core): remove detached spawn flag to prevent SIGHUP in PTY enviro…#27248ProthamD wants to merge 7 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue where shell commands in Gemini CLI were failing immediately with a SIGHUP signal on PTY-based environments like WSL2, Kitty, and Alacritty. By removing the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request modifies the ShellExecutionService to disable process detaching for child processes on Unix-like systems, aiming to resolve SIGHUP issues in PTY-based environments like WSL2, Kitty, and Alacritty. However, the review identifies critical security and reliability regressions resulting from this change, including potential terminal hijacking via TIOCSTI, denial of service risks where a child can kill the parent process group, and broken process cleanup logic that relies on the child being a process group leader. It is recommended to revert this change and explore alternative solutions for handling SIGHUP without sacrificing process isolation.
| shell: false, | ||
| detached: !isWindows && !isBun, | ||
| detached: false, | ||
| env: finalEnv, |
There was a problem hiding this comment.
The change from detached: true to detached: false for child processes on Unix systems introduces several security and reliability regressions:
- Terminal Hijacking (Unix): By sharing the same session as the parent process, child processes retain access to the parent's controlling terminal. This allows a malicious or compromised shell command to use the
TIOCSTIioctl (where enabled) to inject characters into the terminal's input buffer, leading to arbitrary command execution in the user's shell after the CLI tool exits. - Denial of Service (DoS): Child processes are now spawned in the same process group as the parent. A command like
kill -9 0executed by the child will send aSIGKILLto the entire process group, including the parent Gemini CLI process, causing it to crash. - Broken Cleanup Logic: The
killProcessGrouputility relies onprocess.kill(-pid)to terminate process groups. This call fails whendetached: falseis used because the child is not a process group leader. This makes process cleanup unreliable and, in cases of PID reuse, could lead to sending signals to unrelated process groups.
Remediation:
Revert to using detached: true on Unix systems to maintain process and session isolation. To resolve the SIGHUP issue in PTY-based environments that motivated this change, consider having the child process explicitly ignore SIGHUP or investigating environment-specific configurations that don't require sacrificing isolation.
There was a problem hiding this comment.
Code Review
This pull request disables process detachment in the ShellExecutionService by setting the detached flag to false for spawned child processes. This change addresses issues where processes were receiving SIGHUP and terminating prematurely in PTY-based environments such as WSL2, Kitty, and Alacritty. The update also removes specific logic for Bun and updates the test suite to align with this new behavior. I have no feedback to provide as there were no review comments to evaluate.
82dd61a to
317d0dc
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a trap '' HUP command prefix for shell executions on Unix-like systems to ensure child processes ignore hangup signals in PTY environments like WSL2, Kitty, and Alacritty. The changes also include updates to the test suite to reflect this new behavior. Feedback was provided regarding a misleading test description that incorrectly stated a process was not detached when it actually was; a suggestion was made to align the description with the implementation.
2036515 to
d2c25d1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a fix for SIGHUP signals on Unix-like systems by prepending a trap '' HUP command to shell executions, ensuring child processes ignore hangup signals from PTY environments like WSL2 or Kitty. The changes include updates to ShellExecutionService and its test suite to reflect this new behavior. Feedback suggests explicitly checking for the 'bash' shell type before applying the trap to improve robustness for potential future shell support.
| const hupGuardedCommand = isWindows | ||
| ? guardedCommand | ||
| : `trap '' HUP; ${guardedCommand}`; | ||
| const spawnArgs = [...argsPrefix, hupGuardedCommand]; |
There was a problem hiding this comment.
While this logic is currently correct because 'getShellConfiguration' returns 'bash' for all non-Windows platforms, it implicitly assumes this will always be the case. To improve robustness and prevent potential issues if other shells are supported in the future, it would be safer to explicitly check if 'shell === bash' before prepending the 'trap' command. This would align with the defensive check already present in 'ensurePromptvarsDisabled'.
| const hupGuardedCommand = isWindows | |
| ? guardedCommand | |
| : `trap '' HUP; ${guardedCommand}`; | |
| const spawnArgs = [...argsPrefix, hupGuardedCommand]; | |
| const hupGuardedCommand = !isWindows && shell === 'bash' | |
| ? "trap '' HUP; " + guardedCommand | |
| : guardedCommand; | |
| const spawnArgs = [...argsPrefix, hupGuardedCommand]; |
484d32c to
d9afe11
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a SIGHUP trap (trap '' HUP) to shell execution commands on Unix systems to prevent premature termination in PTY environments like WSL2, Kitty, and Alacritty. The changes include updating the command preparation logic in ShellExecutionService and adjusting corresponding tests. A security review identified that prepending trap to the command string could potentially bypass existing sandbox command identification logic, which currently does not account for trap as a built-in command. It is recommended to update the filtering logic in shell-utils.ts to include trap to maintain security integrity.
…s to prevent sandbox bypass
a24cf86 to
2f6624a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a trap '' HUP guard for shell commands executed via bash on Unix-like systems, preventing child processes from being terminated by hangup signals in certain PTY environments such as WSL2, Kitty, and Alacritty. Corresponding updates were made to command parsing utilities to ignore the trap builtin and to the test suite to reflect the new command structure. Security feedback identifies a high-severity vulnerability in the getCommandName utility, which is susceptible to privilege escalation via command chaining. The reviewer recommends stricter validation of command roots and suggests centralizing the list of ignored shell builtins to improve maintainability and ensure consistent security filtering.
| const roots = getCommandRoots(stripped).filter( | ||
| (r) => r !== 'shopt' && r !== 'set', | ||
| (r) => r !== 'shopt' && r !== 'set' && r !== 'trap', | ||
| ); |
There was a problem hiding this comment.
The getCommandName utility function is vulnerable to privilege escalation via command chaining. It extracts only the first command root from a potentially chained shell command, allowing an attacker to bypass restrictions by prepending a safe command to a malicious one. Additionally, the list of shell builtins to ignore (shopt, set, trap) is duplicated across shell-utils.ts, MacOsSandboxManager.ts, and commandUtils.ts. Consider extracting this list into a shared constant (e.g., SHELL_BUILTINS_TO_IGNORE) to improve maintainability and ensure consistent security filtering across the codebase.
| const roots = getCommandRoots(stripped).filter( | |
| (r) => r !== 'shopt' && r !== 'set', | |
| (r) => r !== 'shopt' && r !== 'set' && r !== 'trap', | |
| ); | |
| const roots = getCommandRoots(stripped).filter( | |
| (r) => r !== 'shopt' && r !== 'set' && r !== 'trap', | |
| ); | |
| if (roots.length === 1) { | |
| return roots[0]; | |
| } | |
| return path.basename(command); |
| const roots = getCommandRoots(stripped).filter( | ||
| (r) => r !== 'shopt' && r !== 'set', | ||
| (r) => r !== 'shopt' && r !== 'set' && r !== 'trap', | ||
| ); |
There was a problem hiding this comment.
The getCommandName function is vulnerable to privilege escalation via command chaining. It extracts only the first command root (e.g., git from git; malicious_cmd) and uses it to look up and grant persistent permissions for the entire execution. An attacker can exploit this by prepending a privileged command to a malicious one, causing the sandbox to be configured with the privileges of the first command while executing the entire chain.
This issue is exacerbated by the current PR which prepends trap '' HUP; to commands, as the filtering logic now explicitly skips the prepended trap to find the 'first' command, which might be followed by other malicious commands in a chain.
| const roots = getCommandRoots(stripped).filter( | |
| (r) => r !== 'shopt' && r !== 'set', | |
| (r) => r !== 'shopt' && r !== 'set' && r !== 'trap', | |
| ); | |
| const roots = getCommandRoots(stripped).filter( | |
| (r) => r !== 'shopt' && r !== 'set' && r !== 'trap', | |
| ); | |
| if (roots.length === 1) { | |
| return roots[0]; | |
| } | |
| return path.basename(req.command); |
…CommandName against command-chaining
cd57ddb to
0720423
Compare
|
/gemini review |
…hell tool heuristics
57c5ad4 to
6f80088
Compare
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
There was a problem hiding this comment.
Code Review
This pull request introduces a SHELL_BUILTINS_TO_IGNORE set to standardize command filtering across sandbox managers and adds a trap '' HUP prefix to shell commands on Unix to handle SIGHUP signals in detached sessions. Reviewers have raised significant security concerns regarding the inclusion of trap in the ignore list, noting that it could facilitate sandbox bypasses by allowing malicious commands to be hidden within the trap builtin.
| * when identifying the primary command for permission/sandbox checks. | ||
| * Centralised here so all callers stay in sync automatically. | ||
| */ | ||
| export const SHELL_BUILTINS_TO_IGNORE = new Set(['shopt', 'set', 'trap']); |
There was a problem hiding this comment.
Including trap in SHELL_BUILTINS_TO_IGNORE introduces a sandbox bypass vulnerability. This allows an attacker to prepend a malicious trap command, which is ignored by the sandbox manager, leading to the execution of malicious code with elevated permissions when the shell exits. It is crucial to carefully evaluate any additions to SHELL_BUILTINS_TO_IGNORE to prevent similar security bypasses.
| export const SHELL_BUILTINS_TO_IGNORE = new Set(['shopt', 'set', 'trap']); | |
| export const SHELL_BUILTINS_TO_IGNORE = new Set(['shopt', 'set']); |
| const stripped = stripShellWrapper(fullCmd); | ||
| const roots = getCommandRoots(stripped).filter( | ||
| (r) => r !== 'shopt' && r !== 'set', | ||
| (r) => !SHELL_BUILTINS_TO_IGNORE.has(r), |
There was a problem hiding this comment.
Filtering out trap from the command roots allows an attacker to hide malicious code within a trap builtin. If an attacker provides a command like trap 'rm -rf /' EXIT; git status, this filter will remove trap, leaving only git. The logic then incorrectly identifies the command as a single-root git command and grants it the corresponding sandbox permissions, which the malicious trap can then abuse.
| const stripped = stripShellWrapper(fullCmd); | ||
| const roots = getCommandRoots(stripped).filter( | ||
| (r) => r !== 'shopt' && r !== 'set', | ||
| (r) => !SHELL_BUILTINS_TO_IGNORE.has(r), |
There was a problem hiding this comment.
Filtering out trap from the command roots allows an attacker to hide malicious code within a trap builtin. If an attacker provides a command like trap 'rm -rf /' EXIT; git status, this filter will remove trap, leaving only git. The logic then incorrectly identifies the command as a single-root git command and grants it the corresponding sandbox permissions, which the malicious trap can then abuse.
| const stripped = stripShellWrapper(fullCmd); | ||
| const roots = getCommandRoots(stripped).filter( | ||
| (r) => r !== 'shopt' && r !== 'set', | ||
| (r) => !SHELL_BUILTINS_TO_IGNORE.has(r), |
There was a problem hiding this comment.
Filtering out trap from the command roots allows an attacker to hide malicious code within a trap builtin. If an attacker provides a command like trap 'rm -rf /' EXIT; git status, this filter will remove trap, leaving only git. The logic then incorrectly identifies the command as a single-root git command and grants it the corresponding sandbox permissions, which the malicious trap can then abuse.
| const stripped = stripShellWrapper(fullCmd); | ||
| const roots = getCommandRoots(stripped).filter( | ||
| (r) => r !== 'shopt' && r !== 'set', | ||
| (r) => !SHELL_BUILTINS_TO_IGNORE.has(r), |
There was a problem hiding this comment.
Filtering out trap from the command roots allows an attacker to hide malicious code within a trap builtin. If an attacker provides a command like trap 'rm -rf /' EXIT; git status, this filter will remove trap, leaving only git. The logic then incorrectly identifies the command as a single-root git command and grants it the corresponding sandbox permissions, which the malicious trap can then abuse.
Body
Situation:
Users on WSL2, Kitty, and Alacritty reported all shell commands instantly dying with
Command terminated by signal: 1(SIGHUP). These environments aggressively send SIGHUP to detached process groups that lack a controlling terminal.Task:
Fix the SIGHUP crash without sacrificing process isolation (security).
Action:
This PR prepends
trap '' HUP;to every bash command string on non-Windows platforms, insideprepareExecution. This is the exact mechanism the POSIXnohuputility uses — it setsSIG_IGNfor SIGHUP before execution. Unlike a shell-leveltrap,SIG_IGNis inherited acrossexec(), meaning every child process spawned by the command also ignores SIGHUP, making the guard genuinely effective.Critically,
detached: !isWindows && !isBunis preserved, keeping full process-group isolation for security:kill -9 0inside a child command cannot reach the parent process groupResult:
killProcessGroupis unaffected (already handles non-group-leader cleanup viapgrep -Ptree walk)Fixes #16248
First-approach
initially tried detached: false, but as you noted, it introduced severe process isolation vulnerabilities (TIOCSTI and kill -9 0 DoS).
Second-approach
We restored detached: true to keep full isolation. To fix the SIGHUP bug, we now prepend trap '' HUP; to the command. Because SIG_IGN is inherited across exec() (just like nohup), this safely prevents PTY environments from killing the process while keeping security boundaries intact.