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

Support single string input without using the shell option #182

Merged
merged 32 commits into from Apr 30, 2019
Merged

Support single string input without using the shell option #182

merged 32 commits into from Apr 30, 2019

Conversation

ehmicky
Copy link
Collaborator

@ehmicky ehmicky commented Mar 1, 2019

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
Copy link
Collaborator Author

ehmicky commented Mar 6, 2019

Rebased to latest master

@ehmicky ehmicky changed the title Single string input without using the shell Single string input without using the shell option Mar 8, 2019
@sindresorhus sindresorhus added this to the v2 milestone Mar 10, 2019
@sindresorhus
Copy link
Owner

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
Copy link
Owner

Does this work on execa.sync too?

@sindresorhus
Copy link
Owner

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.

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@ehmicky
Copy link
Collaborator Author

ehmicky commented Mar 19, 2019

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.

readme.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@ehmicky
Copy link
Collaborator Author

ehmicky commented Apr 21, 2019

Thanks for the review! This is all fixed now.

@sindresorhus sindresorhus changed the title Single string input without using the shell option Support single string input without using the shell option Apr 30, 2019
@sindresorhus sindresorhus merged commit 075faf3 into sindresorhus:master Apr 30, 2019
@sindresorhus
Copy link
Owner

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

@ehmicky ehmicky deleted the feature/single-string-command branch April 30, 2019 08:13
@ashokaditya
Copy link

ashokaditya commented May 2, 2019

@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 execa@1.0.0 and execa@0.7.0 which still allows Arbitrary Command Injection.

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

@ehmicky
Copy link
Collaborator Author

ehmicky commented May 2, 2019

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
Copy link

@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
Copy link
Collaborator Author

ehmicky commented May 2, 2019

@robcresswell thanks for the quick response!

@sindresorhus
Copy link
Owner

@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.

Repository owner locked as resolved and limited conversation to collaborators May 2, 2019
@ehmicky
Copy link
Collaborator Author

ehmicky commented May 2, 2019

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature proposal] Single string input without using the shell
4 participants