Skip to content

Commit

Permalink
Use execFileSync to launch child process (#790)
Browse files Browse the repository at this point in the history
This uses `child_process.execFileSync` instead of `execSync` to launch the child
process. This further reduces the attack surface, removing a possible point for
command injection in the ShellJS implementation.

This does not affect backwards compatibility for the `shell.exec` API (the
behavior is determined by the call to `child_process.exec` within
`src/exec-child.js`).

Issue #782
  • Loading branch information
nfischer committed Oct 27, 2017
1 parent e9461dc commit b885590
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 9 deletions.
3 changes: 0 additions & 3 deletions src/exec-child.js
Expand Up @@ -5,9 +5,6 @@ if (require.main !== module) {
var childProcess = require('child_process');
var fs = require('fs');

// Note: this will break if `paramFilePath` contains special characters ( '\n',
// '\t', etc.). Such characters are possible if $TMP gets modified. We already
// rely on tempdir() to work for other things, so this is an acceptable risk.
var paramFilePath = process.argv[2];

var serializedParams = fs.readFileSync(paramFilePath, 'utf8');
Expand Down
15 changes: 9 additions & 6 deletions src/exec.js
Expand Up @@ -55,11 +55,10 @@ function execSync(cmd, opts, pipe) {

fs.writeFileSync(paramsFile, JSON.stringify(paramsToSerialize), 'utf8');

var execCommand = [
JSON.stringify(common.config.execPath),
JSON.stringify(path.join(__dirname, 'exec-child.js')),
JSON.stringify(paramsFile),
].join(' ');
var execArgs = [
path.join(__dirname, 'exec-child.js'),
paramsFile,
];

/* istanbul ignore else */
if (opts.silent) {
Expand All @@ -70,7 +69,11 @@ function execSync(cmd, opts, pipe) {

// Welcome to the future
try {
child.execSync(execCommand, opts);
// Bad things if we pass in a `shell` option to child_process.execFileSync,
// so we need to explicitly remove it here.
delete opts.shell;

child.execFileSync(common.config.execPath, execArgs, opts);
} catch (e) {
// Clean up immediately if we have an exception
try { common.unlinkSync(codeFile); } catch (e2) {}
Expand Down

0 comments on commit b885590

Please sign in to comment.