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 the ability to set Content Security Policies #6631

Merged
merged 87 commits into from Jan 16, 2024
Merged

Conversation

fritzmg
Copy link
Contributor

@fritzmg fritzmg commented Dec 17, 2023

This introduces the ability to define Content Security Policies for the front end. There will be new settings for each
website root:

Within the Content Security Policy field you can define your policies - with the exakt same syntax as the
Content-Security-Policy header. For example default-src 'self' will tell the browser to only accept resources from
the same domain and not allow any inline styles or scripts at all (at least not without nonces).

Once a policy for default-src, script-src or style-src is set [1], Contao will automatically generate nonces for
the respective <script> and <style> elements, at least in supported templates. If you have any custom templates
with these tags, then you will need to adjust your template to include the nonce, which is pretty simple:

<script nonce="{{ contao_csp_nonce('script-src') }}">
<script nonce="<?= $this->nonce('script-src') ?>">

In this PR the nonce attribute is added conditionally everywhere, i.e. the attribute will not be set, if no nonce was
necessary and thus no nonce was generated. So there will not be empty nonce attributes in the HTML code (though it
would be fine).

Allowing Specific Sources Dynamically

Some content elements like the video, YouTube and Vimeo players dynamically allow their respective sources. This can be
done directly from within the templates:

{% do add_csp_source('frame-src', iframe_attrs.src) %}
<?php $this->addCspSource('frame-src', $this->src); ?>

So even if you set the policies frame-src 'self'; media-src 'none' these content elements will still work as they
dynamically allow their specific sources.

Reporting

In the CSP spec you also have the possibility to set a reporting URL. If the browser detects a CSP violation it will
then automatically POST a report to that URL. Reporting needs to be specifically enabled in the website root. These reports will then also show up in the system log:

The system log entry will tell you which policy was violated and on which line. The detailed information will contain
the original URL of the violation.

This is of course a public API endpoint where anyone or anything can send any data. As such this needs to be handled
with care. This PR uses the ContentSecurityPolicyController of nelmio/security-bundle which has the ability to
filter out "noise". But instead of using that controller directly this PR implements its own controller and injects the service of Nelmio's controller. Our route always requires a valid page ID and always checks whether the current page's root has reporting enabled. If not, it will respond with 404 - otherwise it will forward the request to the ContentSecurityPolicyController of nelmio/security-bundle.

Twig Components

This PR introduces two new Twig components: _style and _script. When using them you do not have to think about having to check for nonces as the component does this automatically for you. However, I find that using these components makes the templates less readable - as there will also be no syntax highlighting by your editor anymore, since it won't know that the content of the component block is within a <script> or <style> tag.

Thoughts, Decisions, Caveats

Initially I thought may be Contao should set some sensible defaults automatically and also automatically allow the
staticFiles and staticPlugins domains. However, this approach can be difficult and flawed. Contao cannot know in
advance what kind of sources are hosted on its current and any of the static domains, thus we cannot set any -src
policies (we would have to set all of them). It needs to be solely up to the administrator to set appropriate policies.

One exception could be iframe sources, which could be automatically added (see above). However, allowing specific
sources dynamically also has a caveat. Let's take the YouTube content element for example: on the page where such a
content element is added, the Content-Security-Policy will automatically be the following [2]:

frame-src https://www.youtube.com/embed/abc123

However, this will also mean that no other iframes will be allowed on the same page, whereas without this content
element they would be. We could change the helper functions to not add the directive if no existing (or default-src
directive) is already present. Unfortunately the CSPBuilder has no such API at the moment.

Further comments about the code will be down below in threads. PR is still a draft due to missing tests.

Footnotes

@fritzmg fritzmg self-assigned this Dec 17, 2023
core-bundle/composer.json Outdated Show resolved Hide resolved
Toflar
Toflar previously approved these changes Jan 12, 2024
Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

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

I stumbled upon two things:


The "Content Security Policy" field is not mandatory, yet if I leave it blank, nothing is applied. It should therefore be mandatory, shouldn‘t it?


The report in the system log has a strange URL that I cannot open and the page ID is 0. Is this intended?

@fritzmg
Copy link
Contributor Author

fritzmg commented Jan 15, 2024

The "Content Security Policy" field is not mandatory, yet if I leave it blank, nothing is applied. It should therefore be mandatory, shouldn‘t it?

Yeah, I think it would not hurt to make it mandatory.

The report in the system log has a strange URL that I cannot open and the page ID is 0. Is this intended?

The Page ID will always be 0, yes. The URL is the "source file" reported by the browser, but I suppose depending on the violation there could be no source file. We could change https://github.com/contao/contao/pull/6631/files#diff-2b3da269e425b96148c6bfa17b165cf8995271a73b8d8da0cb8d35f36c59040eR38 to fall back to the document URI instead (I think). I'll have a look.

@fritzmg
Copy link
Contributor Author

fritzmg commented Jan 15, 2024

The report in the system log has a strange URL that I cannot open and the page ID is 0. Is this intended?

The Page ID will always be 0, yes. The URL is the "source file" reported by the browser, but I suppose depending on the violation there could be no source file. We could change https://github.com/contao/contao/pull/6631/files#diff-2b3da269e425b96148c6bfa17b165cf8995271a73b8d8da0cb8d35f36c59040eR38 to fall back to the document URI instead (I think). I'll have a look.

Should be fixed in 8f892e0

@fritzmg fritzmg requested a review from leofeyer January 15, 2024 14:39
@leofeyer leofeyer merged commit c3d5726 into contao:5.x Jan 16, 2024
15 of 16 checks passed
@leofeyer
Copy link
Member

Thank you @fritzmg.

leofeyer added a commit that referenced this pull request Jan 22, 2024
Description
-----------

Now that CSP has landed in #6631 (❤️ 🥳 ) we can properly prevent inline styles from being applied randomly which adds yet another layer of security to Contao.

My local tests showed that everything is working perfectly fine, except for inline style attributes on our RTE/tinyMCE/WYSIWYG editor fields. Obviously, if you use something like

```html
<p style="text-decoration: underline">Foobar</p>
```

this won't work anymore now, as this is possibly forbidden if you do not allow inline styles in your CSP (which you shouldn't as it weakens the policy).

Here's a quick draft of how we could improve on this. I thought I'd code it real quick as it's easier to understand for everybody if there's code to look at 😊 
The logic is pretty simple: extract the `style` attributes from HTML and if they match an allow-list of pre-defined properties (for security reasons), auto-generate CSP hashes for them.

Commits
-------

af729ce Support CSP on WYSIWYG editors like tinyMCE
737f2d2 Combine multiple identical styles to one CSS class
343a656 Switch to hashing implementation
e920800 Remove library
5d49663 Switch to regex implementation
9f7cd5a Finished implementation preparing for @ausi
7bd17a4 Update core-bundle/src/Twig/Extension/ContaoExtension.php
7cd9d16 Adjust the pull request template
3e5cf53 CSP WYSIWYG (#15)
a327b52 Added calls on all templates
5e3f647 Fixed tests
054c0eb Make method nullable
347c77f Revert changes
f1a4a31 Fix regex
0e9e215 Decode HTML entities before parsing the styles
db67dc1 Test TemplateTrait::extractStyleAttributesForCsp()
8cda64e Fix font regex
11b5560 Rename extract_styles_for_csp to csp_inline_styles
f991bbd Rename extractStyleAttributesForCsp to cspInlineStyles
6de2fdd Rename inlineStyle to cspInlineStyle

Co-authored-by: ausi <martin@auswoeger.com>
Co-authored-by: leofeyer <1192057+leofeyer@users.noreply.github.com>
leofeyer added a commit to contao/faq-bundle that referenced this pull request Jan 22, 2024
Description
-----------

Now that CSP has landed in contao/contao#6631 (❤️ 🥳 ) we can properly prevent inline styles from being applied randomly which adds yet another layer of security to Contao.

My local tests showed that everything is working perfectly fine, except for inline style attributes on our RTE/tinyMCE/WYSIWYG editor fields. Obviously, if you use something like

```html
<p style="text-decoration: underline">Foobar</p>
```

this won't work anymore now, as this is possibly forbidden if you do not allow inline styles in your CSP (which you shouldn't as it weakens the policy).

Here's a quick draft of how we could improve on this. I thought I'd code it real quick as it's easier to understand for everybody if there's code to look at 😊 
The logic is pretty simple: extract the `style` attributes from HTML and if they match an allow-list of pre-defined properties (for security reasons), auto-generate CSP hashes for them.

Commits
-------

af729ce9 Support CSP on WYSIWYG editors like tinyMCE
737f2d2f Combine multiple identical styles to one CSS class
343a656d Switch to hashing implementation
e920800b Remove library
5d49663a Switch to regex implementation
9f7cd5af Finished implementation preparing for @ausi
7bd17a47 Update core-bundle/src/Twig/Extension/ContaoExtension.php
7cd9d160 Adjust the pull request template
3e5cf53c CSP WYSIWYG (#15)
a327b522 Added calls on all templates
5e3f6477 Fixed tests
054c0eb1 Make method nullable
347c77f4 Revert changes
f1a4a314 Fix regex
0e9e2158 Decode HTML entities before parsing the styles
db67dc12 Test TemplateTrait::extractStyleAttributesForCsp()
8cda64e4 Fix font regex
11b55607 Rename extract_styles_for_csp to csp_inline_styles
f991bbdc Rename extractStyleAttributesForCsp to cspInlineStyles
6de2fdd1 Rename inlineStyle to cspInlineStyle

Co-authored-by: ausi <martin@auswoeger.com>
Co-authored-by: leofeyer <1192057+leofeyer@users.noreply.github.com>
leofeyer added a commit to contao/calendar-bundle that referenced this pull request Jan 22, 2024
Description
-----------

Now that CSP has landed in contao/contao#6631 (❤️ 🥳 ) we can properly prevent inline styles from being applied randomly which adds yet another layer of security to Contao.

My local tests showed that everything is working perfectly fine, except for inline style attributes on our RTE/tinyMCE/WYSIWYG editor fields. Obviously, if you use something like

```html
<p style="text-decoration: underline">Foobar</p>
```

this won't work anymore now, as this is possibly forbidden if you do not allow inline styles in your CSP (which you shouldn't as it weakens the policy).

Here's a quick draft of how we could improve on this. I thought I'd code it real quick as it's easier to understand for everybody if there's code to look at 😊 
The logic is pretty simple: extract the `style` attributes from HTML and if they match an allow-list of pre-defined properties (for security reasons), auto-generate CSP hashes for them.

Commits
-------

af729ce9 Support CSP on WYSIWYG editors like tinyMCE
737f2d2f Combine multiple identical styles to one CSS class
343a656d Switch to hashing implementation
e920800b Remove library
5d49663a Switch to regex implementation
9f7cd5af Finished implementation preparing for @ausi
7bd17a47 Update core-bundle/src/Twig/Extension/ContaoExtension.php
7cd9d160 Adjust the pull request template
3e5cf53c CSP WYSIWYG (#15)
a327b522 Added calls on all templates
5e3f6477 Fixed tests
054c0eb1 Make method nullable
347c77f4 Revert changes
f1a4a314 Fix regex
0e9e2158 Decode HTML entities before parsing the styles
db67dc12 Test TemplateTrait::extractStyleAttributesForCsp()
8cda64e4 Fix font regex
11b55607 Rename extract_styles_for_csp to csp_inline_styles
f991bbdc Rename extractStyleAttributesForCsp to cspInlineStyles
6de2fdd1 Rename inlineStyle to cspInlineStyle

Co-authored-by: ausi <martin@auswoeger.com>
Co-authored-by: leofeyer <1192057+leofeyer@users.noreply.github.com>
leofeyer added a commit to contao/news-bundle that referenced this pull request Jan 22, 2024
Description
-----------

Now that CSP has landed in contao/contao#6631 (❤️ 🥳 ) we can properly prevent inline styles from being applied randomly which adds yet another layer of security to Contao.

My local tests showed that everything is working perfectly fine, except for inline style attributes on our RTE/tinyMCE/WYSIWYG editor fields. Obviously, if you use something like

```html
<p style="text-decoration: underline">Foobar</p>
```

this won't work anymore now, as this is possibly forbidden if you do not allow inline styles in your CSP (which you shouldn't as it weakens the policy).

Here's a quick draft of how we could improve on this. I thought I'd code it real quick as it's easier to understand for everybody if there's code to look at 😊 
The logic is pretty simple: extract the `style` attributes from HTML and if they match an allow-list of pre-defined properties (for security reasons), auto-generate CSP hashes for them.

Commits
-------

af729ce9 Support CSP on WYSIWYG editors like tinyMCE
737f2d2f Combine multiple identical styles to one CSS class
343a656d Switch to hashing implementation
e920800b Remove library
5d49663a Switch to regex implementation
9f7cd5af Finished implementation preparing for @ausi
7bd17a47 Update core-bundle/src/Twig/Extension/ContaoExtension.php
7cd9d160 Adjust the pull request template
3e5cf53c CSP WYSIWYG (#15)
a327b522 Added calls on all templates
5e3f6477 Fixed tests
054c0eb1 Make method nullable
347c77f4 Revert changes
f1a4a314 Fix regex
0e9e2158 Decode HTML entities before parsing the styles
db67dc12 Test TemplateTrait::extractStyleAttributesForCsp()
8cda64e4 Fix font regex
11b55607 Rename extract_styles_for_csp to csp_inline_styles
f991bbdc Rename extractStyleAttributesForCsp to cspInlineStyles
6de2fdd1 Rename inlineStyle to cspInlineStyle

Co-authored-by: ausi <martin@auswoeger.com>
Co-authored-by: leofeyer <1192057+leofeyer@users.noreply.github.com>
leofeyer added a commit to contao/comments-bundle that referenced this pull request Jan 22, 2024
Description
-----------

Now that CSP has landed in contao/contao#6631 (❤️ 🥳 ) we can properly prevent inline styles from being applied randomly which adds yet another layer of security to Contao.

My local tests showed that everything is working perfectly fine, except for inline style attributes on our RTE/tinyMCE/WYSIWYG editor fields. Obviously, if you use something like

```html
<p style="text-decoration: underline">Foobar</p>
```

this won't work anymore now, as this is possibly forbidden if you do not allow inline styles in your CSP (which you shouldn't as it weakens the policy).

Here's a quick draft of how we could improve on this. I thought I'd code it real quick as it's easier to understand for everybody if there's code to look at 😊 
The logic is pretty simple: extract the `style` attributes from HTML and if they match an allow-list of pre-defined properties (for security reasons), auto-generate CSP hashes for them.

Commits
-------

af729ce9 Support CSP on WYSIWYG editors like tinyMCE
737f2d2f Combine multiple identical styles to one CSS class
343a656d Switch to hashing implementation
e920800b Remove library
5d49663a Switch to regex implementation
9f7cd5af Finished implementation preparing for @ausi
7bd17a47 Update core-bundle/src/Twig/Extension/ContaoExtension.php
7cd9d160 Adjust the pull request template
3e5cf53c CSP WYSIWYG (#15)
a327b522 Added calls on all templates
5e3f6477 Fixed tests
054c0eb1 Make method nullable
347c77f4 Revert changes
f1a4a314 Fix regex
0e9e2158 Decode HTML entities before parsing the styles
db67dc12 Test TemplateTrait::extractStyleAttributesForCsp()
8cda64e4 Fix font regex
11b55607 Rename extract_styles_for_csp to csp_inline_styles
f991bbdc Rename extractStyleAttributesForCsp to cspInlineStyles
6de2fdd1 Rename inlineStyle to cspInlineStyle

Co-authored-by: ausi <martin@auswoeger.com>
Co-authored-by: leofeyer <1192057+leofeyer@users.noreply.github.com>
leofeyer added a commit to contao/core-bundle that referenced this pull request Jan 22, 2024
Description
-----------

Now that CSP has landed in contao/contao#6631 (❤️ 🥳 ) we can properly prevent inline styles from being applied randomly which adds yet another layer of security to Contao.

My local tests showed that everything is working perfectly fine, except for inline style attributes on our RTE/tinyMCE/WYSIWYG editor fields. Obviously, if you use something like

```html
<p style="text-decoration: underline">Foobar</p>
```

this won't work anymore now, as this is possibly forbidden if you do not allow inline styles in your CSP (which you shouldn't as it weakens the policy).

Here's a quick draft of how we could improve on this. I thought I'd code it real quick as it's easier to understand for everybody if there's code to look at 😊 
The logic is pretty simple: extract the `style` attributes from HTML and if they match an allow-list of pre-defined properties (for security reasons), auto-generate CSP hashes for them.

Commits
-------

af729ce9 Support CSP on WYSIWYG editors like tinyMCE
737f2d2f Combine multiple identical styles to one CSS class
343a656d Switch to hashing implementation
e920800b Remove library
5d49663a Switch to regex implementation
9f7cd5af Finished implementation preparing for @ausi
7bd17a47 Update core-bundle/src/Twig/Extension/ContaoExtension.php
7cd9d160 Adjust the pull request template
3e5cf53c CSP WYSIWYG (#15)
a327b522 Added calls on all templates
5e3f6477 Fixed tests
054c0eb1 Make method nullable
347c77f4 Revert changes
f1a4a314 Fix regex
0e9e2158 Decode HTML entities before parsing the styles
db67dc12 Test TemplateTrait::extractStyleAttributesForCsp()
8cda64e4 Fix font regex
11b55607 Rename extract_styles_for_csp to csp_inline_styles
f991bbdc Rename extractStyleAttributesForCsp to cspInlineStyles
6de2fdd1 Rename inlineStyle to cspInlineStyle

Co-authored-by: ausi <martin@auswoeger.com>
Co-authored-by: leofeyer <1192057+leofeyer@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants