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

src: fix multiple format string bugs · nodejs/node@f0d841f · GitHub

/ node Public

Commit f0d841f

Browse files
tniessen
authored and
juanarbol
committed
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 <matteo.collina@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
1 parent 566d80f commit f0d841f

5 files changed

Lines changed: 74 additions & 36 deletions

File tree

‎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
@@ -465,7 +465,7 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
465465
// Windows needs to add the filename into the error message
466466
errmsg += *filename;
467467
#endif // _WIN32
468-
THROW_ERR_DLOPEN_FAILED(env, errmsg.c_str());
468+
THROW_ERR_DLOPEN_FAILED(env, "%s", errmsg.c_str());
469469
return false;
470470
}
471471

@@ -490,12 +490,8 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
490490
mp = dlib->GetSavedModuleFromGlobalHandleMap();
491491
if (mp == nullptr || mp->nm_context_register_func == nullptr) {
492492
dlib->Close();
493-
char errmsg[1024];
494-
snprintf(errmsg,
495-
sizeof(errmsg),
496-
"Module did not self-register: '%s'.",
497-
*filename);
498-
THROW_ERR_DLOPEN_FAILED(env, errmsg);
493+
THROW_ERR_DLOPEN_FAILED(
494+
env, "Module did not self-register: '%s'.", *filename);
499495
return false;
500496
}
501497
}
@@ -510,23 +506,22 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
510506
callback(exports, module, context);
511507
return true;
512508
}
513-
char errmsg[1024];
514-
snprintf(errmsg,
515-
sizeof(errmsg),
516-
"The module '%s'"
517-
"\nwas compiled against a different Node.js version using"
518-
"\nNODE_MODULE_VERSION %d. This version of Node.js requires"
519-
"\nNODE_MODULE_VERSION %d. Please try re-compiling or "
520-
"re-installing\nthe module (for instance, using `npm rebuild` "
521-
"or `npm install`).",
522-
*filename,
523-
mp->nm_version,
524-
NODE_MODULE_VERSION);
525509

510+
const int actual_nm_version = mp->nm_version;
526511
// NOTE: `mp` is allocated inside of the shared library's memory, calling
527512
// `dlclose` will deallocate it
528513
dlib->Close();
529-
THROW_ERR_DLOPEN_FAILED(env, errmsg);
514+
THROW_ERR_DLOPEN_FAILED(
515+
env,
516+
"The module '%s'"
517+
"\nwas compiled against a different Node.js version using"
518+
"\nNODE_MODULE_VERSION %d. This version of Node.js requires"
519+
"\nNODE_MODULE_VERSION %d. Please try re-compiling or "
520+
"re-installing\nthe module (for instance, using `npm rebuild` "
521+
"or `npm install`).",
522+
*filename,
523+
actual_nm_version,
524+
NODE_MODULE_VERSION);
530525
return false;
531526
}
532527
CHECK_EQ(mp->nm_flags & NM_F_BUILTIN, 0);
@@ -607,9 +602,7 @@ void GetInternalBinding(const FunctionCallbackInfo<Value>& args) {
607602
env->isolate()))
608603
.FromJust());
609604
} else {
610-
char errmsg[1024];
611-
snprintf(errmsg, sizeof(errmsg), "No such module: %s", *module_v);
612-
return THROW_ERR_INVALID_MODULE(env, errmsg);
605+
return THROW_ERR_INVALID_MODULE(env, "No such module: %s", *module_v);
613606
}
614607

615608
args.GetReturnValue().Set(exports);
@@ -639,12 +632,8 @@ void GetLinkedBinding(const FunctionCallbackInfo<Value>& args) {
639632
mod = FindModule(modlist_linked, name, NM_F_LINKED);
640633

641634
if (mod == nullptr) {
642-
char errmsg[1024];
643-
snprintf(errmsg,
644-
sizeof(errmsg),
645-
"No such module was linked: %s",
646-
*module_name_v);
647-
return THROW_ERR_INVALID_MODULE(env, errmsg);
635+
return THROW_ERR_INVALID_MODULE(
636+
env, "No such module was linked: %s", *module_name_v);
648637
}
649638

650639
Local<Object> module = Object::New(env->isolate());

‎test/parallel/test-process-dlopen-error-message-crash.js‎

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)
Back | FazBrowse Home | New Git URL