fix(security): prevent command injection in findCommand via safe spawnSync#27575
fix(security): prevent command injection in findCommand via safe spawnSync#27575Ashutosh0x wants to merge 3 commits into
Conversation
…nSync Replace shell-interpolated execSync calls with spawnSync argument arrays to prevent command injection in the findCommand() utility function. Vulnerability: - execSync(`where.exe `) on Windows (line 32) - execSync(`command -v `) on Unix (line 41) Both construct shell commands via string interpolation, which allows command injection if the 'command' parameter contains shell metacharacters (e.g., semicolons, pipes, backticks). Fix: 1. Replace execSync with spawnSync using argument arrays (shell: false) to eliminate shell interpretation entirely 2. Add input validation regex to reject command names with metacharacters as defense-in-depth 3. Use 'which' instead of 'command -v' on Unix since spawnSync requires an executable (command is a shell builtin)
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 potential command injection vulnerability in 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
|
There was a problem hiding this comment.
Code Review
This pull request enhances security in packages/core/src/ide/ide-installer.ts by validating the command name against a safe regex pattern and replacing shell-interpolated execSync calls with safe spawnSync executions. The review feedback recommends further securing the output of where.exe by sanitizing and resolving the returned path to prevent potential path traversal or null byte injection vulnerabilities.
| if (result.status === 0 && result.stdout) { | ||
| const firstPath = result.stdout.trim().split(/\r?\n/)[0]; | ||
| if (firstPath) { | ||
| return firstPath; | ||
| } | ||
| } |
There was a problem hiding this comment.
The path returned by where.exe is extracted from command output, which is considered an untrusted source. To prevent potential path traversal (..) or null byte injection (\0) vulnerabilities, the path should be sanitized and resolved before being returned and executed, in accordance with our security rules.
| if (result.status === 0 && result.stdout) { | |
| const firstPath = result.stdout.trim().split(/\r?\n/)[0]; | |
| if (firstPath) { | |
| return firstPath; | |
| } | |
| } | |
| if (result.status === 0 && result.stdout) { | |
| const firstPath = result.stdout.trim().split(/\r?\n/)[0]; | |
| if (firstPath && !firstPath.includes('\0') && !firstPath.includes('..')) { | |
| return path.resolve(firstPath); | |
| } | |
| } |
References
- Sanitize file paths extracted from untrusted sources, such as command output, to prevent path traversal (
..), null byte injection (\0), and other vulnerabilities.
Address Gemini Code Assist review: add path.resolve() and null byte check to sanitize the path returned by where.exe, preventing potential path traversal or null byte injection from untrusted command output.
…commandExistsAsync
Same vulnerability as ide-installer.ts: execSync with shell-interpolated
command names via getCommandExistsCmd(). Replaced with spawnSync/spawn
using argument arrays (shell: false).
- Removed getCommandExistsCmd() helper (no longer needed)
- Removed unused exec, execSync, promisify imports
- commandExists() now uses spawnSync('where.exe'/'which', [cmd])
- commandExistsAsync() now uses spawn('where.exe'/'which', [cmd])
Summary
Security fix: Replace shell-interpolated
execSynccalls with safespawnSync/spawnto prevent command injection via shell metacharacters in two files:packages/core/src/ide/ide-installer.ts—findCommand()packages/core/src/utils/editor.ts—commandExists()andcommandExistsAsync()Fixes #27576
Details
Both files construct shell commands via string interpolation with
execSync:If the parameters contain shell metacharacters (
;,|,`,$()), these are interpreted by the shell, allowing arbitrary command execution.Fixes applied:
ide-installer.tsexecSyncwithspawnSyncusing argument arrays (shell: false)/^[a-zA-Z0-9.\-_/\\]+$/) as defense-in-depthwhichinstead ofcommand -v(shell builtin incompatible withspawnSync)where.exeoutput withpath.resolve()and null byte checkeditor.tsexecSync(getCommandExistsCmd(cmd))withspawnSync('where.exe'/'which', [cmd])execAsync(getCommandExistsCmd(cmd))withspawn('where.exe'/'which', [cmd])getCommandExistsCmd()helper (no longer needed)exec,execSync,promisifyimportsRelated Issues
Fixes #27576
How to Validate
gemini-cli— editor detection and IDE installation should work as beforespawnSync/spawnwithshell: falseprevents shell metacharacter interpretationnpm testinpackages/corePre-Merge Checklist