esm: protect ESM loader from prototype pollution #45044

aduh95 · 2022-10-17T18:18:14Z

nodejs-github-bot · 2022-10-17T18:18:18Z

Review requested:

@nodejs/modules
@nodejs/startup

aduh95 · 2022-10-17T18:20:35Z

lib/internal/bootstrap/loaders.js

+    // TODO(aduh95): move this to C++, alongside the initialization of the class.
+    ObjectSetPrototypeOf(ModuleWrap.prototype, null);


The initialization is done here

node/src/module_wrap.cc

Lines 763 to 773 in 4eaaa17

void ModuleWrap::Initialize(Local<Object> target,

Local<Value> unused,

Local<Context> context,

void* priv) {

Environment* env = Environment::GetCurrent(context);

Isolate* isolate = env->isolate();

Local<FunctionTemplate> tpl = NewFunctionTemplate(isolate, New);

tpl->InstanceTemplate()->SetInternalFieldCount(

ModuleWrap::kInternalFieldCount);

tpl->Inherit(BaseObject::GetConstructorTemplate(env));

I couldn't find a way to change the prototype from there. @RaisinTen do you know how to do that by any chance?

@aduh95 I spoke to @LeszekSwirski about this on the Chromium Slack and it seems like this is not possible to do using V8's public API, so I've opened a feature request asking for this - https://bugs.chromium.org/p/v8/issues/detail?id=13392.

OK bummer. Thanks a lot for looking into this and opening the feature request issue :)

JakobJingleheimer

I'm really inclined to say we should always include __proto__: null in any object instantiation.

GeoffreyBooth · 2022-10-17T18:32:26Z

I’m really inclined to say we should always include __proto__: null in any object instantiation.

Or we need to move all internal code off the main thread (or all user code onto a separate thread) so that there’s no chance of polluting internals from user code, and we don’t have to maintain brittle conventions like primordials or __proto__ for internal code.

aduh95 · 2022-10-17T18:36:22Z

I’m really inclined to say we should always include __proto__: null in any object instantiation.

Or we need to move all internal code off the main thread (or all user code onto a separate thread) so that there’s no chance of polluting internals from user code, and we don’t have to maintain brittle conventions like primordials or __proto__ for internal code.

We would still need to protect ourselves from user code in hooks, no? e.g. node --loader 'data:text/javascript,Object.prototype.then=()=>{}' myEntryPoint.mjs

GeoffreyBooth · 2022-10-17T18:38:53Z

We would still need to protect ourselves from user code in hooks, no?

Yes. By “user code” I mean anything non-internal, so user application code and user loader code.

I know this is way out of scope for this PR, just musing that it feels like the real solution is to not need these patterns at all. Though I don’t know what the performance implications would be of multithreading “regular” Node apps where the user code doesn’t use workers.

JakobJingleheimer · 2022-10-17T18:39:49Z

Actually, maybe that's a better option! We could have a very thin main thread that just orchestrates things, put all the internal node work in thread B and then loaders in thread C. I think there is no chance of prototype pollution from loaders with the current design (nor with the 3-thread design mentioned here).

JakobJingleheimer · 2022-10-17T18:45:19Z

Though I don’t know what the performance implications would be of multithreading “regular” Node apps where the user code doesn’t use workers.

If the preliminary results from the off-thread PoC is any indication and we can leverage Atomics for it, it seems very feasible: the additional overhead is nanoseconds. There's some code complexity, but I'd take that in a heartbeat over all this prototype pollution and primitives stuff (AND the Atomics would mostly be a one-and-done, whereas the primitives stuff is a never-ending story).

But surely out of scope for this PR 😅

GeoffreyBooth · 2022-10-17T19:30:04Z

If the preliminary results from the off-thread PoC is any indication and we can leverage Atomics for it, it seems very feasible: the additional overhead is nanoseconds. There’s some code complexity, but I’d take that in a heartbeat over all this prototype pollution and primitives stuff (AND the Atomics would mostly be a one-and-done, whereas the primitives stuff is a never-ending story).

Also it might solve the issues we’re seeing with async_hooks getting triggered for internal async resources as well as user ones; with internal fully separated, async_hooks should only ever get called for user events. (People relying on that API to report on internal events would need a new solution, though, probably the diagnostics channel.) cc @mcollina

MoLow

I wonder if there should be a lint rule for this or there are cases it is not needed

nodejs-github-bot · 2022-10-18T12:41:00Z

CI: https://ci.nodejs.org/job/node-test-pull-request/47319/

nodejs-github-bot · 2022-10-19T11:58:09Z

CI: https://ci.nodejs.org/job/node-test-pull-request/47338/

nodejs-github-bot · 2022-10-19T18:39:18Z

Landed in f2aac0b

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 `Loader.prototype.import` return value from an `Array` to an array-like object. Refs: nodejs#45044

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: nodejs#45044

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>