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: nodejs/node#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]>
noslate-project · Jan 3, 2023 · 241c051 · 241c051
1 parent 388da8e
commit 241c051
Show file tree

Hide file tree

Showing 5 changed files with 74 additions and 36 deletions.
diff --git 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
@@ -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
@@ -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;
     }
 
@@ -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;
         }
       }
@@ -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);
@@ -608,9 +603,7 @@ void GetInternalBinding(const FunctionCallbackInfo<Value>& args) {
                         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);
@@ -640,12 +633,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
@@ -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
@@ -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');