From 64b67779f72ea9e4a0f444284576df9e591d79a0 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 15 Mar 2024 05:59:51 +0100 Subject: [PATCH] src: disallow direct .bat and .cmd file spawning An undocumented feature of the 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 refused starting today. PR-URL: https://github.com/nodejs-private/node-private/pull/560 Reviewed-By: Matteo Collina Reviewed-By: Michael Dawson Reviewed-By: Rafael Gonzaga CVE-ID: CVE-2024-27980 --- benchmark/_http-benchmarkers.js | 21 +++- src/node_revert.h | 1 + src/process_wrap.cc | 14 ++- src/spawn_sync.cc | 7 ++ src/util-inl.h | 15 +++ src/util.h | 4 + ...-child-process-spawn-windows-batch-file.js | 108 ++++++++++++++++++ 7 files changed, 162 insertions(+), 8 deletions(-) create mode 100644 test/parallel/test-child-process-spawn-windows-batch-file.js diff --git a/benchmark/_http-benchmarkers.js b/benchmark/_http-benchmarkers.js index ae5429fa72..4eba83eccb 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 38b6252a32..908a697508 100644 --- a/src/node_revert.h +++ b/src/node_revert.h @@ -16,6 +16,7 @@ namespace node { #define SECURITY_REVERSIONS(XX) \ + XX(CVE_2024_27980, "CVE-2024-27980", "Unsafe Windows batch file execution") // XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title") enum reversion { diff --git a/src/process_wrap.cc b/src/process_wrap.cc index a9b0cea29c..649747fdbf 100644 --- a/src/process_wrap.cc +++ b/src/process_wrap.cc @@ -157,6 +157,7 @@ class ProcessWrap : public HandleWrap { ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); THROW_IF_INSUFFICIENT_PERMISSIONS( env, permission::PermissionScope::kChildProcess, ""); + int err = 0; Local js_options = args[0]->ToObject(env->context()).ToLocalChecked(); @@ -195,6 +196,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(); @@ -272,8 +280,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 0d5235274f..d03803fe3a 100644 --- a/src/spawn_sync.cc +++ b/src/spawn_sync.cc @@ -765,6 +765,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 80444d73a9..dc13d1063d 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. @@ -639,6 +640,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 778c4614cd..7ac2b3efea 100644 --- a/src/util.h +++ b/src/util.h @@ -1032,6 +1032,10 @@ v8::Maybe GetValidFileMode(Environment* env, v8::Local input, uv_fs_type type); +// 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 0000000000..81d0c64500 --- /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); + } +}