Skip to content
This repository has been archived by the owner on Apr 16, 2021. It is now read-only.

Add an HTTP Content-Security-Policy #444

Merged
merged 1 commit into from Jul 14, 2020
Merged

Conversation

gclssvglx
Copy link
Contributor

@gclssvglx gclssvglx commented Jul 9, 2020

What

As part of a security review of the service, it was noted that a Content-Security-Policy (CSP) is missing from the service.

A useful tool to assess an application's current header posture is Scott Helme's securityheaders.com. Using this on the production service reports an overall grade of B. The highest being A+.

Three issues were reported:

  • A Content-Security-Policy is completely missing, despite Rails providing support for custom CSP's.
  • Likewise, aFeature-Policy is also missing. This is a new(ish) header and currently un-supported in the present version of Rails used on service (v6.0.3). However, support for this will be available in in Rails v6.1 - which expected by the end of July 2020. This PR addresses the missing Feature-Policy.
  • Our Set-Cookie header has no Cookie Prefix on the cookie and that the cookie is not a SameSite Cookie.

Setting headers in a Rails application is done in two places (currently):

  1. In config/application.rb - for default headers
  2. In config/initializers/content_security_policy.rb - for CSP headers

Rails automatically includes sensible defaults for the default headers. These can be overridden if required in config/application.rb. They are as follows...

X-Frame-Options: SAMEORIGIN
X-XSS-Protection: 1; mode=block
X-Content-Type-Options: nosniff
X-Download-Options: noopen
X-Permitted-Cross-Domain-Policies: none
Referrer-Policy: strict-origin-when-cross-origin

# When `config.force_ssl = true` this header is also automatically set...
strict-transport-security: max-age=31536000; includeSubDomains

In addition, the Cache-Control header is already set in the service on a per-environment basis in the config/environments/<environment>.rb files, e.g.

Cache-Control: max-age=0, private, must-revalidate

As no issues are reported for any of the default headers, and that the existing default headers are restrictive in nature, it is felt that there is no need to change the default headers at this point.

This change adds a CSP to the service.

Most CSP headers are set to either :self (the origin from which the content is served should come from the same URL), or :none (effectively blocked or not allowed). However, the script_src header is handled differently as it needs to handle Google Analytics and UJS. Subsequently, the Google Analytics URL is explicitly specified and a nonce has been set. Setting a nonce requires that the site's Javascript knows that a nonce is set. This requires a parameter to be passed to javascript_tag and javascript_include_tag helpers.

How to review

You can (re)view the current HTTP headers via cURL...

# production

$ curl -L -A "Mozilla/5.0 (Windows NT 6.1; WOW64) \
        AppleWebKit/537.36 (KHTML, like Gecko) \
        Chrome/50.0.2661.102 Safari/537.36" \
        -s -D - https://coronavirus-vulnerable-people.service.gov.uk/live-in-england \
        -o /dev/null

# development

$ curl -L -A "Mozilla/5.0 (Windows NT 6.1; WOW64) \
        AppleWebKit/537.36 (KHTML, like Gecko) \
        Chrome/50.0.2661.102 Safari/537.36" \
        -s -D - http://localhost:5000/live-in-england \
        -o /dev/null

Other than viewing the headers, or running securityheaders.com - the best way to test if a header is set appropriately is to open dev tools on console and run the service. Error's will be thrown if a header is too restrictive or set incorrectly. A general rule or best practice, is to start with the most restrictive header setting and adjust based on requirements and/or errors.

Links

@bevanloon bevanloon temporarily deployed to coronavirus-security-re-uxiwxw July 9, 2020 15:32 Inactive
@bevanloon bevanloon temporarily deployed to coronavirus-security-re-uxiwxw July 9, 2020 15:36 Inactive
@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2020

Codecov Report

Merging #444 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #444   +/-   ##
=======================================
  Coverage   99.29%   99.29%           
=======================================
  Files          71       71           
  Lines        1972     1972           
=======================================
  Hits         1958     1958           
  Misses         14       14           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9d7bcd...5921cd9. Read the comment docs.

@pixeltrix
Copy link
Contributor

Just to clarify on Rails 6.1 - we are likely to put out the first beta release by the end of July but final release will be some time in September going by how long it took us to release the final of 5.1.

Also note that we're investigating adding CSP headers to non-HTML responses and error pages due to a loophole where loading content without a CSP into an iframe can bypass the parent's CSP header as described here:

https://lab.wallarm.com/how-to-trick-csp-in-letting-you-run-whatever-you-want-73cb5ff428aa/

The attack requires a XSS in the application to be exploitable. It's almost certain that the feature policy can also be bypassed in a similar manner so we'll be addressing that too.

@bevanloon bevanloon temporarily deployed to coronavirus-security-re-uxiwxw July 10, 2020 07:46 Inactive
@gclssvglx
Copy link
Contributor Author

gclssvglx commented Jul 10, 2020

Just to clarify on Rails 6.1 - we are likely to put out the first beta release by the end of July but final release will be some time in September going by how long it took us to release the final of 5.1.

Also note that we're investigating adding CSP headers to non-HTML responses and error pages due to a loophole where loading content without a CSP into an iframe can bypass the parent's CSP header as described here:

https://lab.wallarm.com/how-to-trick-csp-in-letting-you-run-whatever-you-want-73cb5ff428aa/

The attack requires a XSS in the application to be exploitable. It's almost certain that the feature policy can also be bypassed in a similar manner so we'll be addressing that too.

Thanks @pixeltrix - any advice is always more than welcome. Note the separate PR to add a Feature-Policy.

Copy link
Contributor

@reggieb reggieb 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. One very minor comment that can be ignored.

app/views/layouts/application.html.erb Outdated Show resolved Hide resolved
As part of a security review of the service, it was noted that a
Content-Security-Policy (CSP) is missing.

This change adds a CSP to the service.
@gclssvglx gclssvglx merged commit 786097f into master Jul 14, 2020
@gclssvglx gclssvglx deleted the security-review-headers branch July 14, 2020 13:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants