Send a configurable CSP in every HTML response #9665

pabzm · 2024-10-07T14:26:37Z

The CSP gets adapted to remote objects being allowed or not. It can be configured or disabled via the config option content_security_policy (and
content_security_policy_add_allow_remote).

The default is pretty weak in order to not introduce a breaking change, but still not useless (e.g. it adds another layer of defence against loading remote objects unless allowed.)

Closes #9638

pabzm · 2024-10-22T08:43:35Z

@alecpl Same question for this: would reviews by other developers help you?

alecpl · 2024-10-22T08:58:54Z

I'm not sure it would help me, but it would likely help the pull request move forward.

pabzm · 2024-10-23T13:31:00Z

Are there other things we can do to ease your load? E.g. get more ticket work off your back? Or release/version/patch-management? I notice you're backporting many changes that go into the master-branch to one or two other branches, I guess that's a lot to juggle, too.

pabzm · 2024-10-23T13:53:59Z

@johndoh, @mvorisek Do you feel competent to review this PR? It would help us a lot, even if you're not perfectly sure about all parts, just tell us about your opinion.

mvorisek · 2024-10-24T12:26:11Z

program/include/rcmail_output_html.php

+        $csp = $this->app->config->get('content_security_policy');
+        if (!in_array($csp, ['', false, 'false'])) {
+            $csp_header = "Content-Security-Policy: {$csp}";
+            if (isset($this->env['safemode']) && $this->env['safemode'] === true) {


This needs E2E test as it is it can very easily break security if coded wrongly.

Testing is a good point 👍

Do you have something specific in mind regarding breaking security?

I mean we need to be sure external resources are not fetched (otherwise spammers know what email addresses to spam even more) in every other case than explicitly configured so.

I agree, but I think this change doesn't enlarge that danger. It only adds a Content-Security-Policy, which should reduce that danger. The safe-flag is not touched.

Do you see any way in which this change could be dangerous?

alecpl · 2024-10-24T12:31:39Z

I'm not convinced that providing such configuration options is a good idea.

johndoh · 2024-10-25T18:43:14Z

I know nothing about CSP headers. From a Roundcube code style point of view only I would say that

if (!in_array($csp, ['', false, 'false'])) {

is wrong and it should be

if ($csp = $this->app->config->get('content_security_policy', "default-src 'self' data: blob:; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline';")) {

this means, i think, in common with other config values null reverts to a hard coded default and false disables (e.g imap_ns_*).

I would also say the comment in defaults.inc.php needs to mention that content_security_policy must end in a ; or the header will be invalid when concatenated with content_security_policy_add_allow_remote

pabzm · 2024-10-28T10:26:52Z

I'm not convinced that providing such configuration options is a good idea.

@alecpl Why? I thought some people might need to change the CSP (e.g. for plugins using external resources), and this way they wouldn't need to patch the code.

pabzm · 2024-10-28T14:52:01Z

I know nothing about CSP headers.

@johndoh But your comments are helpful, thank you very much!

this means, i think, in common with other config values null reverts to a hard coded default and false disables

For some values that's right, but defaults.inc.php has a lot of actual default values, too. E.g. most booleans, but also oauth_user_create_map, or blankpage_url.
I would like to avoid repeating the default value, because it's pretty complex in this case – but at the same time it needs to be visible outside of the actual code as a reference. Thus I find the other way more appropriate.
Is there a declared will or style guide towards one of the default value methods?

content_security_policy must end in a ;

Thank you for that catch! I'l solve that in code so admins can't break the CSP so easily by mistake.

The CSP gets adapted to remote objects being allowed or not. It can be configured or disabled via the config option `content_security_policy` (and `content_security_policy_add_allow_remote`).

johndoh · 2024-10-29T20:31:44Z

I would like to avoid repeating the default value,

I agree with this.

I think in the past there has been issues with defaults.inc.php not being properly updated in 3rd party packaged versions of Roundcube (which I acknowledge are not our problem) and people affected by that create issues here. The comment could suggest a value while the code has a reliable default so if defaults.inc.php was not updated there is no unpredictable behavior.

Personally I just don't like the in_array check, I think its unnecessary because it handles the edge case where someone set 'false' vs false and I think in general Roundcube does not do that so why do it for this one var.

If people write a lax default CSP they might set the additional config option to the blank string, or false. Then the CSP header should not contain that value.

This way the code always has values it can work with, no matter how good or broken the configuration is.

pabzm · 2024-10-30T13:19:21Z

I think in the past there has been issues with defaults.inc.php not being properly updated in 3rd party packaged versions of Roundcube

Oh, I wasn't aware of that. Thank you for that explanation! Then we maybe should have a hardcoded value for all defaults.

I changed the approach to specifying the default value in defaults.inc.php, and having a hardcoded default. And to check that these are in sync, I wrote a test. What do you think about this way?

(I'd be happy to extend this approach to more config options, it would reduce some special case handling in rcube_config, too.)

Personally I just don't like the in_array check, I think its unnecessary because it handles the edge case

I also find it ugly, but I really like solid code, which isn't vulnerable to edge cases. But maybe checking against 'false' is too much. Regarding readability I can turn the check around by using is_string($csp) && strlen($csp) > 0.

Previously the code also treated `'false'` (the string) as invalid, but that's a very specific check against a specific edge case, which wouldn't even break the code (but will only make the browser complain), so I'm dropping that check.

The config value might be something else than a string.

In the previous way useless semicolons could have happened.

We still support PHP v7, which doesn't support union types.

phpstan complains that `assertIsArray($config)` will always fail because it doesn't know about the side effects of `require`ing the default config.

pabzm · 2024-11-06T08:39:11Z

I'm not convinced that providing such configuration options is a good idea.

@alecpl Why? Some people might need to change the CSP (e.g. for plugins using external resources).

alecpl · 2024-11-06T10:12:13Z

Because it's easy to do wrong. Because CSP can be set on the http server and Roundcube cannot relax the rules, it can only make them more strict. Because the initial problem was lack of documentation, not lack of options. In other words this might be one of those power-user options that may bring more harm than value.

That being said, I don't really see a middle ground here. Either we set CSP (and need options and ways for plugins) or not.

pabzm · 2024-11-06T13:55:20Z

From your tone and from the experience of other comment threads I assume that you're not interested in an open discussion of the points you listed. (Please prove me wrong.)

Which makes me wonder what you want from this PR. If you want to veto the change, please say so clearly. If not: please suggest a way forward.

alecpl · 2024-11-06T14:23:26Z

I don't want it to sound like a veto, but I think we should not merge this. As other security-related headers that you can set, add the header sample to the .htaccess file or extend CONTENT-SECURITY-POLICY section in the INSTALL file (we'll get rid of the .htaccess file at some point).

It does not mean we shouldn't make Roundcube compatible with more strict rules in the future, but Roundcube should not set the header itself, imo. Of course, there's always exceptions, we use CSP header on file downloads.

pabzm · 2024-11-07T15:05:11Z

This change increases security by introducing a CSP that is stricter if the safe-mode is off. How would you suggest to do that in a HTTP daemon config?

And it increases security of all installations by ensuring from code that the CSP is always sent, not only where an admin took the extra step to configure it.

And it doesn't break anything in a default installation.

Of course people can make themselves a problem with the config option, but that is true for a lot of the config options we already have. How is this one different?

pabzm · 2024-11-08T08:29:48Z

@johndoh @mvorisek Do you have an opinion on this PRs approach vs. Alec's suggestion?

pabzm force-pushed the always-send-csp branch from e7c37da to 2d74fe8 Compare October 7, 2024 14:34

pabzm requested a review from alecpl October 7, 2024 14:40

mvorisek reviewed Oct 24, 2024

View reviewed changes

pabzm force-pushed the always-send-csp branch from e1f10e9 to 54928f4 Compare October 28, 2024 16:45

pabzm added 3 commits October 29, 2024 17:41

Send a configurable CSP in every HTML response

801f375

The CSP gets adapted to remote objects being allowed or not. It can be configured or disabled via the config option `content_security_policy` (and `content_security_policy_add_allow_remote`).

Ensure proper semicolons between CSP-parts.

0d1b036

Also use default value in case of null

3882b73

pabzm force-pushed the always-send-csp branch 2 times, most recently from ba0ab41 to a9e0110 Compare October 29, 2024 20:12

pabzm force-pushed the always-send-csp branch from a9e0110 to 304ae70 Compare October 29, 2024 20:44

pabzm added 2 commits October 29, 2024 23:05

Add unit-tests for adding CSP header

6f00ac2

Add additional CSP header values only if present

feb8e9f

If people write a lax default CSP they might set the additional config option to the blank string, or false. Then the CSP header should not contain that value.

pabzm force-pushed the always-send-csp branch from 304ae70 to feb8e9f Compare October 29, 2024 22:05

pabzm added 3 commits October 30, 2024 10:31

Use invokeMethod instead of overriding it in a mock

bc6b241

Use hardcoded defaults for CSP options

5db41a1

This way the code always has values it can work with, no matter how good or broken the configuration is.

Test to check if hardcoded option values match defaults

383e8b5

pabzm added 4 commits October 30, 2024 14:39

Improve readability of CSP value checks

8b631ab

Previously the code also treated `'false'` (the string) as invalid, but that's a very specific check against a specific edge case, which wouldn't even break the code (but will only make the browser complain), so I'm dropping that check.

Fix static analysis.

10f7598

Fix CSP option type handling

1afa897

The config value might be something else than a string.

Make CSP header building more robust.

37db3ef

In the previous way useless semicolons could have happened.

pabzm added 3 commits October 30, 2024 15:23

Remove type annotation because PHP v7

ef5aae3

We still support PHP v7, which doesn't support union types.

Mitigate erronous complaint of phpstan

b1ea913

phpstan complains that `assertIsArray($config)` will always fail because it doesn't know about the side effects of `require`ing the default config.

Tiny corrections to make the linter happy

fda480b

pabzm force-pushed the always-send-csp branch from cfcabf0 to fda480b Compare October 30, 2024 15:10

pabzm self-assigned this Nov 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send a configurable CSP in every HTML response #9665

Send a configurable CSP in every HTML response #9665

pabzm commented Oct 7, 2024 •

edited

Loading

pabzm commented Oct 22, 2024

alecpl commented Oct 22, 2024

pabzm commented Oct 23, 2024

pabzm commented Oct 23, 2024

mvorisek Oct 24, 2024

pabzm Oct 28, 2024

mvorisek Oct 28, 2024

pabzm Oct 30, 2024

alecpl commented Oct 24, 2024

johndoh commented Oct 25, 2024

pabzm commented Oct 28, 2024

pabzm commented Oct 28, 2024

johndoh commented Oct 29, 2024

pabzm commented Oct 30, 2024

pabzm commented Nov 6, 2024

alecpl commented Nov 6, 2024

pabzm commented Nov 6, 2024

alecpl commented Nov 6, 2024

pabzm commented Nov 7, 2024

pabzm commented Nov 8, 2024

Send a configurable CSP in every HTML response #9665

Are you sure you want to change the base?

Send a configurable CSP in every HTML response #9665

Conversation

pabzm commented Oct 7, 2024 • edited Loading

pabzm commented Oct 22, 2024

alecpl commented Oct 22, 2024

pabzm commented Oct 23, 2024

pabzm commented Oct 23, 2024

mvorisek Oct 24, 2024

Choose a reason for hiding this comment

pabzm Oct 28, 2024

Choose a reason for hiding this comment

mvorisek Oct 28, 2024

Choose a reason for hiding this comment

pabzm Oct 30, 2024

Choose a reason for hiding this comment

alecpl commented Oct 24, 2024

johndoh commented Oct 25, 2024

pabzm commented Oct 28, 2024

pabzm commented Oct 28, 2024

johndoh commented Oct 29, 2024

pabzm commented Oct 30, 2024

pabzm commented Nov 6, 2024

alecpl commented Nov 6, 2024

pabzm commented Nov 6, 2024

alecpl commented Nov 6, 2024

pabzm commented Nov 7, 2024

pabzm commented Nov 8, 2024

pabzm commented Oct 7, 2024 •

edited

Loading