Add new APIs to customize content security policy for non-HTML responses #39398

imtayadeway · 2020-05-22T23:26:50Z

Summary

One feature of the content security policy DSL, though undocumented,
is that it will not generate headers for non-HTML responses, even if a
configuration is explicitly provided. While it may not seem obvious
that anyone would want to send this header in an API response, Mozilla
Observatory, for instance, recommends the following for API responses:

Content-Security-Policy: default-src 'none'; frame-ancestors 'none'

(source: https://observatory.mozilla.org/faq/)

The Secure Headers gem also makes recommendations about the content
security policy for API responses: https://github.com/github/secure_headers#api-configurations

As such, this removes the HTML guard clause from the
ContentSecurityPolicy middleware.

Other Information

I imagine that it may be preferable to gate this behavior, assuming that the change is agreeable, but kindly request feedback before getting too deeply involved with that.

p8 · 2020-06-15T14:15:48Z

It's possible to downgrade CSP headers by loading non-html resources with less strict CSP headers. So I think this would be an improvement.

imtayadeway · 2020-06-15T21:39:57Z

@pixeltrix are you able to review at this time? No worries if not

actionpack/CHANGELOG.md

One feature of the content security policy DSL, though undocumented, is that it will not generate headers for non-HTML responses, even if a configuration is explicitly provided. While it may not seem obvious that anyone would want to send this header in an API response, Mozilla Observatory, for instance, recommends the following for API responses: `Content-Security-Policy: default-src 'none'; frame-ancestors 'none'` (source: https://observatory.mozilla.org/faq/) The Secure Headers gem also makes recommendations about the content security policy for API responses: https://github.com/github/secure_headers#api-configurations As such, this removes the HTML guard clause from the `ContentSecurityPolicy` middleware.

jeremy

Big +1 to making non-HTML CSP possible.

Think we should go further with this, even, and provide a strong default non-HTML CSP for all apps. This isn't something to fine-tune for each app; nobody should have to research & configure this when there's a clear default we can provide.

Content-Security-Policy: default-src 'none'; frame-ancestors 'none'

This is a great app-wide default that for all newly-generated apps, with a config/initializers/new_framework_defaults.rb opt-in for existing/upgrading apps.

We could do this with new top-level config, e.g.

Rails.application.config.api_content_security_policy do |policy|
  # …

Rails.application.config.api_content_security_policy_report_only = false

Using "API" in this way may be misleading, though. It's really all non-HTML responses. But that's a bit of a mouthful:

Rails.application.config.non_html_content_security_policy do |policy|

In any case, we'd update the middleware to still check for HTML responses and use the main CSP with a fallback to the non-HTML CSP, if present, for other responses.

jeremy · 2020-06-16T18:31:33Z

actionpack/CHANGELOG.md

@@ -1,3 +1,6 @@
+*   Allow Content Security Policy DSL to generate for API responses.
+    *Tim Wade*


Let's describe this a bit more. The summary in the pull request is great! We'll now send CSP headers with all responses, not just HTML, to protect API responses that could be loaded in a browser.

pixeltrix · 2020-06-18T08:29:28Z

Sorry, it was a bad assumption on my part that it only applies to HTML requests but looking at the where the policy applies in the spec it doesn't mention API requests. We should definitely remove the limitation but wondering what the justification is for the Mozilla Observatory decision is?

p8 · 2020-06-18T08:42:41Z

This is a pretty good article on downgrading CSP headers by including json endpoint in a frame.
https://lab.wallarm.com/how-to-trick-csp-in-letting-you-run-whatever-you-want-73cb5ff428aa/
Because the json is rendered by the browser as html you could add script tags for external JS files.

pixeltrix · 2020-06-18T08:57:24Z

@p8 thanks for the link - according to that article we also need to be adding it to error pages as well so will need to give this some thought.

pixeltrix · 2020-06-18T12:15:41Z

@jeremy a global non-HTML CSP may be a bit of a broad brush approach - the frame-ancestors directive applies to a range or embedded contexts, not just <frame> and <iframe>, e.g. <object> and <embed> would be affected as well. Put together with a default-src 'none' it could prevent embedded objects from working.

Maybe we could use a similar pattern to respond_to to control the CSP?, e.g.

Rails.application.config.content_security_policy do |format|
  format.html do |policy|
    policy.default_src :self, :https
    policy.font_src    :self, :https, :data
    policy.img_src     :self, :https, :data
    policy.object_src  :none
    policy.script_src  :self, :https
    policy.style_src   :self, :https
    
    # The frame-ancestors directive obsoletes the X-Frame-Options header
    policy.frame_ancestors :self
  end
  
  # Allow PDFs to be embedded in the page
  format.pdf do |policy|
    policy.default_src :self
    policy.frame_ancestors :self
  end
  
  # Prevent any other type of request from being used in an
  # embedded context, e.g. <frame>, <iframe>, <object> or <embed>
  format.any do |policy|
    policy.default_src :none
    policy.frame_ancestors :none
  end
end

The yielded object would apply any of the directives to the html format to maintain backwards compatibility and wouldn't start adding CSPs to non-HTML requests unless updated to the new format. e.g. this:

Rails.application.config.content_security_policy do |policy|
  policy.default_src :self, :https
  policy.font_src    :self, :https, :data
  policy.img_src     :self, :https, :data
  policy.object_src  :none
  policy.script_src  :self, :https
  policy.style_src   :self, :https
end

would be equivalent to this:

Rails.application.config.content_security_policy do |format|
  format.html do |policy|
    policy.default_src :self, :https
    policy.font_src    :self, :https, :data
    policy.img_src     :self, :https, :data
    policy.object_src  :none
    policy.script_src  :self, :https
    policy.style_src   :self, :https
  end
end

To make developers upgrading aware of this change we can log a warning for requests in development mode where there isn't a CSP set and allow that to be switched off via a config option.

Then there's the issue of the public exceptions middleware - it's higher up the stack than the CSP middleware so won't pickup anything from it. It may be possible to move the CSP middleware higher up but that could affect the controller-level stuff so would need some investigation. An easy alternative is a simple config option to the public exceptions middleware to add a CSP header. We'd also probably want some documentation in the security guide about adding a CSP header to static error pages delivered by Apache/nginx or a CDN such as custom error pages for AWS CloudFront.

One other thing that'll need changing is this conditional so that API only apps load the CSP middleware:

rails/railties/lib/rails/application/default_middleware_stack.rb

Lines 69 to 72 in 664e949

    
           unless config.api_only 
        
             middleware.use ::ActionDispatch::ContentSecurityPolicy::Middleware 
        
             middleware.use ::ActionDispatch::FeaturePolicy::Middleware 
        
           end

That also reveals another question - does a similar problem exist with feature policies? Can loading an iframe without a feature policy allow bypassing of the policy on the parent browsing context?

pixeltrix · 2020-06-19T06:20:18Z

One other point - probably need to investigate whether this could affect Active Storage uploads as well since an attacker could upload a document or find an already uploaded document and load it into an iframe.

jeremy · 2020-06-22T14:58:31Z

Maybe we could use a similar pattern to respond_to to control the CSP?, e.g.

Dig that! Particularly that existing policies remain the same, implying HTML, with format.whatever overrides following.

may be possible to move the CSP middleware higher up but that could affect the controller-level stuff so would need some investigation
some documentation in the security guide about adding a CSP header to static error pages delivered by Apache/nginx or a CDN such as custom error pages for AWS CloudFront

+1 to move the middleware above any file service. That'd include AD::Static for folks serving assets and error pages from Rails.

does a similar problem exist with feature policies?

No idea! @imtayadeway @p8 do you know?

need to investigate whether this could affect Active Storage uploads as well since an attacker could upload a document or find an already uploaded document and load it into an iframe

Great point. Think this is likely a separate concern, however, since AS blobs are served by redirect to external service URLs, putting them at arms' length from the app origin. /cc @georgeclaghorn

imtayadeway · 2020-06-22T16:38:15Z

@jeremy @pixeltrix thanks for your thoughtful responses. As it seems you've built a consensus around design, I'm happy to start work on this week.

does a similar problem exist with feature policies?

I also have no idea, but I'll be sure to research this and get back to you.

p8 · 2020-07-01T13:34:45Z

@jeremy I guess the same thing applies to feature policies but it adds to the CSP hack.

If you can inject an iframe with a non-HTML response, and add a link to an external script, (ie what this PR would prevent), you could inject a script to request access to the camera, if the feature policy headers aren't defined for the non-HTML response.

imtayadeway · 2020-07-22T21:30:49Z

@jeremy @pixeltrix thanks for your patience while I've been working on this.

I don't believe I'm quite there yet (I'll certainly need to update documentation, update new defaults), but wondered if I could get some feedback on what I have so far?

It may be possible to move the CSP middleware higher up but that could affect the controller-level stuff so would need some investigation.

This seemed to work, but is there anything I should check/may have missed?

That also reveals another question - does a similar problem exist with feature policies? Can loading an iframe without a feature policy allow bypassing of the policy on the parent browsing context?

Would y'all be OK with my looking into this as a follow up?

rails-bot · 2020-10-20T22:01:03Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

imtayadeway · 2020-10-21T00:24:26Z

@jeremy hi! I've been looking into updating documentation on my branch, and I'd like to do some more testing around the public exceptions middleware, but wondered if you had any feedback on how this is going at this stage? Thanks!

rafaelfranca · 2020-10-21T05:25:18Z

actionpack/lib/action_dispatch/http/content_security_policy.rb

-          headers[header_name(request)] = policy.build(context, nonce, nonce_directives)
+          headers[header_name(request)] = policy.build(context, nonce, nonce_directives, headers[CONTENT_TYPE])
+        else
+          if (Rails.env.development? || Rails.env.test?) && logger(request) && @warn_on_no_content_security_policy


Checks for environment can't enter the action pack code. If we want different behavior per environment this should be a config on this class.

rafaelfranca · 2020-10-21T05:26:51Z

actionpack/lib/action_dispatch/http/content_security_policy.rb

-          @directives[directive] = apply_mappings(sources)
-        else
-          @directives.delete(directive)
+    (Mime::SET.map(&:to_sym) + [:any]).each do |type|


Do we need to generate methods for all the formats? I think we should only generate for the most common ones (html, js, json, css, any) and let the other formats be specified in another method like formats.any(:xml)

rafaelfranca · 2020-10-21T05:28:29Z

railties/lib/rails/application/default_middleware_stack.rb

@@ -46,6 +46,8 @@ def build_stack
          middleware.use ::ActionDispatch::RequestId
          middleware.use ::ActionDispatch::RemoteIp, config.action_dispatch.ip_spoofing_check, config.action_dispatch.trusted_proxies

+          middleware.use ::ActionDispatch::ContentSecurityPolicy::Middleware, config.warn_on_no_content_security_policy


You are changing the position of this middleware to be earlier in the stack before the Rack::Logger. I don't think we should do that. What we want is to move before the public exceptions middleware so this should be probably after the Rack::Logger.

rafaelfranca · 2020-10-21T05:29:53Z

actionpack/lib/action_dispatch/http/content_security_policy.rb

@@ -48,6 +46,10 @@ def header_name(request)
        def policy_present?(headers)
          headers[POLICY] || headers[POLICY_REPORT_ONLY]
        end
+
+        def logger(request)


What is the reason for this method?

rails-bot · 2021-04-14T20:24:52Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

jeremy · 2021-09-17T16:45:24Z

Keeping this alive. Still desirable, and necessary, since non-HTML content types may execute JavaScript.

imtayadeway · 2021-09-17T17:29:49Z

Hi @jeremy ! Apologies - life really got in the way of this one. I'm happy to revisit this soon if it's still desirable

oreoshake · 2021-09-17T17:30:20Z

actionpack/test/dispatch/content_security_policy_test.rb

+    content_security_policy only: :api do |format|
+      format.json do |p|
+        p.default_src :none
+        p.frame_ancestors :none


I suggest including sandbox in the default policy for API requests as well. It's a few more bytes and provides a little more protection against hypothetical situations.

jeremy · 2021-09-21T17:44:01Z

Cool @imtayadeway!

Think it's worth catching up with modern "strict CSP" as well (given that allowlist bypasses are common) since that could affect API design decisions here.

/cc @pixeltrix

tenderlove · 2022-02-01T21:20:38Z

As a short term solution could we just send the CSP for all responses? I'm not sure what the downsides are for just always sending a CSP.

imtayadeway · 2022-02-02T19:51:47Z

@tenderlove I could pull 05344fc into a separate PR if you think that would suffice?

Apologies as I've not been able to get back to this any sooner, I've just been swamped with personal stuff. But looking to get this branch updated and responding to feedback soon 💜

tenderlove · 2022-02-03T00:56:36Z

@tenderlove I could pull 05344fc into a separate PR if you think that would suffice?

Ya, I think it would be great! Please do it 🙇🏻‍♀️

Apologies as I've not been able to get back to this any sooner, I've just been swamped with personal stuff. But looking to get this branch updated and responding to feedback soon

No need to apologize, thank you for the work you've done so far!! ❤️

imtayadeway · 2022-03-08T00:29:51Z

@tenderlove thanks! I pulled that out into #44635. Hoping to get back to this soon though and thank you for your continued patience!

tenderlove · 2022-03-08T21:23:40Z

@tenderlove thanks! I pulled that out into #44635. Hoping to get back to this soon though and thank you for your continued patience!

Great, thanks. I merged it!

rails-bot bot added the actionpack label May 22, 2020

kaspth requested a review from pixeltrix May 23, 2020 07:00

imtayadeway closed this Jun 15, 2020

imtayadeway reopened this Jun 15, 2020

imtayadeway force-pushed the tjw/api-csp branch from d8371b7 to 58b0bc0 Compare June 15, 2020 21:23

rafaelfranca reviewed Jun 16, 2020

View reviewed changes

actionpack/CHANGELOG.md Show resolved Hide resolved

imtayadeway force-pushed the tjw/api-csp branch from 58b0bc0 to 05344fc Compare June 16, 2020 17:55

jeremy reviewed Jun 16, 2020

View reviewed changes

imtayadeway added 8 commits July 1, 2020 17:07

[wip] failing test for html

b840a7d

Add content_security_policy.html

2aa6585

Add some tests for JSON, slime it for now

2ea33e8

Approximating a solution

12a0048

Parse the content_type

0731722

Some metaprogramming

418fa0b

Refactory

0105939

...and we're green

9a7c513

pixeltrix mentioned this pull request Jul 9, 2020

Add an HTTP Content-Security-Policy alphagov/govuk-coronavirus-vulnerable-people-form#444

Merged

imtayadeway added 2 commits July 9, 2020 10:29

sort n merge

d14792f

Warn on no policy

8f54f18

rails-bot bot added the railties label Jul 22, 2020

Fix default middleware tests

88c02f9

rails-bot bot added the stale label Oct 20, 2020

rails-bot bot removed the stale label Oct 21, 2020

rafaelfranca reviewed Oct 21, 2020

View reviewed changes

Base automatically changed from master to main January 14, 2021 17:01

rails-bot bot added the stale label Apr 14, 2021

rails-bot bot closed this Apr 21, 2021

jeremy reopened this Sep 17, 2021

rails-bot bot removed the stale label Sep 17, 2021

ghiculescu added the pinned label Sep 17, 2021

oreoshake reviewed Sep 17, 2021

View reviewed changes

imtayadeway mentioned this pull request Mar 8, 2022

Generate content security policy for non-HTML responses #44635

Merged

imtayadeway changed the title ~~Generate content security policy for non-HTML responses~~ Add new APIs to customize content security policy for non-HTML responses Mar 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new APIs to customize content security policy for non-HTML responses #39398

Add new APIs to customize content security policy for non-HTML responses #39398

imtayadeway commented May 22, 2020

p8 commented Jun 15, 2020

imtayadeway commented Jun 15, 2020

jeremy left a comment

jeremy Jun 16, 2020

pixeltrix commented Jun 18, 2020

p8 commented Jun 18, 2020

pixeltrix commented Jun 18, 2020

pixeltrix commented Jun 18, 2020

pixeltrix commented Jun 19, 2020

jeremy commented Jun 22, 2020

imtayadeway commented Jun 22, 2020

p8 commented Jul 1, 2020

imtayadeway commented Jul 22, 2020

rails-bot bot commented Oct 20, 2020

imtayadeway commented Oct 21, 2020

rafaelfranca Oct 21, 2020

rafaelfranca Oct 21, 2020

rafaelfranca Oct 21, 2020

rafaelfranca Oct 21, 2020

rails-bot bot commented Apr 14, 2021

jeremy commented Sep 17, 2021

imtayadeway commented Sep 17, 2021

oreoshake Sep 17, 2021

jeremy commented Sep 21, 2021

tenderlove commented Feb 1, 2022

imtayadeway commented Feb 2, 2022

tenderlove commented Feb 3, 2022

imtayadeway commented Mar 8, 2022

tenderlove commented Mar 8, 2022

		@@ -1,3 +1,6 @@
		* Allow Content Security Policy DSL to generate for API responses.
		Tim Wade

Add new APIs to customize content security policy for non-HTML responses #39398

Are you sure you want to change the base?

Add new APIs to customize content security policy for non-HTML responses #39398

Conversation

imtayadeway commented May 22, 2020

Summary

Other Information

p8 commented Jun 15, 2020

imtayadeway commented Jun 15, 2020

jeremy left a comment

Choose a reason for hiding this comment

jeremy Jun 16, 2020

Choose a reason for hiding this comment

pixeltrix commented Jun 18, 2020

p8 commented Jun 18, 2020

pixeltrix commented Jun 18, 2020

pixeltrix commented Jun 18, 2020

pixeltrix commented Jun 19, 2020

jeremy commented Jun 22, 2020

imtayadeway commented Jun 22, 2020

p8 commented Jul 1, 2020

imtayadeway commented Jul 22, 2020

rails-bot bot commented Oct 20, 2020

imtayadeway commented Oct 21, 2020

rafaelfranca Oct 21, 2020

Choose a reason for hiding this comment

rafaelfranca Oct 21, 2020

Choose a reason for hiding this comment

rafaelfranca Oct 21, 2020

Choose a reason for hiding this comment

rafaelfranca Oct 21, 2020

Choose a reason for hiding this comment

rails-bot bot commented Apr 14, 2021

jeremy commented Sep 17, 2021

imtayadeway commented Sep 17, 2021

oreoshake Sep 17, 2021

Choose a reason for hiding this comment

jeremy commented Sep 21, 2021

tenderlove commented Feb 1, 2022

imtayadeway commented Feb 2, 2022

tenderlove commented Feb 3, 2022

imtayadeway commented Mar 8, 2022

tenderlove commented Mar 8, 2022