Support single string input without using the shell option

[ehmicky]

Fix #176

This allows passing command+arguments as a single string with execa() without using the shell option:

execa('eslint *.js *.yml --ignore-path=.gitignore --fix --cache --format=codeframe --max-warnings=0')

Instead of:

execa('eslint', ['*.js', '*.yml', '--ignore-path=.gitignore', '--fix', '--cache', '--format=codeframe', '--max-warnings=0'])

This is not only for convenience, the main goal is actually to make execa more secure, cross-platform and faster. The reason is: many users might be tempted to use execa.shell() for the convenience of specifying command and arguments inside a single string, because it feels closer to typing in a terminal. However execa.shell() involves using a shell interpreter which is:

• less secure: it allows for command injection by passing arguments like $(rm -rf /) or && rm -rf /
• less cross-platform: it encourages using shell-specific features such as globbing, single quote escaping or even semicolons which won't work on cmd.exe.
• slower as it goes through an extra step (the shell interpreter)

Giving users the possibility to do this without execa.shell() prevents this issue.

Like execa() (and the underlying child_process.spawn()) arguments do not need any escaping/quoting. One exception: spaces can be escaped with a backslash if not meant as an arguments delimiter.

[ehmicky]

Rebased to latest master

[sindresorhus]

Sorry for the slow response. Too many PRs to review.

Are you sure there's no possibility of injection attacks using this? That's what's been holding me back from adding this in the past.

[sindresorhus]

Does this work on execa.sync too?

[sindresorhus]

Maybe we should add a note in execa.shell about the downsides of using it (like you've mention in the PR description) and that users should prefer the non-shell methods.

[ehmicky]

I have fixed what you mentioned in the code review 👍

Yes this works with execa.sync() as well. I have just added a test for it.

There is already a note in the README mentioning Prefer execa() whenever possible, as it's faster, safer and more cross-platform.. Should we make this more descriptive?

I think for the injection attacks: it can happen with shell: true but not with shell: false. With shell: true (including execa.shell()), the command is interpreted by a shell (usually Bash or cmd.exe) which allows several type of injections:

• environment variables substitution:

const userInput = '$SOME_SECRET_VAR'; 
execa.shell(`echo ${userInput}`);

• spawning commands through sub-shells:

const userInput = '$( rm -rf / )'; 
execa.shell(`echo ${userInput}`);

With shell: false (which this PR uses as default for execa()), the command is not interpreted by a shell and instead passed directly as an OS syscall. This means no injection can be performed, as the arguments will always be passed as is. No substitution/interpration/quoting is performed, they are just string literals.

Actually this PR translates execa('echo foo bar $var') into execa('echo', ['foo', 'bar', '$var']), i.e. it has the same characteristics (including security) as the current execa().

I just checked the Node source code and arguments passed to the underlying childProcess.spawn() with shell: false ends up here then here on Unix and here on Windows. Everything looks good security-wise.

So overall this will actually improve security by discouraging users from using execa.shell() when they just want the convenience of specifying the command and arguments as a single string.

[ehmicky]

Thanks for the review! This is all fixed now.

[sindresorhus]

This is looking great now. Thanks for spending so much time on perfecting it 👌

[ashokaditya]

@sindresorhus @ehmicky It would be super nice if you were to publish these changes to npm quickly. I've been using Snyk for checking security vulnerabilities in my projects and the latest Snyk is using [email protected] and [email protected] which still allows Arbitrary Command Injection.

More on this https://app.snyk.io/vuln/SNYK-JS-EXECA-174564

[ehmicky]

I think Snyk is getting this wrong. I just sent an email to Snyk mostly copy/pasting this message, and asking if they could remove this vulnerability warning. Zendesk ticket here: https://support.snyk.io/hc/en-us/requests/374?flash_digest=9f426e2665b5434970bca8d3421577124b9f64bd (not sure if this can be viewed by anyone).

Note: @sindresorhus has npm publish access.

Details: Node.js child_process.spawn() (and related methods) has a shell option which passes input to the shell (typicall /bin/sh, /bin/bash or cmd.exe). If improperly used, this option can lead to command injection. As the Node.js documentation says:

If the shell option is enabled, do not pass unsanitized user input to this function. Any input containing shell metacharacters may be used to trigger arbitrary command execution.

execa() is a wrapper around child_process.spawn(). As such it inherits that shell option. execa.shell() is just a shortcut for execa(..., { shell: true }) (similar to child_process.exec()). I.e. execa does not introduce any vulnerabilities already present in Node.js itself.

What this PR does is to encourage users not to use the shell option. It does it by allowing the command and its arguments to be specified as a single string, which is convenient and similar to the shell option, but without the command execution risk, because everything is escaped.

const argumentsArray = ['echo', ['rm', '-r', '/some/dir']]
const argumentsString = 'echo rm -r /some/dir'

// Those commands won't execute `rm`
child_process.spawn(...argumentsArray)
execa(...argumentsArray)

// Those commands will execute `rm`
child_process.spawn(argumentsString, { shell: true }) 
child_process.exec(argumentsString)
execa(argumentsString, { shell: true })
execa.shell(argumentsString)

// Added by this PR. It won't execute `rm`
execa(argumentsString)

[robcresswell]

@ehmicky Hey, I'm one of the engineers at Snyk. Thanks for the update. We're pulling it from the vuln db while we re-evaluate. Sorry for the trouble.

[ehmicky]

@robcresswell thanks for the quick response!

[sindresorhus]

@robcresswell This is not ok. Snyk cannot go around publishing false information like this. None of us maintainers were even contacted before the report was published, which would have prevented this mishap. I urge you to improve your processes.

[ehmicky]

Snyk vulnerability is now completely removed. See https://support.snyk.io/hc/en-us/requests/374