Skip to content

Security guidelines

Nate Fischer edited this page Jul 8, 2019 · 5 revisions

exec() command injection

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:

function curl(urlToDownload) {
  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:

curl('https://some/url ; rm -rf $HOME'); // Downloads some URL, but then deletes the user's home directory!

While shell.exec() and similar APIs 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

Dependent modules should look into sanitizing input. The ShellJS project recommends a few modules:

Glob injection (all commands)

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.