FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: disallow direct .bat and .cmd file spawning · nodejs/node@64b6777 · GitHub

/ node Public

Commit 64b6777

Browse files
bnoordhuis
authored and
RafaelGSS
committed
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#560 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> CVE-ID: CVE-2024-27980
1 parent 423ad47 commit 64b6777

7 files changed

Lines changed: 162 additions & 8 deletions

File tree

‎benchmark/_http-benchmarkers.js‎

Lines changed: 15 additions & 6 deletions
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‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
namespace node {
1717

1818
#define SECURITY_REVERSIONS(XX) \
19+
XX(CVE_2024_27980, "CVE-2024-27980", "Unsafe Windows batch file execution")
1920
// XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title")
2021

2122
enum reversion {

‎src/process_wrap.cc‎

Lines changed: 12 additions & 2 deletions
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‎

Lines changed: 7 additions & 0 deletions
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‎

Lines changed: 15 additions & 0 deletions
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.
@@ -639,6 +640,20 @@ constexpr std::string_view FastStringKey::as_string_view() const {
639640
return name_;
640641
}
641642

643+
// Inline so the compiler can fully optimize it away on Unix platforms.
644+
bool IsWindowsBatchFile(const char* filename) {
645+
#ifdef _WIN32
646+
static constexpr bool kIsWindows = true;
647+
#else
648+
static constexpr bool kIsWindows = false;
649+
#endif // _WIN32
650+
if (kIsWindows)
651+
if (!IsReverted(SECURITY_REVERT_CVE_2024_27980))
652+
if (const char* p = strrchr(filename, '.'))
653+
return StringEqualNoCase(p, ".bat") || StringEqualNoCase(p, ".cmd");
654+
return false;
655+
}
656+
642657
} // namespace node
643658

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

‎src/util.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1032,6 +1032,10 @@ v8::Maybe<int> GetValidFileMode(Environment* env,
10321032
v8::Local<v8::Value> input,
10331033
uv_fs_type type);
10341034

1035+
// Returns true if OS==Windows and filename ends in .bat or .cmd,
1036+
// case insensitive.
1037+
inline bool IsWindowsBatchFile(const char* filename);
1038+
10351039
} // namespace node
10361040

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

‎test/parallel/test-child-process-spawn-windows-batch-file.js‎

Lines changed: 108 additions & 0 deletions
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)
Back | FazBrowse Home | New Git URL