[Security] Content-Security-Policy warnings with ember-electron

gossi commented 6 years ago

The recent releases of electron fixed a bunch of XSS vulnerabilities so I also updated my verison of electron-prebuilt-compile to 2.0.0. Which introduced a couple of security warnings, which I was able to fix, except one for CSP with ember-electron.

In the document about security for electron they mention a restricted policy for script tags. As suggested I installed the ember-cli-content-security-policy addon. It's great, it adds all the relevant live-reload related urls and stuff. By default it adds them as header when running ember s but fails adding this with ember electron and the serve protocol. It can be configured to use a <meta> element instead.

What I find out, regarding ember-electron. In order to make it work, you must set script-src to include 'unsafe-inline' because of the <script> element that is added to the body. Second is I had to add 'unsafe-eval' to script-src to make the actual ember code work in electron. I'm sceptical about this, whether I turned something wrong.

What I ended up with, was setting a CSP to a value that shouldn't be set as by the security guidelines of electron.

I would wish for some guidance on this topic, because we as developers are quick to lower the barriers to make things work but actually break security with that.

jacobq commented 6 years ago

It should be possible to run w/o needing to allow all of those "unsafe" things, but it depends a lot on your code and its dependencies. For example, I needed one because I use plotly.js, which currently needs this. BTW, this issue looks like it is closely related to https://github.com/felixrieseberg/ember-electron/issues/341

jacobq commented 6 years ago

@gossi Have you learned more about this? Do you have any suggestions for how we could improve the ember-electron documentation or code in this regard?

gossi commented 6 years ago

No I haven't really. If there is some dependency (or transient dependency) it's basically fishing for the unknown for it and it is out of the responsibility of ones code, not much we can do about it.

What I can say for sure is: ember-cli-hot-loader doesn't work with ember-electron most basically because it's eval'ing in the new javascript send in

jacobq commented 6 years ago

ember-cli-hot-loader doesn't work with ember-electron most basically because it's eval'ing in the new javascript sent in.

I assume you mean that it doesn't work when using strict CSPs, right? (It could "work" with lax / insecure settings.) I guess what I'm trying to figure out is what the next steps are for resolving this issue -- do we just need improved docs (with which I feel qualified to help) or is there something in the code?

gossi commented 6 years ago

It didn't work with lax settings for me, I enabled 'unsafe-eval' and it didn't work.

Regarding your original question, I have no idea what can be done code-wise to tackle this. For documentation I think these things help:

I can be, that even your code looks fine, some dependencies trigger security concerns for electron, e.g. plotly.js for you. It is very hard to find those libraries (if at all). But a warning, your deps have an impact on this
Do use ember-cli-content-security-policy addon to handle CSP in a nice way

Does that help?

jacobq commented 6 years ago

@gossi Thanks for your reply; that helps clarify a little bit, but I still don't quite understand what constitutes "working" and "not working" in this context (e.g. if the app runs correctly but the security warnings appear in dev is it "working" or "not working"?)

As for documentation, did you see the security FAQ page about this? It shows how to set up ember-cli-content-security-policy using the <meta> tag option and reasonable default settings.

Also, I'm not entirely sure what is specific to ember-electron here. If you have a restrictive CSP (e.g. no unsafe-eval or unsafe-inline) in any web or electron app won't you have this same problem? (I was asking Ben about something like this once before, which was when I learned about the ember-cli-content-security-policy addon.)

If I understand you correctly, these are some of the current pain points:

Offending dependencies are hard to track down
It is often not possible to fix upstream problems except by reducing security via unsafe-inline and unsafe-eval
Some ember tools that normally work, like ember-cli-hot-loader don't work with ember-electron apps (I haven't seen this for myself, but it sounds like that was your experience)

gossi commented 6 years ago

I set up e-csp correctly using <meta> anyway it won't work with electron 😄

I had to add 'unsafe-eval'. Whenever you do this, electron will put up the warning message in the console (should be added to the docs as well), so for some reasons, you have to.

For ember-cli-hot-loader (I used the first public version) that did break my app. After reading about, it evals new code and also it was proclaimed it's highly unlikely to work with CSP in place (I read that somewhere). Maybe that's something, that can be worked out.

Anyway you summary nails it :+1:

evoactivity commented 3 years ago

unsafe-eval is most likely down to using ember-auto-import, you can configure it to not use eval by passing the forbidEval options. https://github.com/ef4/ember-auto-import#customizing-build-behavior

My CSP ended up looking like this

// ember-cli-content-security-policy
contentSecurityPolicy: {
  "default-src": ["'none'"],
  "script-src": ["'self' 'unsafe-inline' http://localhost:7020"],
  "font-src": ["'self'"],
  "connect-src": ["'self'"],
  "img-src": ["'self'"],
  "style-src": ["'self'"],
  "media-src": ["'self'"],
},
contentSecurityPolicyMeta: true,

I had to manually add the http://localhost:7020 to get the live reload script to load.

RobbieTheWagner commented 3 years ago

@evoactivity this setup works to remove the warnings for you and you get a working app when you build for production?

evoactivity commented 3 years ago

I'll let you know in the next day or so, I'm still getting a new project setup and have not tried a production build just yet.

evoactivity commented 3 years ago

@rwwagner90 I've just tried ember electron:package and ran the resulting app, there were no warnings logged and the app ran as expected with the CSP settings outlined above and forbidEval set to true for ember-auto-import.

RobbieTheWagner commented 3 years ago

@evoactivity nice! Would you be open to submitting a PR adding this info to the docs?

RobbieTheWagner commented 3 years ago

@evoactivity does running ember t work for you? When I add in CSP, ember t seems to hang for me.

RobbieTheWagner commented 3 years ago

False alarm, I had to remove frame-src and then tests ran. However, ember electron:test still fails. I guess we need extra config for Electron tests?

RobbieTheWagner commented 3 years ago

@evoactivity can you confirm that ember electron:test hangs for you? My guess is we need something like http://testemserver/ in the CSP somewhere, so testem can connect, but I haven't been able to figure that out. @bendemboski any ideas?

bendemboski commented 3 years ago

Yeah, we load testem.js from http://testemserver/testem.js, and then I think it dynamically creates an iframe that points to http://testemserver/connection.html or something like that. The test tooling that we install in the main process intercepts them and 304-redirects them to http://localhost:<testem's port>/..., but the browser first seems them as http://testemserver/... URLs.

I don't use CSP myself, so I can't really give any more specific info.

evoactivity commented 3 years ago

Sorry for not replying sooner @rwwagner90, I've been away from my project using this for the last week or so, I should be back on it today and I'll let you know if I see the same behaviour or find a way around it.

RobbieTheWagner commented 3 years ago

@bendemboski so we probably need to allow both http://testemserver and http://localhost:<testem's port>. Is there any way to know what testems port is going to be or set it explicitly?

bendemboski commented 3 years ago

I think you can for all practical purposes count on the testem port being 7357. That's testem's default, and I don't think we have any tooling to override it except maybe testem-electron.js (although I'm not sure if you can even configure it there). It's complicated because ember-cli jumps through some hoops to support running the testem server and the Ember dev server run on the same port, but we don't have a dev server, so I forget exactly how all that fits together.

So I think you could get away with that. The only place I know of where we know the testem URL/port for sure (rather than assuming we're using testem's default port) is here, but that's running in the main process at runtime. Maybe if we rewrite tests/index.html dynamically at launch time (one of the things we discussed re: embroider support) then we could look for and rewrite the CSP meta tag.

So yeah, it's complicated™️ but you could probably get away with just assuming http://localhost:7357.

RobbieTheWagner commented 3 years ago

@bendemboski hmm, no luck with this:

contentSecurityPolicy: {
      'default-src': ["'none'"],
      'script-src': [
        'http://localhost:7020',
        'http://localhost:7357',
        'http://testemserver',
        "'self'",
        "'unsafe-inline'"
      ],
      'font-src': ["'self'"],
      'connect-src': ["'self'"],
      'img-src': ['data:', "'self'"],
      'style-src': ["'self'", "'unsafe-inline'"],
      'media-src': ["'self'"]
    },
    contentSecurityPolicyMeta: true,

bendemboski commented 3 years ago

I haven't CSP'ed in a while, but don't you see messages about the blocked content in the console, or can't you enable a warning mode or something to get the browser to tell what it's blocking and why?

RobbieTheWagner commented 3 years ago

@bendemboski I am not sure. I think typically, when running the code in the browser, you get something in the console about it, but not sure if the same is true for Electron. I think it may not know it's blocking anything, since it's only blocking testem stuff, and the part it is blocking is only giving control back to the terminal.

bendemboski commented 3 years ago

I'd be quite surprised if Electron suppressed that kind of console warning...if you run ember electron:test -s and then open the dev tools, do you see anything in the console?

RobbieTheWagner commented 3 years ago

@bendemboski yeah, I did that yesterday and got all the ones I could fixed. I think the only ones not fixable are the contentFor ones. There are a couple open issues about that and here is one of them https://github.com/rwjblue/ember-cli-content-security-policy/issues/149. I wonder if we should manually add in the script content via some blueprints instead of contentFor? Then we could add a nonce. Perhaps we could also add it via contentFor somehow, but that is unclear.

RobbieTheWagner commented 3 years ago

With this PR https://github.com/adopted-ember-addons/ember-electron/pull/695 everything works with this config:

contentSecurityPolicy: {
      'default-src': ["'none'"],
      'script-src': [
        'http://localhost:7020',
        'http://localhost:7357',
        'http://testemserver',
        "'nonce-ember-electron-shim-head'",
        "'nonce-ember-electron-shim-footer'",
        "'nonce-ember-electron-shim-test-head'",
        "'self'",
        "'unsafe-inline'"
      ],
      'font-src': ["'self'"],
      'frame-src': ['http://localhost:7357', 'http://testemserver/', "'self'"],
      'connect-src': ["'self'"],
      'img-src': ['data:', "'self'"],
      'style-src': ["'self'", "'unsafe-inline'"],
      'media-src': ["'self'"]
    },
    contentSecurityPolicyMeta: true,

jelhan commented 3 years ago

Ember CLI Content Security Policy should inject the source required to run tests with testem and to support Ember CLI live reload automatically. I'm wondering why this is not working for Ember Electron apps. Is Ember Electron doing something special in regards to this two features?

You can find tests for both here:

Did you tested with latest pre-release of Ember CLI Content Security Policy? This is currently v2.0.0-2. The latest stable release (v1.1.1) is very old and had way less test coverage.

Sadly there is no good work-a-round for scripts injected by an addon in a contentFor hook yet. I discussed this here in detail: https://github.com/rwjblue/ember-cli-content-security-policy/issues/149#issuecomment-764678261

RobbieTheWagner commented 3 years ago

@jelhan yeah, we have some hacks to make testem work. @bendemboski knows more than I do, but essentially since electron doesn't run a server and runs from file urls, we are not able to use the normal testem setup.

jacobq commented 2 years ago

In case anyone else stumbles upon this nowadays, I want to make a note that ember-cli-content-security-policy now uses config/content-security-policy.js rather than the environment for its configuration.

https://github.com/rwjblue/ember-cli-content-security-policy/blob/8f2bb08841c39973295d6129981f9fa07f3d8028/DEPRECATIONS.md

adopted-ember-addons / ember-electron

[Security] Content-Security-Policy warnings with ember-electron #349