esm: protect ESM loader from prototype pollution

Fixes: #45035 PR-URL: #45044 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs · Nov 10, 2022 · 2e5d8e7 · 2e5d8e7
1 parent 17349a2
commit 2e5d8e7
Show file tree

Hide file tree

Showing 8 changed files with 67 additions and 6 deletions.
diff --git 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
@@ -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
@@ -664,6 +664,7 @@ class ESMLoader {
     }
 
     return {
+      __proto__: null,
       format,
       responseURL,
       source,
@@ -880,6 +881,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
@@ -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
@@ -1017,7 +1017,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
@@ -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
@@ -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
@@ -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()));