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

feat: Add content script world isolation #17032

Merged
merged 7 commits into from Mar 11, 2019

Conversation

Projects
None yet
3 participants
@samuelmaddock
Copy link
Member

samuelmaddock commented Feb 18, 2019

Description of Change

Executes Chrome extension content scripts in an isolated world.

When an isolated world script context is created for a content script, content_script_bundle is injected. The bundle contains code to initialize Chrome APIs and Electron setup.

Checklist

Release Notes

Notes: Added world isolation to Chrome extension content scripts.

@samuelmaddock samuelmaddock requested review from zcbenz and miniak Feb 18, 2019

@samuelmaddock samuelmaddock requested a review from as a code owner Feb 18, 2019

@MarshallOfSound
Copy link
Member

MarshallOfSound left a comment

The implementation details of the approach seem OK, I have some questions RE the approach in general. Basically where the isolation boundary we are trying to draw is. See comments below.

Show resolved Hide resolved BUILD.gn
Show resolved Hide resolved atom/renderer/atom_render_frame_observer.cc Outdated
// Numbers for isolated worlds for extensions are set in
// lib/renderer/content-script-injector.js, and are greater than or equal to
// this number.
ISOLATED_WORLD_EXTENSIONS = 1000,

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Feb 19, 2019

Member

This will have to be explicitly documented in the executeJavaScriptInIsolatedWorld API docs that these numbers are "reserved" / "used" by chrome extensions

This comment has been minimized.

@samuelmaddock

samuelmaddock Feb 19, 2019

Author Member

Good point. Since users may have use cases for a large range, perhaps the extensions range should be moved up as well (1000 => 100000?).

This comment has been minimized.

@samuelmaddock

samuelmaddock Feb 20, 2019

Author Member

I've defined the range between [1 << 20, 1 << 29) with more info in the commit message of 8d02015

const chromeAPI = require('@electron/internal/renderer/chrome-api')

// Await initialization with extension ID
window.__init = (extensionId) => {

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Feb 19, 2019

Member

I feel as though there should be a better way of doing this, it also implies that this initalizer and the users chrome extension aren't isolated?

I may be missing where the isolation boundary is that this PR is trying to create.

E.g.

Chrome Extension |||| Electron Code but this PR appears to be more Chrome Extension + Electron Code ||| User Code which implies a high level of trust in the Chrome Extension code? Maybe I just need some clarification on that

This comment has been minimized.

@samuelmaddock

samuelmaddock Feb 19, 2019

Author Member

The __init function is coupled with Electron code in the content-scripts-injector.ts script. Prior to running a Chrome extension's content script, it will call the __init function with the associated extensionId.

const runContentScript = function (this: any, extensionId: string, url: string, code: string) {
  const sources = [
    // initialize Chrome API in isolated world
    { code: `typeof __init === 'function' && __init('${extensionId}')` },
    // then run content script in isolated world
    { code, url }
  ]

  // ...

  webFrame.executeJavaScriptInIsolatedWorld(worldId, sources)
}

Since __init is called first from Electron code, user code won't be able to access it after it has been removed. The exception would be if the user creates an isolated world within the reserved extension ID range, outside of the content script injector.

This still feels dirty to me, but I'm not yet sure how to initialize the content_script_bundle with the extension ID ahead of time.

This comment has been minimized.

@samuelmaddock

samuelmaddock Feb 24, 2019

Author Member

@MarshallOfSound I found another way to pass the extension ID to the content_script_bundle script using a v8 private. fb42cd0

Now there is a clear isolation between user code and Electron code for initializing the content script isolated world.

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Feb 19, 2019

I can't seem to tag wg-security in the reviewers list so just cc'ing here

cc @electron/wg-security

@samuelmaddock samuelmaddock changed the title [wip] Add content script world isolation [wip] feat: Add content script world isolation Feb 19, 2019

@samuelmaddock samuelmaddock force-pushed the samuelmaddock:chrome-extension/isolated-world-staging branch from ea57e9a to 2736e61 Feb 20, 2019

@samuelmaddock samuelmaddock requested a review from Feb 20, 2019

@electron electron deleted a comment Feb 24, 2019

@samuelmaddock samuelmaddock referenced this pull request Feb 26, 2019

Open

Improve browser security #1

4 of 7 tasks complete

@samuelmaddock samuelmaddock changed the title [wip] feat: Add content script world isolation feat: Add content script world isolation Feb 27, 2019

@MarshallOfSound
Copy link
Member

MarshallOfSound left a comment

This all seems good, there's a few TODO's here but they're all things we didn't have before and are good to track 👍

Would like sign-off from someone else in @electron/wg-security that this is 🆗

@samuelmaddock samuelmaddock force-pushed the samuelmaddock:chrome-extension/isolated-world-staging branch from fb42cd0 to cf8cdcd Mar 8, 2019

samuelmaddock added some commits Feb 13, 2019

Define Chrome extension isolated world ID range
1 << 20 was chosen as it provides a sufficiently large range of IDs for extensions, but also provides a large enough buffer for any user worlds in [1000, 1 << 20).

Ultimately this range can be changed if any user application raises it as an issue.
Insert content script CSS into document
This now avoids a script wrapper to inject the style sheet. This closely matches the code used by chromium in `ScriptInjection::InjectCss`.

@samuelmaddock samuelmaddock force-pushed the samuelmaddock:chrome-extension/isolated-world-staging branch from cf8cdcd to 882adc7 Mar 8, 2019

@MarshallOfSound MarshallOfSound requested a review from electron/wg-security Mar 8, 2019

@zcbenz

zcbenz approved these changes Mar 11, 2019

@MarshallOfSound MarshallOfSound merged commit f943db7 into electron:master Mar 11, 2019

4 checks passed

Semantic Pull Request ready to be squashed
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Mar 11, 2019

Release Notes Persisted

Added world isolation to Chrome extension content scripts.

@samuelmaddock samuelmaddock deleted the samuelmaddock:chrome-extension/isolated-world-staging branch Mar 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.