-
Notifications
You must be signed in to change notification settings - Fork 739
Security guidelines
TL;DR You can avoid the problem by switching to
shell.cmd()in ShellJS v0.9.2 and above.
Important: this advice applies to child_process.exec() just as much as it applies to shell.exec(). You should use the same scrutiny for all calls to child_process.exec() in your module or its transitive dependencies.
shell.exec() is, due to the design of its API surface, possible to use unsafely. Using this unsafely creates a command injection vulnerability in your module, where an attacker can, through improperly sanitized input, execute arbitrary code. This is illustrated in the following example:
shell.exec('curl ' + urlToDownload);Simple cases, such as urlToDownload === 'https://github.com/shelljs/shelljs', seem to work correctly. However, malicious input can cause security issues:
var urlToDownload = 'https://some/url ; rm -rf $HOME'; // Suppose the attacker can control the value stored in this variable.
shell.exec('curl ' + urlToDownload);
// This executes: `curl https://some/url ; rm -rf $HOME`
// Downloads some URL, but then deletes the user's home directory!While shell.exec() and child_process.execSync() can be very powerful, dependent modules must take care to carefully sanitize all uncontrolled input. Input can come from:
- User input into CLI or GUI interfaces
- File or directory names read from the filesystem
- Environmental or other shell variable values
- Output from other modules or external system commands
If you depend on ShellJS, consider switching to shell.cmd() (for synchronous use case) or child_process.execFile() (for async use case). Both of these functions are designed to solve the problem of command injection. These functions work because they don't use a shell to execute the code. Instead, they directly spawn a subprocess for the desired command, which means there's no special treatment of special characters. This enforces that your shell.cmd() call will execute one and only one command, regardless of the specified input.
TL;DR You may choose to temporarily disable wildcard expansion with
shell.set('-f').
Most ShellJS commands support glob expansion, expanding wildcards such as * to match files. While this is very powerful, dependent modules should exercise caution. Unsanitized user input may contain wildcard characters. Consider the following example:
// Delete all files except IMPORTANT.txt
shell.ls('*.txt').forEach(function (filename) {
if (filename !== 'IMPORTANT.txt') {
// if the file isn't IMPORTANT.txt, it's safe to delete
shell.rm(filename);
}
});However, this falls apart if there's a file in the current directory named *.txt. Not only is this a valid unix filename, but shell.ls('*.txt') will match all .txt files, including the literally-named *.txt. We'll execute shell.rm('*.txt') on one of the loop iterations, which will then delete all .txt files, including IMPORTANT.txt.
Dependent modules can temporarily disable glob expansion using set('-f') (see docs) like so:
var files = shell.ls('*.txt');
shell.set('-f'); // Disable glob expansion for rm().
files.forEach(function (filename) {
if (filename !== 'IMPORTANT.txt') {
// if the file isn't IMPORTANT.txt, it's safe to delete
shell.rm(filename);
}
});
shell.set('+f'); // Safe to re-enable glob expansion.