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

feat: add content security policy header #172

Merged
merged 3 commits into from
Aug 15, 2022

Conversation

vasco-santos
Copy link
Contributor

Based on proposal from @Gozala https://gozala.io/workspace/#/page/ipfs%20content-security-policy let's add Content-Security-Policy header to only allow requests within same origin.

This is added to the response from the gateway race before it is cached, so that responses from cache will also have the header already set.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Aug 12, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 05b00e9
Status: ✅  Deploy successful!
Preview URL: https://052ee5cb.nftstorage-link.pages.dev
Branch Preview URL: https://feat-add-content-security-po.nftstorage-link.pages.dev

View logs

@vasco-santos vasco-santos requested a review from Gozala August 12, 2022 10:30
packages/edge-gateway/src/gateway.js Outdated Show resolved Hide resolved
packages/edge-gateway/src/gateway.js Outdated Show resolved Hide resolved
@vasco-santos vasco-santos force-pushed the feat/add-content-security-policy-header branch from 0fcc485 to df29e91 Compare August 12, 2022 11:08
@vasco-santos vasco-santos requested a review from alanshaw August 12, 2022 12:04

clonedResponse.headers.set(
'content-security-policy',
"connect-src 'self'; script-src 'self'"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use default-src otherwise embedded content could be used to sidestep restrictions here e.g. embed image from URL passing secret as query param or part of the path, font, css etc....

Suggested change
"connect-src 'self'; script-src 'self'"
"default-src 'self'; script-src 'self'"
  1. We should utilize report-uri directive to collect data about what resources get blocked & inform our future decisions by it.
  2. I don't know how common it is, but I think there is no reason to not allow unsafe-inline (along with self) so that inline scripts, css will continue to work. Without it script tags and style tags will be ignored unless I'm mistaken.
  3. We may also want to enable unsafe-eval, I don't see a reason to block JS eval as some pages may be using them. On the other hand we can also not enable and see if they get reported.
  4. I also think data:* and blob:* URLs could be enabled as those can not be utilized for data smuggling.
  5. I think default-src does not cover navigato-to so we should restrict that as well, otherwise attacker can simply make a submit button be a link that takes you to adversaries site delivering stolen secret as query param or even a URL path fragment.
  6. It looks like default-src does not cover form-action either which again could be used to steal data via HTML forms.

I think if we add all the above we should cover all the possibilities, but then again CSPs are notoriously complicated so I think we should have at some tests to verify desired behavior.

Copy link
Contributor Author

@vasco-santos vasco-santos Aug 12, 2022

Choose a reason for hiding this comment

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

Thanks for all these details @Gozala ❤️

  1. Definitely, I will create an issue to get that part of Malware mitigation initiative given we need to add an endpoint, persist data and see how to act from there
  2. Agreed, adding that ✅
  3. it should be fine to also add, but yeah let's wait and see if issues are reported
  4. makes total sense thanks, didn't find these in mdn docs but now I see they are part of the spec ✅
  5. yeah, default-src follows only *-src. navigate-to looks experimental and not supported in any browser yet. I will add anyway as is part of the spec ✅
  6. ✅ like navigate-to, it only makes sense self in this scenario

I think if we add all the above we should cover all the possibilities, but then again CSPs are notoriously complicated so I think we should have at some tests to verify desired behavior.

Definitely, I want to try a few of the reported CIDs with this and have a look on the behaviour. Once we have it in staging, I will be able to test it. Later on, we can actually consider having real tests part of the test suite, but this will be quite tricky and time consuming for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added work track for report-uri at 2nd point in https://hackmd.io/@vasco-santos/rJWZsz-Cc

@vasco-santos vasco-santos requested a review from Gozala August 12, 2022 12:56
Copy link
Contributor

@Gozala Gozala left a comment

Choose a reason for hiding this comment

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

Looks good to me

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.

3 participants