Allow arguments to be passed out-of-band from script text

[charles-dyfis-net]

Templating filenames as strings into a shell script -- as gulp-shell recommends -- is extremely dangerous practice from a security perspective. The best-practice approach is for script text to be hardcoded, and for parameters to be passed out-of-band, as arguments or in the environment. Demonstrating two possible argument vectors with the same literal argument:

dangerString="/tmp/evil/$(rm -rf $HOME)'$(rm -rf $HOME)'"

# this argument vector is harmless, and will correctly make a directory with the given name
['sh', '-c', 'directory=$1; mkdir -p "$directory", _, dangerString]

# this argument vector is dangerous, and will destroy the user's home directory
['sh', '-c', "mkdir -p <%= dangerString %>"]

Note the literal single quotes in dangerString, such that even putting single quotes around the expansion cannot prevent one or the other of the command substitutions contained within from being expanded.

Not only should a tool that's advertised as a way to call a shell from JavaScript implement a safe way to do so, but it should document and advertise safe practices as the preferred way to use it.

[charles-dyfis-net]

To be clear, by the way, the given dangerString value here is a completely legitimate UNIX path, including the $ and the single-quotes (while the portable character set which POSIX systems are required to support is smaller, modern Unixes in practice -- including Linux and MacOS -- tend to be permissive, allowing any character other than / and NUL in a filename, and any character but NUL in a path). You could thus genuinely receive this as a value from a directory listing or traversal operation.

[sun-zheng-an]

Templating filenames as strings into a shell script -- as gulp-shell recommends -- is extremely dangerous practice from a security perspective.

Yes, I agree with you. It's dangerous.

But considering most (if not all) of the users of gulp-shell use it for their own projects, it's very unlikely that we have files named like $(rm -rf $HOME) and inject it as an argument to shoot ourself in the foot.

IMHO, injections like '<%= f(x) %>' can provide more flexibility than 'directory=$1; mkdir -p "$directory"' and looks simpler.

Thanks for your advice anyway. I will put a warning on the documentation.

[pvgoran]

It's not just dangerous, it can stop things from working if, say, the filename contains a space. (And I suspect this can happen very easily under Windows!)

So, ideally the interpolation operation (<%= %> or whatever) needs to shell-escape the value to ensure that the shell will treat it like a single word. This way, it will be at the same time convenient, reliable and safe.

[charles-dyfis-net]

Escaping at interpolation time is only safe if you know the quoting context into which those results are going to be used. The escaping that's appropriate for mycommand 'foo-<% ... %>' is different from the escaping for mycommand <% ... %>, which is again different from that for mycommand "foo-<% ... %>", and since the differences only come up with corner cases that require quoting or escaping to take place, it's easy for someone using the tools in an unsafe way to fail to notice until they're hit with unexpected content in production.

(As an example, if you just have a name with a space, foo bar, in an unquoted context that would be expanded as foo\ bar or 'foo bar', but if the substitution is taking place inside preexisting single or double quotes, either of those is incorrect in at least one scenario: Putting \ inside either single or double quotes makes the slash literal (in some common shells; in the double-quoted case, the actual behavior is unspecified by the POSIX sh standard); putting ' inside single quotes ends the quoted context and makes the space unquoted; putting' inside double quotes inserts a literal single quote into the data.)

Passing data out-of-band from code, by contrast, there is no such thing as a quoting context -- the entire range of issues is moot.

[pvgoran]

Escaping at interpolation time is only safe if you know the quoting context into which those results are going to be used.

Yes, that's how any escaping works. :) However, this could at least produce one correct way of embedding dynamic data into the shell invocation, without changing the overall usage pattern. And this one correct way can then be explicitly documented. Definitely an improvement over the current situation, when there is no reliable/safe way to embed the data at all.

Passing data out-of-band from code, by contrast, there is no such thing as a quoting context -- the entire range of issues is moot.

To be honest, one can screw the shell command even in such circumstances. For example, if the file name is passed as an environment variable, you can forget to quote it (for example, in [ $foo = "bar" ]) and get a problem.

[charles-dyfis-net]

"A problem", certainly. Arbitrary code execution, more rarely -- the example given runs an arbitrary test command (and certainly, this means someone could pass something like evil -o bar instead of bar and still have the test for equality with bar pass despite those strings not in fact being equal), but "an arbitrary test" is significant, and useful, distance from "an arbitrary command".

---

A solution that split the difference -- offering both quoted expansion into a string parsed as code and out-of-band argument-passing, with choice at the user's discretion -- would be well in line with practices in other parts of the world (ie. the Python standard library providing both a quote() function and the ability to pass an arbitrary argument vector of literal strings even with shell=True set in the subprocess.Popen family of functions).