fix(security): remove shell injection vector in Windows process spawning#2687
fix(security): remove shell injection vector in Windows process spawning#2687Ivorisnoob wants to merge 6 commits into
Conversation
ssh, tailscale, git, and powershell.exe are native executables and do not need cmd.exe as an intermediary. Passing shell:true caused Node.js to route args through `cmd.exe /d /s /c "..."`, where metacharacters like & and | are interpreted as command separators. A user-controlled hostname in the SSH flow could trigger arbitrary command execution. npm-installed CLIs (claude, codex, cursor) are intentionally left with shell:true as they may only exist as .cmd wrappers on Windows.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ApprovabilityVerdict: Needs human review Changes to SSH process spawning code in a security-sensitive package. While removing shell execution hardens against injection, the original Windows-specific behavior existed for functional reasons and this change could affect Windows runtime behavior. Security-sensitive code paths warrant human review. You can customize Macroscope's approvability policy. Learn more. |
|
problem is most of these just doesn't work without |
Yeah you're right about the others, reverted those. Kept only the SSH one because ssh.exe is a native exe on Windows 11 (confirmed at C:\Windows\System32\OpenSSH\ssh.exe), so it doesn't need cmd.exe at all. And the hostname is literally the only spot in the whole codebase where user-typed input goes straight into spawn args without any sanitization, so that one's a real issue. |
What Changed
Replaced
shell: process.platform === "win32"withshell: falseforseven spawn sites that invoke native Windows executables:
packages/ssh/src/command.ts—sshpackages/ssh/src/tunnel.ts—sshpackages/tailscale/src/tailscale.ts(×2) —tailscaleapps/server/src/project/Layers/RepositoryIdentityResolver.ts(×2) —gitapps/server/src/diagnostics/ProcessDiagnostics.ts—powershell.exe/psapps/server/src/terminal/Layers/Manager.ts—powershell.exenpm-installed CLIs (
claude,codex,cursor,opencode) are leftunchanged — on Windows these may only exist as
.cmdbatch wrappers,which require shell resolution to execute.
Why
With
shell: true, Node.js constructs:cmd.exeinterprets&,|, and;as command separators eveninside this string. The SSH hostname is user-controlled and passed
through with no sanitization, so a value like:
causes
cmd.exeto download and execute the payload immediately afterthe (failing) SSH attempt — silent RCE on the developer's machine.
ssh.exe,tailscale.exe,git.exe, andpowershell.exeare allnative executables available on
%PATH%. They spawn correctly withshell: false, which passes the args array directly to the OS with noshell interpretation.
UI Changes
N/A
Checklist
Note
Medium Risk
Changes how SSH commands/tunnels are spawned (no shell) which reduces Windows command-injection risk but could impact Windows execution behavior if
sshresolution differed previously.Overview
Removes the Windows-specific
shell: process.platform === "win32"behavior and now spawnssshwithshell: falsein bothrunSshCommandInScope(command.ts) andstartSshTunnel(tunnel.ts).This prevents
cmd.exeargument interpretation on Windows (mitigating shell-injection via user-controlled SSH target values) while keeping the rest of the SSH argument construction unchanged.Reviewed by Cursor Bugbot for commit 0a7b7f3. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Remove shell injection vector in Windows SSH process spawning
Sets
shell: falseunconditionally in bothrunSshCommandInScopeandstartSshTunnelin command.ts and tunnel.ts, replacing the previousprocess.platform === "win32"conditional. This eliminates a shell injection risk that existed when spawning SSH child processes on Windows.Macroscope summarized 0a7b7f3.