Fixing child_process module to check values passed strictly to the options object

lib/child_process.js

@@ -135,8 +135,10 @@ function normalizeExecArgs(command, options, callback) {
     options = undefined;
   }
 
-  // Make a shallow copy so we don't clobber the user's options object.
-  options = Object.assign({}, options);
+  // Make a shallow copy so we don't clobber the user's options object
+  // and we strictly make use of Object.create(null) so that only self
+  // properties are checked.
+  options = Object.assign(Object.create(null), options);
   options.shell = typeof options.shell === 'string' ? options.shell : true;
 
   return {
@@ -416,6 +418,11 @@ function normalizeSpawnArguments(file, args, options) {
   else if (options === null || typeof options !== 'object')
     throw new ERR_INVALID_ARG_TYPE('options', 'object', options);
 
+  // Make a shallow copy so we don't clobber the user's options object
+  // and we strictly make use of Object.create(null) so that only self
+  // properties are checked.
+  options = Object.assign(Object.create(null), options);
+
   // Validate the cwd, if present.
   if (options.cwd != null &&
       typeof options.cwd !== 'string') {
@@ -468,9 +475,6 @@ function normalizeSpawnArguments(file, args, options) {
                                    options.windowsVerbatimArguments);
   }
 
-  // Make a shallow copy so we don't clobber the user's options object.
-  options = Object.assign({}, options);
-
   if (options.shell) {
     const command = [file].concat(args).join(' ');
     // Set the shell, switches, and commands.

test/parallel/test-child-process-execfile.js

@@ -2,7 +2,7 @@
 
 const common = require('../common');
 const assert = require('assert');
-const execFile = require('child_process').execFile;
+const { execFile, execFileSync } = require('child_process');
 const { getSystemErrorName } = require('util');
 const fixtures = require('../common/fixtures');
 
@@ -47,3 +47,39 @@ const execOpts = { encoding: 'utf8', shell: true };
     assert.ifError(err);
   }));
 }
+
+{
+  // Verify the shell option is only passed explicitly
+  // in the options object and not inherited from the
+  // object prototype
+  Object.prototype.shell = true;
+
+  execFile(
+    'date',
+    ['; echo baz'],
+    common.mustCall((e, stdout) => {
+      assert.notStrictEqual(e, null);
+      assert.strictEqual(stdout.trim().indexOf('baz'), -1);
+    })
+  );
+
+  Reflect.deleteProperty(Object.prototype, 'shell');
+}
+
+{
+  // Verify the shell option is only passed explicitly
+  // in the options object and not inherited from the
+  // object prototype
+  Object.prototype.shell = true;
+
+  assert.throws(function() {
+    execFileSync(
+      'date',
+      ['; echo baz']
+    );
+  }, {
+    status: 1
+  }, 'No safe-guard detected against value present on the prototype chain');
+
+  Reflect.deleteProperty(Object.prototype, 'shell');
+}

test/parallel/test-child-process-spawn-shell.js

@@ -2,63 +2,92 @@
 const common = require('../common');
 const assert = require('assert');
 const cp = require('child_process');
+{
+  // Verify that a shell is, in fact, executed
+  const doesNotExist = cp.spawn('does-not-exist', { shell: true });
 
-// Verify that a shell is, in fact, executed
-const doesNotExist = cp.spawn('does-not-exist', { shell: true });
+  assert.notStrictEqual(doesNotExist.spawnfile, 'does-not-exist');
+  doesNotExist.on('error', common.mustNotCall());
+  doesNotExist.on('exit', common.mustCall((code, signal) => {
+    assert.strictEqual(signal, null);
 
-assert.notStrictEqual(doesNotExist.spawnfile, 'does-not-exist');
-doesNotExist.on('error', common.mustNotCall());
-doesNotExist.on('exit', common.mustCall((code, signal) => {
-  assert.strictEqual(signal, null);
+    if (common.isWindows)
+      assert.strictEqual(code, 1);    // Exit code of cmd.exe
+    else
+      assert.strictEqual(code, 127);  // Exit code of /bin/sh
+  }));
+}
 
-  if (common.isWindows)
-    assert.strictEqual(code, 1);    // Exit code of cmd.exe
-  else
-    assert.strictEqual(code, 127);  // Exit code of /bin/sh
-}));
+{
+  // Verify that passing arguments works
+  const echo = cp.spawn('echo', ['foo'], {
+    encoding: 'utf8',
+    shell: true
+  });
+  let echoOutput = '';
 
-// Verify that passing arguments works
-const echo = cp.spawn('echo', ['foo'], {
-  encoding: 'utf8',
-  shell: true
-});
-let echoOutput = '';
+  assert.strictEqual(echo.spawnargs[echo.spawnargs.length - 1].replace(/"/g, ''),
+                     'echo foo');
+  echo.stdout.on('data', (data) => {
+    echoOutput += data;
+  });
+  echo.on('close', common.mustCall((code, signal) => {
+    assert.strictEqual(echoOutput.trim(), 'foo');
+  }));
+}
 
-assert.strictEqual(echo.spawnargs[echo.spawnargs.length - 1].replace(/"/g, ''),
-                   'echo foo');
-echo.stdout.on('data', (data) => {
-  echoOutput += data;
-});
-echo.on('close', common.mustCall((code, signal) => {
-  assert.strictEqual(echoOutput.trim(), 'foo');
-}));
+{
+  // Verify that shell features can be used
+  const cmd = 'echo bar | cat';
+  const command = cp.spawn(cmd, {
+    encoding: 'utf8',
+    shell: true
+  });
+  let commandOutput = '';
 
-// Verify that shell features can be used
-const cmd = 'echo bar | cat';
-const command = cp.spawn(cmd, {
-  encoding: 'utf8',
-  shell: true
-});
-let commandOutput = '';
+  command.stdout.on('data', (data) => {
+    commandOutput += data;
+  });
+  command.on('close', common.mustCall((code, signal) => {
+    assert.strictEqual(commandOutput.trim(), 'bar');
+  }));
+}
 
-command.stdout.on('data', (data) => {
-  commandOutput += data;
-});
-command.on('close', common.mustCall((code, signal) => {
-  assert.strictEqual(commandOutput.trim(), 'bar');
-}));
+{
+  // Verify that shell features can not be passed through
+  // object prototype manipulation, and must be strictly
+  // passed through the options object
+  const spawnCmd = 'date';
+  const spawnCmdArgs = ['; echo baz'];
+  Object.prototype.shell = true;
 
-// Verify that the environment is properly inherited
-const env = cp.spawn(`"${process.execPath}" -pe process.env.BAZ`, {
-  env: Object.assign({}, process.env, { BAZ: 'buzz' }),
-  encoding: 'utf8',
-  shell: true
-});
-let envOutput = '';
+  const spawnHandler = cp.spawn(spawnCmd, spawnCmdArgs, {
+    encoding: 'utf8'
+  });
 
-env.stdout.on('data', (data) => {
-  envOutput += data;
-});
-env.on('close', common.mustCall((code, signal) => {
-  assert.strictEqual(envOutput.trim(), 'buzz');
-}));
+  // assertion is expected to fail due to `spawnCmdArgs` not
+  // being a valid argument to the date command
+  const SUCCESS_CODE = 0;
+  spawnHandler.on('close', common.mustCall((code) => {
+    assert.notStrictEqual(code, SUCCESS_CODE);
+  }));
+
+  Reflect.deleteProperty(Object.prototype, 'shell');
+}
+
+{
+  // Verify that the environment is properly inherited
+  const env = cp.spawn(`"${process.execPath}" -pe process.env.BAZ`, {
+    env: Object.assign({}, process.env, { BAZ: 'buzz' }),
+    encoding: 'utf8',
+    shell: true
+  });
+  let envOutput = '';
+
+  env.stdout.on('data', (data) => {
+    envOutput += data;
+  });
+  env.on('close', common.mustCall((code, signal) => {
+    assert.strictEqual(envOutput.trim(), 'buzz');
+  }));
+}