Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esm: protect ESM loader from prototype pollution #45175

Merged
merged 5 commits into from
Oct 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -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

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;
aduh95 marked this conversation as resolved.
Show resolved Hide resolved

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];
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
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]);
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
} 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 @@ -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: [
{
Expand Down Expand Up @@ -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/ }]
Expand Down
Loading