Skip to content

Commit

Permalink
esm: protect ESM loader from prototype pollution
Browse files Browse the repository at this point in the history
Fixes: #45035
PR-URL: #45044
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
aduh95 authored and danielleadams committed Dec 28, 2022
1 parent 8f1b639 commit 137309e
Show file tree
Hide file tree
Showing 8 changed files with 67 additions and 6 deletions.
3 changes: 3 additions & 0 deletions lib/internal/bootstrap/loaders.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ const {
ObjectDefineProperty,
ObjectKeys,
ObjectPrototypeHasOwnProperty,
ObjectSetPrototypeOf,
ReflectGet,
SafeMap,
SafeSet,
Expand Down Expand Up @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion lib/internal/modules/esm/load.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
}


Expand Down Expand Up @@ -93,6 +93,7 @@ async function defaultLoad(url, context) {
}

return {
__proto__: null,
format,
responseURL,
source,
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,7 @@ class ESMLoader {
}

return {
__proto__: null,
format,
responseURL,
source,
Expand Down Expand Up @@ -892,6 +893,7 @@ class ESMLoader {
}

return {
__proto__: null,
format,
url,
};
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/modules/esm/module_job.js
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ class ModuleJob {
}
throw e;
}
return { module: this.module };
return { __proto__: null, module: this.module };
}
}
ObjectSetPrototypeOf(ModuleJob.prototype, null);
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/modules/esm/resolve.js
Original file line number Diff line number Diff line change
Expand Up @@ -1082,7 +1082,7 @@ async function defaultResolve(specifier, context = {}) {
)
)
) {
return { url: parsed.href };
return { __proto__: null, url: parsed.href };
}
} catch {
// Ignore exception
Expand Down
17 changes: 17 additions & 0 deletions test/es-module/test-cjs-prototype-pollution.js
Original file line number Diff line number Diff line change
@@ -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());
15 changes: 15 additions & 0 deletions test/es-module/test-esm-prototype-pollution.mjs
Original file line number Diff line number Diff line change
@@ -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());
29 changes: 26 additions & 3 deletions test/parallel/test-primordials-promise.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand Down

0 comments on commit 137309e

Please sign in to comment.