CSP improvement opportunities

[Nantris]

I came across this issue just now which makes a good point about nonce reuse due to it being defined at build time.

I also wonder if it wouldn't be better to set a CSP from the main process to ensure it will apply to any newly created windows? Doing it from within Electron would allow for setting nonce at runtime rather than build time, and additionally would allow for users to access the nonce for packages they use which need the value, like Emotion (and I assume Styled-Components.)

   session.defaultSession.webRequest.onHeadersReceived((details, callback) => {
     details.responseHeaders['Content-Security-Policy'] = cspString;
     callback({ responseHeaders: details.responseHeaders });
   });

[Nantris]

Hm... Not sure how we'd set the nonce on the injected scripts/styles though...

[reZach]

Hm... Not sure how we'd set the nonce on the injected scripts/styles though...

Yes, I'm not sure how to handle that one either yet.

[Nantris]

@reZach have you played with __webpack_nonce__ at all? It's theoretically the solution, but I wasn't able to get it working.

https://webpack.js.org/guides/csp/

[reZach]

@reZach have you played with __webpack_nonce__ at all? It's theoretically the solution, but I wasn't able to get it working.

https://webpack.js.org/guides/csp/

I have not yet. Can you give me an example on what you are thinking a dynamic CSP would be used for (a use case)? Or a small demo MVP would be helpful to toy around with this (I'm assuming you have one based on your earlier comments).

[Nantris]

@reZach I don't have any demo unfortunately since I scrapped all the non-working attempts.

The good news though is that nonce may be entirely redundant for Electron environments. It seems like hashes can be used instead of nonce with no degradation in security: slackhq/csp-html-webpack-plugin#82 (comment)

Though it couldn't hurt to keep the static nonce configuration used now.

But that said, I'm not sure how to integrate hashes with the Electron session-level CSP configuration - which I think should probably be our aim based on my most up to date understanding.

[reZach]

@slapbox isn't the CSP configuration going to depend on the dependencies loaded into this template, like Emotion or styled components? I don't feel it's the responsibility of this template to account for everything, but I'm not opposed to setting defaults that all applications could use.

How do you feel about this comment? (I'm still struggling to understand a good default that would work in all cases; leading to my hesitation).

[Nantris]

Disclaimer: I'm no expert; thinking out loud ahead.

---

I suppose specific projects may make need nonce as opposed to hashes, and if users do then they would theoretically be able to integrate that value into Emotion/Styled Components IF we could get __webpack_nonce__ working.

I think that hashes are probably sufficient unless users are loading content from external sources where they might not be able to get a hash value for the resource.

The thing I'd really like to find a way to address is to implement a solid CSP at the session level like the code in the first comment. But in that case then hashes aren't going to work...

I struggle to think of a way to cover all the bases at once here. I say that not only in terms of covering all the bases in the template, but even with heavy customization I can't conceive of a way; although if we got __webpack_nonce__ working then maybe.

I think that would provide the best security and flexibility, but I think we'd also need to rotate nonce values so that each new window would receive a fresh nonce. Or maybe even each page load?

Our app is a SPA that runs locally without loading external scripts or styles. So how I'm envisioning this working might not even be applicable to certain other use cases I'm not thinking of.

---

TLDR:

At the end of the day though, no sense in rushing headlong into this. Better to measure twice and cut once. Let's keep discussing.

I think the defaults now are reasonable and hashes provide decent security. If a user can't use hashes though, like if they're loading external resources, then the nonce value is going to be the same every run since it's defined at build time and that's not great.

[Nantris]

@reZach I'm thinking we should implement CSP in both the headers, via Electron's session and also via the HtmlWebpackPlugin.

It looks like it could only improve the security: https://stackoverflow.com/a/51153816/6025788

---

Also I believe we can shorten these lines not related to CSP, but still related to session.

  const ses = session;
  const partition = "default";
  ses.fromPartition(partition)
    .setPermissionRequestHandler(...)

I believe we can just use:

session.defaultSession.setPermissionRequestHandler(...)

[Nantris]

@reZach hashes don't work at all so we're back at square one.

slackhq/csp-html-webpack-plugin#35 (comment)

[reZach]

@slapbox it looks like my busyness and lack of response here led us back to square one.

[Nantris]

Ah it's definitely not on you, this is a tough problem.

I see two potential approaches at this point:

1. Wait for the CSP plugin to support automatic hashing (this seems like our best bet, but puts things entirely outside of our control)
2. Use the generated html files as templates, and then interpolate the __webpack_nonce__. It seems like the __webpack_nonce__ is meant more for stuff like server side rendering and I don't think there's any way we can take advantage of it without templating. But templating seems like major overkill and like it could add new security vulnerabilities somewhere.

[reZach]

I'm thinking option 1 makes the most sense here, @slapbox.

[TStod]

I have landed at the same unbreakable wall here where option 1 makes the most sense. Is there any way to expedite the feature request in the CSP plugin that you can think of? Thanks again for doing all this digging. in my searches I have found your questions and insight in countless issues @slapbox

[Nantris]

Thanks for the kind words @TStod. Unfortunately I'm not sure how we could get the issue expedited. It seems like the CSP plugin is mostly tailored to the Slack team's use cases.

I know you already found this link, but I'll share it for anyone else who wants to try to make some more noise around the issue slackhq/csp-html-webpack-plugin#50

---

@reZach I agree that trying to get this addressed in the CSP plugin is our best bet. Maybe go give that issue a thumbs up, for what those are worth. The issue has been open for two years though so I'm thinking it's unlikely to suddenly move forward, but we can try to get the ball rolling again.

[reZach]

@slapbox Upvoted the issue, hopefully the issue can start picking up steam.

[reZach]

@slapbox the issue is being marked as In Development now, maybe we'll get a change soon!

[Nantris]

I saw and I'm hoping! Code review was initiated on the associated PR.

It seems like the team is leaning away from nonce due to Webpack's limitations, so that would definitely require adding support for hashing.

[reZach]

@slapbox the PR was closed, I briefly read it - did you have a recommendation for moving forward with this one?

[Nantris]

Sorry for the lengthy delay @reZach - I was leaving this unread until I could get to it, but somehow it ended up read.

The dynamic nonce approach I mentioned doesn't seem worth the extra effort that resource hashes might have been worth.

If someone does have the bandwidth to complete that PR to satisfy the requested changes, that would probably be best - but I'm not 100% sure that that would result in the PR being merged so it's hard to feel like it's worth the effort (for us at least) compared with other objectives we're working to complete.

Unless someone does undertake completing that PR, I'm in the "wait and see" boat for this one. It would be a nice security improvement, but it's not like security is completely broken without it.