From 46ded6b96e7a92ff9abe96f55948d61681fb89e9 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 19 Oct 2022 13:39:16 -0500 Subject: [PATCH] esm: protect ESM loader from prototype pollution Fixes: https://github.com/nodejs/node/issues/45035 PR-URL: https://github.com/nodejs/node/pull/45044 Reviewed-By: Jacob Smith Reviewed-By: Geoffrey Booth Reviewed-By: Moshe Atlow Reviewed-By: Darshan Sen Reviewed-By: James M Snell --- lib/internal/bootstrap/loaders.js | 3 ++ lib/internal/modules/esm/load.js | 3 +- lib/internal/modules/esm/loader.js | 2 ++ lib/internal/modules/esm/module_job.js | 2 +- lib/internal/modules/esm/resolve.js | 2 +- .../es-module/test-cjs-prototype-pollution.js | 17 +++++++++++ .../test-esm-prototype-pollution.mjs | 15 ++++++++++ test/parallel/test-primordials-promise.js | 29 +++++++++++++++++-- 8 files changed, 67 insertions(+), 6 deletions(-) create mode 100644 test/es-module/test-cjs-prototype-pollution.js create mode 100644 test/es-module/test-esm-prototype-pollution.mjs diff --git a/lib/internal/bootstrap/loaders.js b/lib/internal/bootstrap/loaders.js index a3c3d375f7090e..f25fb7c9f44391 100644 --- a/lib/internal/bootstrap/loaders.js +++ b/lib/internal/bootstrap/loaders.js @@ -53,6 +53,7 @@ const { ObjectDefineProperty, ObjectKeys, ObjectPrototypeHasOwnProperty, + ObjectSetPrototypeOf, ReflectGet, SafeMap, SafeSet, @@ -281,6 +282,8 @@ class BuiltinModule { getESMFacade() { if (this.module) return this.module; const { ModuleWrap } = internalBinding('module_wrap'); + // TODO(aduh95): move this to C++, alongside the initialization of the class. + ObjectSetPrototypeOf(ModuleWrap.prototype, null); const url = `node:${this.id}`; const nativeModule = this; const exportsKeys = ArrayPrototypeSlice(this.exportKeys); diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js index 86b31830457240..a01852a241f9b4 100644 --- a/lib/internal/modules/esm/load.js +++ b/lib/internal/modules/esm/load.js @@ -59,7 +59,7 @@ async function getSource(url, context) { if (policy?.manifest) { policy.manifest.assertIntegrity(parsed, source); } - return { responseURL, source }; + return { __proto__: null, responseURL, source }; } @@ -93,6 +93,7 @@ async function defaultLoad(url, context) { } return { + __proto__: null, format, responseURL, source, diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index d247f5327eda8b..258bf1b59aee98 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -676,6 +676,7 @@ class ESMLoader { } return { + __proto__: null, format, responseURL, source, @@ -892,6 +893,7 @@ class ESMLoader { } return { + __proto__: null, format, url, }; diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 2dd69b32f77cb5..2f8f267ce1bb0b 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -215,7 +215,7 @@ class ModuleJob { } throw e; } - return { module: this.module }; + return { __proto__: null, module: this.module }; } } ObjectSetPrototypeOf(ModuleJob.prototype, null); diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 7a8cc2114cade3..76cff6dd3ee581 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -1083,7 +1083,7 @@ async function defaultResolve(specifier, context = {}) { ) ) ) { - return { url: parsed.href }; + return { __proto__: null, url: parsed.href }; } } catch { // Ignore exception diff --git a/test/es-module/test-cjs-prototype-pollution.js b/test/es-module/test-cjs-prototype-pollution.js new file mode 100644 index 00000000000000..ea24407ee75c40 --- /dev/null +++ b/test/es-module/test-cjs-prototype-pollution.js @@ -0,0 +1,17 @@ +'use strict'; + +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'), + get: mustNotCall('get %Object.prototype%.then'), + }, +}); + +import('data:text/javascript,').then(mustCall()); diff --git a/test/es-module/test-esm-prototype-pollution.mjs b/test/es-module/test-esm-prototype-pollution.mjs new file mode 100644 index 00000000000000..3a311394adc224 --- /dev/null +++ b/test/es-module/test-esm-prototype-pollution.mjs @@ -0,0 +1,15 @@ +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'), + get: mustNotCall('get %Object.prototype%.then'), + }, +}); + +import('data:text/javascript,').then(mustCall()); diff --git a/test/parallel/test-primordials-promise.js b/test/parallel/test-primordials-promise.js index c753b4b7e79912..ae9a381b1a90a6 100644 --- a/test/parallel/test-primordials-promise.js +++ b/test/parallel/test-primordials-promise.js @@ -18,9 +18,32 @@ Promise.all = common.mustNotCall('%Promise%.all'); Promise.allSettled = common.mustNotCall('%Promise%.allSettled'); Promise.any = common.mustNotCall('%Promise%.any'); Promise.race = common.mustNotCall('%Promise%.race'); -Promise.prototype.catch = common.mustNotCall('%Promise.prototype%.catch'); -Promise.prototype.finally = common.mustNotCall('%Promise.prototype%.finally'); -Promise.prototype.then = common.mustNotCall('%Promise.prototype%.then'); + +Object.defineProperties(Promise.prototype, { + catch: { + set: common.mustNotCall('set %Promise.prototype%.catch'), + get: common.mustNotCall('get %Promise.prototype%.catch'), + }, + finally: { + set: common.mustNotCall('set %Promise.prototype%.finally'), + get: common.mustNotCall('get %Promise.prototype%.finally'), + }, + then: { + set: common.mustNotCall('set %Promise.prototype%.then'), + get: common.mustNotCall('get %Promise.prototype%.then'), + }, +}); +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: common.mustNotCall('set %Object.prototype%.then'), + get: common.mustNotCall('get %Object.prototype%.then'), + }, +}); assertIsPromise(PromisePrototypeThen(test(), common.mustCall())); assertIsPromise(SafePromisePrototypeFinally(test(), common.mustCall()));