Skip to content

Commit

Permalink
esm: protect ESM loader from prototype pollution
Browse files Browse the repository at this point in the history
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: #45044
PR-URL: #45175
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
  • Loading branch information
aduh95 authored and danielleadams committed Jan 3, 2023
1 parent d07a5e2 commit 2f27d05
Show file tree
Hide file tree
Showing 10 changed files with 204 additions and 46 deletions.
43 changes: 31 additions & 12 deletions doc/contributing/primordials.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
```

</details>
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const {
ObjectDefineProperty,
ObjectSetPrototypeOf,
RegExpPrototypeExec,
SafePromiseAll,
SafePromiseAllReturnArrayLike,
SafeWeakMap,
StringPrototypeSlice,
StringPrototypeToUpperCase,
Expand Down Expand Up @@ -527,7 +527,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

Expand Down
9 changes: 5 additions & 4 deletions lib/internal/modules/esm/module_job.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ const {
ReflectApply,
RegExpPrototypeExec,
RegExpPrototypeSymbolReplace,
SafePromiseAll,
SafePromiseAllReturnArrayLike,
SafePromiseAllReturnVoid,
SafeSet,
StringPrototypeIncludes,
StringPrototypeSplit,
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);

Expand Down
93 changes: 82 additions & 11 deletions lib/internal/per_context/primordials.js
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ function copyPrototype(src, dest, prefix) {
/* eslint-enable node-core/prefer-primordials */

const {
Array: ArrayConstructor,
ArrayPrototypeForEach,
ArrayPrototypeMap,
FinalizationRegistry,
Expand All @@ -272,6 +273,7 @@ const {
ObjectSetPrototypeOf,
Promise,
PromisePrototypeThen,
PromiseResolve,
ReflectApply,
ReflectConstruct,
ReflectSet,
Expand Down Expand Up @@ -466,9 +468,10 @@ const arrayToSafePromiseIterable = (promises, mapFn) =>
);

/**
* @param {Promise<any>[]} promises
* @param {(v: Promise<any>, k: number) => Promise<any>} [mapFn]
* @returns {Promise<any[]>}
* @template T,U
* @param {Array<T | PromiseLike<T>>} promises
* @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
* @returns {Promise<Awaited<U>[]>}
*/
primordials.SafePromiseAll = (promises, mapFn) =>
// Wrapping on a new Promise is necessary to not expose the SafePromise
Expand All @@ -478,8 +481,56 @@ primordials.SafePromiseAll = (promises, mapFn) =>
);

/**
* @param {Promise<any>[]} promises
* @param {(v: Promise<any>, k: number) => Promise<any>} [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<T | PromiseLike<T>>} promises
* @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
* @returns {Promise<ArrayLike<Awaited<U>>>}
*/
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<T | PromiseLike<T>>} promises
* @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
* @returns {Promise<void>}
*/
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<T|PromiseLike<T>>} promises
* @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
* @returns {Promise<PromiseSettledResult<any>[]>}
*/
primordials.SafePromiseAllSettled = (promises, mapFn) =>
Expand All @@ -490,9 +541,28 @@ primordials.SafePromiseAllSettled = (promises, mapFn) =>
);

/**
* @param {Promise<any>[]} promises
* @param {(v: Promise<any>, k: number) => Promise<any>} [mapFn]
* @returns {Promise<any>}
* 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<T|PromiseLike<T>>} promises
* @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
* @returns {Promise<void>}
*/
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<T|PromiseLike<T>>} promises
* @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
* @returns {Promise<Awaited<U>>}
*/
primordials.SafePromiseAny = (promises, mapFn) =>
// Wrapping on a new Promise is necessary to not expose the SafePromise
Expand All @@ -502,9 +572,10 @@ primordials.SafePromiseAny = (promises, mapFn) =>
);

/**
* @param {Promise<any>[]} promises
* @param {(v: Promise<any>, k: number) => Promise<any>} [mapFn]
* @returns {Promise<any>}
* @template T,U
* @param {Array<T|PromiseLike<T>>} promises
* @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
* @returns {Promise<Awaited<U>>}
*/
primordials.SafePromiseRace = (promises, mapFn) =>
// Wrapping on a new Promise is necessary to not expose the SafePromise
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/vm/module.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const {
ObjectGetPrototypeOf,
ObjectSetPrototypeOf,
ReflectApply,
SafePromiseAll,
SafePromiseAllReturnVoid,
SafeWeakMap,
Symbol,
SymbolToStringTag,
Expand Down Expand Up @@ -330,7 +330,7 @@ class SourceTextModule extends Module {

try {
if (promises !== undefined) {
await SafePromiseAll(promises);
await SafePromiseAllReturnVoid(promises);
}
} catch (e) {
this.#error = e;
Expand Down
5 changes: 0 additions & 5 deletions test/es-module/test-cjs-prototype-pollution.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down
5 changes: 0 additions & 5 deletions test/es-module/test-esm-prototype-pollution.mjs
Original file line number Diff line number Diff line change
@@ -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'),
Expand Down
10 changes: 10 additions & 0 deletions test/parallel/test-eslint-avoid-prototype-pollution.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,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: [
{
Expand Down Expand Up @@ -234,6 +236,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/ }]
Expand Down
Loading

0 comments on commit 2f27d05

Please sign in to comment.