Skip to content
This repository has been archived by the owner on Feb 26, 2020. It is now read-only.

Implement Electron security guidelines #129

Merged
merged 13 commits into from
Jun 8, 2018
Merged

Implement Electron security guidelines #129

merged 13 commits into from
Jun 8, 2018

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented May 31, 2018

Electron has a 12-point checklist called "Security, Native Capabilities, and Your Responsibility". Parity UI should follow those guidelines.

  • Only load secure content
  • Disable the Node.js integration in all renderers that display remote content
  • Enable context isolation in all renderers that display remote content.
  • Use ses.setPermissionRequestHandler() in all sessions that load remote content
  • Do not disable webSecurity
  • Define a Content-Security-Policy and use restrictive rules (i.e. script-src 'self').
  • Override and disable eval, which allows strings to be executed as code. Not enabling this one as Web3 Console dapp needs eval(). However all networks dapps disallow eval() in their CSP, so we actually don't need this.
  • Do not set allowRunningInsecureContent to true
  • Do not enable experimental features
  • Do not use blinkFeatures
  • WebViews: Do not use allowpopups
  • WebViews: Verify the options and params of all tags

Also removing the proxy to /parity-utils, so that the only inject.js the dapps are receiving now is the one injected by Electron.

@amaury1093 amaury1093 added A3-inprogress Pull request is in progress. No review needed at this stage. F1-security The client fails to follow expected, security-sensitive, behaviour. B0-patchthis Pull request should also be back-ported to the beta/stable branch(es). labels May 31, 2018
@amaury1093
Copy link
Contributor Author

amaury1093 commented Jun 1, 2018

"3. Enable context isolation in all renderers that display remote content" needs some explanation (for future reference).

TL;DR There are some gymnastics to do in order to have Parity UI follow all these guidelines.

Without contextIsolation (i.e. before this PR), I did:

// In inject.js
window.ethereum = ...;

// In dapp
console.log(window.ethereum); // Will print something

With contextIsolation, we now have:

// In inject.js
window.ethereum = ...;

// In dapp
console.log(window.ethereum); // undefined, because preload and dapp do not share context
// Also, dapp cannot send IPC message to shell anymore (due to no nodeintegration)
  1. How to inject window.ethereum in the dapp?
  2. How to make dapp and shell communicate? IPC and postMessage don't work between those two.

Solution

  1. Easy, a preload script can call webFrame.executeJavaScript(...). So instead of injecting directly inject.js (which wouldn't work because of above), we inject preload.js, which will do webFrame.executeJavaScript(/* the content of inject.js as a huge string*/). Hence the introduction of preload.js.

  2. After asking here the solution seems to be having preload.js as a proxy:

Electron <---IPC---> preload.js <---postMessage---> dapp

Because the shell can't communicate with the dapp via postMessage nor IPC. But the shell can communicate with preload.js via IPC, and the dapp can communicate with preload.js via postMessage. So preload.js acts as a message relayer.

Copy link
Contributor

@jacogr jacogr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks sane, it feels sane when running - however worried that there may be edge-cases I have not seen with "doing some dapp stuff" testing.

It would be good to get a bit wider testing with binaries.

// https://electronjs.org/docs/tutorial/security#4-handle-session-permission-requests-from-remote-content
session.defaultSession
.setPermissionRequestHandler((webContents, permission, callback) => {
if (!url.startsWith('file:')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

url refers to the library here. You forgot: const url = webContents.getURL()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F*ck... Nice catch, thanks!

});

// Load inject.js as a string, and inject it into the webview with executeJavaScript
fs.readFile(path.join(remote.getGlobal('dirName'), '..', '.build', 'inject.js'), 'utf8', (err, injectFile) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use getBuildPath() from src/util/host.js here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not, I was getting a window.require is not a function.

Reason is: in preload.js, isElectron() is true, however it's not a renderer process so you would do require('electron') instead of window.require('electron')

Copy link
Contributor

@axelchalon axelchalon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be working fine! HoweverParity/Web3 console dapp (tried on kovan) doesn't seem to work anymore: Error: Sorry, this app does not support window.eval().

@amaury1093 amaury1093 added A0-pleasereview Pull request needs code review. and removed A3-inprogress Pull request is in progress. No review needed at this stage. labels Jun 4, 2018
@amaury1093 amaury1093 merged commit 31d6e86 into master Jun 8, 2018
@amaury1093 amaury1093 deleted the am-security branch June 8, 2018 11:37
This was referenced Jun 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview Pull request needs code review. B0-patchthis Pull request should also be back-ported to the beta/stable branch(es). F1-security The client fails to follow expected, security-sensitive, behaviour.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants