Skip to content

fix(security): replace new Function template evaluation and shelljs with execFile - #7

Open
zachariah-mithani wants to merge 1 commit into
runnerty:mainfrom
zachariah-mithani:fix/command-injection-execfile
Open

fix(security): replace new Function template evaluation and shelljs with execFile#7
zachariah-mithani wants to merge 1 commit into
runnerty:mainfrom
zachariah-mithani:fix/command-injection-execfile

Conversation

@zachariah-mithani

Copy link
Copy Markdown

Summary

exec() in index.js dynamically rendered shell script strings via render() (which instantiated new Function) and passed the resulting string directly to shell.exec().

If compress_file or decompress_dir contained shell metacharacters or template literals (e.g. ${...}), arbitrary code/command execution would occur on the host (CWE-78: OS Command Injection).

Fix

  • Replace render() and dynamic template generation with discrete argument vectors in COMPRESS_PROFILE.
  • Replace shell.exec() with child_process.execFile() passing discrete argument arrays to tar, unzip, bunzip2, unrar, 7z, and gunzip.
  • Safely ensure destination directory existence using fs.mkdirSync(dir, { recursive: true }).

@zachariah-mithani

Copy link
Copy Markdown
Author

Adding execution evidence — and correcting one thing I got wrong in the PR description.

Reproduced on 3.1.1 — the current npm latest, driving the module's own render() + shell.exec() steps:

fileName = 'x$(touch canary.txt)'
// rendered:  mkdir -p ./out && unzip -d ./out -o "x$(touch canary.txt)"
// -> canary.txt is created

Command substitution executes inside the double quotes, so quoting the path is not a mitigation. Negative control: a benign fileName creates nothing.

The correction: my PR description implied the new Function template rendering was itself injectable. It isn't — opts values are substituted once and are not re-evaluated, so a ${...} inside a value does nothing. The exploitable path is only shell.exec() on the rendered string. The fix in this PR still addresses it (discrete argv via execFile), but I wanted the reasoning on record accurately rather than overstated.

Affected range: <= 3.1.1. No advisory filed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant