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: https://github.com/nodejs/node/pull/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>

3 files changed, 23 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());