Skip to content

Commit 6627222

Browse files
bnoordhuisRafaelGSS
authored andcommittedApr 10, 2024
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#564 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> CVE-ID: CVE-2024-27980
1 parent 380e557 commit 6627222

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
@@ -147,6 +147,7 @@ class ProcessWrap : public HandleWrap {
147147
Local<Context> context = env->context();
148148
ProcessWrap* wrap;
149149
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());
150+
int err = 0;
150151

151152
Local<Object> js_options =
152153
args[0]->ToObject(env->context()).ToLocalChecked();
@@ -185,6 +186,13 @@ class ProcessWrap : public HandleWrap {
185186
node::Utf8Value file(env->isolate(), file_v);
186187
options.file = *file;
187188

189+
// Undocumented feature of Win32 CreateProcess API allows spawning
190+
// batch files directly but is potentially insecure because arguments
191+
// are not escaped (and sometimes cannot be unambiguously escaped),
192+
// hence why they are rejected here.
193+
if (IsWindowsBatchFile(options.file))
194+
err = UV_EINVAL;
195+
188196
// options.args
189197
Local<Value> argv_v =
190198
js_options->Get(context, env->args_string()).ToLocalChecked();
@@ -262,8 +270,10 @@ class ProcessWrap : public HandleWrap {
262270
options.flags |= UV_PROCESS_DETACHED;
263271
}
264272

265-
int err = uv_spawn(env->event_loop(), &wrap->process_, &options);
266-
wrap->MarkAsInitialized();
273+
if (err == 0) {
274+
err = uv_spawn(env->event_loop(), &wrap->process_, &options);
275+
wrap->MarkAsInitialized();
276+
}
267277

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

‎src/spawn_sync.cc

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

761+
// Undocumented feature of Win32 CreateProcess API allows spawning
762+
// batch files directly but is potentially insecure because arguments
763+
// are not escaped (and sometimes cannot be unambiguously escaped),
764+
// hence why they are rejected here.
765+
if (IsWindowsBatchFile(uv_process_options_.file))
766+
return Just<int>(UV_EINVAL);
767+
761768
Local<Value> js_args =
762769
js_options->Get(context, env()->args_string()).ToLocalChecked();
763770
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
@@ -919,6 +919,10 @@ void SetConstructorFunction(v8::Local<v8::Context> context,
919919
SetConstructorFunctionFlag flag =
920920
SetConstructorFunctionFlag::SET_CLASS_NAME);
921921

922+
// Returns true if OS==Windows and filename ends in .bat or .cmd,
923+
// case insensitive.
924+
inline bool IsWindowsBatchFile(const char* filename);
925+
922926
} // namespace node
923927

924928
#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)
Please sign in to comment.