From 40a5e223289ec8075d364a388c6ef6f4af6f9037 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Thu, 27 Oct 2022 15:09:07 -0500 Subject: [PATCH] esm: protect ESM loader from prototype pollution In a previous commit, the loader implementation was modified to be protected against most prototype pollution, but was kept vulnerable to `Array.prototype` pollution. This commit fixes that, the tradeoff is that it modifies the `ESMLoader.prototype.import` return type from an `Array` to an array-like object. Refs: https://github.com/nodejs/node/pull/45044 PR-URL: https://github.com/nodejs/node/pull/45175 Reviewed-By: Geoffrey Booth Reviewed-By: Matteo Collina Reviewed-By: Yagiz Nizipli --- doc/contributing/primordials.md | 43 ++++++--- lib/internal/modules/esm/loader.js | 4 +- lib/internal/modules/esm/module_job.js | 9 +- lib/internal/per_context/primordials.js | 93 ++++++++++++++++--- lib/internal/vm/module.js | 4 +- .../es-module/test-cjs-prototype-pollution.js | 5 - .../test-esm-prototype-pollution.mjs | 5 - .../test-eslint-avoid-prototype-pollution.js | 10 ++ test/parallel/test-primordials-promise.js | 70 +++++++++++++- .../eslint-rules/avoid-prototype-pollution.js | 7 ++ 10 files changed, 204 insertions(+), 46 deletions(-) diff --git a/doc/contributing/primordials.md b/doc/contributing/primordials.md index e240c0302c79eb..48f260559f0c39 100644 --- a/doc/contributing/primordials.md +++ b/doc/contributing/primordials.md @@ -363,33 +363,52 @@ Object.defineProperty(Object.prototype, Symbol.isConcatSpreadable, { // 1. Lookup @@iterator property on `array` (user-mutable if user-provided). // 2. Lookup @@iterator property on %Array.prototype% (user-mutable). // 3. Lookup `next` property on %ArrayIteratorPrototype% (user-mutable). +// 4. Lookup `then` property on %Array.Prototype% (user-mutable). +// 5. Lookup `then` property on %Object.Prototype% (user-mutable). PromiseAll([]); // unsafe -PromiseAll(new SafeArrayIterator([])); // safe +// 1. Lookup `then` property on %Array.Prototype% (user-mutable). +// 2. Lookup `then` property on %Object.Prototype% (user-mutable). +PromiseAll(new SafeArrayIterator([])); // still unsafe +SafePromiseAll([]); // still unsafe + +SafePromiseAllReturnVoid([]); // safe +SafePromiseAllReturnArrayLike([]); // safe const array = [promise]; const set = new SafeSet().add(promise); // When running one of these functions on a non-empty iterable, it will also: -// 4. Lookup `then` property on `promise` (user-mutable if user-provided). -// 5. Lookup `then` property on `%Promise.prototype%` (user-mutable). +// 1. Lookup `then` property on `promise` (user-mutable if user-provided). +// 2. Lookup `then` property on `%Promise.prototype%` (user-mutable). +// 3. Lookup `then` property on %Array.Prototype% (user-mutable). +// 4. Lookup `then` property on %Object.Prototype% (user-mutable). PromiseAll(new SafeArrayIterator(array)); // unsafe - PromiseAll(set); // unsafe -SafePromiseAll(array); // safe +SafePromiseAllReturnVoid(array); // safe +SafePromiseAllReturnArrayLike(array); // safe // Some key differences between `SafePromise[...]` and `Promise[...]` methods: -// 1. SafePromiseAll, SafePromiseAllSettled, SafePromiseAny, and SafePromiseRace -// support passing a mapperFunction as second argument. +// 1. SafePromiseAll, SafePromiseAllSettled, SafePromiseAny, SafePromiseRace, +// SafePromiseAllReturnArrayLike, SafePromiseAllReturnVoid, and +// SafePromiseAllSettledReturnVoid support passing a mapperFunction as second +// argument. SafePromiseAll(ArrayPrototypeMap(array, someFunction)); SafePromiseAll(array, someFunction); // Same as the above, but more efficient. -// 2. SafePromiseAll, SafePromiseAllSettled, SafePromiseAny, and SafePromiseRace -// only support arrays, not iterables. Use ArrayFrom to convert an iterable -// to an array. -SafePromiseAll(set); // ignores set content. -SafePromiseAll(ArrayFrom(set)); // safe +// 2. SafePromiseAll, SafePromiseAllSettled, SafePromiseAny, SafePromiseRace, +// SafePromiseAllReturnArrayLike, SafePromiseAllReturnVoid, and +// SafePromiseAllSettledReturnVoid only support arrays and array-like +// objects, not iterables. Use ArrayFrom to convert an iterable to an array. +SafePromiseAllReturnVoid(set); // ignores set content. +SafePromiseAllReturnVoid(ArrayFrom(set)); // works + +// 3. SafePromiseAllReturnArrayLike is safer than SafePromiseAll, however you +// should not use them when its return value is passed to the user as it can +// be surprising for them not to receive a genuine array. +SafePromiseAllReturnArrayLike(array).then((val) => val instanceof Array); // false +SafePromiseAll(array).then((val) => val instanceof Array); // true ``` diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index c4b69442ccff6b..0f23d48084abe8 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -14,7 +14,7 @@ const { ObjectDefineProperty, ObjectSetPrototypeOf, RegExpPrototypeExec, - SafePromiseAll, + SafePromiseAllReturnArrayLike, SafeWeakMap, StringPrototypeSlice, StringPrototypeToUpperCase, @@ -516,7 +516,7 @@ class ESMLoader { .then(({ module }) => module.getNamespace()); } - const namespaces = await SafePromiseAll(jobs); + const namespaces = await SafePromiseAllReturnArrayLike(jobs); if (!wasArr) { return namespaces[0]; } // We can skip the pairing below diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 2f8f267ce1bb0b..ddf574b07a8e8a 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -12,7 +12,8 @@ const { ReflectApply, RegExpPrototypeExec, RegExpPrototypeSymbolReplace, - SafePromiseAll, + SafePromiseAllReturnArrayLike, + SafePromiseAllReturnVoid, SafeSet, StringPrototypeIncludes, StringPrototypeSplit, @@ -80,9 +81,9 @@ class ModuleJob { }); if (promises !== undefined) - await SafePromiseAll(promises); + await SafePromiseAllReturnVoid(promises); - return SafePromiseAll(dependencyJobs); + return SafePromiseAllReturnArrayLike(dependencyJobs); }; // Promise for the list of all dependencyJobs. this.linked = link(); @@ -110,7 +111,7 @@ class ModuleJob { } jobsInGraph.add(moduleJob); const dependencyJobs = await moduleJob.linked; - return SafePromiseAll(dependencyJobs, addJobsToDependencyGraph); + return SafePromiseAllReturnVoid(dependencyJobs, addJobsToDependencyGraph); }; await addJobsToDependencyGraph(this); diff --git a/lib/internal/per_context/primordials.js b/lib/internal/per_context/primordials.js index a29d609ec8dbf4..bc903d02849e41 100644 --- a/lib/internal/per_context/primordials.js +++ b/lib/internal/per_context/primordials.js @@ -261,6 +261,7 @@ function copyPrototype(src, dest, prefix) { /* eslint-enable node-core/prefer-primordials */ const { + Array: ArrayConstructor, ArrayPrototypeForEach, ArrayPrototypeMap, FinalizationRegistry, @@ -272,6 +273,7 @@ const { ObjectSetPrototypeOf, Promise, PromisePrototypeThen, + PromiseResolve, ReflectApply, ReflectConstruct, ReflectSet, @@ -466,9 +468,10 @@ const arrayToSafePromiseIterable = (promises, mapFn) => ); /** - * @param {Promise[]} promises - * @param {(v: Promise, k: number) => Promise} [mapFn] - * @returns {Promise} + * @template T,U + * @param {Array>} promises + * @param {(v: T|PromiseLike, k: number) => U|PromiseLike} [mapFn] + * @returns {Promise[]>} */ primordials.SafePromiseAll = (promises, mapFn) => // Wrapping on a new Promise is necessary to not expose the SafePromise @@ -478,8 +481,56 @@ primordials.SafePromiseAll = (promises, mapFn) => ); /** - * @param {Promise[]} promises - * @param {(v: Promise, k: number) => Promise} [mapFn] + * Should only be used for internal functions, this would produce similar + * results as `Promise.all` but without prototype pollution, and the return + * value is not a genuine Array but an array-like object. + * @template T,U + * @param {ArrayLike>} promises + * @param {(v: T|PromiseLike, k: number) => U|PromiseLike} [mapFn] + * @returns {Promise>>} + */ +primordials.SafePromiseAllReturnArrayLike = (promises, mapFn) => + new Promise((resolve, reject) => { + const { length } = promises; + + const returnVal = ArrayConstructor(length); + ObjectSetPrototypeOf(returnVal, null); + if (length === 0) resolve(returnVal); + + let pendingPromises = length; + for (let i = 0; i < length; i++) { + const promise = mapFn != null ? mapFn(promises[i], i) : promises[i]; + PromisePrototypeThen(PromiseResolve(promise), (result) => { + returnVal[i] = result; + if (--pendingPromises === 0) resolve(returnVal); + }, reject); + } + }); + +/** + * Should only be used when we only care about waiting for all the promises to + * resolve, not what value they resolve to. + * @template T,U + * @param {ArrayLike>} promises + * @param {(v: T|PromiseLike, k: number) => U|PromiseLike} [mapFn] + * @returns {Promise} + */ +primordials.SafePromiseAllReturnVoid = (promises, mapFn) => + new Promise((resolve, reject) => { + let pendingPromises = promises.length; + if (pendingPromises === 0) resolve(); + for (let i = 0; i < promises.length; i++) { + const promise = mapFn != null ? mapFn(promises[i], i) : promises[i]; + PromisePrototypeThen(PromiseResolve(promise), () => { + if (--pendingPromises === 0) resolve(); + }, reject); + } + }); + +/** + * @template T,U + * @param {Array>} promises + * @param {(v: T|PromiseLike, k: number) => U|PromiseLike} [mapFn] * @returns {Promise[]>} */ primordials.SafePromiseAllSettled = (promises, mapFn) => @@ -490,9 +541,28 @@ primordials.SafePromiseAllSettled = (promises, mapFn) => ); /** - * @param {Promise[]} promises - * @param {(v: Promise, k: number) => Promise} [mapFn] - * @returns {Promise} + * Should only be used when we only care about waiting for all the promises to + * settle, not what value they resolve or reject to. + * @template T,U + * @param {ArrayLike>} promises + * @param {(v: T|PromiseLike, k: number) => U|PromiseLike} [mapFn] + * @returns {Promise} + */ +primordials.SafePromiseAllSettledReturnVoid = async (promises, mapFn) => { + for (let i = 0; i < promises.length; i++) { + try { + await (mapFn != null ? mapFn(promises[i], i) : promises[i]); + } catch { + // In all settled, we can ignore errors. + } + } +}; + +/** + * @template T,U + * @param {Array>} promises + * @param {(v: T|PromiseLike, k: number) => U|PromiseLike} [mapFn] + * @returns {Promise>} */ primordials.SafePromiseAny = (promises, mapFn) => // Wrapping on a new Promise is necessary to not expose the SafePromise @@ -502,9 +572,10 @@ primordials.SafePromiseAny = (promises, mapFn) => ); /** - * @param {Promise[]} promises - * @param {(v: Promise, k: number) => Promise} [mapFn] - * @returns {Promise} + * @template T,U + * @param {Array>} promises + * @param {(v: T|PromiseLike, k: number) => U|PromiseLike} [mapFn] + * @returns {Promise>} */ primordials.SafePromiseRace = (promises, mapFn) => // Wrapping on a new Promise is necessary to not expose the SafePromise diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js index fe72c16a97665d..7e0131c7db2872 100644 --- a/lib/internal/vm/module.js +++ b/lib/internal/vm/module.js @@ -11,7 +11,7 @@ const { ObjectGetPrototypeOf, ObjectSetPrototypeOf, ReflectApply, - SafePromiseAll, + SafePromiseAllReturnVoid, SafeWeakMap, Symbol, SymbolToStringTag, @@ -330,7 +330,7 @@ class SourceTextModule extends Module { try { if (promises !== undefined) { - await SafePromiseAll(promises); + await SafePromiseAllReturnVoid(promises); } } catch (e) { this.#error = e; diff --git a/test/es-module/test-cjs-prototype-pollution.js b/test/es-module/test-cjs-prototype-pollution.js index ea24407ee75c40..9f6291eff14d49 100644 --- a/test/es-module/test-cjs-prototype-pollution.js +++ b/test/es-module/test-cjs-prototype-pollution.js @@ -2,11 +2,6 @@ const { mustNotCall, mustCall } = require('../common'); -Object.defineProperties(Array.prototype, { - // %Promise.all% and %Promise.allSettled% are depending on the value of - // `%Array.prototype%.then`. - then: {}, -}); Object.defineProperties(Object.prototype, { then: { set: mustNotCall('set %Object.prototype%.then'), diff --git a/test/es-module/test-esm-prototype-pollution.mjs b/test/es-module/test-esm-prototype-pollution.mjs index 3a311394adc224..6ba1fd8d64025f 100644 --- a/test/es-module/test-esm-prototype-pollution.mjs +++ b/test/es-module/test-esm-prototype-pollution.mjs @@ -1,10 +1,5 @@ import { mustNotCall, mustCall } from '../common/index.mjs'; -Object.defineProperties(Array.prototype, { - // %Promise.all% and %Promise.allSettled% are depending on the value of - // `%Array.prototype%.then`. - then: {}, -}); Object.defineProperties(Object.prototype, { then: { set: mustNotCall('set %Object.prototype%.then'), diff --git a/test/parallel/test-eslint-avoid-prototype-pollution.js b/test/parallel/test-eslint-avoid-prototype-pollution.js index 0a39dd489499e6..76eee4379b965d 100644 --- a/test/parallel/test-eslint-avoid-prototype-pollution.js +++ b/test/parallel/test-eslint-avoid-prototype-pollution.js @@ -63,6 +63,8 @@ new RuleTester({ 'new Proxy({}, someFactory())', 'new Proxy({}, { __proto__: null })', 'new Proxy({}, { __proto__: null, ...{} })', + 'async function name(){return await SafePromiseAll([])}', + 'async function name(){const val = await SafePromiseAll([])}', ], invalid: [ { @@ -273,6 +275,14 @@ new RuleTester({ code: 'PromiseAll([])', errors: [{ message: /\bSafePromiseAll\b/ }] }, + { + code: 'async function fn(){await SafePromiseAll([])}', + errors: [{ message: /\bSafePromiseAllReturnVoid\b/ }] + }, + { + code: 'async function fn(){await SafePromiseAllSettled([])}', + errors: [{ message: /\bSafePromiseAllSettledReturnVoid\b/ }] + }, { code: 'PromiseAllSettled([])', errors: [{ message: /\bSafePromiseAllSettled\b/ }] diff --git a/test/parallel/test-primordials-promise.js b/test/parallel/test-primordials-promise.js index ae9a381b1a90a6..58a65d73bf6618 100644 --- a/test/parallel/test-primordials-promise.js +++ b/test/parallel/test-primordials-promise.js @@ -7,7 +7,10 @@ const assert = require('assert'); const { PromisePrototypeThen, SafePromiseAll, + SafePromiseAllReturnArrayLike, + SafePromiseAllReturnVoid, SafePromiseAllSettled, + SafePromiseAllSettledReturnVoid, SafePromiseAny, SafePromisePrototypeFinally, SafePromiseRace, @@ -34,9 +37,11 @@ Object.defineProperties(Promise.prototype, { }, }); Object.defineProperties(Array.prototype, { - // %Promise.all% and %Promise.allSettled% are depending on the value of - // `%Array.prototype%.then`. - then: {}, + then: { + configurable: true, + set: common.mustNotCall('set %Array.prototype%.then'), + get: common.mustNotCall('get %Array.prototype%.then'), + }, }); Object.defineProperties(Object.prototype, { then: { @@ -48,11 +53,65 @@ Object.defineProperties(Object.prototype, { assertIsPromise(PromisePrototypeThen(test(), common.mustCall())); assertIsPromise(SafePromisePrototypeFinally(test(), common.mustCall())); -assertIsPromise(SafePromiseAll([test()])); -assertIsPromise(SafePromiseAllSettled([test()])); +assertIsPromise(SafePromiseAllReturnArrayLike([test()])); +assertIsPromise(SafePromiseAllReturnVoid([test()])); +assertIsPromise(SafePromiseAllSettledReturnVoid([test()])); assertIsPromise(SafePromiseAny([test()])); assertIsPromise(SafePromiseRace([test()])); +assertIsPromise(SafePromiseAllReturnArrayLike([])); +assertIsPromise(SafePromiseAllReturnVoid([])); +assertIsPromise(SafePromiseAllSettledReturnVoid([])); + +{ + const val1 = Symbol(); + const val2 = Symbol(); + PromisePrototypeThen( + SafePromiseAllReturnArrayLike([Promise.resolve(val1), { then(resolve) { resolve(val2); } }]), + common.mustCall((val) => { + assert.strictEqual(Array.isArray(val), true); + const expected = [val1, val2]; + assert.deepStrictEqual(val.length, expected.length); + assert.strictEqual(val[0], expected[0]); + assert.strictEqual(val[1], expected[1]); + }) + ); +} + +{ + // Never settling promises should not block the resulting promise to be rejected: + const error = new Error(); + PromisePrototypeThen( + SafePromiseAllReturnArrayLike([new Promise(() => {}), Promise.reject(error)]), + common.mustNotCall('Should have rejected'), + common.mustCall((err) => { + assert.strictEqual(err, error); + }) + ); + PromisePrototypeThen( + SafePromiseAllReturnVoid([new Promise(() => {}), Promise.reject(error)]), + common.mustNotCall('Should have rejected'), + common.mustCall((err) => { + assert.strictEqual(err, error); + }) + ); +} + +Object.defineProperties(Array.prototype, { + // %Promise.all% and %Promise.allSettled% are depending on the value of + // `%Array.prototype%.then`. + then: { + __proto__: undefined, + value: undefined, + }, +}); + +assertIsPromise(SafePromiseAll([test()])); +assertIsPromise(SafePromiseAllSettled([test()])); + +assertIsPromise(SafePromiseAll([])); +assertIsPromise(SafePromiseAllSettled([])); + async function test() { const catchFn = common.mustCall(); const finallyFn = common.mustCall(); @@ -70,4 +129,5 @@ 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); + PromisePrototypeThen(promise, common.mustCall()); } diff --git a/tools/eslint-rules/avoid-prototype-pollution.js b/tools/eslint-rules/avoid-prototype-pollution.js index 343dbaf39d15e0..fdc43c259d9a5b 100644 --- a/tools/eslint-rules/avoid-prototype-pollution.js +++ b/tools/eslint-rules/avoid-prototype-pollution.js @@ -194,6 +194,13 @@ module.exports = { }); }, + [`ExpressionStatement>AwaitExpression>${CallExpression(/^(Safe)?PromiseAll(Settled)?$/)}`](node) { + context.report({ + node, + message: `Use ${node.callee.name}ReturnVoid`, + }); + }, + [CallExpression('PromisePrototypeCatch')](node) { context.report({ node,