Hi again

I'm done making changes and adding tests, so this is ready for review.

This PR currently has some issues:

• The C++ implementation of sandboxed renderer shares little code with the non-sandboxed renderer. As a consequence, sandboxed renderers have some missing functionality(setting background color doesn't work, for example). This can be fixed by extracting a base common class for AtomRendererClient and AtomSandboxedRendererClient.
• The javascript code that supports sandboxed renderers(lib/sandboxed_renderer/init.js) only shares part of ipcRenderer with the non-sandboxed version(lib/renderer/init.js), but nothing stops more code to be shared by the two. For example, I believe it is possible to expose the most electron/node.js API to sandboxed renderers by augmenting the browser-renderer RPC module.
• Right now, the code which determines which navigations will create a new SiteInstance is reimplementing some logic from content module. This is necessary because renderers spawned from sandboxed navigations should also be sandboxed, and I've found no way of associating newly spawned renderers(AppendExtraCommandLineSwitches) with sandboxed parents other than reusing the pending process logic in OverrideSiteIntanceForNavigation. The ideal implementation would probably require patching libchromiumcontent to notify AtomBrowserClient whenever a new SiteInstance is created, and populating pending_processes_ with the renderer id for the new instance.

I want to fix all of these issues, but I prefer to do it in separate PRs as this one is very big already.

Another possible issue(depending on the POV) is that browserify is now a build dependency and is used to provide commonjs for preload scripts in sandboxed renderers. I've opted to use browserify because is a very mature browser commonjs implementation and also was the fastest path to get a working commonjs environment in sandboxed renders.

An alternative to using browserify would be to implement a commonjs environment that uses IPC to read files from disk using the main process, though I'd prefer to stick with browserify as it is cleaner than repeatedly loading a bunch of files via IPC.

Finally think its worth mentioning that this PR adds a lot of code but the existing use cases of electron should not be affected by the changes made here. My goal is to allow electron to be used in more scenarios, such as implementing a web browser that shares some of chromium security features.

👍

@tarruda this seems interesting, are you going to add some docs to explain the option and what it gived to developers. There are some information in this thread, but it is not easy to find.

Hi @tarruda,

Understood ipcRenderer is exposed in the sandboxed renderer. Is there a plan to extend the list of exposed modules? for example websocket ?

Thanks.

@tarruda this seems interesting, are you going to add some docs to explain the option and what it gived to developers. There are some information in this thread, but it is not easy to find.

I will add documentation later

Understood ipcRenderer is exposed in the sandboxed renderer. Is there a plan to extend the list of exposed modules? for example websocket ?

It is possible that we'll eventually expose every module available to non-sandboxed renderers, but you can already achieve anything with ipcRenderer alone. From a security POV, it is better that only ipcRenderer is exposed.

@tarruda I agree for the security constraints and this the reason I'm interested in your contribution. However I also need to establish a communication with a standalone node.js process (non electron process). Websocket seems an easy option for a electron renderer to communicate with the node.js process. Is it possible to require the websocket module in the preload in js or this can only be achievable by modifying electron code ? Thanks

I think the list of modules to expose should be passed with a new parameter such as sandboxAllowedModules. The default electron behavior will not be impacted and the change will give an opportunity for those who needs to support some more modules. Not sure if there is way to raise a change request here or if I need to create a PR. Thanks.

However I also need to establish a communication with a standalone node.js process (non electron process). Websocket seems an easy option for a electron renderer to communicate with the node.js process. Is it possible to require the websocket module in the preload in js or this can only be achievable by modifying electron code ? Thanks

I'm not an expert in the chrome sandbox architecture, but if I understood it correctly, the renderer is only allowed to communicate with the browser process, so even if we allow arbitrary node/electron modules to be required from the renderer, the entire API would be proxied using IPC(it would simply be hidden from you). Even network access is done via the browser process, so the renderer is never opening any network connections directly.

If you simply want to use WebSocket web api, then it should already be available to sandboxed renderers, there's no need to modify anything.

Not sure if there is way to raise a change request here or if I need to create a PR. Thanks.

No need to create an issue a, simply use #6712 to track progress. I plan to implement it as soon as I get a free weekend, work/family have been demanding a lot lately :)

That's fine, many thanks.
Exposing ipcRenderer by default in sandboxed mode will still be unsafe as security hole in the main process can be exploited to harm the user machine.
So,I think ipcRenderer should be disabled by default and it will be the responsibility of the developer to select it in the allowed modules if he wills. Another option is to expose a new safeIpcRender module that will only process user defined messages.

@tarruda I would like your opinion about having a default preload used when we are in sandbox mode (and in that case no need to have preload attribute in webpreferences).
We are very interested by using the sandbox but having a preload script for each application is not so easy to maintain (depending if the application need specific code in the preload)

@tarruda I would like your opinion about having a default preload used when we are in sandbox mode (and in that case no need to have preload attribute in webpreferences).
We are very interested by using the sandbox but having a preload script for each application is not so easy to maintain (depending if the application need specific code in the preload)

Not sure if I understand what you mean by "default preload". Can you give an example?

If you want to have a single preload API that exports all electron/node.js APIs to the renderer, then the sandbox won't help much(assuming you are using it for security purposes).

Today with your solution all the developers who want to use sandbox have to manage on their side a preload for initiliazing some code (if they want to use it) for each application and depending if they want to activate or not the sandbox.
My main concern is for having window.open works in sandbox mode, we need some specific code in the preload and it's not very helpful to adding manually the code every time we need to use sandbox (it should be automatic).

There's no need to have a preload, you can simply use new BrowserWindow({webPreferences: {sandbox: true}}) and it will create a sandboxed window. This should work for loading most web applications and offer the same security as chrome. window.open will also work without a preload.

@tarruda,

I think security whole in the master process can be exploited via IpcRenderer. If I need a pre-load file to expose ipcRenderer this is fine, I will just avoid to expose it. But if it is automatically exposed what is the best way to disable it before starting the renderer process ?
Thanks.

Yes but for having window.open works correctly in sandbox mode I need to have some code to be loaded. That's why I ask the question (if some was interesting to have a fix for window.open).

But if it is automatically exposed what is the best way to disable it before starting the renderer process ?

It is not automatically exposed. Since one of the main use cases of sandbox mode is enabling chromium sandbox for security, you have to explicitly create any extra APIs that use ipcRenderer to communicate with the main process.

I already got that. I need that ipcRenderer not even be available in the Renderer process.
How to achieve that ?

In order to give more details: 'default preload' means for me specific code in init.js for renderer sandboxed

I already got that. I need that ipcRenderer not even be available in the Renderer process.
How to achieve that ?

ipcRenderer is only available if your preload script exposes it as a global variable. The preload script is executed in the scope of an anonymous function before any javascript from the web page is loaded. The only way ipcRenderer is available to javascript loaded by web page is if you do something like this:

// preload.js
window.ipcRenderer = require('electron').ipcRenderer
// ipcRenderer is a global variable visible to untrusted javascript.

Great. This is what I wanted to confirm with you.
Thank you.