FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

esm: protect ESM loader from prototype pollution · nodejs/node@40a5e22 · GitHub

/ node Public

Commit 40a5e22

Browse files
aduh95
authored and
RafaelGSS
committed
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: #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>
1 parent 6e30d22 commit 40a5e22

10 files changed

Lines changed: 204 additions & 46 deletions

File tree

‎doc/contributing/primordials.md‎

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -363,33 +363,52 @@ Object.defineProperty(Object.prototype, Symbol.isConcatSpreadable, {
363363
// 1. Lookup @@iterator property on `array` (user-mutable if user-provided).
364364
// 2. Lookup @@iterator property on %Array.prototype% (user-mutable).
365365
// 3. Lookup `next` property on %ArrayIteratorPrototype% (user-mutable).
366+
// 4. Lookup `then` property on %Array.Prototype% (user-mutable).
367+
// 5. Lookup `then` property on %Object.Prototype% (user-mutable).
366368
PromiseAll([]); // unsafe
367369

368-
PromiseAll(new SafeArrayIterator([])); // safe
370+
// 1. Lookup `then` property on %Array.Prototype% (user-mutable).
371+
// 2. Lookup `then` property on %Object.Prototype% (user-mutable).
372+
PromiseAll(new SafeArrayIterator([])); // still unsafe
373+
SafePromiseAll([]); // still unsafe
374+
375+
SafePromiseAllReturnVoid([]); // safe
376+
SafePromiseAllReturnArrayLike([]); // safe
369377

370378
const array = [promise];
371379
const set = new SafeSet().add(promise);
372380
// When running one of these functions on a non-empty iterable, it will also:
373-
// 4. Lookup `then` property on `promise` (user-mutable if user-provided).
374-
// 5. Lookup `then` property on `%Promise.prototype%` (user-mutable).
381+
// 1. Lookup `then` property on `promise` (user-mutable if user-provided).
382+
// 2. Lookup `then` property on `%Promise.prototype%` (user-mutable).
383+
// 3. Lookup `then` property on %Array.Prototype% (user-mutable).
384+
// 4. Lookup `then` property on %Object.Prototype% (user-mutable).
375385
PromiseAll(new SafeArrayIterator(array)); // unsafe
376-
377386
PromiseAll(set); // unsafe
378387

379-
SafePromiseAll(array); // safe
388+
SafePromiseAllReturnVoid(array); // safe
389+
SafePromiseAllReturnArrayLike(array); // safe
380390

381391
// Some key differences between `SafePromise[...]` and `Promise[...]` methods:
382392

383-
// 1. SafePromiseAll, SafePromiseAllSettled, SafePromiseAny, and SafePromiseRace
384-
// support passing a mapperFunction as second argument.
393+
// 1. SafePromiseAll, SafePromiseAllSettled, SafePromiseAny, SafePromiseRace,
394+
// SafePromiseAllReturnArrayLike, SafePromiseAllReturnVoid, and
395+
// SafePromiseAllSettledReturnVoid support passing a mapperFunction as second
396+
// argument.
385397
SafePromiseAll(ArrayPrototypeMap(array, someFunction));
386398
SafePromiseAll(array, someFunction); // Same as the above, but more efficient.
387399

388-
// 2. SafePromiseAll, SafePromiseAllSettled, SafePromiseAny, and SafePromiseRace
389-
// only support arrays, not iterables. Use ArrayFrom to convert an iterable
390-
// to an array.
391-
SafePromiseAll(set); // ignores set content.
392-
SafePromiseAll(ArrayFrom(set)); // safe
400+
// 2. SafePromiseAll, SafePromiseAllSettled, SafePromiseAny, SafePromiseRace,
401+
// SafePromiseAllReturnArrayLike, SafePromiseAllReturnVoid, and
402+
// SafePromiseAllSettledReturnVoid only support arrays and array-like
403+
// objects, not iterables. Use ArrayFrom to convert an iterable to an array.
404+
SafePromiseAllReturnVoid(set); // ignores set content.
405+
SafePromiseAllReturnVoid(ArrayFrom(set)); // works
406+
407+
// 3. SafePromiseAllReturnArrayLike is safer than SafePromiseAll, however you
408+
// should not use them when its return value is passed to the user as it can
409+
// be surprising for them not to receive a genuine array.
410+
SafePromiseAllReturnArrayLike(array).then((val) => val instanceof Array); // false
411+
SafePromiseAll(array).then((val) => val instanceof Array); // true
393412
```
394413

395414
</details>

‎lib/internal/modules/esm/loader.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ const {
1414
ObjectDefineProperty,
1515
ObjectSetPrototypeOf,
1616
RegExpPrototypeExec,
17-
SafePromiseAll,
17+
SafePromiseAllReturnArrayLike,
1818
SafeWeakMap,
1919
StringPrototypeSlice,
2020
StringPrototypeToUpperCase,
@@ -516,7 +516,7 @@ class ESMLoader {
516516
.then(({ module }) => module.getNamespace());
517517
}
518518

519-
const namespaces = await SafePromiseAll(jobs);
519+
const namespaces = await SafePromiseAllReturnArrayLike(jobs);
520520

521521
if (!wasArr) { return namespaces[0]; } // We can skip the pairing below
522522

‎lib/internal/modules/esm/module_job.js‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,8 @@ const {
1212
ReflectApply,
1313
RegExpPrototypeExec,
1414
RegExpPrototypeSymbolReplace,
15-
SafePromiseAll,
15+
SafePromiseAllReturnArrayLike,
16+
SafePromiseAllReturnVoid,
1617
SafeSet,
1718
StringPrototypeIncludes,
1819
StringPrototypeSplit,
@@ -80,9 +81,9 @@ class ModuleJob {
8081
});
8182

8283
if (promises !== undefined)
83-
await SafePromiseAll(promises);
84+
await SafePromiseAllReturnVoid(promises);
8485

85-
return SafePromiseAll(dependencyJobs);
86+
return SafePromiseAllReturnArrayLike(dependencyJobs);
8687
};
8788
// Promise for the list of all dependencyJobs.
8889
this.linked = link();
@@ -110,7 +111,7 @@ class ModuleJob {
110111
}
111112
jobsInGraph.add(moduleJob);
112113
const dependencyJobs = await moduleJob.linked;
113-
return SafePromiseAll(dependencyJobs, addJobsToDependencyGraph);
114+
return SafePromiseAllReturnVoid(dependencyJobs, addJobsToDependencyGraph);
114115
};
115116
await addJobsToDependencyGraph(this);
116117

‎lib/internal/per_context/primordials.js‎

Lines changed: 82 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,7 @@ function copyPrototype(src, dest, prefix) {
261261
/* eslint-enable node-core/prefer-primordials */
262262

263263
const {
264+
Array: ArrayConstructor,
264265
ArrayPrototypeForEach,
265266
ArrayPrototypeMap,
266267
FinalizationRegistry,
@@ -272,6 +273,7 @@ const {
272273
ObjectSetPrototypeOf,
273274
Promise,
274275
PromisePrototypeThen,
276+
PromiseResolve,
275277
ReflectApply,
276278
ReflectConstruct,
277279
ReflectSet,
@@ -466,9 +468,10 @@ const arrayToSafePromiseIterable = (promises, mapFn) =>
466468
);
467469

468470
/**
469-
* @param {Promise<any>[]} promises
470-
* @param {(v: Promise<any>, k: number) => Promise<any>} [mapFn]
471-
* @returns {Promise<any[]>}
471+
* @template T,U
472+
* @param {Array<T | PromiseLike<T>>} promises
473+
* @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
474+
* @returns {Promise<Awaited<U>[]>}
472475
*/
473476
primordials.SafePromiseAll = (promises, mapFn) =>
474477
// Wrapping on a new Promise is necessary to not expose the SafePromise
@@ -478,8 +481,56 @@ primordials.SafePromiseAll = (promises, mapFn) =>
478481
);
479482

480483
/**
481-
* @param {Promise<any>[]} promises
482-
* @param {(v: Promise<any>, k: number) => Promise<any>} [mapFn]
484+
* Should only be used for internal functions, this would produce similar
485+
* results as `Promise.all` but without prototype pollution, and the return
486+
* value is not a genuine Array but an array-like object.
487+
* @template T,U
488+
* @param {ArrayLike<T | PromiseLike<T>>} promises
489+
* @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
490+
* @returns {Promise<ArrayLike<Awaited<U>>>}
491+
*/
492+
primordials.SafePromiseAllReturnArrayLike = (promises, mapFn) =>
493+
new Promise((resolve, reject) => {
494+
const { length } = promises;
495+
496+
const returnVal = ArrayConstructor(length);
497+
ObjectSetPrototypeOf(returnVal, null);
498+
if (length === 0) resolve(returnVal);
499+
500+
let pendingPromises = length;
501+
for (let i = 0; i < length; i++) {
502+
const promise = mapFn != null ? mapFn(promises[i], i) : promises[i];
503+
PromisePrototypeThen(PromiseResolve(promise), (result) => {
504+
returnVal[i] = result;
505+
if (--pendingPromises === 0) resolve(returnVal);
506+
}, reject);
507+
}
508+
});
509+
510+
/**
511+
* Should only be used when we only care about waiting for all the promises to
512+
* resolve, not what value they resolve to.
513+
* @template T,U
514+
* @param {ArrayLike<T | PromiseLike<T>>} promises
515+
* @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
516+
* @returns {Promise<void>}
517+
*/
518+
primordials.SafePromiseAllReturnVoid = (promises, mapFn) =>
519+
new Promise((resolve, reject) => {
520+
let pendingPromises = promises.length;
521+
if (pendingPromises === 0) resolve();
522+
for (let i = 0; i < promises.length; i++) {
523+
const promise = mapFn != null ? mapFn(promises[i], i) : promises[i];
524+
PromisePrototypeThen(PromiseResolve(promise), () => {
525+
if (--pendingPromises === 0) resolve();
526+
}, reject);
527+
}
528+
});
529+
530+
/**
531+
* @template T,U
532+
* @param {Array<T|PromiseLike<T>>} promises
533+
* @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
483534
* @returns {Promise<PromiseSettledResult<any>[]>}
484535
*/
485536
primordials.SafePromiseAllSettled = (promises, mapFn) =>
@@ -490,9 +541,28 @@ primordials.SafePromiseAllSettled = (promises, mapFn) =>
490541
);
491542

492543
/**
493-
* @param {Promise<any>[]} promises
494-
* @param {(v: Promise<any>, k: number) => Promise<any>} [mapFn]
495-
* @returns {Promise<any>}
544+
* Should only be used when we only care about waiting for all the promises to
545+
* settle, not what value they resolve or reject to.
546+
* @template T,U
547+
* @param {ArrayLike<T|PromiseLike<T>>} promises
548+
* @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
549+
* @returns {Promise<void>}
550+
*/
551+
primordials.SafePromiseAllSettledReturnVoid = async (promises, mapFn) => {
552+
for (let i = 0; i < promises.length; i++) {
553+
try {
554+
await (mapFn != null ? mapFn(promises[i], i) : promises[i]);
555+
} catch {
556+
// In all settled, we can ignore errors.
557+
}
558+
}
559+
};
560+
561+
/**
562+
* @template T,U
563+
* @param {Array<T|PromiseLike<T>>} promises
564+
* @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
565+
* @returns {Promise<Awaited<U>>}
496566
*/
497567
primordials.SafePromiseAny = (promises, mapFn) =>
498568
// Wrapping on a new Promise is necessary to not expose the SafePromise
@@ -502,9 +572,10 @@ primordials.SafePromiseAny = (promises, mapFn) =>
502572
);
503573

504574
/**
505-
* @param {Promise<any>[]} promises
506-
* @param {(v: Promise<any>, k: number) => Promise<any>} [mapFn]
507-
* @returns {Promise<any>}
575+
* @template T,U
576+
* @param {Array<T|PromiseLike<T>>} promises
577+
* @param {(v: T|PromiseLike<T>, k: number) => U|PromiseLike<U>} [mapFn]
578+
* @returns {Promise<Awaited<U>>}
508579
*/
509580
primordials.SafePromiseRace = (promises, mapFn) =>
510581
// Wrapping on a new Promise is necessary to not expose the SafePromise

‎lib/internal/vm/module.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ const {
1111
ObjectGetPrototypeOf,
1212
ObjectSetPrototypeOf,
1313
ReflectApply,
14-
SafePromiseAll,
14+
SafePromiseAllReturnVoid,
1515
SafeWeakMap,
1616
Symbol,
1717
SymbolToStringTag,
@@ -330,7 +330,7 @@ class SourceTextModule extends Module {
330330

331331
try {
332332
if (promises !== undefined) {
333-
await SafePromiseAll(promises);
333+
await SafePromiseAllReturnVoid(promises);
334334
}
335335
} catch (e) {
336336
this.#error = e;

‎test/es-module/test-cjs-prototype-pollution.js‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,6 @@
22

33
const { mustNotCall, mustCall } = require('../common');
44

5-
Object.defineProperties(Array.prototype, {
6-
// %Promise.all% and %Promise.allSettled% are depending on the value of
7-
// `%Array.prototype%.then`.
8-
then: {},
9-
});
105
Object.defineProperties(Object.prototype, {
116
then: {
127
set: mustNotCall('set %Object.prototype%.then'),

‎test/es-module/test-esm-prototype-pollution.mjs‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,5 @@
11
import { mustNotCall, mustCall } from '../common/index.mjs';
22

3-
Object.defineProperties(Array.prototype, {
4-
// %Promise.all% and %Promise.allSettled% are depending on the value of
5-
// `%Array.prototype%.then`.
6-
then: {},
7-
});
83
Object.defineProperties(Object.prototype, {
94
then: {
105
set: mustNotCall('set %Object.prototype%.then'),

‎test/parallel/test-eslint-avoid-prototype-pollution.js‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,8 @@ new RuleTester({
6363
'new Proxy({}, someFactory())',
6464
'new Proxy({}, { __proto__: null })',
6565
'new Proxy({}, { __proto__: null, ...{} })',
66+
'async function name(){return await SafePromiseAll([])}',
67+
'async function name(){const val = await SafePromiseAll([])}',
6668
],
6769
invalid: [
6870
{
@@ -273,6 +275,14 @@ new RuleTester({
273275
code: 'PromiseAll([])',
274276
errors: [{ message: /\bSafePromiseAll\b/ }]
275277
},
278+
{
279+
code: 'async function fn(){await SafePromiseAll([])}',
280+
errors: [{ message: /\bSafePromiseAllReturnVoid\b/ }]
281+
},
282+
{
283+
code: 'async function fn(){await SafePromiseAllSettled([])}',
284+
errors: [{ message: /\bSafePromiseAllSettledReturnVoid\b/ }]
285+
},
276286
{
277287
code: 'PromiseAllSettled([])',
278288
errors: [{ message: /\bSafePromiseAllSettled\b/ }]

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL