Sitelet https://web.archive.org/web/20201123154046/https://github.com/shelljs/shelljs/issues/766
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

Snyk "high severity" issue #766

Closed
Unitech opened this issue Aug 26, 2017 · 3 comments
Closed

Snyk "high severity" issue #766

Unitech opened this issue Aug 26, 2017 · 3 comments
Labels

Comments

@Unitech
Copy link

@Unitech Unitech commented Aug 26, 2017 •

Hello,

Is there any plan to fix this security issue detected by Snyk? It breaks some of our users builds

https://snyk.io/test/npm/shelljs

image

Thanks,

@freitagbr
Copy link
Contributor

@freitagbr freitagbr commented Aug 26, 2017

Hi @Unitech,

The issue detected is the shell.exec API, which uses child_process.exec from Node.js core internally. The issue is that one could potentially pass user-input directly to the function. In order to maintain API backwards-compatibility, we cannot change this particular method, and so we are working on introducing a new, safer API in #524.

Are your users depending on ShellJS directly, or through another dependency? If they are depending on it directly, it is enough to just not pass un-sanitized input to shell.exec. If it is through a dependency, you could make sure that shell.exec is not passed input through its API somehow. ESLint, for example, does not use shell.exec in its API, so it is safe.

@Unitech
Copy link
Author

@Unitech Unitech commented Aug 30, 2017

Hello @freitagbr
I understand the problem. Snyk "security detection" is really too simplistic... No we do not pass anything variable to shelljs, we just use the .which method and one .exec...
Thanks for your answer

@nfischer
Copy link
Member

@nfischer nfischer commented Aug 30, 2017

The vulnerability is known. As @freitagbr points out, it's all about how you use it. shell.exec('printf foo') is totally safe, however shell.exec('printf ' + msg) might not be.

shell.which() has no known vulnerabilities 😄

kontrollanten added a commit to kontrollanten/algolia-places-react that referenced this issue Mar 5, 2018
kontrollanten added a commit to kontrollanten/algolia-places-react that referenced this issue Apr 18, 2018
nfischer added a commit that referenced this issue Jun 26, 2019
No change to logic.

This adds documentation about `shell.exec()`'s inherent vulnerability to
command injection and links to a more detailed security notice.

Issue #103, #143, #495, #765, #766, #810, #842, #938, #945
nfischer added a commit that referenced this issue Jun 26, 2019
No change to logic.

This adds documentation about `shell.exec()`'s inherent vulnerability to
command injection and links to a more detailed security notice.

Issue #103, #143, #495, #765, #766, #810, #842, #938, #945
@shelljs shelljs locked as resolved and limited conversation to collaborators Jul 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.