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

Allow multiple string arguments for exec() #103

Closed
nzakas opened this issue Jan 31, 2014 · 18 comments
Closed

Allow multiple string arguments for exec() #103

nzakas opened this issue Jan 31, 2014 · 18 comments
Labels
exec high priority security
Milestone

Comments

@nzakas
Copy link
Contributor

@nzakas nzakas commented Jan 31, 2014

I've found that I end up doing a lot of messy string concatenation with exec() commands. It would be nice to allow multiple string arguments that would be joined together with a space, such as:

exec('npm', 'run-script', 'test');   // run "npm run-script test"

This could easily be backwards compatible with the current function since only the first argument is currently a string.

@svnlto
Copy link

@svnlto svnlto commented Feb 5, 2014

👍

@isRuslan
Copy link

@isRuslan isRuslan commented May 29, 2014

+1

@nfischer
Copy link
Member

@nfischer nfischer commented Feb 3, 2016

@nzakas would this syntax work for you?

exec(['npm', 'run-script', 'test'].join(' '));   // run "npm run-script test"

The concern is that this makes parsing slightly harder, since exec() can take options as well as a callback.

@nzakas
Copy link
Contributor Author

@nzakas nzakas commented Feb 4, 2016

No, that's what I want to avoid. I implemented something similar here:
https://github.com/nzakas/shelljs-nodecli/blob/master/lib/shelljs-nodecli.js#L82

Feel free to use that code if you'd like.

@nfischer
Copy link
Member

@nfischer nfischer commented Feb 4, 2016

I'll look into this option. Unfortunately, there's also some people who would like to be able to run multiple commands from one exec() call, i.e.

exec('git status', 'git add .', 'git commit -am message');

as an alternative for

exec('git status');
exec('git add .');
// etc...

@sedwards2009
Copy link

@sedwards2009 sedwards2009 commented Feb 5, 2016

shelljs pulls directly from the typical unix command set, and in unix exec() with a list of strings means command in the first place and separate argument strings following. The main reason to use exec() like this is so that you don't have to do any kind of shell escaping on the arguments. People get this wrong all the time and it leads to severe security problems like: https://www.owasp.org/index.php/Command_Injection

I strongly recommend that exec() with a list of strings be a safe way to build and run commands.

@nzakas
Copy link
Contributor Author

@nzakas nzakas commented Feb 5, 2016

I don't really care at this point, I have my own wrapper that does what I need. I opened this issue two years ago, so feel free to close it if you don't want to address it.

@nfischer
Copy link
Member

@nfischer nfischer commented Feb 7, 2016

There's been some discussion of this in #143 as well.

@sedwards2009 What do you mean that exec doesn't require shell escaping? Bash has an exec command (which our exec() aims to emulate), but I don't think it provides that feature. For example:

# assume an empty directory
$ touch a.txt
$ touch b.txt
$ echo * # the star gets expanded by the shell
a.txt b.txt
$ (exec echo *) # use exec (wrap this in a subshell so that we can see the output)
a.txt b.txt
# * still gets expanded, even though we used exec

I agree that the posix system call, exec(3), doesn't do any sort of shell globbing, but that's because it's a syscall, not a shell command. I can't think of any system call that supports glob characters, except glob(3) (which is specifically for that purpose).

@sedwards2009
Copy link

@sedwards2009 sedwards2009 commented Feb 7, 2016

@nfischer Sorry for being unclear. I was actually specifically thinking of the posix family of exec*() calls.

The point being, it is a good idea to offer safe ways to use commands like exec, maybe even make that the default way of working with some commands. For example, you could make commands accept a single string argument which is parsed the way a shell would, and also offer a safer array-of-strings way which doesn't perform globbing or confuse filenames for command switches. (just thinking)

@nfischer
Copy link
Member

@nfischer nfischer commented Feb 7, 2016

@sedwards2009 what do you think of my proposal in #143? It would be a safer alternative

@sedwards2009
Copy link

@sedwards2009 sedwards2009 commented Feb 7, 2016

@nfischer I think you're going to need a shellEscape() function anyway, but it is important to make the safe thing as easy as possible (and default if possible). In this case a function which accepts a list of strings as arguments and doesn't interpret the strings (e.g. no globs, escapes etc) and treats them as being plain, is probably the best option.

The big problem with requiring people to apply shellEscape() to get safe code is that: 1) the unsafe code appears to work fine, 2) the unsafe looks simpler, 3) people have to remember to use shellEscape(). It is a combo which is just going to go wrong.

@nfischer nfischer self-assigned this Feb 8, 2016
@nfischer
Copy link
Member

@nfischer nfischer commented Feb 8, 2016

@sedwards2009 I think something like shellEscape() would be a nice feature, although this may not be a complete solution. I would hate to make that the default behavior, because then cp('*.txt', 'dest') wouldn't work as most people would expect. But it would be nice for those that want extra-safe code in general.

I think the solution for exec() in particular will be along the lines of what I proposed here. That's not necessarily incompatible with shellEscape(), but it'll capture the problems that are unique to exec() (like semicolons in filenames, for example, which doesn't adversely affect touch()).

@sedwards2009
Copy link

@sedwards2009 sedwards2009 commented Feb 9, 2016

@nfischer The goals of shelljs seem to conflict sometimes. You want to provide something which brings the important idioms and conveniences of the unix shell to JS, but you also want something safe, easy to use and predicable. Unix shells often collide with the last few desired features. :-/

I agree that cp('*.txt', 'dest') and rm('*.bak') should glob, otherwise you might as well be using fs.unlinkSync() directly.

I like the proposal for exec(), and I think in this case it should default to very safe behaviour simply because exec() is so powerful and dangerous, and also because I suspect it won't be used very often compared to rm() and cp() etc. It is a compromise.

@nfischer
Copy link
Member

@nfischer nfischer commented Feb 9, 2016

@sedwards2009 Excellent point. This is the default behavior I'm leaning toward right now:

exec('git', 'add', fname); // arguments are listed, so let's default to no globbing
exec('git add ' + fname); // this is one string, so the default should be globbing

This is for a few reasons:

  1. This is easier. We would impement the "safe" exec using execFile() or spawn(), which takes arguments kind-of in the multi-argument style. We would implement globbing using child_process.exec(), which takes a single string argument
  2. This is backwards compatible
  3. The only way to make truly "safe" arguments is with a list-like syntax, since we would split a single-string on its spaces (cmdString.split(' ');), which wouldn't play well with files containing spaces

What are your thoughts?

@nfischer nfischer added the exec label Feb 9, 2016
@nfischer nfischer added this to the v0.7.0 milestone Feb 16, 2016
@ariporad
Copy link
Contributor

@ariporad ariporad commented Feb 16, 2016

@nfischer: See my comment on #143.

@nfischer nfischer removed this from the v0.7.0 milestone Mar 11, 2016
@nfischer nfischer added this to the v0.8.0 milestone Mar 11, 2016
@nfischer nfischer added this to the v0.8.0 milestone Mar 11, 2016
@nfischer nfischer removed this from the v0.7.0 milestone Mar 11, 2016
@nfischer
Copy link
Member

@nfischer nfischer commented Jul 1, 2016

We're still working on an implementation in shelljs itself, but I implemented a shelljs extension for node v6+ that addresses some of the security concerns here, while also giving a very convenient syntax:
https://github.com/nfischer/shelljs-exec-proxy

@nfischer
Copy link
Member

@nfischer nfischer commented Oct 20, 2017

Bumping these back to v0.9, but this should be quick to fix once v0.8 is released.

@nfischer nfischer removed this from the v0.8.0 milestone Oct 20, 2017
@nfischer nfischer added this to the v0.9.0 milestone Oct 20, 2017
@nfischer
Copy link
Member

@nfischer nfischer commented Jul 11, 2018

Merging into #495.

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, #810, #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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exec high priority security
Projects
None yet
Development

No branches or pull requests

6 participants