Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a strict Content-Security-Policy (CSP) #1197

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from
Open

Conversation

maudnals
Copy link

@maudnals maudnals commented Dec 15, 2021

Leverage the strict-csp npm package to:

  • Generate a strict CSP
  • Set it as a HTML meta tag in the Squoosh page.

Try it out

  • (Re- npm i)
  • npm run dev
  • Observe that a strict, hash-based CSP has been added as a meta tag.

image

This code uses strict-csp. What is it?
See strict-csp.

⚠️ Please note
One of the goals of this is to experiment with strict-CSP and see whether it can cause any issue in a real application.
Yes, this means that Squoosh is used as a guinea-pig web application for this library⏤if that's not something desirable anymore because usage increased since that idea was first discussed, please let's talk!

@maudnals maudnals requested a review from surma December 16, 2021 14:41
@maudnals maudnals marked this pull request as ready for review December 16, 2021 14:41
@maudnals
Copy link
Author

maudnals commented Dec 16, 2021

@surma This is ready for your review!
Could we also please add @lweichselbaum to this repo, so he can be requested as a reviewer on this PR, too? Thanks!

@surma
Copy link
Collaborator

surma commented Dec 16, 2021

The initial load seems to work fine, but once you click an image, the app breaks:

Screen Shot 2021-12-16 at 15 57 07

I am pretty sure that we actually have some fun new Function() stuff in the JS code generated by Emscripten, so that might be something we’ll have to allow and should probably work with the Emscripten folks (cc @kripken @sbc100 @RReverser) to remove from Emscripten in the long run.

@maudnals Can we allow eval-like stuff in workers only somehow?

@RReverser
Copy link
Contributor

I am pretty sure that we actually have some fun new Function() stuff in the JS code generated by Emscripten, so that might be something we’ll have to allow and should probably work with the Emscripten folks (cc @kripken @sbc100 @RReverser) to remove from Emscripten in the long run.

That should be possible to disable via -s DYNAMIC_EXECUTION=0. I think new Function codepaths are used only as an optimization in e.g. Embind.

@maudnals
Copy link
Author

maudnals commented Dec 22, 2021

Thanks everyone!

@RReverser Are you suggesting to set DYNAMIC_EXECUTION to 0? Shall we make it a separate PR? Are there any other impacts of doing this? Also, I found a bunch of places in Squoosh that seem to set emscripten flags but I'm not versed into this, would you maybe have a pointer on what file should be edited here (or maybe you'd rather look into this yourself?)?

@maudnals Can we allow eval-like stuff in workers only somehow?

Not that I know of, I'll check in with @lweichselbaum.

One alternative here in case the emscripten flag change isn't viable would be to add unsafe-eval to the policy, which does make it less secure but also mitigates the issue (plus, a suboptimal CSP is more secure than no CSP!).

@surma
Copy link
Collaborator

surma commented Dec 22, 2021

One alternative here in case the emscripten flag change isn't viable would be to add unsafe-eval to the policy,

Happy to start with this to land this PR and then explore @RReverser’s suggestion + removing unsafe-eval in a new PR. WDYT?

@maudnals
Copy link
Author

maudnals commented Dec 23, 2021

@surma Sounds good to me!

I've just pushed that change (unsafe-eval) to the PR.
Now, no error is thrown upon clicking an image; the app works properly.

const strictCsp = StrictCsp.getStrictCsp(scriptHashes, false, true);
// enableTrustedTypes: false, enableBrowserFallbacks: true
// enableUnsafeEval: true, to accomodate for uses of eval by emscripten. Enabling eval makes the CSP a bit less secure
const strictCsp = StrictCsp.getStrictCsp(scriptHashes, false, true, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a drive-by: standalone boolean parameters are a bit of an API smell as they are not self-explanatory.

If you can, I’d recommend refactoring it into an options object like so

const strictCsp = StrictCsp.getStrictCsp(scriptHashes, {
  enabledTrustedTypes: false, 
  enableBrowserFallbacks: true, 
  enableUnsafeEval: true
});

Copy link
Author

@maudnals maudnals Jan 10, 2022

Choose a reason for hiding this comment

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

yes thank you, the options object is way better. Done!
(the strict-csp package is also republished with this change)

@maudnals
Copy link
Author

maudnals commented Feb 2, 2022

Hi folks, is this something you'd want to merge?

@RReverser
Copy link
Contributor

Hi folks, is this something you'd want to merge?

Surma was the main reviewer on this PR, but he's no longer with us, so not sure how to proceed. @surma do you still want to do reviews on this repo?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants