Allow electron renderers to be run inside chromium sandbox

[tarruda]

Progress on supporting sandbox in electron started in #6919, but the remaining work will be done by future PRs. This issue will be used to track progress on all relevant tasks.

• Add sandbox option to webPreferences of BrowserWindow. (Add sandbox option and support native window.open #6919)
• Support chromium native window.open API in sandboxed renderers. (Add sandbox option and support native window.open #6919)
• Extract code that is not specific to node.js from AtomRendererClient into a base class that is also shared by AtomSandboxedRendererClient.
• Patch libchromiumcontent to allow reuse of code that determines which navigations should spawn a new renderer. More details here
• Add sandbox option to <webview> tag.
• Expose node.js builtin modules via IPC to sandboxed renderers.
• Reuse most javascript code under lib/renderer/ and lib/common. The goal is to support the full electron API in sandboxed renderers.
• Add documentation

[zcbenz]

This is needed by a few special projects and as far as I know some organizations are already doing this in their own forks.

But moving Electron's API stack from Node is too ambitious, a more practical way is to provide a very small set of APIs under sandbox to allow communicating with the main process.

[tarruda]

But moving Electron's API stack from Node is too ambitious, a more practical way is to provide a very small set of APIs under sandbox to allow communicating with the main process.

Do you mean by providing an implementation of ipcRenderer that doesn't rely on node.js, and enabling this implementation in a new mode that completely disables node.js integration(and enables the sandbox)?

I think this would be a good start, but what would be even better is to completely decouple electron's renderer API from node.js, to make it fully useable not only with nodeIntegration enabled but also in a sandboxed renderer. In other words, nodeIntegration would simply add node.js APIs to the renderer, but it would not be required to access electron's APIs.

In any case, having a simple implementation of ipcRenderer without node.js would certainly help me get to what I'm trying to achieve here.

[jprichardson]

@sindresorhus will you elaborate on your thumbs down? I'm curious to know your perspective here.

[ThomasMoutel]

Hi @tarruda I'm willing to get the crash-reporter working in sandboxed renderers.
It is not working currently, probably by design, no?
Is this something you've already looked into and/or planned to do in the near future?

[tarruda]

It is not working currently, probably by design, no?

Not by design, I simply left this for later(Feel free to take this task)

[anaisbetts]

I think this would be a good start, but what would be even better is to completely decouple electron's renderer API from node.js, to make it fully useable not only with nodeIntegration enabled but also in a sandboxed renderer. In other words, nodeIntegration would simply add node.js APIs to the renderer, but it would not be required to access electron's APIs.

I think that this will create a pretty massive maintenance burden, I'd rather enable simple IPC and leave it at that

[dregenor]

need add access to webFrame in sandbox_renderer, or any another way to interact with webFrame when used sandbox_renderer.

[pfrazee]

A +1 to what @dregenor said. We need the webFrame (or similar) in the preload so that we can call registerURLSchemeAsPrivileged. (Correct me if I'm wrong, and there's somewhere else the call can be made.)

[liliakai]

I also need access to webFrame from a sandboxed renderer in order to setSpellCheckProvider.

[jack-mo]

@tarruda Hi, I'm currently working on a project using Electron where we want to render a webpage on a BrowserWindow and I'm trying to figure out which is more secure (prevent remote content from accessing Node or other APIs, plus standard security like the chromium sandbox).

• Option 1: BrowserWindows with sandbox=true and nodeIntegration=false, loading the remote URL. Nice because it has the sandbox

• Option 2: in a normal BrowserWindow loading the remote URL. This seems to be recommended for remote content, but the lack of sandbox isn't ideal.

Which option is more secure?
Many thanks in advance

[tarruda]

Which option is more secure?

electron sandbox is not very secure since the renderer has full system access via IPC(plus no context isolation yet). I can't say which is more secure, but I definitely wouldn't recommend loading untrusted content into electron.

[ColinEberhardt]

@jack-mo there's actually a great documentation page about these options available here:

https://electron.atom.io/docs/api/sandbox-option/

(it's a bit buried at the moment, only accessible via a link from the BrowserWindow options doc).

[jprichardson]

electron sandbox is not very secure since the renderer has full system access via IPC(plus no context isolation yet). I can't say which is more secure, but I definitely wouldn't recommend loading untrusted content into electron.

@tarruda what's the benefit of using sandbox as opposed to nodeIntegration set at false? Is it performance?

[alexstrat]

@tarruda I read there brave/muon#165 that you have made some progress on the support of sandbox for webview (crash reported in #8378). Can you share more details on your findings and on the blockers? (worth re-opening #8378 to discuss in details?)

[tarruda]

@tarruda what's the benefit of using sandbox as opposed to nodeIntegration set at false? Is it performance?

With nodeIntegration set to false, you still have a node.js engine running in the renderer, the main difference is that the APIs are not accessible to all javascript running in page.

With sandbox, node.js is not even running in the renderer, all you have is a plain chromium javascript environment that emulates a commonjs/node API using browserify.(To access these APIs from page you need to explicitly expose them from preload). It could also result in a slightly more efficient renderer since no integration between chromium and libuv event loops are necessary, but this is not something I have tested.

If this is considered a benefit, depends on POV. Personally I don't like the idea of node.js running in the same process untrusted code is executed, since bugs in node.js could increase the attack surface.

[tarruda]

@tarruda I read there brave/muon#165 that you have made some progress on the support of sandbox for webview (crash reported in #8378). Can you share more details on your findings and on the blockers? (worth re-opening #8378 to discuss in details?)

The top 2 commits of this branch enable sandbox for webview as well as window.open from webview:
https://github.com/tarruda/electron/tree/enable-webview-on-sandbox

As far as I can see, the main problem with that branch is that it relies on chromium internals(casting ot WebContentsImpl class), which is why I didn't open a PR yet.

[oldbam]

@tarruda According to documentation,

One planned enhancement that should greatly increase security is to block IPC messages from sandboxed renderers by default, allowing the main process to explicitly define a set of messages the renderer is allowed to send.

Is there a dedicated issue I can track to check on the progress of this item?

Thank you!

[codebytere]

@tarruda is this still being worked on?

[tarruda]

@codebytere Not currently, these tasks still must be done. If required I can close the issue and track them somewhere else.

[mhuggins]

Is sandboxing still only an option for the entire app, not just webviews? This documentation seems to imply that sandboxing a webview is possible, but it's not clear:

To enable OS-enforced sandbox on BrowserWindow or webview process with sandbox:true without causing entire app to be in sandbox, --enable-mixed-sandbox command-line argument must be passed to electron. This option is currently only supported on macOS and Windows.

[usergit]

I second @mhuggins the documentation implies that sandboxing is possible for webview

[miniak]

@tarruda I ran into this issue and I am not sure if it's meant to be like that or not #13073

[dtaipov]

@mhuggins @usergit , sandbox option for webview, but without window.open support was implemented here:
#9644
Please, correct me anybody if I’m wrong

[miniak]

I believe all this work has already been done. Feel free to create follow-up issues in case there is something missing.