Skip to content

Switch _cloneWithGit + fetchRefs from child_process.exec to execFile (CWE-78 shell-injection fix)#404

Closed
JAE0Y2N wants to merge 2 commits into
Rich-Harris:masterfrom
JAE0Y2N:fix/cwe78-shell-injection-execfile
Closed

Switch _cloneWithGit + fetchRefs from child_process.exec to execFile (CWE-78 shell-injection fix)#404
JAE0Y2N wants to merge 2 commits into
Rich-Harris:masterfrom
JAE0Y2N:fix/cwe78-shell-injection-execfile

Conversation

@JAE0Y2N

@JAE0Y2N JAE0Y2N commented May 20, 2026

Copy link
Copy Markdown

Picks up the disclosure I emailed to Rich + Yog yesterday (CWE-78 shell-injection via child_process.exec template-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: exec becomes promisify(execFile) instead of promisify(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])). The rm -rf <dir>/.git call gets replaced with fs.promises.rm(dir, { recursive: true, force: true }) (available since Node 14.14.0), which removes the second exec entirely.
  • src/index.js fetchRefs: 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 / name capture groups (today: /bin/sh interprets them; after this PR: git rejects the path with a normal "invalid argument" error).

Reproducer for the pre-fix shape (still works on main as of HEAD)

$ mkdir /tmp/degitpoc && cd /tmp/degitpoc
$ rm -f marker_PWN
$ node /path/to/degit/src/bin.js 'github:foo/bar;:>marker_PWN' --mode=git --force test
> cloned foo/bar;:>marker_PWN#HEAD to test
$ ls -la marker_PWN
-rw-r--r--  1 user  staff  0 May 20 16:58 marker_PWN

;:>marker_PWN is parsed by /bin/sh before git clone errors out. After this PR, the same input is passed verbatim to git clone as 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.

@JAE0Y2N

JAE0Y2N commented May 20, 2026

Copy link
Copy Markdown
Author

Heads-up for the maintainer team: the failing anti-slop check is max-daily-forks (13 in 24h, max 6). The high fork-count is from a security-disclosure drive I'm running today across multiple OSS repos (langchain-ai/langgraph#7873 same Zip Slip class, huggingface/transformers#46118 sibling fix, plus others under coordinated disclosure with various security teams). Every fork was created to file a corresponding fix PR; none were drive-by.

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:

  1. Wait until tomorrow when the 24h window resets and reopen, or
  2. Have Yog / a maintainer add the exempt label so the check passes, or
  3. Coordinate the patch through the email thread Yog and I are already on.

Whichever works best for the repo's workflow.

@YogliB YogliB added the exempt label May 20, 2026
@YogliB YogliB closed this May 20, 2026
@YogliB YogliB reopened this May 20, 2026
@YogliB YogliB self-requested a review as a code owner May 20, 2026 18:26
Comment thread src/utils.js
Comment on lines +33 to +36
// 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...]).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pleaes remove the comment

@YogliB YogliB left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests are failing, care to fix them? :)

@YogliB

YogliB commented May 20, 2026

Copy link
Copy Markdown
Collaborator

Closed in favor of #410, which contains the merged replacement changes.

@YogliB YogliB closed this May 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants