[GEOS-11346] Add a configurable Content-Security-Policy header

[sikeoka]

This PR adds the Content-Security-Policy header to GeoServer responses based on the request path and query and server properties, a default configuration that should support existing GeoServer functionality, a web interface for administrators to update the configuration and documentation updates. This PR is NOT intended to be backported.

The primary benefit of this work is to provide mitigations for security issues with WMS GetFeatureInfo HTML templates (also applies to the new WFS GetFeature HTML output), the TestWfsPost servlet, static HTML files and the OGC API community modules and to encourage developers contributing new HTML functionality to follow more secure coding practices.

Checklist

• I have read the contribution guidelines.
• I have sent a Contribution Licence Agreement (not required for small changes, e.g., fixing typos in documentation).
• First PR targets the main branch (backports managed later; ignore for branch specific issues).
• All the build checks are green (see automated QA checks).

For core and extension modules:

• New unit tests have been added covering the changes.
• Documentation has been updated (if change is visible to end users).
• The REST API docs have been updated (when changing configuration objects or the REST controllers).
• There is an issue in the GeoServer Jira (except for changes that do not affect administrators or end users in any way).
• Commit message(s) must be in the form [GEOS-XYZWV] Title of the Jira ticket.
• Bug fixes and small new features are presented as a single commit.
• Each commit has a single objective (if there are multiple commits, each has a separate JIRA ticket describing its goal).

[aaime]

While I'm providing some comments on this PR, it should not be considered a review. I just don't have enough knowledge to understand what is going on it this PR.
Even compared to other security parts, this seems very complex, and I would expect most end users to leave it as-is.
If anything, I'd urge to simplify and clarify the system as much as possible, a tool for experts is normally dangerous in the hands of uneducated users.

[aaime]

Some more notes on the configuration:

• The configuration refers directly to GeoServer class names, something an end user will have no knowledge about, and introduces refactoring issues: if any class name of package changes, the default CSP policy will have to be updated (but developers responsible for that change are unlikely to know about it)
• The configuration uses a simple predicate language. The parser seems to be susceptible to extra spaces (might be wrong here). I guess the reason for the language is to avoid having to build a GUI for every test type? A GUI for each type backed by objects that then serialize to XML would have been more natural in GeoServer.

[jodygarnett]

This is getting ... very complicated! We need some assurance for users that the defaults are sensible.

[sikeoka]

Some more notes on the configuration:

• The configuration refers directly to GeoServer class names, something an end user will have no knowledge about, and introduces refactoring issues: if any class name of package changes, the default CSP policy will have to be updated (but developers responsible for that change are unlikely to know about it)

The class name filtering isn't very important to this feature so it can be removed to simplify this a little.

• The configuration uses a simple predicate language. The parser seems to be susceptible to extra spaces (might be wrong here). I guess the reason for the language is to avoid having to build a GUI for every test type? A GUI for each type backed by objects that then serialize to XML would have been more natural in GeoServer.

Making an even more complicated GUI for this seems unnecessary especially since most of the work is in writing regular expressions which would still have to be done manually by an administrator.

[aaime]

The current setup seems geared towards developers that know the internal data structures and the system variables, on top of good knowledge of CSP. Wondering if the situation should be flipped: anything in the code that has HTML output declares its presence and checks if CSP should be activated at all (sysvar checks and the like), and describes itself in terms that an end user can understand (e.g., "OGC API Features HTML representation"), there is a lookup that finds all these bits, and presents them to the user with a simple setup: do you want to enable CSP on it (yes by default), and do you need to customize the CSP headers for it, or use a default?

[sikeoka]

I removed the method type and class name filtering from this PR to simplify the configuration.

An administrator obviously needs to understand CSP in order to modify this and I don't think it would be possible to expect someone who doesn't understand CSP to create proper and secure policies.

Besides that, an administrator would really only need to know is how to create regular expression to match the URL path and query parameters for specific HTML outputs and this should only be necessary if someone is working on new GeoServer HTML output using unsafe JavaScript that is not a Wicket page, FreeMarker template or in the www directory. It should probably be preferred that new HTML output being added to GeoServer not use unsafe JavaScript if that is feasible.

There are new system properties for this feature but if an administrator were configuring this, they would probably just configure it directly to their specific needs rather than using system properties.

One advantage of this approach is that if someone wanted to, they could create very specific rules to only allow unsafe JavaScript on specific static HTML files or specific GetFeatureInfo requests instead of using the more generic rules in the default configuration.

[sikeoka]

I created five separate PRs to refactor inline JavaScript and simplify the default configuration:

GeoWebCache/geowebcache#1235
GeoWebCache/geowebcache#1236
#7554
#7560
#7587

index.html can continue to use inline JavaScript since it's a static file that someone would need permissions to modify the GeoServer WAR file to change. The wicket rule will be removed after the Wicket 9 upgrade.

With these changes, there are now only three predicates (path, query parameter and system property) and the documentation makes it clear that administrators probably won't need to use the property predicate when configuring the CSP.

[sikeoka]

Do you have any additional feedback on this PR? I think that all of your initial feedback has been addressed and the configuration has also been significantly simplified from the original version although it now relies on additional JavaScript refactoring PRs. The Wicket rule can also be removed after the Wicket 9 upgrade which will further simplify the default config. With the JavaScript refactoring, unsafe JavaScript is now only enabled on index.html and the www directory by default. Adding this header will not only mitigate existing XSS issues but also help prevent new XSS issue from getting into future releases.

[jodygarnett]

I think my feedback has been addressed. Thank you.

[jodygarnett]

I guess I join @aaime in feeling out of my depth reviewing this feature.

Reading above this is still a very tricky subject, a system administrator that has secured their system cannot really predict what new URLs will be available when upgrading GeoServer. As an example when installing OGCAPI services a whole host of new URLs is now available (that are not so static in nature as they are based on workspace and layer names...

Andrea your idea of turning the problem inside out (forcing any code output html to be checked) - could that be done at the dispatcher level if it see html output?

[sikeoka]

I'm not sure what is the really tricky part about this. All an administrator needs to this is create a regular expression to match the path, optional regular expressions to match query parameters and then add the CSP directives. The regular expressions for this feature seems slightly simpler to me than what an administrator would need to understand for the URL checks feature. CSP directives can be tricky but the default config is lenient enough to cover most use cases so it should only need to be changed to enable unsafe JavaScript or to allow connecting to remote hosts.

The only change from the CSP directives in the default rule is whether or not unsafe JavaScript is enabled. By default, unsafe-inline script is only added to index.html and www html files and unsafe-inline and unsafe-eval is added to Wicket pages although this will be removed after the Wicket 9 upgrade. There were five additional cases (including OGC API) where unsafe-inline was needed but I created separate PRs to refactor those features to not need unsafe-inline which simplifies the default configuration.
See #7514 (comment) above.

In my opinion, any new HTML functionality added to GeoServer should be designed to work without enabling unsafe JavaScript unless it is absolutely necessary which will help prevent new XSS vulnerabilities. If it is necessary to add new HTML features with unsafe JavaScript, then it would be the developer's responsibility to add it to the default configuration and this PR already includes functionality to automatically update the default config if it is changed in geoserver.

www and wicket (before upgrade)

base-uri 'self'; form-action 'self'; default-src 'none'; child-src 'self'; connect-src 'self'; font-src 'self'; img-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-inline' 'unsafe-eval';, frame-ancestors 'self';

index.html

base-uri 'self'; form-action 'self'; default-src 'none'; child-src 'self'; connect-src 'self'; font-src 'self'; img-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-inline';, frame-ancestors 'self';

all other requests

base-uri 'self'; form-action 'self'; default-src 'none'; child-src 'self'; connect-src 'self'; font-src 'self'; img-src 'self'; style-src 'self' 'unsafe-inline'; script-src 'self';, frame-ancestors 'self';