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

Desktop app uses --no-sandbox #4073

Closed
alexgleason opened this issue May 30, 2017 · 8 comments
Closed

Desktop app uses --no-sandbox #4073

alexgleason opened this issue May 30, 2017 · 8 comments

Comments

@alexgleason
Copy link

alexgleason commented May 30, 2017

This could be a security vulnerability if untrusted javascript manages to get run within the application.

Edit: to clarify, this is a Chromium flag and I was told by a Chromium developer that this could be a security concern when he saw the --no-sandbox flag on the process in htop.

@lampholder
Copy link
Member

I've assigned @dbkr as the most likely person to know why we've got the --no-sandbox flag on there right now :\

@lampholder lampholder added the P1 label May 31, 2017
@t3chguy
Copy link
Member

t3chguy commented Jun 3, 2017

sandbox Boolean (optional) - If set, this will sandbox the renderer associated with the window, making it compatible with the Chromium OS-level sandbox and disabling the Node.js engine. This is not the same as the nodeIntegration option and the APIs available to the preload script are more limited. Read more about the option here. Note: This option is currently experimental and may change or be removed in future Electron releases.

last sentence isn't very re-assuring :/

@alexgleason
Copy link
Author

I've just noticed that Atom also uses this. I still don't understand the purpose of it.

screenshot from 2017-06-03 13 37 57

@alexgleason
Copy link
Author

alexgleason commented Jun 3, 2017

Ah, here we go. An explanation from the Electron docs: https://github.com/electron/electron/blob/master/docs/api/sandbox-option.md

It seems that no sandbox is actually the default for Electron.

One of the key security features of Chromium is that all blink rendering/JavaScript code is executed within a sandbox. This sandbox uses OS-specific features to ensure that exploits in the renderer process cannot harm the system.

In other words, when the sandbox is enabled, the renderers can only make changes to the system by delegating tasks to the main process via IPC. Here's more information about the sandbox.

Since a major feature in electron is the ability to run node.js in the renderer process (making it easier to develop desktop applications using web technologies), the sandbox is disabled by electron. This is because most node.js APIs require system access. require() for example, is not possible without file system permissions, which are not available in a sandboxed environment.

Usually this is not a problem for desktop applications since the code is always trusted, but it makes electron less secure than chromium for displaying untrusted web content. For applications that require more security, the sandbox flag will force electron to spawn a classic chromium renderer that is compatible with the sandbox.

A sandboxed renderer doesn't have a node.js environment running and doesn't expose node.js JavaScript APIs to client code. The only exception is the preload script, which has access to a subset of the electron renderer API.

Another difference is that sandboxed renderers don't modify any of the default JavaScript APIs. Consequently, some APIs such as window.open will work as they do in chromium (i.e. they do not return a BrowserWindowProxy).

Please see the above link for the full details. It's thorough.

@alexgleason
Copy link
Author

I'm thinking the only solution is to enable the sandbox. This could be an undertaking because any code using require() will have to change to use IPC instead. But the added security here may truly be worth that effort.

Otherwise, every bit of code that renders things from external sources must be sanitized so that untrusted JS cannot be run. I'm thinking along the lines of opengraph meta content that shows up when you share a link. This would be fine, except that all new code would need to be carefully analyzed from a security standpoint, and this may be even more of an undertaking in the long term than just enabling the sandbox.

@MTRNord
Copy link
Contributor

MTRNord commented Jun 3, 2017

@alexgleason I think using no requires would require to change the hole riot-web and matrix-react-sdk project as it is not a only electron project

@0x4E69676874466F78
Copy link

Running Riot with --enable-sandbox works.
Keys --no-sandbox were gone, instead of them now --enable-sandbox
Did it really become in the sandbox or is it just an outward manifestation?

@t3chguy
Copy link
Member

t3chguy commented Aug 8, 2019

Electron v5 defaults to sandboxing (causing #10509)

@t3chguy t3chguy closed this as completed Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants