Unsafe use of exec

[cristianstaicu]

The module does not sanitize the input before passing it to exec. Therefore, the following code snippet may produce unexpected results for some of the users of the library:

var growl = require("growl");
growl("test`ls`");

Use a sanitization npm module like shell-quote or replace exec with spawn!

[keymandll]

Something like the below might work.

  // To mitigate code-injection vulnerability
  var command = args.join(' ');
  command = command.replace(/[\\`$]/g, function(s) {
    return '\\' + s;
  });
  exec(command, fn);

[cristianstaicu]

It is a bit more complicated than that! Your solution does not protect against stuff like this:

growl("my message $(ls)");

A more complete solution here: https://github.com/substack/node-shell-quote/blob/master/index.js

[keymandll]

Did you actually test my code? Could you please provide an example that bypasses it? (other than what you have provided as that one does not) No offense, I'm just curious.

[cristianstaicu]

Oops, saw the dollar in the regex later. :( I did not check the code to be honest, but I know for a fact that just replacing one or two characters is usually not enough, you want to maybe quote the string as well. Take a look at the php description of the same thing:
http://php.net/manual/en/function.escapeshellarg.php

[cristianstaicu]

Even something like this might bypass it:

growl("my message; touch a-file")

[keymandll]

Well, I have just checked your example by updating my code to print out the value of the resulting command variable:

x@pw:/home/x# cat ./test.js
var growl = require('growl')
growl("my message; echo 'test' > /tmp/this_should_not_work")

x@pw:/home/x# node ./test.js
notify-send "my message; echo 'test' > /tmp/this_should_not_work"
x@pw:/home/x# ls -l /tmp/
total 4
srwxrwxrwx 1 mongodb nogroup    0 Sep  5 13:44 mongodb-27017.sock
drwxr-xr-x 3 root    root    4096 Sep  5 13:45 npm-1326-f77d6571

So there are double quotes added for any string. (not by my code). Now you may say that let's break out by adding a double-quote. It will not work. :)

[cristianstaicu]

Oops, sorry I did not see the quote part in growl's source code. So, yes probably it works for most of the cases, but I was just trying to convince you to use a more standard solution, rather than a self-baked one. One reason to do so, is that the standard solutions are extensively tested by the community and new patches are added now and then:
https://nodesecurity.io/advisories/117

[keymandll]

So, I'm not the developer of Growl but I'm happy to play around with things a bit to help improve stuff.
Yes, I agree with you in general. Standard, well tested things are the way to go.

I have checked the shellescape you have linked earlier and it solves the problem by wrapping all the stuff in single quotes:

x@pw:/home/x# node ./test.js
notify-send '"my message $(/usr/bin/id > /tmp/this_should_not_work)"'

The only side-effect is that you will have double quotes in all parts of the notification.
So, so far as I see my solution requires 3 extra lines added in terms of changes and for now it seems to addresses the original issue. Versus using shellescape which means one additional dependency, a little bit slower code (not that it really matters), and additional changes (removing all quote() 's ) needed in the Growl source.

Ok, let's conclude that the optimal for long term would be to eliminate the use of the quote()'s and use shellescape. Deal? :)

[keymandll]

Actually, using spawn() of child_process seems to be a much better solution. And then there's no need for the extra dependency.