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

fix: filter exceptions passed to blink message handler #38912

Closed
wants to merge 1 commit into from

Conversation

zcbenz
Copy link
Member

@zcbenz zcbenz commented Jun 23, 2023

Description of Change

This is a partial fix for #37404.

Blink installs a global V8 message handler to print exceptions to devtools, which will crash when seeing an exception from a foreign context. Since we can not prevent Node from using a non-blink context (from vm module, or from native modules, or from Node's internal code), we should add a wrapper to prevent blink from accessing foreign contexts in the message handler.

Checklist

Release Notes

Notes: Fix crash when throwing exception from context created by Node.js vm module.

Blink installs a global V8 message handler to print exceptions to
devtools, which will crash when seeing an exception from a foreign
context.
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 23, 2023
@zcbenz zcbenz added semver/patch backwards-compatible bug fixes target/23-x-y PR should also be added to the "23-x-y" branch. target/24-x-y PR should also be added to the "24-x-y" branch. target/25-x-y PR should also be added to the "25-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. labels Jun 23, 2023
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

I believe this check is present for security reasons in blink, this effectively changes the check to be a no-op if I'm reading this right as any maliciously created context will be allowed to leak between blink worlds.

There was a discussion at one point around simply disabling vm in the renderer process, I think that should be revisited instead of consistently patching around something that blink does not intend to operate with

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 24, 2023
@zcbenz
Copy link
Member Author

zcbenz commented Jun 26, 2023

The blink message handler is where blink captures exceptions and errors and print them out to the devtools, what this PR does is to check whether the exceptions and errors come from blink, and only handle them to the blink message handler if so. It does not disable blink's security check on contexts, blink still crashes when seeing a foreign context, I don't this PR would introduce any security issue.

@zcbenz
Copy link
Member Author

zcbenz commented Jul 2, 2023

I'm closing this PR since https://github.com/electron/electron/blob/main/patches/chromium/fix_harden_blink_scriptstate_maybefrom.patch provides a better way to fix the issue.

@zcbenz zcbenz closed this Jul 2, 2023
@zcbenz zcbenz deleted the fix-blink-vm-crash branch July 2, 2023 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes target/23-x-y PR should also be added to the "23-x-y" branch. target/24-x-y PR should also be added to the "24-x-y" branch. target/25-x-y PR should also be added to the "25-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants