lib: make primordials Promise methods safe

`catch` and `finally` methods on %Promise.prototype% looks up the `then`
property of the instance, making it at risk of prototype pollution.

PR-URL: #38650
Backport-PR-URL: #38878
Refs: https://tc39.es/ecma262/#sec-promise.prototype.catch
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

Author: aduh95

lib/internal/fs/promises.js

@@ -7,10 +7,10 @@ const {
   MathMin,
   NumberIsSafeInteger,
   Promise,
-  PromisePrototypeFinally,
   PromisePrototypeThen,
   PromiseResolve,
   SafeArrayIterator,
+  SafePromisePrototypeFinally,
   Symbol,
   Uint8Array,
 } = primordials;
@@ -186,12 +186,12 @@ class FileHandle extends EventEmitterMixin(JSTransferable) {
     this[kRefs]--;
     if (this[kRefs] === 0) {
       this[kFd] = -1;
-      this[kClosePromise] = PromisePrototypeFinally(
+      this[kClosePromise] = SafePromisePrototypeFinally(
         this[kHandle].close(),
         () => { this[kClosePromise] = undefined; }
       );
     } else {
-      this[kClosePromise] = PromisePrototypeFinally(
+      this[kClosePromise] = SafePromisePrototypeFinally(
         new Promise((resolve, reject) => {
           this[kCloseResolve] = resolve;
           this[kCloseReject] = reject;
@@ -507,7 +507,7 @@ async function rename(oldPath, newPath) {
 
 async function truncate(path, len = 0) {
   const fd = await open(path, 'r+');
-  return PromisePrototypeFinally(ftruncate(fd, len), fd.close);
+  return SafePromisePrototypeFinally(ftruncate(fd, len), fd.close);
 }
 
 async function ftruncate(handle, len = 0) {
@@ -638,7 +638,7 @@ async function lchmod(path, mode) {
     throw new ERR_METHOD_NOT_IMPLEMENTED('lchmod()');
 
   const fd = await open(path, O_WRONLY | O_SYMLINK);
-  return PromisePrototypeFinally(fchmod(fd, mode), fd.close);
+  return SafePromisePrototypeFinally(fchmod(fd, mode), fd.close);
 }
 
 async function lchown(path, uid, gid) {
@@ -717,7 +717,7 @@ async function writeFile(path, data, options) {
   checkAborted(options.signal);
 
   const fd = await open(path, flag, options.mode);
-  return PromisePrototypeFinally(
+  return SafePromisePrototypeFinally(
     writeFileHandle(fd, data, options.signal, options.encoding), fd.close);
 }
 
@@ -742,7 +742,7 @@ async function readFile(path, options) {
   checkAborted(options.signal);
 
   const fd = await open(path, flag, 0o666);
-  return PromisePrototypeFinally(readFileHandle(fd, options), fd.close);
+  return SafePromisePrototypeFinally(readFileHandle(fd, options), fd.close);
 }
 
 module.exports = {

lib/internal/modules/run_main.js

@@ -1,7 +1,6 @@
 'use strict';
 
 const {
-  PromisePrototypeFinally,
   StringPrototypeEndsWith,
 } = primordials;
 const CJSLoader = require('internal/modules/cjs/loader');
@@ -51,7 +50,7 @@ function runMainESM(mainPath) {
   }));
 }
 
-function handleMainPromise(promise) {
+async function handleMainPromise(promise) {
   // Handle a Promise from running code that potentially does Top-Level Await.
   // In that case, it makes sense to set the exit code to a specific non-zero
   // value if the main code never finishes running.
@@ -60,7 +59,11 @@ function handleMainPromise(promise) {
       process.exitCode = 13;
   }
   process.on('exit', handler);
-  return PromisePrototypeFinally(promise, () => process.off('exit', handler));
+  try {
+    return await promise;
+  } finally {
+    process.off('exit', handler);
+  }
 }
 
 // For backwards compatibility, we have to run a bunch of

lib/internal/per_context/primordials.js

@@ -253,6 +253,8 @@ const {
   Map,
   ObjectFreeze,
   ObjectSetPrototypeOf,
+  Promise,
+  PromisePrototypeThen,
   Set,
   SymbolIterator,
   WeakMap,
@@ -384,5 +386,34 @@ primordials.SafeWeakRef = makeSafe(
   }
 );
 
+const SafePromise = makeSafe(
+  Promise,
+  class SafePromise extends Promise {
+    // eslint-disable-next-line no-useless-constructor
+    constructor(executor) { super(executor); }
+  }
+);
+
+primordials.PromisePrototypeCatch = (thisPromise, onRejected) =>
+  PromisePrototypeThen(thisPromise, undefined, onRejected);
+
+/**
+ * Attaches a callback that is invoked when the Promise is settled (fulfilled or
+ * rejected). The resolved value cannot be modified from the callback.
+ * Prefer using async functions when possible.
+ * @param {Promise<any>} thisPromise
+ * @param {() => void) | undefined | null} onFinally The callback to execute
+ *        when the Promise is settled (fulfilled or rejected).
+ * @returns A Promise for the completion of the callback.
+ */
+primordials.SafePromisePrototypeFinally = (thisPromise, onFinally) =>
+  // Wrapping on a new Promise is necessary to not expose the SafePromise
+  // prototype to user-land.
+  new Promise((a, b) =>
+    new SafePromise((a, b) => PromisePrototypeThen(thisPromise, a, b))
+      .finally(onFinally)
+      .then(a, b)
+  );
+
 ObjectSetPrototypeOf(primordials, null);
 ObjectFreeze(primordials);

lib/timers/promises.js

@@ -3,8 +3,8 @@
 const {
   FunctionPrototypeBind,
   Promise,
-  PromisePrototypeFinally,
   PromiseReject,
+  SafePromisePrototypeFinally,
 } = primordials;
 
 const {
@@ -71,7 +71,7 @@ function setTimeout(after, value, options = {}) {
     }
   });
   return oncancel !== undefined ?
-    PromisePrototypeFinally(
+    SafePromisePrototypeFinally(
       ret,
       () => signal.removeEventListener('abort', oncancel)) : ret;
 }
@@ -115,7 +115,7 @@ function setImmediate(value, options = {}) {
     }
   });
   return oncancel !== undefined ?
-    PromisePrototypeFinally(
+    SafePromisePrototypeFinally(
       ret,
       () => signal.removeEventListener('abort', oncancel)) : ret;
 }

test/message/esm_display_syntax_error_import.out

@@ -6,3 +6,4 @@ SyntaxError: The requested module '../fixtures/es-module-loaders/module-named-ex
     at async ModuleJob.run (node:internal/modules/esm/module_job:*:*)
     at async Loader.import (node:internal/modules/esm/loader:*:*)
     at async Object.loadESM (node:internal/process/esm_loader:*:*)
+    at async handleMainPromise (node:internal/modules/run_main:*:*)

test/message/esm_display_syntax_error_import_module.out

@@ -6,3 +6,4 @@ SyntaxError: The requested module './module-named-exports.mjs' does not provide
     at async ModuleJob.run (node:internal/modules/esm/module_job:*:*)
     at async Loader.import (node:internal/modules/esm/loader:*:*)
     at async Object.loadESM (node:internal/process/esm_loader:*:*)
+    at async handleMainPromise (node:internal/modules/run_main:*:*)

test/parallel/test-primordials-promise.js

@@ -0,0 +1,38 @@
+// Flags: --expose-internals
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+
+const {
+  PromisePrototypeCatch,
+  PromisePrototypeThen,
+  SafePromisePrototypeFinally,
+} = require('internal/test/binding').primordials;
+
+Promise.prototype.catch = common.mustNotCall();
+Promise.prototype.finally = common.mustNotCall();
+Promise.prototype.then = common.mustNotCall();
+
+assertIsPromise(PromisePrototypeCatch(Promise.reject(), common.mustCall()));
+assertIsPromise(PromisePrototypeThen(test(), common.mustCall()));
+assertIsPromise(SafePromisePrototypeFinally(test(), common.mustCall()));
+
+async function test() {
+  const catchFn = common.mustCall();
+  const finallyFn = common.mustCall();
+
+  try {
+    await Promise.reject();
+  } catch {
+    catchFn();
+  } finally {
+    finallyFn();
+  }
+}
+
+function assertIsPromise(promise) {
+  // Make sure the returned promise is a genuine %Promise% object and not a
+  // subclass instance.
+  assert.strictEqual(Object.getPrototypeOf(promise), Promise.prototype);
+}