Add support for execFile-like interface to exec #495

nfischer · 2016-07-28T01:33:20Z

It would be great if exec had an interface, perhaps like the following:

var fileName = 'foo.txt';
exec('git', 'add', fileName);

This is great because of the following:

We can declare that each argument is its own non-splittable word (so fileName = 'file1.txt file2.txt' means the git call will search for one file named file1.txt file2.txt, not two separate files)
We can implement this fairly easily with child_process.execFile
This would allow us to do our own glob expansion, which would be good for Windows (Windows exec() won't glob-expand)
This would be useful in shx (shelljs/shx#65 & shelljs/shx#68)
Variables would have to be referenced as JavaScript variables just like in the rest of ShellJS, not as shell variables (exec('echo', env.PATH) vs. exec('ls $PATH')), which is the safer approach
Doing our own globbing means that exec will respect a call to set('-f')
This would give more security to shelljs-exec-proxy for the same reasons

The issue with this interface is what to do when we have only 1 argument (do we use the old behavior or the new execFile implementation?). An alternative approach is exec([arg1, arg2, arg3]), which uses a list to remove ambiguity. This is easier to parse, but perhaps not as nice to work with.

Edit: to clarify, this is a feature request, not a security vulnerability. This new API would be easier to use safely than shell.exec(), but shell.exec() is not itself vulnerable and can in fact be used safely without much difficulty (see security guidelines).

This adds an initial implementation of shell.cmd(), which is intended as the eventual replacement for shell.exec(). This PR does not fully implement the API, but demonstrates a simple and secure alternative, and will allow further iteration to cover other use cases in follow-up PRs. Design doc: https://shelljs.page.link/cmd-design Issue #495 Test: automated test suite

runk · 2019-01-28T23:33:11Z

Any news for this one?

https://app.snyk.io/vuln/npm:shelljs:20140723

nfischer · 2019-01-29T23:38:45Z

I'm still hoping to work on this. Unfortunately, it's really hard to provide reasonable behavior on Windows (#866 basically works for unix). I vaguely remember issues with v4/v5--now that we've dropped those, I need to check if this project is feasible.

MaksPob · 2019-04-03T06:10:39Z

Any news for this one?
When will you fix the bug?

https://app.snyk.io/vuln/npm:shelljs:20140723

@nfischer

nfischer · 2019-06-04T19:43:16Z

No major update. This is quite difficult to get right for Windows, and I don't have a machine anymore to speed up investigation. This is something I'm very interested in solving when I have more time & resources.

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, #810, #938, #945

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

Add a new command to launch external shell commands, providing a less vulnerable alternative to exec(), with cross-platform globbing. Fixes #143

nfischer · 2019-07-08T21:04:34Z

Updated the description, but this is not a security vulnerability/fix, rather this is an implementation of a feature request. Please follow #945 if you'd like to read discussion regarding how Snyk's report is misattributed.

If you're interested in being a consumer of this particular API, you're welcome to continue asking questions here (but you'll probably hear an update from me as soon as there's something to update).

This adds an initial implementation of shell.cmd(), which is intended as the eventual replacement for shell.exec(). This PR does not fully implement the API, but demonstrates a simple and secure alternative, and will allow further iteration to cover other use cases in follow-up PRs. Design doc: https://shelljs.page.link/cmd-design Issue #495 Test: automated test suite

wmertens · 2019-12-05T17:27:24Z

I just had to look into this, and I found child_process.spawn.

It seems to me that this should be used (with shell kept false); it looks like that works on Windows too, requires arguments as an array, preventing delimiter and shell expansion issues, and it provides the output as streams, thereby fixing #979.

If it turns out that windows can't execute scripts directly, it should be just a matter of passing the shell executable as the cmd and the script as the first argument.

nfischer · 2019-12-06T05:43:51Z

I worked around those issues in #866 by using https://github.com/sindresorhus/execa, which seems to handle Windows much more nicely than anything I had figured out by hand.

That PR implemented "step 1" outlined in https://shelljs.page.link/cmd-design, but I think we'll need to implement step 2 before we can deprecate shell.exec() and ship this. If you're interested in testing out the behavior or performance of the new API, you can clone the repo and uncomment this line (just don't do this in production code until the API is actually released).

nfischer added high priority exec security labels Jul 28, 2016

nfischer mentioned this issue Jul 28, 2016

Globbing arguments to an external command shelljs/shx#68

Closed

nfischer mentioned this issue Oct 9, 2016

feat: new alternative to exec #524

Closed

nfischer mentioned this issue Jun 14, 2017

shelljs auto generate some files? #619

Closed

calligraphic mentioned this issue Jan 10, 2018

Snyk vulnerability DB reporting command injection vulnerability in ShellJS #810

Closed

nfischer mentioned this issue Jul 3, 2018

feat: add shell.cmd to replace exec #866

Merged

nfischer added this to the v0.9.0 milestone Jul 12, 2018

corpetty mentioned this issue Feb 13, 2019

Snyk Vuln Report: Command Injection through `shelljs` embarklabs/embark#1329

Open

halhelal mentioned this issue Mar 22, 2019

Snyk Vulnerability: Command Injection through `shelljs` angular/angular#29460

Closed

nfischer mentioned this issue May 6, 2019

Solution to apostrophes within filenames used in shell commands? #938

Closed

nfischer mentioned this issue Jun 21, 2019

0.8.3 high severity vuln #945

Closed

kleber7777 mentioned this issue Jun 21, 2019

fix(handlebars): atualização do package-lock weareinlog/inlog-maps#26

Merged

nfischer mentioned this issue Jun 26, 2019

docs(exec): document security concerns #950

Merged

nfischer referenced this issue Jul 8, 2019

feat: new alternative to exec

7a9f8e5

Add a new command to launch external shell commands, providing a less vulnerable alternative to exec(), with cross-platform globbing. Fixes #143

nfischer added the feature label Jul 8, 2019

caiomdias mentioned this issue Jul 12, 2019

Solve security problems stone-payments/emerald-web-framework#231

Merged

nfischer mentioned this issue Oct 2, 2019

Explicitly require the commands to help JS bundlers find all the code #962

Closed

nfischer mentioned this issue Dec 4, 2019

temp files? #979

Closed

shelljs / shelljs

Add support for execFile-like interface to exec #495

Add support for execFile-like interface to exec #495

nfischer commented Jul 28, 2016 •

edited

runk commented Jan 28, 2019

nfischer commented Jan 29, 2019

MaksPob commented Apr 3, 2019 •

edited

nfischer commented Jun 4, 2019

nfischer commented Jul 8, 2019

wmertens commented Dec 5, 2019 •

edited

nfischer commented Dec 6, 2019

shelljs / shelljs

Join GitHub today

GitHub is where the world builds software

Add support for execFile-like interface to exec #495

Add support for execFile-like interface to exec #495

Comments

nfischer commented Jul 28, 2016 • edited

runk commented Jan 28, 2019

nfischer commented Jan 29, 2019

MaksPob commented Apr 3, 2019 • edited

nfischer commented Jun 4, 2019

nfischer commented Jul 8, 2019

wmertens commented Dec 5, 2019 • edited

nfischer commented Dec 6, 2019

Essential cookies

Always active

Analytics cookies

nfischer commented Jul 28, 2016 •

edited

MaksPob commented Apr 3, 2019 •

edited

wmertens commented Dec 5, 2019 •

edited