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

Escaping shell arguments to exec() #143

Closed
d11wtq opened this issue Jul 23, 2014 · 27 comments
Closed

Escaping shell arguments to exec() #143

d11wtq opened this issue Jul 23, 2014 · 27 comments
Labels
exec feature high priority security
Milestone

Comments

@d11wtq
Copy link

@d11wtq d11wtq commented Jul 23, 2014

First of all, I just want to say thanks for writing shelljs. It cleans up my code a lot.

I need to invoke commands with shell.exec(), however, those commands will include some input from external sources, to be passed as arguments to system executables. Is there a "safe" way to do this? I was hoping for the classic array syntax, like:

shell.exec(["ls", "-l", "/some/path"])

Though this doesn't seem to be implemented. It doesn't appear nodejs itself even has a shell escape function neither. Any plans on this front?

@manuelstofer
Copy link

@manuelstofer manuelstofer commented Oct 8, 2014

+1

1 similar comment
@Nieralyte
Copy link

@Nieralyte Nieralyte commented Oct 12, 2015

+1

@prankymat
Copy link

@prankymat prankymat commented Oct 31, 2015

+10000000

@nfischer
Copy link
Member

@nfischer nfischer commented Feb 3, 2016

@d11wtq we don't currently support an array syntax for exec(), but you can achieve that with ['ls', '-A', '/path/to/dir'].join().

Or is your concern about escaping values (such as how printf "\\" is written as exec('printf "\\\\"') in shelljs)?

@cscott
Copy link

@cscott cscott commented Feb 7, 2016

Please. Providing only a string argument here is a recipe for disaster, honestly. As pointed out, it's basically impossible to "properly" quote any arguments given. As is, exec is vulnerable to file names containing spaces, special characters, semicolons, the works. Cf http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/handle-metacharacters.html and the "motivation" section of (for example) https://www.python.org/dev/peps/pep-0324/. A security-conscious API should strongly encourage providing arguments as an array by default.

@nfischer
Copy link
Member

@nfischer nfischer commented Feb 7, 2016

Looks like this might be related to #103.

@cscott The concern is that, under-the-hood, we rely on Node's child_process module (specifically, exec() and execSync(). According to the docs, these only support a string argument. So although we could write something to allow exec() to take an array, I suspect we'd have to .join() the strings anyway.

As pointed out, it's basically impossible to "properly" quote any arguments given.

Could you give an example where it's impossible to quote arguments? I thought it was just a matter of escaping the appropriate characters, i.e. exec('head file\\ with\\ spaces.txt'). The point of exec is to take a string like you'd use for a /bin/bash command.

Maybe it would be better to expose a function for shell-escaping? Then we can write things such as:

// This gives us an alternative syntax
exec('printf "\\\\"'); // this works, but that's lots of backslashes! Very error-prone
exec(shellEscape('printf "\\"')); // this looks like the bash command: `printf "\\"`

// or, this example
ls('src/').forEach(function (fileName) {
  exec('head ' + shellEscape(fileName)); // it's safe to use the output of ls() now
}

@cscott
Copy link

@cscott cscott commented Feb 8, 2016

child_process.execFile takes separated arguments -- as does the
underlying c function. The point is to avoid using the shell (bash or
whatever) where it is not necessary, which avoids having to
reverse-engineer the shell's parsing algorithm and avoids any shell-related
exploits. The point is to provide a mechanism that is "secure by default"
and force folks to jump through a few hoops perhaps if they need to do
something insecure --- not to provide an "insecure by default" interface
and hope that all clients remember to use the appropriate escape functions
(hint: they won't).
On Feb 7, 2016 2:52 AM, "Nate Fischer" notifications@github.com wrote:

Looks like this might be related to #103
#103.

@cscott https://github.com/cscott The concern is that, under-the-hood,
we rely on Node's child_process module (specifically, exec() and
execSync(). According to the docs
https://nodejs.org/api/child_process.html#child_process_child_process_exec_command_options_callback,
these only support a string argument. So although we could write something
to allow exec() to take an array, I suspect we'd have to .join() the
strings anyway.

As pointed out, it's basically impossible to "properly" quote any
arguments given.

Could you give an example where it's impossible to quote arguments? I
thought it was just a matter of escaping the appropriate characters, i.e. exec('head
file\ with\ spaces.txt'). The point of exec is to take a string like
you'd use for a /bin/bash command.

Maybe it would be better to expose a function for shell-escaping? Then we
can write things such as:

// This gives us an alternative syntaxexec('printf "\"'); // this works, but that's lots of backslashes! Very error-proneexec(shellEscape('printf ""')); // this looks like the bash command: printf "\\"
// or, this examplels('src/').forEach(function (fileName) {
exec('head ' + shellEscape(fileName)); // it's safe to use the output of ls() now
}


Reply to this email directly or view it on GitHub
#143 (comment).

@cscott
Copy link

@cscott cscott commented Feb 8, 2016

Just spitballing potential issues with "just the quotes" escaping -- what happens if you follow a Unicode surrogate character with a quote. Does node/bash treat that as an actual quote, or as a invalid utf character (and replace it with a different utf character)? Is this the same on all platforms and all shells?

This is just the first example I can think of---I guarantee you that if you are passing untrusted values to the shell via string concatenation and relying on naïve escaping you will find plenty of similar edge cases and surprises. This is really security 101.

@nfischer
Copy link
Member

@nfischer nfischer commented Feb 8, 2016

@cscott from the docs for ShellJS's exec():

Executes the given command synchronously, unless otherwise specified.

Since it says the word "command," I argue that this should, by default, behave like executing a command in the shell (we don't call them "commands" in C and other relevant languages). I interpret exec('git status') as translating to git status, although I suppose another valid interpretation is translating to exec git status. Either way, however, these commands respect special characters (I don't see it as a security flaw, per se, since I think glob characters are a nice feature).

I'm not opposed to secure code. exec() is not the only function that can have unintended effects due to glob characters:

# in an empty directory
$ touch a.txt # creates a normal file
$ touch b.txt # again, works as expected
$ touch *.txt # does not create a file, this just updates time stamps for the other two
$ touch '*.txt' # this creates a file named *.txt
$ for k in *.txt; do echo $k; done # no quotes around $k is insecure
a.txt
b.txt
a.txt b.txt *.txt
$ for k in *.txt; do echo "$k"; done # double quotes allow $k to work sensibly
a.txt
b.txt
*.txt

One thing that's unique about exec is if you try exec('git add ' + filename);, where filename is actually the string test.txt;rm -rf ~. That's a valid filename, and if we allow code to create that file, the user then does an ls() & exec(cmd + filename) loop, they would get unintended side effects.

The point being, we can't just have one solution for exec(), because that doesn't really solve the issue for the other commands. Would you be opposed to exposing an execFile() alternative for exec() that handles the case involving characters like semicolons, redirection, etc. in a non-shell way, more similar to C's exec() system call?

@nfischer
Copy link
Member

@nfischer nfischer commented Feb 8, 2016

Actually, a better idea would be to add an option to exec called "noGlob". When true, all characters are treated literally (as if the shell argument were wrapped in single quotes, or like an argument passed to C's execvp system call). As well as an explicit shellEscape function that prevents glob expansion on its arguments

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

@cscott cscott commented Feb 8, 2016

@nfischer
Copy link
Member

@nfischer nfischer commented Feb 8, 2016

@cscott Here's what I'm thinking the interface should look like.

exec(cmd); // this will use a default value for glob (haven't decided if that should be false or true)
exec(cmd, {glob: true}); // '*' expands like a shell glob. Other characters have special meanings
exec(cmd, {glob: false}); // ';', '*', '?', '>' are all taken literally
exec(arg1, arg2, arg3); // executes something like `'arg1' 'arg2' 'arg3'` in the shell. arg2 could have a space, and it will still count as one argument
exec(arg1, arg2, arg3, {glob: true}); // arg2 can have a space, but it will still be one argument. * is a glob character
exec(arg1, arg2, arg3, {glob: false}); // same as above, but * is a literal *. ? is a literal ?. Etc.

The question remains, should this option be named glob, noGlob, shell, etc.? I'm leaning toward glob or shell, depending on what behavior it really provides. Also, we also have to figure out its default value (true or false). Could you provide some example ShellJS scripts that exhibit potentially unintended behavior?

Also, while we're on the subject, it may be worth considering adding a -f option to set() to disable globbing (see https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html). I think we should migrate to supporting globbing by default on all commands, so set('-f') would be a nice way to let people have extra-safe code.

@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: +1 on set('-f'). IMHO, option should be glob, should be true by default. The rest of your idea looks great.

@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
@ariporad
Copy link
Contributor

@ariporad ariporad commented Jun 27, 2016

I've got an idea for an API:

What if we did this (using ES6 string tags):

shell`mycommand --opt ${unsafeVar}`

That gets transpiled to:

shell('mycommand --opt ', unsafeVar);

(Which is a totally acceptable syntax for <ES6.)
Then we make exec (which we could alias to shell itself) escape those arguments.

@nfischer
Copy link
Member

@nfischer nfischer commented Jul 1, 2016

@cscott Although I think we may try to add in the execFile-like interface for shelljs proper, here's an extension I wrote that handles most (not all!) of the security concerns raised in this issue:
https://github.com/nfischer/shelljs-exec-proxy

@nfischer nfischer removed their assignment Sep 10, 2016
@knownasilya
Copy link

@knownasilya knownasilya commented Sep 19, 2016

I would like to see @ariporad's suggestion implemented, making multi-line scripts possible as well (I think..).

@ipsquare
Copy link

@ipsquare ipsquare commented Apr 15, 2018

can someone please update/resolve this issue please

@nfischer
Copy link
Member

@nfischer nfischer commented Jul 11, 2018

Merging into #495, which is starting to see some traction (see #866).

Design doc: https://shelljs.page.link/cmd-design

@cmichelQT
Copy link

@cmichelQT cmichelQT commented Nov 25, 2018

Update on this please, this is now a 4 year old bug.

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 feature high priority security
Projects
None yet
Development

No branches or pull requests