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: nodejs#45044

Author: aduh95

doc/contributing/primordials.md

@@ -363,20 +363,30 @@ 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:
 
@@ -385,11 +395,18 @@ SafePromiseAll(array); // safe
 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) => Array.isArray(val)); // false
+SafePromiseAll(array).then((val) => Array.isArray(val)); // true
 ```
 
 </details>

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
 

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 SafePromiseAllReturnArrayLike(dependencyJobs, addJobsToDependencyGraph);
     };
     await addJobsToDependencyGraph(this);
 

lib/internal/per_context/primordials.js

@@ -477,6 +477,34 @@ primordials.SafePromiseAll = (promises, mapFn) =>
     SafePromise.all(arrayToSafePromiseIterable(promises, mapFn)).then(a, b)
   );
 
+/**
+ * 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.
+ * @param {Promise<any>[]} promises
+ * @returns {Promise<any[]>}
+ */
+primordials.SafePromiseAllReturnArrayLike = async (promises) => {
+  const { length } = promises;
+  const returnVal = { __proto__: null, length };
+  for (let i = 0; i < length; i++) {
+    returnVal[i] = await promises[i];
+  }
+  return returnVal;
+};
+
+/**
+ * Should only be used when we only care about waiting for all the promises to
+ * resolve, not what value they resolve to.
+ * @param {Promise<any>[]} promises
+ * @returns {Promise<void>}
+ */
+primordials.SafePromiseAllReturnVoid = async (promises) => {
+  for (let i = 0; i < promises.length; i++) {
+    await promises[i];
+  }
+};
+
 /**
  * @param {Promise<any>[]} promises
  * @param {(v: Promise<any>, k: number) => Promise<any>} [mapFn]
@@ -489,6 +517,22 @@ primordials.SafePromiseAllSettled = (promises, mapFn) =>
     SafePromise.allSettled(arrayToSafePromiseIterable(promises, mapFn)).then(a, b)
   );
 
+/**
+ * Should only be used when we only care about waiting for all the promises to
+ * settle, not what value they resolve or reject to.
+ * @param {Promise<any>[]} promises
+ * @returns {Promise<void>}
+ */
+primordials.SafePromiseAllSettledReturnVoid = async (promises) => {
+  for (let i = 0; i < promises.length; i++) {
+    try {
+      await promises[i];
+    } catch {
+      // In all settled, we can ignore errors.
+    }
+  }
+};
+
 /**
  * @param {Promise<any>[]} promises
  * @param {(v: Promise<any>, k: number) => Promise<any>} [mapFn]

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;

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'),

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/ }]

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,24 @@ 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()]));
 
+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()]));
+
 async function test() {
   const catchFn = common.mustCall();
   const finallyFn = common.mustCall();

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,