diff options
| -rw-r--r-- | src/crypto/crypto_context.cc | 4 | ||||
| -rw-r--r-- | src/crypto/crypto_keygen.cc | 8 | ||||
| -rw-r--r-- | src/node_binding.cc | 47 | ||||
| -rw-r--r-- | test/parallel/test-process-dlopen-error-message-crash.js | 46 | ||||
| -rw-r--r-- | test/parallel/test-tls-min-max-version.js | 5 |
5 files changed, 74 insertions, 36 deletions
diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index f8ea6f9acca..7eab9de37cb 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -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; } } diff --git a/src/crypto/crypto_keygen.cc b/src/crypto/crypto_keygen.cc index e4e9c227458..e8f55806de6 100644 --- a/src/crypto/crypto_keygen.cc +++ b/src/crypto/crypto_keygen.cc @@ -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; diff --git a/src/node_binding.cc b/src/node_binding.cc index 60eca5c9fa5..fa67a45386e 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -459,7 +459,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; } @@ -484,12 +484,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; } } @@ -504,23 +500,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); @@ -600,9 +595,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); @@ -632,12 +625,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()); diff --git a/test/parallel/test-process-dlopen-error-message-crash.js b/test/parallel/test-process-dlopen-error-message-crash.js new file mode 100644 index 00000000000..d678021764e --- /dev/null +++ b/test/parallel/test-process-dlopen-error-message-crash.js @@ -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'\.$/ +}); diff --git a/test/parallel/test-tls-min-max-version.js b/test/parallel/test-tls-min-max-version.js index 8dbef1fa37a..ff337961f9a 100644 --- a/test/parallel/test-tls-min-max-version.js +++ b/test/parallel/test-tls-min-max-version.js @@ -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'); |