Skip to content

Commit fcd31c5

Browse files
authored
src: fix multiple format string bugs
The THROW_ERR_* functions interpret the first argument as a printf-like format string, which is problematic when it contains unsanitized user input. This typically happens when a printf-like function is used to produce the error message, which is then passed to a THROW_ERR_* function, which again interprets the error message as a format string. Fix such occurrences by properly formatting error messages using static format strings only, and in a single step. PR-URL: #44314 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
1 parent a5671e2 commit fcd31c5

File tree

5 files changed

+74
-36
lines changed

5 files changed

+74
-36
lines changed

src/crypto/crypto_context.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -501,8 +501,8 @@ void SecureContext::Init(const FunctionCallbackInfo<Value>& args) {
501501
max_version = TLS1_2_VERSION;
502502
method = TLS_client_method();
503503
} else {
504-
const std::string msg("Unknown method: ");
505-
THROW_ERR_TLS_INVALID_PROTOCOL_METHOD(env, (msg + * sslmethod).c_str());
504+
THROW_ERR_TLS_INVALID_PROTOCOL_METHOD(
505+
env, "Unknown method: %s", *sslmethod);
506506
return;
507507
}
508508
}

src/crypto/crypto_keygen.cc

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -68,11 +68,9 @@ Maybe<bool> SecretKeyGenTraits::AdditionalConfig(
6868
params->length = static_cast<size_t>(
6969
std::trunc(args[*offset].As<Uint32>()->Value() / CHAR_BIT));
7070
if (params->length > INT_MAX) {
71-
const std::string msg{
72-
SPrintF("length must be less than or equal to %s bits",
73-
static_cast<uint64_t>(INT_MAX) * CHAR_BIT)
74-
};
75-
THROW_ERR_OUT_OF_RANGE(env, msg.c_str());
71+
THROW_ERR_OUT_OF_RANGE(env,
72+
"length must be less than or equal to %u bits",
73+
static_cast<uint64_t>(INT_MAX) * CHAR_BIT);
7674
return Nothing<bool>();
7775
}
7876
*offset += 1;

src/node_binding.cc

Lines changed: 18 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -459,7 +459,7 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
459459
// Windows needs to add the filename into the error message
460460
errmsg += *filename;
461461
#endif // _WIN32
462-
THROW_ERR_DLOPEN_FAILED(env, errmsg.c_str());
462+
THROW_ERR_DLOPEN_FAILED(env, "%s", errmsg.c_str());
463463
return false;
464464
}
465465

@@ -484,12 +484,8 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
484484
mp = dlib->GetSavedModuleFromGlobalHandleMap();
485485
if (mp == nullptr || mp->nm_context_register_func == nullptr) {
486486
dlib->Close();
487-
char errmsg[1024];
488-
snprintf(errmsg,
489-
sizeof(errmsg),
490-
"Module did not self-register: '%s'.",
491-
*filename);
492-
THROW_ERR_DLOPEN_FAILED(env, errmsg);
487+
THROW_ERR_DLOPEN_FAILED(
488+
env, "Module did not self-register: '%s'.", *filename);
493489
return false;
494490
}
495491
}
@@ -504,23 +500,22 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
504500
callback(exports, module, context);
505501
return true;
506502
}
507-
char errmsg[1024];
508-
snprintf(errmsg,
509-
sizeof(errmsg),
510-
"The module '%s'"
511-
"\nwas compiled against a different Node.js version using"
512-
"\nNODE_MODULE_VERSION %d. This version of Node.js requires"
513-
"\nNODE_MODULE_VERSION %d. Please try re-compiling or "
514-
"re-installing\nthe module (for instance, using `npm rebuild` "
515-
"or `npm install`).",
516-
*filename,
517-
mp->nm_version,
518-
NODE_MODULE_VERSION);
519503

504+
const int actual_nm_version = mp->nm_version;
520505
// NOTE: `mp` is allocated inside of the shared library's memory, calling
521506
// `dlclose` will deallocate it
522507
dlib->Close();
523-
THROW_ERR_DLOPEN_FAILED(env, errmsg);
508+
THROW_ERR_DLOPEN_FAILED(
509+
env,
510+
"The module '%s'"
511+
"\nwas compiled against a different Node.js version using"
512+
"\nNODE_MODULE_VERSION %d. This version of Node.js requires"
513+
"\nNODE_MODULE_VERSION %d. Please try re-compiling or "
514+
"re-installing\nthe module (for instance, using `npm rebuild` "
515+
"or `npm install`).",
516+
*filename,
517+
actual_nm_version,
518+
NODE_MODULE_VERSION);
524519
return false;
525520
}
526521
CHECK_EQ(mp->nm_flags & NM_F_BUILTIN, 0);
@@ -600,9 +595,7 @@ void GetInternalBinding(const FunctionCallbackInfo<Value>& args) {
600595
builtins::BuiltinLoader::GetConfigString(env->isolate()))
601596
.FromJust());
602597
} else {
603-
char errmsg[1024];
604-
snprintf(errmsg, sizeof(errmsg), "No such module: %s", *module_v);
605-
return THROW_ERR_INVALID_MODULE(env, errmsg);
598+
return THROW_ERR_INVALID_MODULE(env, "No such module: %s", *module_v);
606599
}
607600

608601
args.GetReturnValue().Set(exports);
@@ -632,12 +625,8 @@ void GetLinkedBinding(const FunctionCallbackInfo<Value>& args) {
632625
mod = FindModule(modlist_linked, name, NM_F_LINKED);
633626

634627
if (mod == nullptr) {
635-
char errmsg[1024];
636-
snprintf(errmsg,
637-
sizeof(errmsg),
638-
"No such module was linked: %s",
639-
*module_name_v);
640-
return THROW_ERR_INVALID_MODULE(env, errmsg);
628+
return THROW_ERR_INVALID_MODULE(
629+
env, "No such module was linked: %s", *module_name_v);
641630
}
642631

643632
Local<Object> module = Object::New(env->isolate());
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
'use strict';
2+
3+
// This is a regression test for some scenarios in which node would pass
4+
// unsanitized user input to a printf-like formatting function when dlopen
5+
// fails, potentially crashing the process.
6+
7+
const common = require('../common');
8+
const tmpdir = require('../common/tmpdir');
9+
tmpdir.refresh();
10+
11+
const assert = require('assert');
12+
const fs = require('fs');
13+
14+
// This error message should not be passed to a printf-like function.
15+
assert.throws(() => {
16+
process.dlopen({ exports: {} }, 'foo-%s.node');
17+
}, ({ name, code, message }) => {
18+
assert.strictEqual(name, 'Error');
19+
assert.strictEqual(code, 'ERR_DLOPEN_FAILED');
20+
if (!common.isAIX) {
21+
assert.match(message, /foo-%s\.node/);
22+
}
23+
return true;
24+
});
25+
26+
const notBindingDir = 'test/addons/not-a-binding';
27+
const notBindingPath = `${notBindingDir}/build/Release/binding.node`;
28+
const strangeBindingPath = `${tmpdir.path}/binding-%s.node`;
29+
// Ensure that the addon directory exists, but skip the remainder of the test if
30+
// the addon has not been compiled.
31+
fs.accessSync(notBindingDir);
32+
try {
33+
fs.copyFileSync(notBindingPath, strangeBindingPath);
34+
} catch (err) {
35+
if (err.code !== 'ENOENT') throw err;
36+
common.skip(`addon not found: ${notBindingPath}`);
37+
}
38+
39+
// This error message should also not be passed to a printf-like function.
40+
assert.throws(() => {
41+
process.dlopen({ exports: {} }, strangeBindingPath);
42+
}, {
43+
name: 'Error',
44+
code: 'ERR_DLOPEN_FAILED',
45+
message: /^Module did not self-register: '.*binding-%s\.node'\.$/
46+
});

test/parallel/test-tls-min-max-version.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,11 @@ test(U, U, 'hokey-pokey', U, U, U,
9797
test(U, U, U, U, U, 'hokey-pokey',
9898
U, U, 'ERR_TLS_INVALID_PROTOCOL_METHOD');
9999

100+
// Regression test: this should not crash because node should not pass the error
101+
// message (including unsanitized user input) to a printf-like function.
102+
test(U, U, U, U, U, '%s_method',
103+
U, U, 'ERR_TLS_INVALID_PROTOCOL_METHOD');
104+
100105
// Cannot use secureProtocol and min/max versions simultaneously.
101106
test(U, U, U, U, 'TLSv1.2', 'TLS1_2_method',
102107
U, U, 'ERR_TLS_PROTOCOL_VERSION_CONFLICT');

0 commit comments

Comments
 (0)