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?