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(
 newPromise((resolve,reject)=>{
 this[kCloseResolve]=resolve;
 this[kCloseReject]=reject;
@@ -507,7 +507,7 @@ async function rename(oldPath, newPath){
 
 asyncfunctiontruncate(path,len=0){
 constfd=awaitopen(path,'r+');
-returnPromisePrototypeFinally(ftruncate(fd,len),fd.close);
+returnSafePromisePrototypeFinally(ftruncate(fd,len),fd.close);
 }
 
 asyncfunctionftruncate(handle,len=0){
@@ -638,7 +638,7 @@ async function lchmod(path, mode){
 thrownewERR_METHOD_NOT_IMPLEMENTED('lchmod()');
 
 constfd=awaitopen(path,O_WRONLY|O_SYMLINK);
-returnPromisePrototypeFinally(fchmod(fd,mode),fd.close);
+returnSafePromisePrototypeFinally(fchmod(fd,mode),fd.close);
 }
 
 asyncfunctionlchown(path,uid,gid){
@@ -717,7 +717,7 @@ async function writeFile(path, data, options){
 checkAborted(options.signal);
 
 constfd=awaitopen(path,flag,options.mode);
-returnPromisePrototypeFinally(
+returnSafePromisePrototypeFinally(
 writeFileHandle(fd,data,options.signal,options.encoding),fd.close);
 }
 
@@ -742,7 +742,7 @@ async function readFile(path, options){
 checkAborted(options.signal);
 
 constfd=awaitopen(path,flag,0o666);
-returnPromisePrototypeFinally(readFileHandle(fd,options),fd.close);
+returnSafePromisePrototypeFinally(readFileHandle(fd,options),fd.close);
 }
 
 module.exports={

lib/internal/modules/run_main.js

@@ -1,7 +1,6 @@
 'use strict';
 
 const{
- PromisePrototypeFinally,
  StringPrototypeEndsWith,
 }=primordials;
 constCJSLoader=require('internal/modules/cjs/loader');
@@ -51,7 +50,7 @@ function runMainESM(mainPath){
 }));
 }
 
-functionhandleMainPromise(promise){
+asyncfunctionhandleMainPromise(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);
-returnPromisePrototypeFinally(promise,()=>process.off('exit',handler));
+try{
+returnawaitpromise;
+}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(
 }
 );
 
+constSafePromise=makeSafe(
+Promise,
+classSafePromiseextendsPromise{
+// 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.
+newPromise((a,b)=>
+newSafePromise((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 ={}){
 }
 });
 returnoncancel!==undefined ?
-PromisePrototypeFinally(
+SafePromisePrototypeFinally(
 ret,
 ()=>signal.removeEventListener('abort',oncancel)) : ret;
 }
@@ -115,7 +115,7 @@ function setImmediate(value, options ={}){
 }
 });
 returnoncancel!==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';
+
+constcommon=require('../common');
+constassert=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()));
+
+asyncfunctiontest(){
+constcatchFn=common.mustCall();
+constfinallyFn=common.mustCall();
+
+try{
+awaitPromise.reject();
+}catch{
+catchFn();
+}finally{
+finallyFn();
+}
+}
+
+functionassertIsPromise(promise){
+// Make sure the returned promise is a genuine %Promise% object and not a
+// subclass instance.
+assert.strictEqual(Object.getPrototypeOf(promise),Promise.prototype);
+}