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

[4.0] Improve CSP #32893

Closed
wants to merge 32 commits into from
Closed

[4.0] Improve CSP #32893

wants to merge 32 commits into from

Conversation

bembelimen
Copy link
Contributor

@bembelimen bembelimen commented Mar 28, 2021

Pull Request for Issue #31709 .

Summary of Changes

Rebuild of the com_csp extension.

Instead of generic configuration one can apply specific entries.

Changes:

  • Detect mode separated from reporting
  • Custom reporting URL
  • Full URL + information saving
  • Individual entries
  • Minor improvements

Testing Instructions

Probably needs some fine tuning here and there. Feedback is really appreciated.

Known issues

  • base-uri
  • plugin-types
  • form-action
  • frame-ancestors
  • navigate-to

not fully supported. Not sure if they're needed in the first version, could be added later.

@zero-24

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-4.0-dev labels Mar 28, 2021
@bembelimen bembelimen changed the title Improve CSP [4.0] Improve CSP Mar 28, 2021
@brianteeman
Copy link
Contributor

As soon as the component is enabled in either enforce or report mode and site or admin then every page says an error has occurred

image

bembelimen and others added 6 commits March 28, 2021 14:11
Co-authored-by: Brian Teeman <brian@teeman.net>
Co-authored-by: Brian Teeman <brian@teeman.net>
…er.php

Co-authored-by: Brian Teeman <brian@teeman.net>
Co-authored-by: Brian Teeman <brian@teeman.net>
Co-authored-by: Brian Teeman <brian@teeman.net>
Co-authored-by: Brian Teeman <brian@teeman.net>
bembelimen and others added 5 commits March 28, 2021 14:29
@bembelimen
Copy link
Contributor Author

When you publish a report (which is now renamed to enabled/disabled) the set value is used in the header of the CSP header.

@brianteeman
Copy link
Contributor

brianteeman commented Apr 1, 2021

Click on the status icon
Expected behavior: The item status is toggled
Actual behavior: The item is selected only

This only happens in enforce mode and means that you cannot disable a line that is breaking your site without disabling com_csp first

@brianteeman
Copy link
Contributor

Run CSP in detect mote and gather some reports
Disable CSP in the options
image

The list view is now completely broken
image

@brianteeman
Copy link
Contributor

So for me I still think that even with all of the changes in this PR this component is not usable by the average user and should not be included in the core of Joomla. The UI is far too confusing and its far too easy to completely break your site. I can foresee that the default response to any bug report in the future will be "did you enable com_csp? please disable it and try again"

The problems are from my perspective

  1. An assumption that csp is there to protect my site from "unwelcome" insecure content on my site. I would not expect that enabling one of these rules would have any impact on a clean install of Joomla. Try it and you will see your admin break.
  2. I have worked out that the url and information columns are for reference only. The columns that count are blocked element and directive. This is the cause of the main problem with the component. In a regular joomla component eg com_content the clickable item in the list is the unique identifier. So the expected behaviour when you enable that item is that it applies to the url. But what it actually refers to is the Blocked Element and Directive
  3. Which leads directly to the third problem. You only need to enable one line that has a blocked element of "unsafe-inline" and a directive of "script-src-attr" for this to be applied to the entire site. However there is no change in the UI for the other lines so a user would not expect that line to be enabled.

Looking at the screenshot below a typical Joomla user eg like me who is still learning about content-security-policy would assume that only the enabled rows are "active" and that the items in the red box are all disabled. After all that is what the status says.
image

However its not true. enabling one line will make any other line with the same blocked element and directive also enabled but the UI will never show that

As a result I would expect that a user would (just like I did) enable mutliple rows with the same blocked element and directive. Then on finding that their site is broken they go to the com_csp (that would be a guess from the user based on it perhaps being the last they thing did - otherwise they waste hours tracking down the bug) and look for a line that is enabled for the URL they have observed the problem on. Seeing that there is an enabled rule they make the educated guess that the rule is what is breaking their site and they disable the line. But guess what their site is still broken. Because there is another line for a completely different component which is unrelated to the bug you are investigating but happens by coincidence have the same blocked element and directive and that is enabled. So disabling the first line had no impact as the second line (which might be on page 99) also needs to be disabled.

Sorry but I can't see a way around these issues. Its not suitable for the core and should be an extension. I have to be honest and say that even though I now understand the different csp directives etc I wouldn't install it myself.

@brianteeman
Copy link
Contributor

Final comment.
On a completely clean install of Joomla (with this PR of course) go to com_csp and enable it and set it to enforce and admin. Do not change anything else
image

As I have not enabled anything in the options OR enabled anything that has been reported I would expect that Joomla will continue to work as before.

However that is not the case. The skipto plugin is broken, the ability to select an item in a list is broken, tinymce is broken and I suspect if I continued there would be even more things broken.

This is because by default without doing anything more than enabling the component the following policy is enabled
content-security-policy: default-src 'self' ; frame-ancestors 'self'; upgrade-insecure-requests; block-all-mixed-content

Even if you go into the component options again and disable the only two options that were enabled by default you still have a broken site as you still have a csp that breaks the core

content-security-policy: default-src 'self' ; block-all-mixed-content

@brianteeman
Copy link
Contributor

@rdeutz please re-open my pr to remove com_csp

@zero-24
Copy link
Contributor

zero-24 commented Apr 1, 2021

Yes but thats not an issue of com_csp but that the core still uses inline scripts. The intention for 4.x is the focus on the frontend, for that reason its also the default option. ;-)

And when the complete process is followed meaning: report only, detect mode, review the reports, enforce mode. Than also the backend works.

With the extensions done here even that can be avoided when the hashes have been added to the reports.

CSP isnt as easy as just enabeling two options but with the tooling here it is much simpler than running all of that manually.

@brianteeman
Copy link
Contributor

Yes but thats not an issue of com_csp but that the core still uses inline scripts.

You cant separate the two

And when the complete process is followed meaning: report only, detect mode, review the reports, enforce mode. Than also the backend works.

That would be the undocumented process that I'be been requesting documentation for

@bembelimen
Copy link
Contributor Author

I agree with you, that CSP is not that easy and there are still some big gaps in the component, but I'm more in favour of disabling minor stuff we don't need and try to improve the rest. (Now as the component is tested the first time in a "real" environment)

So for starter (I hope you're willing to continue giving feedback) I would suggest:

  • remove the admin client to prevent breaking in the backend
  • find a good solution for the list link. THB I have no good idea yet, the link is only important to see, where the violation occoured. So it can be made small.
  • Add some more hints/warnings when enabling e.g. the enforce mode (a text separator with showon?)
  • Get some documentation written for the help page (maybe @zero-24 can help here?)
  • Improve the warning text recognizion when unsafe-inline is activated somewhere (there is still a check, but I'm not sure if it's still correct).

@brianteeman
Copy link
Contributor

The intention for 4.x is the focus on the frontend, for that reason its also the default option. ;-)

Just by enabling com_csp for the site (instead of the admin as I did before) you block 5 assets including the main banner image

Now as the component is tested the first time in a "real" environment

I tested this before and made many of these comments before. However almost no one else has tested it because they simply didnt understand what it was or how to use it. Thats still the case and I am still the only person testing. I am aware that both the bug squad and cms release team have said that they are working on testing the release blockers so I can only assume that they are also not testing this because theydont understand it.

Do you see a pattern here. Software merged before it was ready so it could be "fixed later" only for the originaly committer to abdon it for others to try and fix. All the while adding further delay to the release of J4.

@bembelimen you should know this better than most as 15 months ago there was a debate about pulling workflows or rewriting as it was seen as THE release blocker for J4. Some of us pointed out that there were many release blockers and here we are 15 moths later still with the same release blockers.

find a good solution for the list link. THB I have no good idea yet, the link is only important to see, where the violation occoured. So it can be made small.

I would change it so that the first column is the blocked element / directive. and then the individual reports eg url and sample are grouped as sub-record of that master row

Get some documentation written for the help page (maybe @zero-24 can help here?)

I've been asking for this since the very first day as @zero-24 knows and still no signs of it.

@bembelimen
Copy link
Contributor Author

@bembelimen you should know this better than most as 15 months ago there was a debate about pulling workflows or rewriting as it was seen as THE release blocker for J4. Some of us pointed out that there were many release blockers and here we are 15 moths later still with the same release blockers.

Yep, I know that the workflow was also on the brick, but I also hope, that you saw, that I try to hold my promises by fixing it. (And I'm aware, that there are still two RB concerning the workflow).

I also see, that you spend a lot of time into the CSP, that's why I'm grateful, that you give feedback here. I'm willing to spend time to get this component release ready (probably by cutting different features out). But it will not work without you helping, so I would give you the choice: if you see that we get this component working, I'm willing to spend the time to finish this PR. If you say: no way, then we can stop and throw it away.
I'm very unemotional here, both ways are good for me, I just wanted to try the positive way first.

@brianteeman
Copy link
Contributor

I also hope, that you saw, that I try to hold my promises by fixing it.

Exactly which is why when you asked for two weeks I sat back and waited patiently

I also see, that you spend a lot of time into the CSP, that's why I'm grateful, that you give feedback here.

Just as I spent a lot of time testing workflows, atum and cassiopeia

But it will not work without you helping,

What about the rest of the contributors. There are 46 voting members of the production team. Are you really saying that if I (not a member of any team) dont spend my time testing this then there is no one else?

If you say: no way, then we can stop and throw it away.

Now who is being emotional ;) I can guarantee that if I say no way then the code wont be removed.

If the release lead doesn't have the time to make decisions then hopefully the department lead will make those decisions.

It shouldnt all be left to one person writing code and one person testing it

@zero-24
Copy link
Contributor

zero-24 commented Apr 1, 2021

I've been asking for this since the very first day as @zero-24 knows and still no signs of it.

Initial docs and help pages have been written long before a few other joomla 4 features have even been added to the docs ans how com_csp works haven been even recored on video and reported on in the magazine ;-)

But as mentiond many times i'm happy to extend that docs with whatever is usefull. Given its changing here a bit again i will add the docs required label here and review the docs again once thats merged here.

It shouldnt all be left to one person writing code and one person testing it

Its not only you and Benjamin i'm also following here but have not found the time yet to test this PR but its still on my radar.

Just by enabling com_csp for the site (instead of the admin as I did before) you block 5 assets including the main banner image

What browser? When the image directive is not set it should fallback to the default-src and therby allow all images loaded from the local site. When there are issues with that i would like to take a look into that browser.

And what other settings do you have enabled?

Without any rules set (no detect have been run) it should block google fonts but thats expected as it has not been allowed yet.

@brianteeman
Copy link
Contributor

Clean install with blog sample data
Enable csp and set to enforce. Leave all other settings at default

Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self' ". Either the 'unsafe-inline' keyword, a hash ('sha256-DAE1fuxB77vgFrnzh/MDpvXhwa2NBTvJI03Bi5bjlao='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'style-src' was not explicitly set, so 'default-src' is used as a fallback.

localhost/:569 Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self' ". Either the 'unsafe-inline' keyword, a hash ('sha256-QrY9xLGRXYstoA3w0N9CbHwyo4ImyVQ9hbncN+0qbXM='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'style-src' was not explicitly set, so 'default-src' is used as a fallback.

localhost/:574 Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self' ". Either the 'unsafe-inline' keyword, a hash ('sha256-QrY9xLGRXYstoA3w0N9CbHwyo4ImyVQ9hbncN+0qbXM='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'style-src' was not explicitly set, so 'default-src' is used as a fallback.

localhost/:579 Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self' ". Either the 'unsafe-inline' keyword, a hash ('sha256-QrY9xLGRXYstoA3w0N9CbHwyo4ImyVQ9hbncN+0qbXM='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'style-src' was not explicitly set, so 'default-src' is used as a fallback.

localhost/:584 Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self' ". Either the 'unsafe-inline' keyword, a hash ('sha256-QrY9xLGRXYstoA3w0N9CbHwyo4ImyVQ9hbncN+0qbXM='), or a nonce ('nonce-...') is required to enable inline execution. Note also that 'style-src' was not explicitly set, so 'default-src' is used as a fallback.

The first of those blocks the hero image
<div class="mod-custom custom banner-overlay" style="background-image: url(/joomla-cms/images/banners/banner.jpg)" >

@zero-24
Copy link
Contributor

zero-24 commented Apr 1, 2021

Thats are all inline style issues and are blocked because inline styles are not allowed by your CSP. Using the report stuff done here you can whitelist them by using the hash and dont relay on unsafe-inline.

@brianteeman
Copy link
Contributor

brianteeman commented Apr 1, 2021

You are completely missing the point.

Simply enabling com_csp on a default joomla install should not break it.

@alikon
Copy link
Contributor

alikon commented Apr 1, 2021

it's already disabled by default
so use it if you know what your are doing could help ?
no release blocker imho

@zero-24
Copy link
Contributor

zero-24 commented Apr 1, 2021

You are completely missing the point.

Simply enabling com_csp on a default joomla install should not break it.

Agree but thats not an issue within the CSP Component but that some core stuff still uses inline styles or inline scripts while it should not.

So the sample data has to be patched in this case. ;-)

@zero-24
Copy link
Contributor

zero-24 commented Apr 1, 2021

The component only Highlights that issue now, so please dont shout the mesanger ;-)

@brianteeman
Copy link
Contributor

brianteeman commented Apr 1, 2021

Whichever way you look at it and pass the blame. Enabling the component breaks both the site and the admin. They both work perfectly without it. Even if you know what csp is you would not expect it to break a default installation just by turning it on. The fact that you dont see it as a release blocker or worthy of your time to test it is because like far too many members of the production department you never use Joomla in the real world.

@bembelimen now I see why people don't test this. they are told that they dont know what they are doing or that the error is elsewhere.

To answer your previous question if I was still willing to test your changes - the answer is therefore no. I would rather clean the fluff from my navel, it would be more productive.

@alikon
Copy link
Contributor

alikon commented Apr 1, 2021

They both work perfectly without it.

that's my point
😃

@bembelimen
Copy link
Contributor Author

To answer your previous question if I was still willing to test your changes - the answer is therefore no. I would rather clean the fluff from my navel, it would be more productive.

Thanks for your past tests.

@bembelimen bembelimen closed this Apr 1, 2021
@brianteeman
Copy link
Contributor

@bembelimen thanks for trying

@brianteeman
Copy link
Contributor

So the sample data has to be patched in this case. ;-)

Just for future reference it is NOT the sample data that has the background style - it is a core functionality of the custom module

@zero-24
Copy link
Contributor

zero-24 commented Apr 2, 2021

I'm sad that this PR got closed :( I would still be happy to work and improve the CSP stuff.

Enabling the component breaks both the site and the admin.
They both work perfectly without it.

Thats true for most security systems right? When you install a lock on your door you cant got into it without giving the key to everyone who still should use it (setting up an allowlist) but it works too without it but than everyone can come into that door. And without that seccond step the door is locked for everyone else but yourself.

Even if you know what csp is you would not expect it to break a default installation just by turning it on.

Agree but the people that know CSP also know that when i still use inline styles or scripts on my site there is no magic button i can hit and expect everything to work too as CSP is usually very site specific and intended to block inline stuff by default.

The fact that you dont see it as a release blocker

That has not been said here right? The label is also here right?

or worthy of your time to test it is

Its 5 days since this PR was opend, I'm sorry that i dont test all PRs i'm looking into insted after they are opend here? I only said i have not found yet the time to test it.

Just for future reference it is NOT the sample data that has the background style - it is a core functionality of the custom module

Point taken while the sample data also has a few places where it still uses inline styles too. I thourgh this would be one of them.

@brianteeman
Copy link
Contributor

brianteeman commented Apr 2, 2021

The fact that you dont see it as a release blocker

That has not been said here right? The label is also here right?

Not every comment is addressed at Tobias. Yes it has been said here.

Its 5 days since this PR was opend,

And how many months since people started reporting that it was not usable?

Your analogy about the door is a good one. When you fit a door with a new lock you can still open the door with the key. You dont have to find a keymaker who is able to make a new key for you. Even if you did have to find a keymaker then the door would be closed for everyone and not left hanging on its hinges only partially ,opening and closing.

The simple fact is that as I have said since day one this component is not suitable for inclusion in the core of joomla as it is beyond the skillset of the average user to configure. If it didn't break their site if they enabled it to see what it did it wouldn't be so bad but as it does then its really bad experience.

As for the documentation do you really think that this is a correct description and enough information https://help.joomla.org/proxy?keyref=Help40:Content_Security_Policy_Reports&lang=en

image

@bembelimen bembelimen deleted the 4.0/31709 branch January 23, 2024 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants