Skip to content
This repository was archived by the owner on Jul 29, 2022. It is now read-only.

Improve CSP#303

Merged
pmespresso merged 2 commits into
yj-securityfrom
am-security
Apr 30, 2019
Merged

Improve CSP#303
pmespresso merged 2 commits into
yj-securityfrom
am-security

Conversation

@amaury1093
Copy link
Copy Markdown
Contributor

@amaury1093 amaury1093 commented Apr 29, 2019

I read some more stuff about CSP:

  • onHeadersReceived doesn't work in prod mode with file://. This means that we need to set a <meta> tag for CSP in index.html, which I did. Edit: Created an issue on Fether too, it's a big security risk there.
  • removed unsafe-eval everywhere, it's actually not needed by CRA. However, now we cannot run wasm code, so it'll default to the JS one. We should wait for the wasm-eval CSP (maybe in Electron 5?)
  • tightened CSP where I could.

After testing, these CSP work in both prod and dev

<meta http-equiv="Content-Security-Policy" content="
block-all-mixed-content;
child-src 'none';
connect-src http: ws:;
Copy link
Copy Markdown
Contributor Author

@amaury1093 amaury1093 Apr 29, 2019

Choose a reason for hiding this comment

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

The http here is not needed in prod, and should be overwritten in the onHeadersReceived in dev. However, somehow it doesn't work: https://stackoverflow.com/questions/51969512/define-csp-http-header-in-electron-app#answer-52243138

So we can:

  • either add http in prod, which I believe doesn't introduce a large attack surface, and allow CRA hot reloading
  • or remove http in prod, but then CRA hot reloading breaks.

@amaury1093
Copy link
Copy Markdown
Contributor Author

@yjkimjunior I would like you to try yarn start and yarn package to make sure everything works. Also, in the binary, there shouldn't be any electron warning messages (confirmed working on my machine)

Copy link
Copy Markdown
Contributor

@pmespresso pmespresso left a comment

Choose a reason for hiding this comment

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

@amaurymartiny confirming this works on my machine as well.

Once we bundle the light client though, wouldn't we need unsafe-eval? Especially as it looks like the wasm-eval/wasm-unsafe-eval proposal for Chromium has been very slowly moving toward implementation....maybe.

anderejd/electron-wasm-rust-example#6
WebAssembly/content-security-policy#7

@amaury1093
Copy link
Copy Markdown
Contributor Author

Once we bundle the light client though, wouldn't we need unsafe-eval?

No, we bundle the light client inside Electron, and spawn it from the electron process, there's no wasm involved. The day we'll have a light client in wasm (which realistically won't happen in the next 6 months imo), we'll need to enable unsafe-eval

@pmespresso pmespresso merged commit 7d1fc1c into yj-security Apr 30, 2019
@pmespresso pmespresso deleted the am-security branch April 30, 2019 09:55
amaury1093 pushed a commit that referenced this pull request Apr 30, 2019
* init

* update with needed permissions

* update with needed permissions

* nbs

* checklist cheatsheet

* set web preferences

* lint

* feat(csp):use the csp from parity-js shell and fether

* fix: set csp on headers received

* fix lint

* fix: font src csp, comment out webview event

* Improve CSP (#303)

* fix(csp): Make CSP work

* fix: Remove unsafe-eval
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants