Security vulnerability: Sanitize command arguments

[martin-braun]

By using this package unsanitized input from a command line argument flows into gulp-run, where it is used to build a shell command. Forwarding command-line arguments or file paths from the local environment to a function that executes a system command can change the meaning of the command unexpectedly due to unescaped special characters, which may result in a Indirect Command Injection vulnerability.

run()/run.Command() should accept an array and wrap arguments in double quotes if they contain spaces.

This is easy to fix by ourselves, but Snyk will complain rightfully.

[cbarrick]

I'm not sure I completely understand. Can you provide an example of the vulnerability?

The design of gulp-run is to take as input a script written in sh and execute it, e.g. sh -c "$THE_SCRIPT".

If that script is being generated by another tool, then that tool assumes the responsibility of generating a valid sh script.

In other words, gulp-run does not operate as a simple exec frontend; it operates above a shell. If users have not understood this, and this has caused security concerns, then we should update the documentation to make this more clear.

But again I am not sure I completely understand this issue, so maybe what I've said is beside the point.

[martin-braun]

Hi @cbarrick, I get what you are saying. gulp-run is more or less a wrapper of something like child_process.exec, but I'm looking for the same result, but with inputs like on child_process.spawn. The benefit is there is no possible command injection, when passing user inputs into your function.

Maybe this is beyond the scope of this package, since it can become a very complex topic to convert an array of arguments to a full command line for Windows and *nix systems, given the fact that character escaping plays a role in here as well.

I bypassed the issue by using child_process.spawn directly, as a shell is not mandatory to me. Feel free to close this, if it's beyond the scope of this project, but keep in mind that Snyk will recommend against using your package.

[cbarrick]

It's best to not think of gulp-run as a wrapper around child_process.spawn.

Instead, gulp-run is a wrapper around a shell. The shell is a language while spawn/exec are syscalls. This design was explicitly chosen so that you can use things like the Unix pipe operator, which not not available when calling exec directly.

So the input to gulp-run should be thought of in terms of the underlying shell language (usually sh) and not as an argv list to be passed to exec.

Also, gulp-run does not construct the command to run, and so it has no notion of "sanitizing" it's inputs. In other words, the concept of a command injection vulnerability doesn't make sense in the context of gulp-run because gulp-run is not constructing a command.

Therefore, if another package is constructing commands to send to gulp-run, that package is responsible for implementing command injection prevention.

So in your specific case, I think you either want a layer on top of gulp-run which constructs a command to pass to the shell (with command injection prevention), or you want to use a different tool all together which passes the argv list directly to spawn/exec without invoking a shell.