Switch _cloneWithGit + fetchRefs from child_process.exec to execFile (CWE-78 shell-injection fix)#404
Conversation
… companion to _cloneWithGit/fetchRefs)
|
Heads-up for the maintainer team: the failing anti-slop check is Yog reached out on the original email thread inviting a contribution, so I'm not surprised this PR shape isn't slop. If the anti-slop rule auto-closes this, happy to either:
Whichever works best for the repo's workflow. |
| // CWE-78 fix: switch from child_process.exec (which spawns /bin/sh -c | ||
| // <command> and parses shell metacharacters in interpolated args) to | ||
| // execFile, which takes the command + argv array and bypasses the shell | ||
| // entirely. Callers pass (cmd, [args...]). |
YogliB
left a comment
There was a problem hiding this comment.
tests are failing, care to fix them? :)
|
Closed in favor of #410, which contains the merged replacement changes. |
Picks up the disclosure I emailed to Rich + Yog yesterday (CWE-78 shell-injection via
child_process.exectemplate-string in_cloneWithGit+fetchRefs). Yog encouraged contributions on the thread, so here it is — same fix the original disclosure suggested.What this changes
src/utils.js:execbecomespromisify(execFile)instead ofpromisify(child_process.exec). Calls site has to pass argv-array instead of a shell-formatted string, but no shell parses the command line, so template-string interpolation of user-influenced values becomes safe even with shell metacharacters present.src/index.js_cloneWithGit: switch to argv form (exec('git', ['clone', this.repo.ssh, dest])). Therm -rf <dir>/.gitcall gets replaced withfs.promises.rm(dir, { recursive: true, force: true })(available since Node 14.14.0), which removes the second exec entirely.src/index.jsfetchRefs: switch to argv form (exec('git', ['ls-remote', repo.url])).Behavior change for normal inputs
Zero. The only difference is what happens when shell metacharacters appear in the parsed
user/namecapture groups (today:/bin/shinterprets them; after this PR:gitrejects the path with a normal "invalid argument" error).Reproducer for the pre-fix shape (still works on
mainas of HEAD);:>marker_PWNis parsed by/bin/shbeforegit cloneerrors out. After this PR, the same input is passed verbatim togit cloneas one argv slot and fails cleanly with "repository not found" — no shell interpretation.I tested locally that the well-formed cases (
degit user/repo,degit github:user/repo#ref, ssh form) all still work after the change. Happy to add a unit test under tests/ if you want to lock the regression in.