Electron security considerations

[thegecko]

The security recommendations from electron outline best practice for securing the main process from malicious attacks in the renderer process. As electron is a little way behind chromium releases, known security holes may be publicised long before theia users receive updates to their apps. This is especially relevant if theia ever loads remote content.

Have these guidelines been considered in the design of theia and is there interest in applying them?

For example, I would recommend adding webPreferences to the BrowserWindow options for these items:

• nodeIntegration: false to prevent malicious code from running node functionality from the renderer
• preload to control the exact functionality and APIs available to the renderer process from the main process. This could simply be restricted to the IPC interfaces

This may have an impact on how electron-browser modules access the main electron process, however and require some re-architecture. I'm happy to propose a PR if this is agreed as being needed.

[marcdumais-work]

@thegecko

Have these guidelines been considered in the design of theia and is there interest in applying them?

To be frank, I am not sure. Maybe someone else can answer this.

I'm happy to propose a PR if this is agreed as being needed

+1. Security is very important.

@marechal-p this might impact your work, to run Theia in an Electron FE, using a remote BE.

[thegecko]

Getting my head around the backend/frontend architecture of theia...

The renderer process security should be implicit based on the mapping between the frontend/backend extensions and the platform separation.

e.g. I wouldn't expect access to the node runtime from the browser unless using the frontendElectron extension and implementing for the electron-browser platform.

Have such relationships been discussed/designed previously?

[paul-marechal]

From the link you mentioned (http://www.theia-ide.org/doc/architecture):

[...] any frontend code may assume browser as a platform but not Node.js.

I think so far the idea was to not rely on any Node API in the frontend, because of the fact that it will run on both environments. In any case, Electron's Node API is just sugar that we don't really need because of the connection between the backend/frontend, that should only provide the necessary protocols and thus accesses to the Node API on the backend from the frontend.

(even when running the electron application, there is still a backend that is started, so we should use it for Node operations)

[paul-marechal]

After more work on #2340, it turns out that using nodeIntegration: false makes the frontend fail.

I don't know exactly why, but monaco was assuming some annoying things about the environment, making it crash when on Electron on http content. I wonder how many packages also try to guess the current env and fail while doing so...

[thegecko]

We've received a report that proves a potential vulnerability using the electron version of Theia due to the nodeIntegration configuration defaulting to true as mentioned here.

This is due to the node runtime being exposed in pages opened within the IDE context which could read local files, etc.

We need to switch off nodeIntegration in the frontend modules and deal with the fallout of breakages.

It looks like electron changed this default to false in version 5, but the configuration option still exists:

https://electronjs.org/releases/stable?page=5#release-notes-for-v500

[paul-marechal]

Are you able to get a stable application by turning this flag off? Last time I tried I had to fight...

[thegecko]

Are you able to get a stable application by turning this flag off

I haven't tried, but it needs to be done :)

[paul-marechal]

Putting here a hack from when I was trying to load http(s) content into Electron:

theia/packages/monaco/src/electron-browser/monaco-electron-module.ts

Lines 50 to 55 in a9506fc

	// vscode-loader patching: https://github.com/Microsoft/vscode-loader/issues/12
	if (isRemote) {
	Object.defineProperty(globals.AMDLoader.Environment.prototype, 'isNode', {
	get: () => false
	});
	}

Without this, vscode's AMDLoader detects that it is running in Electron, and starts doing require calls, which we don't want if nodeIntegration is set to false.

Unless they changed something, this should be relevant.

[akosyakov]

@marechal-p is it the same with latest Monaco? VS Code was refactored to not use Node.js apis in the renderer process, maybe it fixed the issue?

[paul-marechal]

Hopefully... Thanks for the info. But the issue came from the AMDLoader that they use, so unless this component changed as well, then I don't know. But if the problem arise again, I figured it would help having the hack here.

[thegecko]

I've opened a draft PR with this security switched on in case anyone fancies investigating the issues:

#6343

Regarding electron versions, the default to turn off node integration was introduced in Electron 5. We will need to explicitly turn it on when moving to that version or fix the issues apparent in the PR above.

[paul-marechal]

Regarding electron versions, the default to turn off node integration was introduced in Electron 5. We will need to explicitly turn it on when moving to that version or fix the issues apparent in the PR above.

It doesn't seem like an issue for VS Code: https://github.com/microsoft/vscode/blob/a143fc36f8ee58db069c0bd2a14ec4ddeb29a009/src/vs/code/electron-main/sharedProcess.ts#L40

What kind of attack would we avoid by turning it off?

[spoenemann]

The Electron example app shows the following in the developer tools console:

Electron Deprecation Warning (nodeIntegration default change) This window has node integration enabled by default. In Electron 5.0.0, node integration will be disabled by default. To prepare for this change, set {nodeIntegration: true} in the webPreferences for this window, or ensure that this window does not rely on node integration and set {nodeIntegration: false}.

Electron Security Warning (Insecure Content-Security-Policy) This renderer process has either no Content Security Policy set or a policy with "unsafe-eval" enabled. This exposes users of this app to unnecessary security risks.

For more information and help, consult
https://electronjs.org/docs/tutorial/security.
This warning will not show up once the app is packaged.

[kittaakos]

We have to remove remote from the browser and implement the functionality in the electron-main (#7964):

Electron Deprecation Warning The 'remote' module is deprecated and will be disabled by default in a future version of Electron. To ensure a smooth upgrade and silence this warning, specify {enableRemoteModule: true} in the WebPreferences for this window

[tsmaeder]

Using context isolation would also enable secondary window support for electron, which is currently blocked by electron/electron#36858

[tsmaeder]

There are a couple of things we would need to do in order to make this happen:

1. Introduce a preload script: since extensions will have to contribute to this script, it needs to be a new generator target in the app generator and a new module kind preload.
2. Move all usages of ipcMain to the corresponding preload script and set up browser-side API for it. This is usually straightforward.
3. Replace all usages of the @electron/remote package with API set up in the a preload script
   Often, electron-remote is only used to access some property of the current window, like the zoom level. These are simple to replace. Generating the application menu seems the most complicated case. The secondary window support also uses some back-and-forth between the main and electron processes. That would need some rework.

If Monaco is capable of running in a context-isolated browser window, I would estimate the work to do at around 8 days. @paul-marechal since you seem to have looked into this in the past, would you concur?`

Also: VS Code seems to support sandboxing, see microsoft/vscode#156440

[tsmaeder]

While the initial setup of the preload script needs to be in one chunk, a lot of the work could be done incrementally.

[tsmaeder]

A quick hack to introduce context isolation in Theia (basically removing most uses of electron-remote) let me do the scenarios with closing secondary windows around 30-40 times without provoking the dreaded illegal access. I've been wrong before on this one, but I'm tempted to say we have a fix!
I'm going to work on introducing context isolation in the electron-browser case. This means no more nodejs integration and no more electron-remote. The workarounds are usually pretty straightforward, but there might be some disruption for adopters.