Skip to content

Commit 9095c91

Browse files
bnoordhuisRafaelGSS
authored andcommitted
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: nodejs-private/node-private#562 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> CVE-ID: CVE-2024-27980
1 parent bb78a8f commit 9095c91

7 files changed

+163
-9
lines changed

benchmark/_http-benchmarkers.js

+15-6
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,16 @@ exports.PORT = Number(process.env.PORT) || 12346;
1212

1313
class AutocannonBenchmarker {
1414
constructor() {
15+
const shell = (process.platform === 'win32');
1516
this.name = 'autocannon';
16-
this.executable =
17-
process.platform === 'win32' ? 'autocannon.cmd' : 'autocannon';
18-
const result = child_process.spawnSync(this.executable, ['-h']);
19-
this.present = !(result.error && result.error.code === 'ENOENT');
17+
this.opts = { shell };
18+
this.executable = shell ? 'autocannon.cmd' : 'autocannon';
19+
const result = child_process.spawnSync(this.executable, ['-h'], this.opts);
20+
if (shell) {
21+
this.present = (result.status === 0);
22+
} else {
23+
this.present = !(result.error && result.error.code === 'ENOENT');
24+
}
2025
}
2126

2227
create(options) {
@@ -27,11 +32,15 @@ class AutocannonBenchmarker {
2732
'-n',
2833
];
2934
for (const field in options.headers) {
30-
args.push('-H', `${field}=${options.headers[field]}`);
35+
if (this.opts.shell) {
36+
args.push('-H', `'${field}=${options.headers[field]}'`);
37+
} else {
38+
args.push('-H', `${field}=${options.headers[field]}`);
39+
}
3140
}
3241
const scheme = options.scheme || 'http';
3342
args.push(`${scheme}://127.0.0.1:${options.port}${options.path}`);
34-
const child = child_process.spawn(this.executable, args);
43+
const child = child_process.spawn(this.executable, args, this.opts);
3544
return child;
3645
}
3746

src/node_revert.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,8 @@
1616
namespace node {
1717

1818
#define SECURITY_REVERSIONS(XX) \
19-
XX(CVE_2023_46809, "CVE-2023-46809", "Marvin attack on PKCS#1 padding")
19+
XX(CVE_2023_46809, "CVE-2023-46809", "Marvin attack on PKCS#1 padding") \
20+
XX(CVE_2024_27980, "CVE-2024-27980", "Unsafe Windows batch file execution")
2021

2122
enum reversion {
2223
#define V(code, ...) SECURITY_REVERT_##code,

src/process_wrap.cc

+12-2
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,7 @@ class ProcessWrap : public HandleWrap {
157157
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
158158
THROW_IF_INSUFFICIENT_PERMISSIONS(
159159
env, permission::PermissionScope::kChildProcess, "");
160+
int err = 0;
160161

161162
Local<Object> js_options =
162163
args[0]->ToObject(env->context()).ToLocalChecked();
@@ -195,6 +196,13 @@ class ProcessWrap : public HandleWrap {
195196
node::Utf8Value file(env->isolate(), file_v);
196197
options.file = *file;
197198

199+
// Undocumented feature of Win32 CreateProcess API allows spawning
200+
// batch files directly but is potentially insecure because arguments
201+
// are not escaped (and sometimes cannot be unambiguously escaped),
202+
// hence why they are rejected here.
203+
if (IsWindowsBatchFile(options.file))
204+
err = UV_EINVAL;
205+
198206
// options.args
199207
Local<Value> argv_v =
200208
js_options->Get(context, env->args_string()).ToLocalChecked();
@@ -272,8 +280,10 @@ class ProcessWrap : public HandleWrap {
272280
options.flags |= UV_PROCESS_DETACHED;
273281
}
274282

275-
int err = uv_spawn(env->event_loop(), &wrap->process_, &options);
276-
wrap->MarkAsInitialized();
283+
if (err == 0) {
284+
err = uv_spawn(env->event_loop(), &wrap->process_, &options);
285+
wrap->MarkAsInitialized();
286+
}
277287

278288
if (err == 0) {
279289
CHECK_EQ(wrap->process_.data, wrap);

src/spawn_sync.cc

+7
Original file line numberDiff line numberDiff line change
@@ -765,6 +765,13 @@ Maybe<int> SyncProcessRunner::ParseOptions(Local<Value> js_value) {
765765
if (r < 0) return Just(r);
766766
uv_process_options_.file = file_buffer_;
767767

768+
// Undocumented feature of Win32 CreateProcess API allows spawning
769+
// batch files directly but is potentially insecure because arguments
770+
// are not escaped (and sometimes cannot be unambiguously escaped),
771+
// hence why they are rejected here.
772+
if (IsWindowsBatchFile(uv_process_options_.file))
773+
return Just<int>(UV_EINVAL);
774+
768775
Local<Value> js_args =
769776
js_options->Get(context, env()->args_string()).ToLocalChecked();
770777
if (!CopyJsStringArray(js_args, &args_buffer_).To(&r)) return Nothing<int>();

src/util-inl.h

+15
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
#include <cmath>
2828
#include <cstring>
2929
#include <locale>
30+
#include "node_revert.h"
3031
#include "util.h"
3132

3233
// These are defined by <sys/byteorder.h> or <netinet/in.h> on some systems.
@@ -616,6 +617,20 @@ constexpr std::string_view FastStringKey::as_string_view() const {
616617
return name_;
617618
}
618619

620+
// Inline so the compiler can fully optimize it away on Unix platforms.
621+
bool IsWindowsBatchFile(const char* filename) {
622+
#ifdef _WIN32
623+
static constexpr bool kIsWindows = true;
624+
#else
625+
static constexpr bool kIsWindows = false;
626+
#endif // _WIN32
627+
if (kIsWindows)
628+
if (!IsReverted(SECURITY_REVERT_CVE_2024_27980))
629+
if (const char* p = strrchr(filename, '.'))
630+
return StringEqualNoCase(p, ".bat") || StringEqualNoCase(p, ".cmd");
631+
return false;
632+
}
633+
619634
} // namespace node
620635

621636
#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

src/util.h

+4
Original file line numberDiff line numberDiff line change
@@ -1018,6 +1018,10 @@ std::string DetermineSpecificErrorType(Environment* env,
10181018

10191019
v8::Maybe<int32_t> GetValidatedFd(Environment* env, v8::Local<v8::Value> input);
10201020

1021+
// Returns true if OS==Windows and filename ends in .bat or .cmd,
1022+
// case insensitive.
1023+
inline bool IsWindowsBatchFile(const char* filename);
1024+
10211025
} // namespace node
10221026

10231027
#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
'use strict';
2+
3+
// Node.js on Windows should not be able to spawn batch files directly,
4+
// only when the 'shell' option is set. An undocumented feature of the
5+
// Win32 CreateProcess API allows spawning .bat and .cmd files directly
6+
// but it does not sanitize arguments. We cannot do that automatically
7+
// because it's sometimes impossible to escape arguments unambiguously.
8+
//
9+
// Expectation: spawn() and spawnSync() raise EINVAL if and only if:
10+
//
11+
// 1. 'shell' option is unset
12+
// 2. Platform is Windows
13+
// 3. Filename ends in .bat or .cmd, case-insensitive
14+
//
15+
// exec() and execSync() are unchanged.
16+
17+
const common = require('../common');
18+
const cp = require('child_process');
19+
const assert = require('assert');
20+
const { isWindows } = common;
21+
22+
const arg = '--security-revert=CVE-2024-27980';
23+
const isRevert = process.execArgv.includes(arg);
24+
25+
const expectedCode = isWindows && !isRevert ? 'EINVAL' : 'ENOENT';
26+
const expectedStatus = isWindows ? 1 : 127;
27+
28+
const suffixes =
29+
'BAT bAT BaT baT BAt bAt Bat bat CMD cMD CmD cmD CMd cMd Cmd cmd'
30+
.split(' ');
31+
32+
if (process.argv[2] === undefined) {
33+
const a = cp.spawnSync(process.execPath, [__filename, 'child']);
34+
const b = cp.spawnSync(process.execPath, [arg, __filename, 'child']);
35+
assert.strictEqual(a.status, 0);
36+
assert.strictEqual(b.status, 0);
37+
return;
38+
}
39+
40+
function testExec(filename) {
41+
return new Promise((resolve) => {
42+
cp.exec(filename).once('exit', common.mustCall(function(status) {
43+
assert.strictEqual(status, expectedStatus);
44+
resolve();
45+
}));
46+
});
47+
}
48+
49+
function testExecSync(filename) {
50+
let e;
51+
try {
52+
cp.execSync(filename);
53+
} catch (_e) {
54+
e = _e;
55+
}
56+
if (!e) throw new Error(`Exception expected for ${filename}`);
57+
assert.strictEqual(e.status, expectedStatus);
58+
}
59+
60+
function testSpawn(filename, code) {
61+
// Batch file case is a synchronous error, file-not-found is asynchronous.
62+
if (code === 'EINVAL') {
63+
let e;
64+
try {
65+
cp.spawn(filename);
66+
} catch (_e) {
67+
e = _e;
68+
}
69+
if (!e) throw new Error(`Exception expected for ${filename}`);
70+
assert.strictEqual(e.code, code);
71+
} else {
72+
return new Promise((resolve) => {
73+
cp.spawn(filename).once('error', common.mustCall(function(e) {
74+
assert.strictEqual(e.code, code);
75+
resolve();
76+
}));
77+
});
78+
}
79+
}
80+
81+
function testSpawnSync(filename, code) {
82+
{
83+
const r = cp.spawnSync(filename);
84+
assert.strictEqual(r.error.code, code);
85+
}
86+
{
87+
const r = cp.spawnSync(filename, { shell: true });
88+
assert.strictEqual(r.status, expectedStatus);
89+
}
90+
}
91+
92+
testExecSync('./nosuchdir/nosuchfile');
93+
testSpawnSync('./nosuchdir/nosuchfile', 'ENOENT');
94+
for (const suffix of suffixes) {
95+
testExecSync(`./nosuchdir/nosuchfile.${suffix}`);
96+
testSpawnSync(`./nosuchdir/nosuchfile.${suffix}`, expectedCode);
97+
}
98+
99+
go().catch((ex) => { throw ex; });
100+
101+
async function go() {
102+
await testExec('./nosuchdir/nosuchfile');
103+
await testSpawn('./nosuchdir/nosuchfile', 'ENOENT');
104+
for (const suffix of suffixes) {
105+
await testExec(`./nosuchdir/nosuchfile.${suffix}`);
106+
await testSpawn(`./nosuchdir/nosuchfile.${suffix}`, expectedCode);
107+
}
108+
}

0 commit comments

Comments
 (0)