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

Refactor sandbox preload initialization. #12877

Merged
merged 1 commit into from May 21, 2018

Conversation

Projects
None yet
4 participants
@tarruda
Contributor

tarruda commented May 10, 2018

Use a single synchronous IPC call to retrieve data required by early
sandbox scripts. This has two purposes:

  • Optimize preload script initialization by:
    • Using one synchronous IPC call to retrieve preload script,
      webContentsId (more on that later), process.{platform,execPath,env}
    • Lazy loading as many modules as possible.
  • Fix #12316 for sandbox. @MarshallOfSound addressed the issue in
    #12342, but it was still present in sandbox mode. By loading
    webContentsId very early and skipping remote module at early
    startup, we fix it for sandbox.

@tarruda tarruda requested a review from electron/reviewers as a code owner May 10, 2018

@miniak

This comment has been minimized.

Contributor

miniak commented May 10, 2018

@tarruda This PR is in conflict with mine #12832. But it could actually be a better solution to the issue I was trying to solve.

@@ -449,3 +450,13 @@ ipcMain.on('ELECTRON_BROWSER_WINDOW_CLOSE', function (event) {
}
event.returnValue = null
})
ipcMain.on('ELECTRON_BROWSER_SANDBOX_LOAD', function (event, preloadPath) {

This comment has been minimized.

@miniak

miniak May 10, 2018

Contributor

I would not pass the preloadPath as an argument. We know which webContents sent the IPC so it should be possible to get the preload script path from the webPreferences.

This comment has been minimized.

@miniak

miniak May 10, 2018

Contributor

We need to assume that the renderer process is compromised and can send arbitrary IPC messages. We cannot allow it to load random files from disk pretending it is a preload script.

This comment has been minimized.

@miniak

miniak May 10, 2018

Contributor

Apart from this issue I like this change, batching multiple synchronous IPC calls into one to make loading faster is definitely welcomed 👍

This comment has been minimized.

@tarruda

tarruda May 10, 2018

Contributor

We need to assume that the renderer process is compromised

Electron is not designed to run untrusted code, so we can never assume that. Untrusted code with access to ipcRenderer can easily control the browser process since it is possible to access any node.js module remotely and AFAIK there's no IPC filtering yet.

In any case, I agree that it would be cleaner to take preloadPath from webPreferences directly as it would also remove the need to pass the path as a renderer argument.

This comment has been minimized.

@miniak

miniak May 10, 2018

Contributor

We are actually planning to add some filtering for remote.require, remote.getGlobal, etc.

This comment has been minimized.

@miniak

miniak May 11, 2018

Contributor

Also if you just let fs.readFileSync throw and not assign the returnValue, sendSync in the renderer process will get stuck waiting.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound May 12, 2018

Member

I agree with @miniak this is basically a one-liner "read any file plz" API, I'd rather we (in this PR) used event.sender.getLastWebPreferences()

This comment has been minimized.

@miniak

miniak May 16, 2018

Contributor

@tarruda this is the last remaining issue

This comment has been minimized.

@tarruda

tarruda May 16, 2018

Contributor

I agree with @miniak this is basically a one-liner "read any file plz" API, I'd rather we (in this PR) used event.sender.getLastWebPreferences()

We currently give access to electron.remote.fs which can not only read any file, but also invoke any FS function. There's no regression in terms of security here by allowing the renderer to specify the preloadPath.

These are my arguments for keeping the current behavior and addressing this issue in a separate PR:

  • If we start using event.sender.getLastWebPreferences(), there's no reason to pass the preload path as argument to the renderer command line, so it would become necessary to refactor more code in C++ both in browser/renderer. (Unless we are OK with leaving dead code to be handled in a separate PR)
  • There's no guarantee that this will work as expected without introducing new regressions. Maybe it will, but I will have to investigate if preload will be present in every event.sender.webPreferences (in windows created by window.open, for example, which follow a different webContents creation path).
  • This is a completely separate change, not related to the goal of this PR which is to fix #12316. I'd rather push refactoring changes in different PRs.

If you guys still insist that this should be done in the current PR, I will do it, but not right now since it will require more testing/verification which I can't do this week.

This comment has been minimized.

@miniak

miniak May 17, 2018

Contributor

@MarshallOfSound? I guess I am ok if this gets addressed in a separate PR.

process.on('exit', () => preloadProcess.emit('exit'))
// This is the `require` function that will be visible to the preload script
function preloadRequire (module) {
if (preloadModules.has(module)) {
return preloadModules.get(module)
return require(module)

This comment has been minimized.

@miniak

miniak May 10, 2018

Contributor

Does this work with browserify?

This comment has been minimized.

@tarruda

tarruda May 10, 2018

Contributor

If the preload script is a browserify bundle, then it will have access to the preloadRequire function. This require is private to the sandbox initialization script (which is also a browserify bundle, but independent from what the user creates).

This comment has been minimized.

@miniak

miniak May 11, 2018

Contributor

According to the docs

Browserify parses the AST for require() calls to traverse the entire dependency graph of your project.

doesn't that mean that all the require calls have to have the module name as literal string?

This comment has been minimized.

@tarruda

tarruda May 11, 2018

Contributor

I see what you mean. I will split the remotely-loaded modules which should be lazy loaded (and have -r flags) from the browserify modules which must have static requires. Thanks for the catch

@miniak

This comment has been minimized.

Contributor

miniak commented May 10, 2018

@tarruda off-topic question, why is clearImmediate not exposed to the preload script, only setImmediate?

const preloadModules = new Set([
'child_process',
'electron',
'fs',

This comment has been minimized.

@miniak

miniak May 10, 2018

Contributor

why did you remove events?

This comment has been minimized.

@miniak

miniak May 10, 2018

Contributor

It was added recently in #12828

This comment has been minimized.

@tarruda

tarruda May 11, 2018

Contributor

It was a merge error, I took this patch from an older electron version we use in our app. Will add it back 👍

@tarruda

This comment has been minimized.

Contributor

tarruda commented May 11, 2018

@tarruda off-topic question, why is clearImmediate not exposed to the preload script, only setImmediate?

I forgot to add it when this file was created. Will add it now 👍

@tarruda

This comment has been minimized.

Contributor

tarruda commented May 11, 2018

@miniak about this change:

In any case, I agree that it would be cleaner to take preloadPath from webPreferences directly as it would also remove the need to pass the path as a renderer argument.

Since it involves some C++ changes I will do it in a separate PR ok?

return loadedModules.get(module)
}
if (remoteModules.has(module)) {
return require(module)

This comment has been minimized.

@miniak

miniak May 11, 2018

Contributor

shouldn't this be electron.remote.require instead?

This comment has been minimized.

@miniak

miniak May 11, 2018

Contributor

in which case do we still need to keep these files?
lib/sandboxed_renderer/api/exports/child_process.js
lib/sandboxed_renderer/api/exports/fs.js
lib/sandboxed_renderer/api/exports/os.js
lib/sandboxed_renderer/api/exports/path.js

This comment has been minimized.

@miniak

miniak May 11, 2018

Contributor

and remove these from electron.gyp

'./lib/sandboxed_renderer/api/exports/fs.js:fs',
'-r',
'./lib/sandboxed_renderer/api/exports/os.js:os',
'-r',
'./lib/sandboxed_renderer/api/exports/path.js:path',
'-r',
'./lib/sandboxed_renderer/api/exports/child_process.js:child_process'

This comment has been minimized.

@tarruda

tarruda May 11, 2018

Contributor

No, we still require these files to emulate a real node.js require for electron renderer modules that are reused in sandbox. Those are bundled in the executable and don't know they are running in a browserify environment.

This comment has been minimized.

@tarruda

tarruda May 11, 2018

Contributor

What I mean is, electron modules such as crashReporter won't use this require function, so we need to tell browserify that when crashReporter requires fs, it is requiring /lib/sandboxed_renderer/api/exports/fs.js and not browserify shim.

This comment has been minimized.

@miniak

miniak May 11, 2018

Contributor

ok

'path'
])
const loadedModules = new Map([

This comment has been minimized.

@miniak

miniak May 11, 2018

Contributor

maybe bundledModules could be a better name?

This comment has been minimized.

@tarruda

tarruda May 11, 2018

Contributor

Technically, all modules are bundled. The ones that delegate to electron.remote are loaded lazily, but are still bundled.

@tarruda

This comment has been minimized.

Contributor

tarruda commented May 11, 2018

@miniak With exception of taking preload path from webPreferences (which I want to do in a separate PR), I believe I've addressed all your comments. Let me know when you've finished your review so I can squash the fixup commits.

${preloadSrc}
})`
// eval in window scope:
// http://www.ecma-international.org/ecma-262/5.1/#sec-10.4.2
const geval = eval
const preloadFn = geval(preloadWrapperSrc)
const {setImmediate} = require('timers')
preloadFn(preloadRequire, preloadProcess, Buffer, global, setImmediate)
const {setImmediate, clearImmediate} = require('timers')

This comment has been minimized.

@miniak

miniak May 11, 2018

Contributor

👍

if (preloadPath) {
const preloadSrc = fs.readFileSync(preloadPath).toString()
const preloadWrapperSrc = `(function(require, process, Buffer, global, setImmediate) {
if (preloadSrc) {

This comment has been minimized.

@miniak

miniak May 11, 2018

Contributor

you need to check here if preloadSrc is an Error

This comment has been minimized.

@miniak

miniak May 14, 2018

Contributor

you are not logging the error here

This comment has been minimized.

@tarruda

tarruda May 14, 2018

Contributor

I've used preloadError for the error

try {
preloadSrc = fs.readFileSync(preloadPath).toString()
} catch (err) {
preloadSrc = err

This comment has been minimized.

@miniak

miniak May 11, 2018

Contributor

maybe a separate variable? preloadError?

This comment has been minimized.

@MarshallOfSound

MarshallOfSound May 12, 2018

Member

Also we need to be aware the Error objects to not JSON stringify very well by default, you'll need to manually pull out the stack and message attributes into a new object

@miniak miniak requested a review from MarshallOfSound May 11, 2018

preloadSrc, webContentsId, platform, execPath, env
} = electron.ipcRenderer.sendSync('ELECTRON_BROWSER_SANDBOX_LOAD', preloadPath)
process.webContentsId = webContentsId

This comment has been minimized.

@MarshallOfSound

MarshallOfSound May 12, 2018

Member

For safety

Object.defineProperty(process, 'webContentsId', {
  configurable: false,
  value: webContentsId
})

This way nothing can ever override that value

@MarshallOfSound

left some comments

@@ -103,4 +106,6 @@ if (preloadSrc) {
const preloadFn = geval(preloadWrapperSrc)
const {setImmediate, clearImmediate} = require('timers')
preloadFn(preloadRequire, preloadProcess, Buffer, global, setImmediate, clearImmediate)
} else if (preloadError) {
console.error(preloadError.stack)

This comment has been minimized.

@tarruda

tarruda May 14, 2018

Contributor

@miniak is this how you wanted to handle preload error? I can't think of a better way right now, logging via console.error seems like a sane way to do it, since it would be available in the terminal if electron is started with --enable-logging

try {
preloadSrc = fs.readFileSync(preloadPath).toString()
} catch (err) {
preloadError = {stack: err.stack}

This comment has been minimized.

@miniak

miniak May 14, 2018

Contributor

why don't you include name and message as well?

This comment has been minimized.

@tarruda

tarruda May 15, 2018

Contributor

The stack property already contains the message and Error class name. Example from the repl:

> console.log(new Error('msg').stack)
Error: msg
    at repl:1:13
    at ContextifyScript.Script.runInThisContext (vm.js:50:33)
    at REPLServer.defaultEval (repl.js:240:29)
    at bound (domain.js:301:14)
    at REPLServer.runBound [as eval] (domain.js:314:12)
    at REPLServer.onLine (repl.js:468:10)
    at emitOne (events.js:121:20)
    at REPLServer.emit (events.js:211:7)
    at REPLServer.Interface._onLine (readline.js:282:10)
    at REPLServer.Interface._line (readline.js:631:8)
try {
preloadSrc = fs.readFileSync(preloadPath).toString()
} catch (err) {
preloadError = {stack: err.stack}

This comment has been minimized.

@MarshallOfSound

MarshallOfSound May 16, 2018

Member

I hate to be that person 😆 But welcome to javascript where you can throw null

This should check if we can get to .stack before actually trying or our error handling will error 😆

This comment has been minimized.

@tarruda

tarruda May 16, 2018

Contributor

👍

@tarruda

This comment has been minimized.

Contributor

tarruda commented May 16, 2018

Rebased/squashed commits

@miniak

miniak approved these changes May 18, 2018

} = electron.ipcRenderer.sendSync('ELECTRON_BROWSER_SANDBOX_LOAD', preloadPath)
Object.defineProperty(process, 'webContentsId', {
configurable: false,

This comment has been minimized.

@alexeykuzmin

alexeykuzmin May 21, 2018

Contributor

false is a default value of the configurable attribute, no reasons to explicitly set it.
If you want to be explicit then you should also set writable: false (which is also a default value).

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented May 21, 2018

@tarruda the PR got conflicts, can you please rebase it on the latest master? Thanks!

Refactor sandbox preload initialization.
Use a single synchronous IPC call to retrieve data required by early
sandbox scripts. This has two purposes:

- Optimize preload script initialization by:
  - Using one synchronous IPC call to retrieve preload script,
  webContentsId (more on that later), process.{platform,execPath,env}
  - Lazy loading as many modules as possible.
- Fix #12316 for sandbox. @MarshallOfSound addressed the issue in
  #12342, but it was still present in sandbox mode. By loading
  webContentsId very early and skipping remote module at early
  startup, we fix it for sandbox.
@tarruda

This comment has been minimized.

Contributor

tarruda commented May 21, 2018

@alexeykuzmin Rebased and added writable: false

@MarshallOfSound MarshallOfSound merged commit 6f076f7 into master May 21, 2018

11 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@MarshallOfSound MarshallOfSound deleted the refactor-sandbox-initialization-to-prevent-race-condition branch May 21, 2018

@trop

This comment has been minimized.

Contributor

trop bot commented May 21, 2018

An error occurred while attempting to backport this PR to "2-0-x", you will need to perform this backport manually

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented May 21, 2018

@tarruda Can you please backport the changes to 2-0-x?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented May 21, 2018

cc @tarruda ^^

I swear I commented first 😆

@tarruda

This comment has been minimized.

Contributor

tarruda commented May 21, 2018

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment