Skip to content

Commit

Permalink
src: disallow direct .bat and .cmd file spawning
Browse files Browse the repository at this point in the history
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: nodejs-private/node-private#564
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
CVE-ID: CVE-2024-27980
  • Loading branch information
bnoordhuis authored and RafaelGSS committed Apr 10, 2024
1 parent 380e557 commit 6627222
Show file tree
Hide file tree
Showing 7 changed files with 163 additions and 9 deletions.
21 changes: 15 additions & 6 deletions benchmark/_http-benchmarkers.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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;
}

Expand Down
3 changes: 2 additions & 1 deletion src/node_revert.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
14 changes: 12 additions & 2 deletions src/process_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ class ProcessWrap : public HandleWrap {
Local<Context> context = env->context();
ProcessWrap* wrap;
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
int err = 0;

Local<Object> js_options =
args[0]->ToObject(env->context()).ToLocalChecked();
Expand Down Expand Up @@ -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<Value> argv_v =
js_options->Get(context, env->args_string()).ToLocalChecked();
Expand Down Expand Up @@ -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);
Expand Down
7 changes: 7 additions & 0 deletions src/spawn_sync.cc
Original file line number Diff line number Diff line change
Expand Up @@ -758,6 +758,13 @@ Maybe<int> SyncProcessRunner::ParseOptions(Local<Value> 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<int>(UV_EINVAL);

Local<Value> js_args =
js_options->Get(context, env()->args_string()).ToLocalChecked();
if (!CopyJsStringArray(js_args, &args_buffer_).To(&r)) return Nothing<int>();
Expand Down
15 changes: 15 additions & 0 deletions src/util-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <cmath>
#include <cstring>
#include <locale>
#include "node_revert.h"
#include "util.h"

// These are defined by <sys/byteorder.h> or <netinet/in.h> on some systems.
Expand Down Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions src/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -919,6 +919,10 @@ void SetConstructorFunction(v8::Local<v8::Context> 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
Expand Down
108 changes: 108 additions & 0 deletions test/parallel/test-child-process-spawn-windows-batch-file.js
Original file line number Diff line number Diff line change
@@ -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);
}
}

0 comments on commit 6627222

Please sign in to comment.