JS: js/shell-command-injection-from-environment

[ghost]

Adds a query for command line injections caused by the inclusion of unsanitized values from the execution environment, specifically file and directory names. Depending on the threat model, dynamically obtained path values should be regarded as being potentially malicious, in practice a TP from this query probably just means that a command line execution will fail when the user has spaces in his paths.

Misc. build/test scripts are particularly prone to TPs due to their missing handling of spaces in absolute paths, other results are practically safe as they quote the file names.

Additional sources of absolute paths and environment values that should be sanitized are welcome.

Results are plenty, but none seem worth a vulnerability report, the best result is exec('rm -rf ' + __dirname + '/screenshots', ..., which may remove too many files if there are spaces in the path of the currently executing file (inspires the qhelp).

Standalone performance is unsurprisingly comparable to js/command-line-injection. I have started a slightly more precise performance evaluation Performance when evaluated with the two other command-injection queries seems to be in line with a new taint query.

[erik-krogh]

If you include process.execPath as a source, shouldn't you then also include reads from process.argv, process.execArgv (and possibly process_env?)

[ghost]

process.argv is covered by js/indirect-command-line-injection/https://lgtm.com/rules/1509670596145/, with a much higher FP rate in practice, I am sure it is the right choice to keep these two queries separate.

process.execArgv maybe belongs as a source in js/indirect-command-line-injection, but I think the FP rate will be abysmal.

The properties of process.env.X is an interesting (and obvious, doh) source, I have made a note about it here as I suspect it will lead to noisy false positives that require special casing on specific environment variable names.

[erik-krogh]

LGTM. But someone else should also take a look.

(The build fails due to mixed tabs and spaces).

[xiemaisi]

Mostly LGTM, a number of minor suggestions and comments.

[mchammer01]

@esben-semmle - first editorial review completed. One minor suggestion (and I agree with @xiemaisi's comments too). LGTM apart from that.
Have we lost the HTML previews?

[ghost]

All comments addressed.
I have rebased away the conflict, and squashed the review-fixups at the same time.

[ghost]

Have we lost the HTML previews?

No, it is there now. Maybe it was not produced because of the merge conflict (I did not check).

[mchammer01]

Hi @esben-semmle - thanks for the quick fixes. I checked the fixes on the preview and noticed a typo and a punctuation minor issue (sorry for not reporting them before) - see inline suggestions for more detail.

[ghost]

Thanks. I have applied the suggestions.

[xiemaisi]

This looks good to go; could you squash the fixups?

[ghost]

Done

[xiemaisi]

@mchammer01, could you re-approve this PR?

[mchammer01]

done 😉