Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] JSON.stringify should not be used for shell escaping #14

Closed
jbms opened this issue Oct 28, 2020 · 1 comment
Closed

[BUG] JSON.stringify should not be used for shell escaping #14

jbms opened this issue Oct 28, 2020 · 1 comment

Comments

@jbms
Copy link

jbms commented Oct 28, 2020

run-script uses JSON.stringify to quote/escape command arguments:

cmd = pkg.scripts[event] + args.map(a => ` ${JSON.stringify(a)}`).join('')

However, shells don't use the same quoting rule as JSON, which leads to incorrect behavior. In particular, literal newline characters (NL) get converted to \n (BACKSLASH N) by JSON.stringify, but bash does not treat \n as an escape sequence, and as a result the script receives \n (BACKSLASH N) as its argument.

To fix this problem, shell escaping should be used instead of JSON stringify.

I ran into this calling a script that invokes webpack with a --define argument containing a literal newline (NL) character. npm 7 converts this to a \n, which results in webpack substituting invalid javascript code leading to a parse error.

@isaacs isaacs closed this as completed in da14d3b Nov 17, 2020
Alhadis added a commit to Alhadis/Utils that referenced this issue May 25, 2021
Backticks aren't escaped in npx(1)'s positional arguments like they used
to, and they're also passed to sh(1) as double-quoted strings (which the
FUCK would you not use single-quotes??!). Because the `--header` comment
contained "run `make types` to update", NPM was essentially fork-bombing
its process.

References: npm/run-script#14
@Alhadis
Copy link

Alhadis commented May 25, 2021

This is a serious security risk. Consider something like

npx write-script > output.js \
	--footer '// Run `rm -rf $PATH_TO_YOUR_PROJECT/**` to clean'

where the script author has enclosed rm -rf … in single-quotes to avoid executing it. Since $PATH_TO_YOUR_PROJECT is unlikely to be defined in one's environment, NPM ends up running something like

sh -c write-script "--footer" "// Run `rm -rf /**` to clean"

... at which point, it's game over.

The problem extends to usage of $ as well, which isn't escaped because it isn't a metacharacter in JSON, but it sure as hell is in shell-scripts ($(cmd), ${variable_name}`, etc).

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 a pull request may close this issue.

3 participants
@Alhadis @jbms and others