From 2eeb4e1d944b4ebebcf80261d9250bc86eadc89a Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 12 May 2021 12:16:43 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/38650 Refs: https://tc39.es/ecma262/#sec-promise.prototype.catch Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- lib/internal/fs/promises.js | 6 +-- lib/internal/modules/run_main.js | 9 +++-- lib/internal/per_context/primordials.js | 31 +++++++++++++++ lib/timers/promises.js | 6 +-- .../esm_display_syntax_error_import.out | 1 + ...esm_display_syntax_error_import_module.out | 1 + test/parallel/test-primordials-promise.js | 38 +++++++++++++++++++ 7 files changed, 83 insertions(+), 9 deletions(-) create mode 100644 test/parallel/test-primordials-promise.js diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 0253a29c7d5..b30af8fdf55 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -7,11 +7,11 @@ const { MathMin, NumberIsSafeInteger, Promise, - PromisePrototypeFinally, PromisePrototypeThen, PromiseResolve, PromiseReject, SafeArrayIterator, + SafePromisePrototypeFinally, Symbol, Uint8Array, } = primordials; @@ -188,12 +188,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; diff --git a/lib/internal/modules/run_main.js b/lib/internal/modules/run_main.js index bc6f90ce7d0..b8238a1dd2c 100644 --- a/lib/internal/modules/run_main.js +++ b/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 diff --git a/lib/internal/per_context/primordials.js b/lib/internal/per_context/primordials.js index 78f778b6570..42250ffb422 100644 --- a/lib/internal/per_context/primordials.js +++ b/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} 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); diff --git a/lib/timers/promises.js b/lib/timers/promises.js index 1f245580f86..162f465da29 100644 --- a/lib/timers/promises.js +++ b/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; } diff --git a/test/message/esm_display_syntax_error_import.out b/test/message/esm_display_syntax_error_import.out index 8f94e4a5cf1..ad906af431b 100644 --- a/test/message/esm_display_syntax_error_import.out +++ b/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:*:*) diff --git a/test/message/esm_display_syntax_error_import_module.out b/test/message/esm_display_syntax_error_import_module.out index fe13011c4aa..60a208d534a 100644 --- a/test/message/esm_display_syntax_error_import_module.out +++ b/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:*:*) diff --git a/test/parallel/test-primordials-promise.js b/test/parallel/test-primordials-promise.js new file mode 100644 index 00000000000..61651929384 --- /dev/null +++ b/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); +}