Skip to content

Commit

Permalink
src: fix multiple format string bugs
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
tniessen authored and RafaelGSS committed Sep 5, 2022
1 parent 28781a1 commit 768c9cb
Show file tree
Hide file tree
Showing 5 changed files with 74 additions and 36 deletions.
4 changes: 2 additions & 2 deletions src/crypto/crypto_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -501,8 +501,8 @@ void SecureContext::Init(const FunctionCallbackInfo<Value>& args) {
max_version = TLS1_2_VERSION;
method = TLS_client_method();
} else {
const std::string msg("Unknown method: ");
THROW_ERR_TLS_INVALID_PROTOCOL_METHOD(env, (msg + * sslmethod).c_str());
THROW_ERR_TLS_INVALID_PROTOCOL_METHOD(
env, "Unknown method: %s", *sslmethod);
return;
}
}
Expand Down
8 changes: 3 additions & 5 deletions src/crypto/crypto_keygen.cc
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,9 @@ Maybe<bool> SecretKeyGenTraits::AdditionalConfig(
params->length = static_cast<size_t>(
std::trunc(args[*offset].As<Uint32>()->Value() / CHAR_BIT));
if (params->length > INT_MAX) {
const std::string msg{
SPrintF("length must be less than or equal to %s bits",
static_cast<uint64_t>(INT_MAX) * CHAR_BIT)
};
THROW_ERR_OUT_OF_RANGE(env, msg.c_str());
THROW_ERR_OUT_OF_RANGE(env,
"length must be less than or equal to %u bits",
static_cast<uint64_t>(INT_MAX) * CHAR_BIT);
return Nothing<bool>();
}
*offset += 1;
Expand Down
47 changes: 18 additions & 29 deletions src/node_binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,7 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
// Windows needs to add the filename into the error message
errmsg += *filename;
#endif // _WIN32
THROW_ERR_DLOPEN_FAILED(env, errmsg.c_str());
THROW_ERR_DLOPEN_FAILED(env, "%s", errmsg.c_str());
return false;
}

Expand All @@ -491,12 +491,8 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
mp = dlib->GetSavedModuleFromGlobalHandleMap();
if (mp == nullptr || mp->nm_context_register_func == nullptr) {
dlib->Close();
char errmsg[1024];
snprintf(errmsg,
sizeof(errmsg),
"Module did not self-register: '%s'.",
*filename);
THROW_ERR_DLOPEN_FAILED(env, errmsg);
THROW_ERR_DLOPEN_FAILED(
env, "Module did not self-register: '%s'.", *filename);
return false;
}
}
Expand All @@ -511,23 +507,22 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
callback(exports, module, context);
return true;
}
char errmsg[1024];
snprintf(errmsg,
sizeof(errmsg),
"The module '%s'"
"\nwas compiled against a different Node.js version using"
"\nNODE_MODULE_VERSION %d. This version of Node.js requires"
"\nNODE_MODULE_VERSION %d. Please try re-compiling or "
"re-installing\nthe module (for instance, using `npm rebuild` "
"or `npm install`).",
*filename,
mp->nm_version,
NODE_MODULE_VERSION);

const int actual_nm_version = mp->nm_version;
// NOTE: `mp` is allocated inside of the shared library's memory, calling
// `dlclose` will deallocate it
dlib->Close();
THROW_ERR_DLOPEN_FAILED(env, errmsg);
THROW_ERR_DLOPEN_FAILED(
env,
"The module '%s'"
"\nwas compiled against a different Node.js version using"
"\nNODE_MODULE_VERSION %d. This version of Node.js requires"
"\nNODE_MODULE_VERSION %d. Please try re-compiling or "
"re-installing\nthe module (for instance, using `npm rebuild` "
"or `npm install`).",
*filename,
actual_nm_version,
NODE_MODULE_VERSION);
return false;
}
CHECK_EQ(mp->nm_flags & NM_F_BUILTIN, 0);
Expand Down Expand Up @@ -607,9 +602,7 @@ void GetInternalBinding(const FunctionCallbackInfo<Value>& args) {
builtins::BuiltinLoader::GetConfigString(env->isolate()))
.FromJust());
} else {
char errmsg[1024];
snprintf(errmsg, sizeof(errmsg), "No such module: %s", *module_v);
return THROW_ERR_INVALID_MODULE(env, errmsg);
return THROW_ERR_INVALID_MODULE(env, "No such module: %s", *module_v);
}

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

if (mod == nullptr) {
char errmsg[1024];
snprintf(errmsg,
sizeof(errmsg),
"No such module was linked: %s",
*module_name_v);
return THROW_ERR_INVALID_MODULE(env, errmsg);
return THROW_ERR_INVALID_MODULE(
env, "No such module was linked: %s", *module_name_v);
}

Local<Object> module = Object::New(env->isolate());
Expand Down
46 changes: 46 additions & 0 deletions test/parallel/test-process-dlopen-error-message-crash.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
'use strict';

// This is a regression test for some scenarios in which node would pass
// unsanitized user input to a printf-like formatting function when dlopen
// fails, potentially crashing the process.

const common = require('../common');
const tmpdir = require('../common/tmpdir');
tmpdir.refresh();

const assert = require('assert');
const fs = require('fs');

// This error message should not be passed to a printf-like function.
assert.throws(() => {
process.dlopen({ exports: {} }, 'foo-%s.node');
}, ({ name, code, message }) => {
assert.strictEqual(name, 'Error');
assert.strictEqual(code, 'ERR_DLOPEN_FAILED');
if (!common.isAIX) {
assert.match(message, /foo-%s\.node/);
}
return true;
});

const notBindingDir = 'test/addons/not-a-binding';
const notBindingPath = `${notBindingDir}/build/Release/binding.node`;
const strangeBindingPath = `${tmpdir.path}/binding-%s.node`;
// Ensure that the addon directory exists, but skip the remainder of the test if
// the addon has not been compiled.
fs.accessSync(notBindingDir);
try {
fs.copyFileSync(notBindingPath, strangeBindingPath);
} catch (err) {
if (err.code !== 'ENOENT') throw err;
common.skip(`addon not found: ${notBindingPath}`);
}

// This error message should also not be passed to a printf-like function.
assert.throws(() => {
process.dlopen({ exports: {} }, strangeBindingPath);
}, {
name: 'Error',
code: 'ERR_DLOPEN_FAILED',
message: /^Module did not self-register: '.*binding-%s\.node'\.$/
});
5 changes: 5 additions & 0 deletions test/parallel/test-tls-min-max-version.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,11 @@ test(U, U, 'hokey-pokey', U, U, U,
test(U, U, U, U, U, 'hokey-pokey',
U, U, 'ERR_TLS_INVALID_PROTOCOL_METHOD');

// Regression test: this should not crash because node should not pass the error
// message (including unsanitized user input) to a printf-like function.
test(U, U, U, U, U, '%s_method',
U, U, 'ERR_TLS_INVALID_PROTOCOL_METHOD');

// Cannot use secureProtocol and min/max versions simultaneously.
test(U, U, U, U, 'TLSv1.2', 'TLS1_2_method',
U, U, 'ERR_TLS_PROTOCOL_VERSION_CONFLICT');
Expand Down

0 comments on commit 768c9cb

Please sign in to comment.