Skip to content

Commit

Permalink
Merge commit '9aedf16f8fb3b2e397a07e3844dd964ce435a8e3' into v18.20.2
Browse files Browse the repository at this point in the history
  • Loading branch information
asana-kristoferbuno committed Apr 17, 2024
2 parents 7164318 + 9aedf16 commit 6376b2a
Show file tree
Hide file tree
Showing 10 changed files with 181 additions and 11 deletions.
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ release.
</tr>
<tr>
<td valign="top">
<b><a href="doc/changelogs/CHANGELOG_V18.md#18.20.1">18.20.1</a></b><br/>
<b><a href="doc/changelogs/CHANGELOG_V18.md#18.20.2">18.20.2</a></b><br/>
<a href="doc/changelogs/CHANGELOG_V18.md#18.20.1">18.20.1</a><br/>
<a href="doc/changelogs/CHANGELOG_V18.md#18.20.0">18.20.0</a><br/>
<a href="doc/changelogs/CHANGELOG_V18.md#18.19.1">18.19.1</a><br/>
<a href="doc/changelogs/CHANGELOG_V18.md#18.19.0">18.19.0</a><br/>
Expand Down
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
15 changes: 15 additions & 0 deletions doc/changelogs/CHANGELOG_V18.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
</tr>
<tr>
<td>
<a href="#18.20.2">18.20.2</a><br/>
<a href="#18.20.1">18.20.1</a><br/>
<a href="#18.20.0">18.20.0</a><br/>
<a href="#18.19.1">18.19.1</a><br/>
Expand Down Expand Up @@ -66,6 +67,20 @@
* [io.js](CHANGELOG_IOJS.md)
* [Archive](CHANGELOG_ARCHIVE.md)

<a id="18.20.2"></a>

## 2024-04-10, Version 18.20.2 'Hydrogen' (LTS), @RafaelGSS

This is a security release.

### Notable Changes

* CVE-2024-27980 - Command injection via args parameter of `child_process.spawn` without shell option enabled on Windows

### Commits

* \[[`6627222409`](https://github.com/nodejs/node/commit/6627222409)] - **src**: disallow direct .bat and .cmd file spawning (Ben Noordhuis) [nodejs-private/node-private#564](https://github.com/nodejs-private/node-private/pull/564)

<a id="18.20.1"></a>

## 2024-04-03, Version 18.20.1 'Hydrogen' (LTS), @RafaelGSS
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
2 changes: 1 addition & 1 deletion src/node_version.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

#define NODE_MAJOR_VERSION 18
#define NODE_MINOR_VERSION 20
#define NODE_PATCH_VERSION 1
#define NODE_PATCH_VERSION 2

#define NODE_VERSION_IS_LTS 1
#define NODE_VERSION_LTS_CODENAME "Hydrogen"
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 6376b2a

Please sign in to comment.