From 662722240928668e4b16822be2f660fc6d957340 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/564 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Rafael Gonzaga CVE-ID: CVE-2024-27980 --- benchmark/_http-benchmarkers.js | 21 +++- src/node_revert.h | 3 +- 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, 163 insertions(+), 9 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 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); + } +}