diff options
author | Antoine du Hamel <duhamelantoine1995@gmail.com> | 2021-05-12 13:16:43 +0300 |
---|---|---|
committer | Michaƫl Zasso <targos@protonmail.com> | 2021-06-14 09:17:31 +0300 |
commit | ded83350a01882df7bb6115ebf25edf8ab76c098 (patch) | |
tree | f15008beac3c1aa9ac8623425a10d4a5a6f9454f /lib | |
parent | 864fe9910bf0a09dfe5b9e1d0072718d17920338 (diff) |
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
Backport-PR-URL: https://github.com/nodejs/node/pull/38878
Refs: https://tc39.es/ecma262/#sec-promise.prototype.catch
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Diffstat (limited to 'lib')
-rw-r--r-- | lib/internal/fs/promises.js | 14 | ||||
-rw-r--r-- | lib/internal/modules/run_main.js | 9 | ||||
-rw-r--r-- | lib/internal/per_context/primordials.js | 31 | ||||
-rw-r--r-- | lib/timers/promises.js | 6 |
4 files changed, 47 insertions, 13 deletions
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index ced98ae30a2..62b4ae10b21 100644 --- a/lib/internal/fs/promises.js +++ b/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 = { 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<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); 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; } |