Configure Content Security Policy to remove risky allowed properties #279
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kevindew
added a commit
to alphagov/government-frontend
that referenced
this pull request
Jan 13, 2023
The usage of inline styles is going to be disallowed at the CSP level to reduce the damage HTML injection can do [1]. This replaces the usages of these inline styles with Design System override classes. I went for these rather than creating a new class as there didn't seem to be any precedent for specific classes for this part of the site, yet a variety of precedence in using override classes. [1]: alphagov/govuk_app_config#279
kevindew
added a commit
to alphagov/government-frontend
that referenced
this pull request
Jan 13, 2023
The usage of inline styles is going to be disallowed at the CSP level to reduce the damage HTML injection can do [1]. This replaces the usages of these inline styles with Design System override classes. I went for these rather than creating a new class as there didn't seem to be any precedent for specific classes for this part of the site, yet a variety of precedence in using override classes. [1]: alphagov/govuk_app_config#279
kevindew
added a commit
to alphagov/finder-frontend
that referenced
this pull request
Jan 16, 2023
This replaces the use of an input[type=text] for a hidden field. The motivation for replacing this is that these depend on inline styles and these will no longer be permitted by the Content Security Policy in alphagov/govuk_app_config#279 This also serves as a semantic improvement to the HTML.
kevindew
added a commit
to alphagov/finder-frontend
that referenced
this pull request
Jan 16, 2023
This replaces the usage of an inline data image from a CSS file. The motivation for doing this is the Content Security Policy removing the ability to embed data images for improved security [1]. Unfortunately this duplicates an asset that is in the component gem [2], it felt fragile and delicate to reference the asset directly from publishing components. Ideally we'd actually use the component from the gem, but this seemed impractical to achieve just now (mostly because of extra margins). [1]: alphagov/govuk_app_config#279 [2]: https://github.com/alphagov/govuk_publishing_components/blob/main/app/assets/images/govuk_publishing_components/icon-search.svg
kevindew
added a commit
to alphagov/smart-answers
that referenced
this pull request
Jan 17, 2023
This change has been made so that Smart Answers is compatible with upcoming changes to the GOV.UK Content Security Policy [1]. This adds a nonce so this JavaScript will be allowed to execute. This changes the use of defer="false" to be just a defer attribute. The usage of defer on inline script is unusual as it only applies when src is set [2]. I'm not sure why the `defer="false"` attribute was originally added (the commit [3] doesn't explain it) however it is now needed as part of Slimmer reorganising JavaScripts. Slimmer moves this JS to the head (and strips the `="false"`) so the change to `defer: true` is just to make the code for JS reflect what it actually used. Since it now seems to be used as a hack for Slimmer, I've labelled it as so. [1]: alphagov/govuk_app_config#279 [2]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#attr-defer [3]: a5c4e14
kevindew
added a commit
to alphagov/smart-answers
that referenced
this pull request
Jan 18, 2023
This change has been made so that Smart Answers is compatible with upcoming changes to the GOV.UK Content Security Policy [1]. This adds a nonce so this JavaScript will be allowed to execute. If the CSP does not have a nonce generator then setting nonce: true will have no effect. As this switches to using the Rails `javascript_tag` helper it also wraps the output JS in old-school CDATA tags, which looks a bit legacy but is benign. This changes the use of defer="false" to be just a defer attribute. The usage of defer on inline script is unusual as it only applies when src is set [2]. I'm not sure why the `defer="false"` attribute was originally added (the commit [3] doesn't explain it) however it is now needed as part of Slimmer reorganising JavaScripts. Slimmer moves this JS to the head (and strips the `="false"`) so the change to `defer: true` is just to make the code for JS reflect what it actually used. Since it now seems to be used as a hack for Slimmer, I've labelled it as so. [1]: alphagov/govuk_app_config#279 [2]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#attr-defer [3]: a5c4e14
kevindew
force-pushed
the
aspirational-csp
branch
from
32d86ae to
ce26053
Compare
|
I've amended this to leave unsafe-inline for style. This was done after finding out that is output by Govspeak, via Kramdown, for text alignment in tables. I have an idea on how to fix this (convert this to a class in Govspeak post-processing) but I still need to write that and then do a rather tedious republishing process to apply it to existing places. |
kevindew
force-pushed
the
aspirational-csp
branch
from
ce26053 to
305aa89
Compare
|
I've tested this on integration with all the frontend apps (those listed above) deployed with it. All Smokey tests continue to pass, I've checked individual pages on each app and no warnings. |
kevindew
force-pushed
the
aspirational-csp
branch
from
305aa89 to
a6f0ecf
Compare
ChrisBAshton
approved these changes
Jan 23, 2023
CHANGELOG.md
Outdated
| @@ -1,3 +1,7 @@ | |||
| # Unreleased | |||
|
|
|||
| * BREAKING: Content Security Policy forbids unsafe-inline script-src and data: image-src. It provides a nonce generator. Apps that can't support this will need to amend their CSP configuration in an initializer. | |||
There was a problem hiding this comment.
Is it worth linking to a doc explaining more about this initializer?
There was a problem hiding this comment.
Yup, that's a good shout. I thought in practice that no apps would need to do anything since I'd sorted them out the frontend apps. But now I've discovered the leak into the admin apps that seems a good idea.
There was a problem hiding this comment.
I'm going to get an example together before updating this item.
| "www.youtube-nocookie.com", | ||
| # This allows inline scripts and thus is a XSS risk. | ||
| # As of December 2022, we intend to work towards removing | ||
| # this from apps that don't use jQuery 1.12 (which needs |
There was a problem hiding this comment.
Also is it worth keeping this reference somewhere (e.g. the change note)? Might be a useful prompt for devs to check what version of jQuery they're running...?
According to the dependency management spreadsheet, most jQuery has been removed, but not everywhere.
There was a problem hiding this comment.
Good shout. I thought this was a non-concern as I ensured the removal of jQuery 1.x on all frontend apps.
I, however, had no idea that that in the 2020 Rails update work a bunch of admin applications had the Content Security Policy switched on (including oddly some API only apps): https://sourcegraph.com/search?q=context:global+repo:alphagov+GovukContentSecurityPolicy&patternType=standard&sm=0&groupBy=repo
There was a problem hiding this comment.
Should now be covered in the changelog not, but I've been and swept through apps already to have them ready.
kevindew
force-pushed
the
aspirational-csp
branch
2 times, most recently
from
5b24d8a to
8cf869e
Compare
kevindew
added a commit
to alphagov/signon
that referenced
this pull request
Jan 24, 2023
This configures the content security policy in preparation for the breaking changes coming from alphagov/govuk_app_config#279. As this app uses govuk_admin_template and that uses jQuery 1.x and inline script tags, then this app needs unsafe_inline for script and the nonce generator disabled. As an aside, It was a surprise that this application had configured the GovukContentSecurityPolicy as this had been initially done just in Frontend apps and it looks like this made it through in some outsourced Rails updates [1]. I'm leaving this config in so there is an example of an app outside of frontend using it to build on and as a case study in configuring an app. [1]: 45a5a51
kevindew
added a commit
to alphagov/support
that referenced
this pull request
Jan 24, 2023
GOV.UK hadn't intended for this app to have the GOV.UK Content Security Policy yet, with us first planning to roll out this to frontend app. It looks like this was added as part of an outsourced Rails update [1], where the dev couldn't have known about our nuanced context. As this is an app that doesn't receive a lot of developer attention I'm disabling this as I don't want breaking changes to the CSP [2] to end up in this app. [1]: #815 [2]:alphagov/govuk_app_config#279
kevindew
added a commit
to alphagov/service-manual-publisher
that referenced
this pull request
Jan 24, 2023
GOV.UK hadn't intended for this app to have the GOV.UK Content Security Policy yet, with us first planning to roll out this to frontend app. It looks like this was added as part of an outsourced Rails update [1], where the dev couldn't have known about our nuanced context. As this is an app that doesn't receive a lot of developer attention I'm disabling this as I don't want breaking changes to the CSP [2] to end up in this app. [1]: alphagov/support#815 [2]: alphagov/govuk_app_config#279
kevindew
added a commit
to alphagov/support
that referenced
this pull request
Jan 24, 2023
GOV.UK hadn't intended for this app to have the GOV.UK Content Security Policy yet, with us first planning to roll out this to frontend app. It looks like this was added as part of an outsourced Rails update [1], where the dev couldn't have known about our nuanced context. As this is an app that doesn't receive a lot of developer attention I'm disabling this as I don't want breaking changes to the CSP [2] to end up in this app. [1]: #815 [2]: alphagov/govuk_app_config#279
kevindew
added a commit
to alphagov/service-manual-publisher
that referenced
this pull request
Jan 24, 2023
GOV.UK hadn't intended for this app to have the GOV.UK Content Security Policy yet, with us first planning to roll out this to frontend app. It looks like this was added as part of an outsourced Rails update [1], where the dev couldn't have known about our nuanced context. As this is an app that doesn't receive a lot of developer attention I'm disabling this as I don't want breaking changes to the CSP [2] to end up in this app. [1]: #828 [2]: alphagov/govuk_app_config#279
kevindew
added a commit
to alphagov/content-data-admin
that referenced
this pull request
Jan 24, 2023
GOV.UK hadn't intended for this app to have the GOV.UK Content Security Policy yet, with us first planning to roll out this to frontend app. It looks like this was added as part of an outsourced Rails update [1], where the dev couldn't have known about our nuanced context. As this is an app that doesn't receive a lot of developer attention I'm disabling this as I don't want breaking changes to the CSP [2] to end up in this app. [1]: #812 [2]: alphagov/govuk_app_config#279
kevindew
added a commit
to alphagov/travel-advice-publisher
that referenced
this pull request
Jan 24, 2023
GOV.UK hadn't intended for this app to have the GOV.UK Content Security Policy yet, with us first planning to roll out this to frontend app. It looks like this was added as part of an outsourced Rails update [1], where the dev couldn't have known about our nuanced context. As this is an app that doesn't receive a lot of developer attention I'm disabling this as I don't want breaking changes to the CSP [2] to end up in this app. [1]: #919 [2]: alphagov/govuk_app_config#279
kevindew
added a commit
to alphagov/release
that referenced
this pull request
Jan 24, 2023
GOV.UK hadn't intended for this app to have the GOV.UK Content Security Policy yet, with us first planning to roll out this to frontend app. It looks like this was added as part of an outsourced Rails update [1], where the dev couldn't have known about our nuanced context. As this is an app that doesn't receive a lot of developer attention I'm disabling this as I don't want breaking changes to the CSP [2] to end up in this app. [1]: #667 [2]: alphagov/govuk_app_config#279
kevindew
added a commit
to alphagov/support
that referenced
this pull request
Jan 24, 2023
GOV.UK hadn't intended for this app to have the GOV.UK Content Security Policy yet, with us first planning to roll out this to frontend app. It looks like this was added as part of an outsourced Rails update [1], where the dev couldn't have known about our nuanced context. As this is an app that doesn't receive a lot of developer attention I'm disabling this as I don't want breaking changes to the CSP [2] to end up in this app. [1]: #815 [2]: alphagov/govuk_app_config#279
kevindew
added a commit
to alphagov/service-manual-publisher
that referenced
this pull request
Jan 24, 2023
GOV.UK hadn't intended for this app to have the GOV.UK Content Security Policy yet, with us first planning to roll out this to frontend app. It looks like this was added as part of an outsourced Rails update [1], where the dev couldn't have known about our nuanced context. As this is an app that doesn't receive a lot of developer attention I'm disabling this as I don't want breaking changes to the CSP [2] to end up in this app. [1]: #828 [2]: alphagov/govuk_app_config#279
kevindew
added a commit
to alphagov/content-data-admin
that referenced
this pull request
Jan 24, 2023
GOV.UK hadn't intended for this app to have the GOV.UK Content Security Policy yet, with us first planning to roll out this to frontend app. It looks like this was added as part of an outsourced Rails update [1], where the dev couldn't have known about our nuanced context. As this is an app that doesn't receive a lot of developer attention I'm disabling this as I don't want breaking changes to the CSP [2] to end up in this app. [1]: #812 [2]: alphagov/govuk_app_config#279
kevindew
added a commit
to alphagov/search-admin
that referenced
this pull request
Jan 24, 2023
GOV.UK hadn't intended for this app to have the GOV.UK Content Security Policy yet, with us first planning to roll out this to frontend app. It looks like this was added as part of an outsourced Rails update [1], where the dev couldn't have known about our nuanced context. As this is an app that doesn't receive a lot of developer attention I'm disabling this as I don't want breaking changes to the CSP [2] to end up in this app. [1]: #464 [2]: alphagov/govuk_app_config#279
This was referenced Jan 25, 2023
kevindew
added a commit
to alphagov/travel-advice-publisher
that referenced
this pull request
Jan 25, 2023
GOV.UK hadn't intended for this app to have the GOV.UK Content Security Policy yet, with us first planning to roll out this to frontend app. It looks like this was added as part of an outsourced Rails update [1], where the dev couldn't have known about our nuanced context. As this is an app that doesn't receive a lot of developer attention I'm disabling this as I don't want breaking changes to the CSP [2] to end up in this app. [1]: #919 [2]: alphagov/govuk_app_config#279
This was referenced Jan 25, 2023
kevindew
added a commit
to alphagov/search-admin
that referenced
this pull request
Jan 25, 2023
GOV.UK hadn't intended for this app to have the GOV.UK Content Security Policy yet, with us first planning to roll out this to frontend app. It looks like this was added as part of an outsourced Rails update [1], where the dev couldn't have known about our nuanced context. As this is an app that doesn't receive a lot of developer attention I'm disabling this as I don't want breaking changes to the CSP [2] to end up in this app. [1]: #464 [2]: alphagov/govuk_app_config#279
kevindew
added a commit
to alphagov/search-admin
that referenced
this pull request
Jan 25, 2023
GOV.UK hadn't intended for this app to have the GOV.UK Content Security Policy yet, with us first planning to roll out this to frontend app. It looks like this was added as part of an outsourced Rails update [1], where the dev couldn't have known about our nuanced context. As this is an app that doesn't receive a lot of developer attention I'm disabling this as I don't want breaking changes to the CSP [2] to end up in this app. [1]: #464 [2]: alphagov/govuk_app_config#279
kevindew
added a commit
to alphagov/travel-advice-publisher
that referenced
this pull request
Jan 25, 2023
GOV.UK hadn't intended for this app to have the GOV.UK Content Security Policy yet, with us first planning to roll out this to frontend app. It looks like this was added as part of an outsourced Rails update [1], where the dev couldn't have known about our nuanced context. As this is an app that doesn't receive a lot of developer attention I'm disabling this as I don't want breaking changes to the CSP [2] to end up in this app. [1]: #919 [2]: alphagov/govuk_app_config#279
kevindew
force-pushed
the
aspirational-csp
branch
from
8cf869e to
660a900
Compare
|
Thanks for the review @ChrisBAshton, some great spots. Should be ready for another look. |
ChrisBAshton
approved these changes
Jan 26, 2023
| @@ -1,3 +1,7 @@ | |||
| # Unreleased | |||
|
|
|||
| * BREAKING: Content Security Policy forbids unsafe-inline script-src and data: image-src. It provides a nonce generator. Apps that can't support this will need to amend their CSP configuration in an initializer, see [example](https://github.com/alphagov/signon/commit/ddcf31f5c30b8fd334e4aea74986b24bf2b0e9be) in signon. Any apps that still use jQuery 1.x will need unsafe-inline for Firefox compatibility. | |||
This removes unsafe-inline from script and data images. It sets a nonce-generator which will apply to script-src. Having a nonce in a CSP will cause any unsafe-inline rules to be ignored, so if an app needs them they will have to both add the directive unsafe-inline to script-src and also disable the nonce-generator. I initially planned for this to remove unsafe-inline from style as well, however I learnt in the latter stages of testing, that there is Govspeak that uses inline style attributes [1][2] (example page: [3]). I have ideas on how to fix this but it will take some time, so I'm deferring this until later. [1]: https://kramdown.gettalong.org/syntax.html#tables [2]: https://github.com/alphagov/govspeak/blob/5642fcc4231f215d1c58ad7feb30ca42fb8cfb91/lib/govspeak/html_sanitizer.rb#L72-L73 [3]: https://www.gov.uk/government/statistics/non-association-independent-schools-inspections-and-outcomes-in-england-august-2022/main-findings-non-association-independent-schools-inspections-and-outcomes-in-england-august-2022
A base element can be used to change the destination of relative paths, this can be used as part of XSS to include a script file on a host an attacker controls. To prevent this we disable all uses of the base element as it is not used on all GOV.UK views, bar one exception. The exception is for CSV previews which are rendered by Whitehall [1] on a different hostname (assets). As this is only for one app the convention would be to modify the CSP in app. It is also unclear at this point in time when or whether we will enable the CSP on Whitehall frontend. [1]: alphagov/whitehall#5764
kevindew
force-pushed
the
aspirational-csp
branch
from
660a900 to
414e539
Compare
kevindew
added a commit
to alphagov/govspeak
that referenced
this pull request
Jan 27, 2023
We aspire to disallow inline style elements on GOV.UK as these present a XSS security risk [1]. However this change was scuppered [2] because Govspeak has one scenario (text alignment in Kramdown) which uses inline styles [3]. The approach I've taken to resolve this is to use the post-processor as a way to replace the style elements on td and th elements with class parameters. This change won't be safe to be merged until govuk_publishing_components is updated to apply the styles of the class. This handles a couple of edge cases, that may not be that necessary. It can cope if the style attribute is given multiple text-align rules and will use the last one (as CSS would). It also can handle the scenario where this already a class on the td/th. Once the GOV.UK stack migrates existing content to use it can remove the leniency that allows style elements in the sanitizer rules. I've left a comment as a reminder that this could be removed (hopefully I'll remember). [1]: https://github.com/alphagov/govuk_app_config/blob/13083f8f8563c4703c14fe8175ca35646ec64442/lib/govuk_app_config/govuk_content_security_policy.rb#L64-L67 [2]: alphagov/govuk_app_config#279 (comment) [3]: https://github.com/gettalong/kramdown/blob/0b0a9e072f9a76e59fe2bbafdf343118fb27c3fa/test/testcases/block/14_table/header.html#L37-L56
kevindew
added a commit
to alphagov/govspeak
that referenced
this pull request
Jan 27, 2023
We aspire to disallow inline style elements on GOV.UK as these present a XSS security risk [1]. However this change was scuppered [2] because Govspeak has one scenario (text alignment in Kramdown) which uses inline styles [3]. The approach I've taken to resolve this is to use the post-processor as a way to replace the style elements on td and th elements with class parameters. This change won't be safe to be merged until govuk_publishing_components is updated to apply the styles of the class. This handles a couple of edge cases, that may not be that necessary. It can cope if the style attribute is given multiple text-align rules and will use the last one (as CSS would). It also can handle the scenario where this already a class on the td/th. Once the GOV.UK stack migrates existing content to use it can remove the leniency that allows style elements in the sanitizer rules. I've left a comment as a reminder that this could be removed (hopefully I'll remember). [1]: https://github.com/alphagov/govuk_app_config/blob/13083f8f8563c4703c14fe8175ca35646ec64442/lib/govuk_app_config/govuk_content_security_policy.rb#L64-L67 [2]: alphagov/govuk_app_config#279 (comment) [3]: https://github.com/gettalong/kramdown/blob/0b0a9e072f9a76e59fe2bbafdf343118fb27c3fa/test/testcases/block/14_table/header.html#L37-L56
kevindew
added a commit
to alphagov/govspeak
that referenced
this pull request
Mar 17, 2023
We aspire to disallow inline style elements on GOV.UK as these present a XSS security risk [1]. However this change was scuppered [2] because Govspeak has one scenario (text alignment in Kramdown) which uses inline styles [3]. The approach I've taken to resolve this is to use the post-processor as a way to replace the style elements on td and th elements with class parameters. This change won't be safe to be merged until govuk_publishing_components is updated to apply the styles of the class. This handles a couple of edge cases, that may not be that necessary. It can cope if the style attribute is given multiple text-align rules and will use the last one (as CSS would). It also can handle the scenario where this already a class on the td/th. Once the GOV.UK stack migrates existing content to use it can remove the leniency that allows style elements in the sanitizer rules. I've left a comment as a reminder that this could be removed (hopefully I'll remember). [1]: https://github.com/alphagov/govuk_app_config/blob/13083f8f8563c4703c14fe8175ca35646ec64442/lib/govuk_app_config/govuk_content_security_policy.rb#L64-L67 [2]: alphagov/govuk_app_config#279 (comment) [3]: https://github.com/gettalong/kramdown/blob/0b0a9e072f9a76e59fe2bbafdf343118fb27c3fa/test/testcases/block/14_table/header.html#L37-L56
kevindew
added a commit
that referenced
this pull request
Apr 27, 2023
This continues the work from #279 to remove risky properties from our Content Security Policy (CSP) by removing unsafe-inline from style properties. We have been to resolve the need for this property by updating Govspeak [1] [1]: alphagov/govspeak#268
kevindew
added a commit
that referenced
this pull request
Apr 27, 2023
This continues the work from #279 to remove risky properties from our Content Security Policy (CSP) by removing unsafe-inline from style properties. We have been to resolve the need for this property by updating Govspeak [1] [1]: alphagov/govspeak#268
kevindew
added a commit
that referenced
this pull request
Jun 2, 2023
This continues the work from #279 to remove risky properties from our Content Security Policy (CSP) by removing unsafe-inline from style properties. We have been able to resolve the need for this property by updating Govspeak [1] [1]: alphagov/govspeak#268
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Trello: https://trello.com/c/lxxx5XLZ/178-govuk-has-a-half-implemented-content-security-policy-csp
This applies a number of changes to the Content Security Policy provided by this gem. It:
and style-src, which means that any usages of<script>orelements requires a nonce<style>and usage of inline styles are forbidden. This is to limit the damage an attacker can do with ability to inject HTML into a page. (Note: I wasn't able to disable unsafe-inline on style due to Govspeak usage)data:URLs. This limits any injected HTML to not display arbitrary images<base>element. This limits the ability of injected HTML to change the destination of scripts and allow execution of 3rd party JavaScriptThis has been opened as a draft while frontend applications are checked for blocking issues:
<script type="application/ld+json">)<script type="application/ld+json">)This is intended to be the draft of the CSP that GOV.UK applies as its initial live offering. Hopefully we will be ready to go once the above list is completed and that we've run this on apps in report only mode for a couple of weeks.
There are some future things I considered out of scope for now:
strict-dynamic- we probably should switch tostrict-dynamicfor scripts in time, as it is a clear improvement. However, as you have to set-up a bunch of extra backwards compatibility changes, I think it makes things extra complicated when we may have some apps still needing things like unsafe-inline for legacy jQuery. Therefore I think this is something to dig into once live.strict-dynamicso we'll reserve this for that.require-trusted-types-for 'script'this is recommended by https://csp-evaluator.withgoogle.com/. Implementation looks very difficult, as it requires identifying every case ofinnerHTMLbeing called, likely extra challenging because of incomplete browser support currently. So I don't see us being able to support this in the forseeable future.