diff --git a/benchmark/_http-benchmarkers.js b/benchmark/_http-benchmarkers.js index ae5429fa721750..4eba83eccb58f0 100644 --- a/benchmark/_http-benchmarkers.js +++ b/benchmark/_http-benchmarkers.js @@ -12,11 +12,16 @@ exports.PORT = Number(process.env.PORT) || 12346; class AutocannonBenchmarker { constructor() { + const shell = (process.platform === 'win32'); this.name = 'autocannon'; - this.executable = - process.platform === 'win32' ? 'autocannon.cmd' : 'autocannon'; - const result = child_process.spawnSync(this.executable, ['-h']); - this.present = !(result.error && result.error.code === 'ENOENT'); + this.opts = { shell }; + this.executable = shell ? 'autocannon.cmd' : 'autocannon'; + const result = child_process.spawnSync(this.executable, ['-h'], this.opts); + if (shell) { + this.present = (result.status === 0); + } else { + this.present = !(result.error && result.error.code === 'ENOENT'); + } } create(options) { @@ -27,11 +32,15 @@ class AutocannonBenchmarker { '-n', ]; for (const field in options.headers) { - args.push('-H', `${field}=${options.headers[field]}`); + if (this.opts.shell) { + args.push('-H', `'${field}=${options.headers[field]}'`); + } else { + args.push('-H', `${field}=${options.headers[field]}`); + } } const scheme = options.scheme || 'http'; args.push(`${scheme}://127.0.0.1:${options.port}${options.path}`); - const child = child_process.spawn(this.executable, args); + const child = child_process.spawn(this.executable, args, this.opts); return child; } diff --git a/src/node_revert.h b/src/node_revert.h index 071673e1290638..e975982bcb5af6 100644 --- a/src/node_revert.h +++ b/src/node_revert.h @@ -16,7 +16,8 @@ namespace node { #define SECURITY_REVERSIONS(XX) \ - XX(CVE_2023_46809, "CVE-2023-46809", "Marvin attack on PKCS#1 padding") + XX(CVE_2023_46809, "CVE-2023-46809", "Marvin attack on PKCS#1 padding") \ + XX(CVE_2024_27980, "CVE-2024-27980", "Unsafe Windows batch file execution") enum reversion { #define V(code, ...) SECURITY_REVERT_##code, diff --git a/src/process_wrap.cc b/src/process_wrap.cc index ffa5dbd1306a6d..4446f7a01c583c 100644 --- a/src/process_wrap.cc +++ b/src/process_wrap.cc @@ -147,6 +147,7 @@ class ProcessWrap : public HandleWrap { Local context = env->context(); ProcessWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); + int err = 0; Local js_options = args[0]->ToObject(env->context()).ToLocalChecked(); @@ -185,6 +186,13 @@ class ProcessWrap : public HandleWrap { node::Utf8Value file(env->isolate(), file_v); options.file = *file; + // Undocumented feature of Win32 CreateProcess API allows spawning + // batch files directly but is potentially insecure because arguments + // are not escaped (and sometimes cannot be unambiguously escaped), + // hence why they are rejected here. + if (IsWindowsBatchFile(options.file)) + err = UV_EINVAL; + // options.args Local argv_v = js_options->Get(context, env->args_string()).ToLocalChecked(); @@ -262,8 +270,10 @@ class ProcessWrap : public HandleWrap { options.flags |= UV_PROCESS_DETACHED; } - int err = uv_spawn(env->event_loop(), &wrap->process_, &options); - wrap->MarkAsInitialized(); + if (err == 0) { + err = uv_spawn(env->event_loop(), &wrap->process_, &options); + wrap->MarkAsInitialized(); + } if (err == 0) { CHECK_EQ(wrap->process_.data, wrap); diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc index ae4a85a42d6166..6529d91b6f4bd1 100644 --- a/src/spawn_sync.cc +++ b/src/spawn_sync.cc @@ -758,6 +758,13 @@ Maybe SyncProcessRunner::ParseOptions(Local js_value) { if (r < 0) return Just(r); uv_process_options_.file = file_buffer_; + // Undocumented feature of Win32 CreateProcess API allows spawning + // batch files directly but is potentially insecure because arguments + // are not escaped (and sometimes cannot be unambiguously escaped), + // hence why they are rejected here. + if (IsWindowsBatchFile(uv_process_options_.file)) + return Just(UV_EINVAL); + Local js_args = js_options->Get(context, env()->args_string()).ToLocalChecked(); if (!CopyJsStringArray(js_args, &args_buffer_).To(&r)) return Nothing(); diff --git a/src/util-inl.h b/src/util-inl.h index 864f6d86cdf689..78eef6de08f6d9 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -27,6 +27,7 @@ #include #include #include +#include "node_revert.h" #include "util.h" // These are defined by or on some systems. @@ -616,6 +617,20 @@ constexpr std::string_view FastStringKey::as_string_view() const { return name_; } +// Inline so the compiler can fully optimize it away on Unix platforms. +bool IsWindowsBatchFile(const char* filename) { +#ifdef _WIN32 + static constexpr bool kIsWindows = true; +#else + static constexpr bool kIsWindows = false; +#endif // _WIN32 + if (kIsWindows) + if (!IsReverted(SECURITY_REVERT_CVE_2024_27980)) + if (const char* p = strrchr(filename, '.')) + return StringEqualNoCase(p, ".bat") || StringEqualNoCase(p, ".cmd"); + return false; +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/util.h b/src/util.h index d8fcfe1978052f..6c59dc3f45ecd7 100644 --- a/src/util.h +++ b/src/util.h @@ -919,6 +919,10 @@ void SetConstructorFunction(v8::Local context, SetConstructorFunctionFlag flag = SetConstructorFunctionFlag::SET_CLASS_NAME); +// Returns true if OS==Windows and filename ends in .bat or .cmd, +// case insensitive. +inline bool IsWindowsBatchFile(const char* filename); + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/test/parallel/test-child-process-spawn-windows-batch-file.js b/test/parallel/test-child-process-spawn-windows-batch-file.js new file mode 100644 index 00000000000000..81d0c64500d7ed --- /dev/null +++ b/test/parallel/test-child-process-spawn-windows-batch-file.js @@ -0,0 +1,108 @@ +'use strict'; + +// Node.js on Windows should not be able to spawn batch files directly, +// only when the 'shell' option is set. An undocumented feature of the +// Win32 CreateProcess API allows spawning .bat and .cmd files directly +// but it does not sanitize arguments. We cannot do that automatically +// because it's sometimes impossible to escape arguments unambiguously. +// +// Expectation: spawn() and spawnSync() raise EINVAL if and only if: +// +// 1. 'shell' option is unset +// 2. Platform is Windows +// 3. Filename ends in .bat or .cmd, case-insensitive +// +// exec() and execSync() are unchanged. + +const common = require('../common'); +const cp = require('child_process'); +const assert = require('assert'); +const { isWindows } = common; + +const arg = '--security-revert=CVE-2024-27980'; +const isRevert = process.execArgv.includes(arg); + +const expectedCode = isWindows && !isRevert ? 'EINVAL' : 'ENOENT'; +const expectedStatus = isWindows ? 1 : 127; + +const suffixes = + 'BAT bAT BaT baT BAt bAt Bat bat CMD cMD CmD cmD CMd cMd Cmd cmd' + .split(' '); + +if (process.argv[2] === undefined) { + const a = cp.spawnSync(process.execPath, [__filename, 'child']); + const b = cp.spawnSync(process.execPath, [arg, __filename, 'child']); + assert.strictEqual(a.status, 0); + assert.strictEqual(b.status, 0); + return; +} + +function testExec(filename) { + return new Promise((resolve) => { + cp.exec(filename).once('exit', common.mustCall(function(status) { + assert.strictEqual(status, expectedStatus); + resolve(); + })); + }); +} + +function testExecSync(filename) { + let e; + try { + cp.execSync(filename); + } catch (_e) { + e = _e; + } + if (!e) throw new Error(`Exception expected for ${filename}`); + assert.strictEqual(e.status, expectedStatus); +} + +function testSpawn(filename, code) { + // Batch file case is a synchronous error, file-not-found is asynchronous. + if (code === 'EINVAL') { + let e; + try { + cp.spawn(filename); + } catch (_e) { + e = _e; + } + if (!e) throw new Error(`Exception expected for ${filename}`); + assert.strictEqual(e.code, code); + } else { + return new Promise((resolve) => { + cp.spawn(filename).once('error', common.mustCall(function(e) { + assert.strictEqual(e.code, code); + resolve(); + })); + }); + } +} + +function testSpawnSync(filename, code) { + { + const r = cp.spawnSync(filename); + assert.strictEqual(r.error.code, code); + } + { + const r = cp.spawnSync(filename, { shell: true }); + assert.strictEqual(r.status, expectedStatus); + } +} + +testExecSync('./nosuchdir/nosuchfile'); +testSpawnSync('./nosuchdir/nosuchfile', 'ENOENT'); +for (const suffix of suffixes) { + testExecSync(`./nosuchdir/nosuchfile.${suffix}`); + testSpawnSync(`./nosuchdir/nosuchfile.${suffix}`, expectedCode); +} + +go().catch((ex) => { throw ex; }); + +async function go() { + await testExec('./nosuchdir/nosuchfile'); + await testSpawn('./nosuchdir/nosuchfile', 'ENOENT'); + for (const suffix of suffixes) { + await testExec(`./nosuchdir/nosuchfile.${suffix}`); + await testSpawn(`./nosuchdir/nosuchfile.${suffix}`, expectedCode); + } +}