Refactor sandbox preload initialization. #12877

tarruda · 2018-05-10T16:22:27Z

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 Error: Cannot get property 'id' on missing remote object #12316 for sandbox. @MarshallOfSound addressed the issue in
Remove the race condition between new process creation and context release #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.

miniak · 2018-05-10T19:55:08Z

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

miniak · 2018-05-10T20:06:11Z

lib/browser/rpc-server.js

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


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.

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.

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

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.

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

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

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()

@tarruda this is the last remaining issue

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 Error: Cannot get property 'id' on missing remote object #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.

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

miniak · 2018-05-10T20:08:02Z

lib/sandboxed_renderer/init.js

 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)


Does this work with browserify?

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

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?

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 · 2018-05-10T23:40:54Z

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

miniak · 2018-05-10T23:41:26Z

lib/sandboxed_renderer/init.js

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


why did you remove events?

It was added recently in #12828

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

tarruda · 2018-05-11T09:25:53Z

@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 · 2018-05-11T09:38:45Z

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

miniak · 2018-05-11T11:18:01Z

lib/sandboxed_renderer/init.js

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


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

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

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'

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.

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.

miniak · 2018-05-11T11:22:38Z

lib/sandboxed_renderer/init.js

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


maybe bundledModules could be a better name?

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

tarruda · 2018-05-11T12:40:58Z

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

miniak · 2018-05-11T17:09:02Z

lib/sandboxed_renderer/init.js

  ${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')


miniak · 2018-05-11T17:10:03Z

lib/sandboxed_renderer/init.js

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


you need to check here if preloadSrc is an Error

you are not logging the error here

I've used preloadError for the error

miniak · 2018-05-11T17:11:04Z

lib/browser/rpc-server.js

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


maybe a separate variable? preloadError?

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

MarshallOfSound · 2018-05-12T02:48:50Z

lib/sandboxed_renderer/init.js

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


For safety

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

This way nothing can ever override that value

MarshallOfSound

left some comments

tarruda · 2018-05-14T17:37:49Z

lib/sandboxed_renderer/init.js

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


@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

miniak · 2018-05-14T19:28:12Z

lib/browser/rpc-server.js

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


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

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)

MarshallOfSound · 2018-05-16T00:14:41Z

lib/browser/rpc-server.js

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


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 😆

tarruda · 2018-05-16T08:54:40Z

Rebased/squashed commits

alexeykuzmin · 2018-05-21T11:58:07Z

lib/sandboxed_renderer/init.js

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


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 · 2018-05-21T12:03:17Z

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

@MarshallOfSound

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 · 2018-05-21T12:24:58Z

@alexeykuzmin Rebased and added writable: false

trop · 2018-05-21T12:57:29Z

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

alexeykuzmin · 2018-05-21T13:03:52Z

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

MarshallOfSound · 2018-05-21T13:05:06Z

~~cc @tarruda ^^~~

I swear I commented first 😆

tarruda · 2018-05-21T13:59:53Z

👍

tarruda requested a review from a team May 10, 2018 16:22

tarruda force-pushed the refactor-sandbox-initialization-to-prevent-race-condition branch from 85ff0a9 to 92adee0 Compare May 10, 2018 16:37

miniak requested changes May 10, 2018

View reviewed changes

tarruda force-pushed the refactor-sandbox-initialization-to-prevent-race-condition branch from d5f6630 to 27bed69 Compare May 11, 2018 11:07

miniak reviewed May 11, 2018

View reviewed changes

miniak mentioned this pull request May 11, 2018

Pass preload script source to lib/sandboxed_renderer/init.js #12832

Closed

miniak reviewed May 11, 2018

View reviewed changes

miniak requested a review from MarshallOfSound May 11, 2018 19:09

MarshallOfSound reviewed May 12, 2018

View reviewed changes

MarshallOfSound requested changes May 12, 2018

View reviewed changes

tarruda commented May 14, 2018

View reviewed changes

miniak reviewed May 14, 2018

View reviewed changes

MarshallOfSound reviewed May 16, 2018

View reviewed changes

tarruda force-pushed the refactor-sandbox-initialization-to-prevent-race-condition branch 2 times, most recently from be27e54 to ebf6bf4 Compare May 16, 2018 08:47

tarruda force-pushed the refactor-sandbox-initialization-to-prevent-race-condition branch 3 times, most recently from 9f0e12a to c8640ef Compare May 16, 2018 10:57

miniak approved these changes May 18, 2018

View reviewed changes

alexeykuzmin suggested changes May 21, 2018

View reviewed changes

tarruda force-pushed the refactor-sandbox-initialization-to-prevent-race-condition branch from c8640ef to 37f42e5 Compare May 21, 2018 12:24

MarshallOfSound approved these changes May 21, 2018

View reviewed changes

alexeykuzmin approved these changes May 21, 2018

View reviewed changes

alexeykuzmin added the target/2-0-x label May 21, 2018

MarshallOfSound merged commit 6f076f7 into master May 21, 2018

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

tarruda mentioned this pull request May 21, 2018

Backport #12877 to 2.0 #13021

Closed

This was referenced May 21, 2018

feat: security: add an option to disable the remote module #13028

Merged

security: don't pass preloadPath via ELECTRON_BROWSER_SANDBOX_LOAD #13031

Merged

Refactor sandbox preload initialization. #12877

Refactor sandbox preload initialization. #12877

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment