From b374ee8c3dfcefe7b22060c1a2073ee07e4e1b8c Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sun, 28 Aug 2016 12:35:13 -0400 Subject: [PATCH 1/3] src: add handle check to spawn_sync This commit verifies that the child process handle is of the correct type before trying to close it in CloseHandlesAndDeleteLoop(). This catches the case where input validation failed, and the child process was never actually spawned. Fixes: https://github.com/nodejs/node/issues/8096 Fixes: https://github.com/nodejs/node/issues/8539 Refs: https://github.com/nodejs/node/issues/9722 PR-URL: https://github.com/nodejs/node/pull/8312 Reviewed-By: Ben Noordhuis --- src/spawn_sync.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc index 79f10a0ea2594d..689f605e1446cd 100644 --- a/src/spawn_sync.cc +++ b/src/spawn_sync.cc @@ -501,7 +501,12 @@ void SyncProcessRunner::CloseHandlesAndDeleteLoop() { // Close the process handle when ExitCallback was not called. uv_handle_t* uv_process_handle = reinterpret_cast(&uv_process_); - if (!uv_is_closing(uv_process_handle)) + + // Close the process handle if it is still open. The handle type also + // needs to be checked because TryInitializeAndRunLoop() won't spawn a + // process if input validation fails. + if (uv_process_handle->type == UV_PROCESS && + !uv_is_closing(uv_process_handle)) uv_close(uv_process_handle, nullptr); // Give closing watchers a chance to finish closing and get their close From fc7b0dda85c006e5830a0e34645d769e20b894d2 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Sun, 28 Aug 2016 14:03:26 -0400 Subject: [PATCH 2/3] child_process: improve input validation This commit applies stricter input validation in normalizeSpawnArguments(), which is run by all of the child_process methods. Additional checks are added for spawnSync() specific inputs. Fixes: https://github.com/nodejs/node/issues/8096 Fixes: https://github.com/nodejs/node/issues/8539 Refs: https://github.com/nodejs/node/issues/9722 PR-URL: https://github.com/nodejs/node/pull/8312 Reviewed-By: Ben Noordhuis --- lib/child_process.js | 66 +++++- .../test-child-process-spawn-typeerror.js | 13 +- ...ild-process-spawnsync-validation-errors.js | 201 ++++++++++++++++++ 3 files changed, 278 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-child-process-spawnsync-validation-errors.js diff --git a/lib/child_process.js b/lib/child_process.js index 2f5dda870d66d7..03956a999e1141 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -315,6 +315,9 @@ function _convertCustomFds(options) { function normalizeSpawnArguments(file /*, args, options*/) { var args, options; + if (typeof file !== 'string' || file.length === 0) + throw new TypeError('"file" argument must be a non-empty string'); + if (Array.isArray(arguments[1])) { args = arguments[1].slice(0); options = arguments[2]; @@ -331,6 +334,47 @@ function normalizeSpawnArguments(file /*, args, options*/) { else if (options === null || typeof options !== 'object') throw new TypeError('"options" argument must be an object'); + // Validate the cwd, if present. + if (options.cwd != null && + typeof options.cwd !== 'string') { + throw new TypeError('"cwd" must be a string'); + } + + // Validate detached, if present. + if (options.detached != null && + typeof options.detached !== 'boolean') { + throw new TypeError('"detached" must be a boolean'); + } + + // Validate the uid, if present. + if (options.uid != null && !Number.isInteger(options.uid)) { + throw new TypeError('"uid" must be an integer'); + } + + // Validate the gid, if present. + if (options.gid != null && !Number.isInteger(options.gid)) { + throw new TypeError('"gid" must be an integer'); + } + + // Validate the shell, if present. + if (options.shell != null && + typeof options.shell !== 'boolean' && + typeof options.shell !== 'string') { + throw new TypeError('"shell" must be a boolean or string'); + } + + // Validate argv0, if present. + if (options.argv0 != null && + typeof options.argv0 !== 'string') { + throw new TypeError('"argv0" must be a string'); + } + + // Validate windowsVerbatimArguments, if present. + if (options.windowsVerbatimArguments != null && + typeof options.windowsVerbatimArguments !== 'boolean') { + throw new TypeError('"windowsVerbatimArguments" must be a boolean'); + } + // Make a shallow copy so we don't clobber the user's options object. options = Object.assign({}, options); @@ -420,13 +464,33 @@ function spawnSync(/*file, args, options*/) { debug('spawnSync', opts.args, options); + // Validate the timeout, if present. + if (options.timeout != null && + !(Number.isInteger(options.timeout) && options.timeout >= 0)) { + throw new TypeError('"timeout" must be an unsigned integer'); + } + + // Validate maxBuffer, if present. + if (options.maxBuffer != null && + !(Number.isInteger(options.maxBuffer) && options.maxBuffer >= 0)) { + throw new TypeError('"maxBuffer" must be an unsigned integer'); + } + options.file = opts.file; options.args = opts.args; options.envPairs = opts.envPairs; - if (options.killSignal) + // Validate the kill signal, if present. + if (typeof options.killSignal === 'string' || + typeof options.killSignal === 'number') { options.killSignal = lookupSignal(options.killSignal); + if (options.killSignal === 0) + throw new RangeError('"killSignal" cannot be 0'); + } else if (options.killSignal != null) { + throw new TypeError('"killSignal" must be a string or number'); + } + options.stdio = _validateStdio(options.stdio || 'pipe', true).stdio; if (options.input) { diff --git a/test/parallel/test-child-process-spawn-typeerror.js b/test/parallel/test-child-process-spawn-typeerror.js index 7b98cedf8ba117..a3ae3d36f5e80e 100644 --- a/test/parallel/test-child-process-spawn-typeerror.js +++ b/test/parallel/test-child-process-spawn-typeerror.js @@ -9,6 +9,8 @@ const cmd = common.isWindows ? 'rundll32' : 'ls'; const invalidcmd = 'hopefully_you_dont_have_this_on_your_machine'; const invalidArgsMsg = /Incorrect value of args option/; const invalidOptionsMsg = /"options" argument must be an object/; +const invalidFileMsg = + /^TypeError: "file" argument must be a non-empty string$/; const empty = common.fixturesDir + '/empty.js'; assert.throws(function() { @@ -36,7 +38,16 @@ assert.doesNotThrow(function() { // verify that invalid argument combinations throw assert.throws(function() { spawn(); -}, /Bad argument/); +}, invalidFileMsg); + +assert.throws(function() { + spawn(''); +}, invalidFileMsg); + +assert.throws(function() { + const file = { toString() { throw new Error('foo'); } }; + spawn(file); +}, invalidFileMsg); assert.throws(function() { spawn(cmd, null); diff --git a/test/parallel/test-child-process-spawnsync-validation-errors.js b/test/parallel/test-child-process-spawnsync-validation-errors.js new file mode 100644 index 00000000000000..e10137624863a6 --- /dev/null +++ b/test/parallel/test-child-process-spawnsync-validation-errors.js @@ -0,0 +1,201 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const spawnSync = require('child_process').spawnSync; +const noop = function() {}; + +function pass(option, value) { + // Run the command with the specified option. Since it's not a real command, + // spawnSync() should run successfully but return an ENOENT error. + const child = spawnSync('not_a_real_command', { [option]: value }); + + assert.strictEqual(child.error.code, 'ENOENT'); +} + +function fail(option, value, message) { + assert.throws(() => { + spawnSync('not_a_real_command', { [option]: value }); + }, message); +} + +{ + // Validate the cwd option + const err = /^TypeError: "cwd" must be a string$/; + + pass('cwd', undefined); + pass('cwd', null); + pass('cwd', __dirname); + fail('cwd', 0, err); + fail('cwd', 1, err); + fail('cwd', true, err); + fail('cwd', false, err); + fail('cwd', [], err); + fail('cwd', {}, err); + fail('cwd', noop, err); +} + +{ + // Validate the detached option + const err = /^TypeError: "detached" must be a boolean$/; + + pass('detached', undefined); + pass('detached', null); + pass('detached', true); + pass('detached', false); + fail('detached', 0, err); + fail('detached', 1, err); + fail('detached', __dirname, err); + fail('detached', [], err); + fail('detached', {}, err); + fail('detached', noop, err); +} + +if (!common.isWindows) { + { + // Validate the uid option + if (process.getuid() !== 0) { + const err = /^TypeError: "uid" must be an integer$/; + + pass('uid', undefined); + pass('uid', null); + pass('uid', process.getuid()); + fail('uid', __dirname, err); + fail('uid', true, err); + fail('uid', false, err); + fail('uid', [], err); + fail('uid', {}, err); + fail('uid', noop, err); + fail('uid', NaN, err); + fail('uid', Infinity, err); + fail('uid', 3.1, err); + fail('uid', -3.1, err); + } + } + + { + // Validate the gid option + if (process.getgid() !== 0) { + const err = /^TypeError: "gid" must be an integer$/; + + pass('gid', undefined); + pass('gid', null); + pass('gid', process.getgid()); + fail('gid', __dirname, err); + fail('gid', true, err); + fail('gid', false, err); + fail('gid', [], err); + fail('gid', {}, err); + fail('gid', noop, err); + fail('gid', NaN, err); + fail('gid', Infinity, err); + fail('gid', 3.1, err); + fail('gid', -3.1, err); + } + } +} + +{ + // Validate the shell option + const err = /^TypeError: "shell" must be a boolean or string$/; + + pass('shell', undefined); + pass('shell', null); + pass('shell', false); + fail('shell', 0, err); + fail('shell', 1, err); + fail('shell', [], err); + fail('shell', {}, err); + fail('shell', noop, err); +} + +{ + // Validate the argv0 option + const err = /^TypeError: "argv0" must be a string$/; + + pass('argv0', undefined); + pass('argv0', null); + pass('argv0', 'myArgv0'); + fail('argv0', 0, err); + fail('argv0', 1, err); + fail('argv0', true, err); + fail('argv0', false, err); + fail('argv0', [], err); + fail('argv0', {}, err); + fail('argv0', noop, err); +} + +{ + // Validate the windowsVerbatimArguments option + const err = /^TypeError: "windowsVerbatimArguments" must be a boolean$/; + + pass('windowsVerbatimArguments', undefined); + pass('windowsVerbatimArguments', null); + pass('windowsVerbatimArguments', true); + pass('windowsVerbatimArguments', false); + fail('windowsVerbatimArguments', 0, err); + fail('windowsVerbatimArguments', 1, err); + fail('windowsVerbatimArguments', __dirname, err); + fail('windowsVerbatimArguments', [], err); + fail('windowsVerbatimArguments', {}, err); + fail('windowsVerbatimArguments', noop, err); +} + +{ + // Validate the timeout option + const err = /^TypeError: "timeout" must be an unsigned integer$/; + + pass('timeout', undefined); + pass('timeout', null); + pass('timeout', 1); + pass('timeout', 0); + fail('timeout', -1, err); + fail('timeout', true, err); + fail('timeout', false, err); + fail('timeout', __dirname, err); + fail('timeout', [], err); + fail('timeout', {}, err); + fail('timeout', noop, err); + fail('timeout', NaN, err); + fail('timeout', Infinity, err); + fail('timeout', 3.1, err); + fail('timeout', -3.1, err); +} + +{ + // Validate the maxBuffer option + const err = /^TypeError: "maxBuffer" must be an unsigned integer$/; + + pass('maxBuffer', undefined); + pass('maxBuffer', null); + pass('maxBuffer', 1); + pass('maxBuffer', 0); + fail('maxBuffer', 3.14, err); + fail('maxBuffer', -1, err); + fail('maxBuffer', NaN, err); + fail('maxBuffer', Infinity, err); + fail('maxBuffer', true, err); + fail('maxBuffer', false, err); + fail('maxBuffer', __dirname, err); + fail('maxBuffer', [], err); + fail('maxBuffer', {}, err); + fail('maxBuffer', noop, err); +} + +{ + // Validate the killSignal option + const typeErr = /^TypeError: "killSignal" must be a string or number$/; + const rangeErr = /^RangeError: "killSignal" cannot be 0$/; + const unknownSignalErr = /^Error: Unknown signal:/; + + pass('killSignal', undefined); + pass('killSignal', null); + pass('killSignal', 'SIGKILL'); + pass('killSignal', 500); + fail('killSignal', 0, rangeErr); + fail('killSignal', 'SIGNOTAVALIDSIGNALNAME', unknownSignalErr); + fail('killSignal', true, typeErr); + fail('killSignal', false, typeErr); + fail('killSignal', [], typeErr); + fail('killSignal', {}, typeErr); + fail('killSignal', noop, typeErr); +} From 45c9ca7fd430022a9c0567e1b9f535b3e3f956fd Mon Sep 17 00:00:00 2001 From: cjihrig Date: Tue, 18 Oct 2016 15:04:41 -0400 Subject: [PATCH 3/3] src: remove redundant spawn/spawnSync type checks This commit removes C++ checks from spawn() and spawnSync() that are duplicates of the JavaScript type checking. Fixes: https://github.com/nodejs/node/issues/8096 Fixes: https://github.com/nodejs/node/issues/8539 Refs: https://github.com/nodejs/node/issues/9722 PR-URL: https://github.com/nodejs/node/pull/8312 Reviewed-By: Ben Noordhuis --- src/process_wrap.cc | 20 +++++++------------- src/spawn_sync.cc | 19 +++++-------------- 2 files changed, 12 insertions(+), 27 deletions(-) diff --git a/src/process_wrap.cc b/src/process_wrap.cc index 3dcde0962af080..8c8e4704be4f82 100644 --- a/src/process_wrap.cc +++ b/src/process_wrap.cc @@ -121,35 +121,29 @@ class ProcessWrap : public HandleWrap { // options.uid Local uid_v = js_options->Get(env->uid_string()); - if (uid_v->IsInt32()) { + if (!uid_v->IsUndefined() && !uid_v->IsNull()) { + CHECK(uid_v->IsInt32()); const int32_t uid = uid_v->Int32Value(env->context()).FromJust(); options.flags |= UV_PROCESS_SETUID; options.uid = static_cast(uid); - } else if (!uid_v->IsUndefined() && !uid_v->IsNull()) { - return env->ThrowTypeError("options.uid should be a number"); } // options.gid Local gid_v = js_options->Get(env->gid_string()); - if (gid_v->IsInt32()) { + if (!gid_v->IsUndefined() && !gid_v->IsNull()) { + CHECK(gid_v->IsInt32()); const int32_t gid = gid_v->Int32Value(env->context()).FromJust(); options.flags |= UV_PROCESS_SETGID; options.gid = static_cast(gid); - } else if (!gid_v->IsUndefined() && !gid_v->IsNull()) { - return env->ThrowTypeError("options.gid should be a number"); } // TODO(bnoordhuis) is this possible to do without mallocing ? // options.file Local file_v = js_options->Get(env->file_string()); - node::Utf8Value file(env->isolate(), - file_v->IsString() ? file_v : Local()); - if (file.length() > 0) { - options.file = *file; - } else { - return env->ThrowTypeError("Bad argument"); - } + CHECK(file_v->IsString()); + node::Utf8Value file(env->isolate(), file_v); + options.file = *file; // options.args Local argv_v = js_options->Get(env->args_string()); diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc index 689f605e1446cd..68e78147578ff1 100644 --- a/src/spawn_sync.cc +++ b/src/spawn_sync.cc @@ -734,8 +734,7 @@ int SyncProcessRunner::ParseOptions(Local js_value) { } Local js_uid = js_options->Get(env()->uid_string()); if (IsSet(js_uid)) { - if (!js_uid->IsInt32()) - return UV_EINVAL; + CHECK(js_uid->IsInt32()); const int32_t uid = js_uid->Int32Value(env()->context()).FromJust(); uv_process_options_.uid = static_cast(uid); uv_process_options_.flags |= UV_PROCESS_SETUID; @@ -743,8 +742,7 @@ int SyncProcessRunner::ParseOptions(Local js_value) { Local js_gid = js_options->Get(env()->gid_string()); if (IsSet(js_gid)) { - if (!js_gid->IsInt32()) - return UV_EINVAL; + CHECK(js_gid->IsInt32()); const int32_t gid = js_gid->Int32Value(env()->context()).FromJust(); uv_process_options_.gid = static_cast(gid); uv_process_options_.flags |= UV_PROCESS_SETGID; @@ -760,28 +758,21 @@ int SyncProcessRunner::ParseOptions(Local js_value) { Local js_timeout = js_options->Get(env()->timeout_string()); if (IsSet(js_timeout)) { - if (!js_timeout->IsNumber()) - return UV_EINVAL; + CHECK(js_timeout->IsNumber()); int64_t timeout = js_timeout->IntegerValue(); - if (timeout < 0) - return UV_EINVAL; timeout_ = static_cast(timeout); } Local js_max_buffer = js_options->Get(env()->max_buffer_string()); if (IsSet(js_max_buffer)) { - if (!js_max_buffer->IsUint32()) - return UV_EINVAL; + CHECK(js_max_buffer->IsUint32()); max_buffer_ = js_max_buffer->Uint32Value(); } Local js_kill_signal = js_options->Get(env()->kill_signal_string()); if (IsSet(js_kill_signal)) { - if (!js_kill_signal->IsInt32()) - return UV_EINVAL; + CHECK(js_kill_signal->IsInt32()); kill_signal_ = js_kill_signal->Int32Value(); - if (kill_signal_ == 0) - return UV_EINVAL; } Local js_stdio = js_options->Get(env()->stdio_string());