Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esm: protect ESM loader from prototype pollution #45044

Merged
merged 1 commit into from
Oct 19, 2022

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Oct 17, 2022

Fixes: #45035

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 17, 2022
Comment on lines +285 to +286
// TODO(aduh95): move this to C++, alongside the initialization of the class.
ObjectSetPrototypeOf(ModuleWrap.prototype, null);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@GeoffreyBooth
Copy link
Member

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
Copy link
Contributor Author

aduh95 commented Oct 17, 2022

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

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

Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 18, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 18, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 19, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 19, 2022
@nodejs-github-bot nodejs-github-bot merged commit f2aac0b into nodejs:main Oct 19, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in f2aac0b

@aduh95 aduh95 deleted the object-prototype-pollution branch October 19, 2022 18:52
aduh95 added a commit to aduh95/node that referenced this pull request Oct 25, 2022
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
aduh95 added a commit to aduh95/node that referenced this pull request Oct 25, 2022
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
nodejs-github-bot pushed a commit that referenced this pull request Oct 27, 2022
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>
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
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>
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
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>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
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>
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
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>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
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>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
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>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
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>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
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>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
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>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

defining Object.prototype.then causes esm loader to throw an error
7 participants