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

Send a configurable CSP in every HTML response #9665

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions config/defaults.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -1563,3 +1563,14 @@
// 0 - Reply-All always
// 1 - Reply-List if mailing list is detected
$config['reply_all_mode'] = 0;

// The Content-Security-Policy to use if no remote objects are allowed to
// be loaded. If you use plugins you might need to extend this.
// Only change this if you know what you're doing! You can break the whole
// application with changes to this setting!
// To disable completely set the value to `false`;
$config['content_security_policy'] = "default-src 'self' data: blob:; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline'";

// Additions to the Content-Security-Policy to use if remote objects *are*
// allowed to be loaded.
$config['content_security_policy_add_allow_remote'] = 'img-src *; media-src *; font-src: *; frame-src: *';
39 changes: 39 additions & 0 deletions program/include/rcmail_output_html.php
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,8 @@ public function page_headers()
$this->header('X-Frame-Options: sameorigin', true);
}
}

$this->add_csp_header();
}

/**
Expand Down Expand Up @@ -2717,4 +2719,41 @@ protected function get_template_logo($type = null, $match = null)

return $template_logo;
}

/**
* Add the Content-Security-Policy to the HTTP response headers (unless it
* is disabled).
*/
protected function add_csp_header(): void
{
$csp = $this->get_csp_value('content_security_policy');
if ($csp !== false) {
$csp_parts = [$csp];
if (isset($this->env['safemode']) && $this->env['safemode'] === true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Testing is a good point 👍

Do you have something specific in mind regarding breaking security?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

$csp_allow_remote = $this->get_csp_value('content_security_policy_add_allow_remote');
if ($csp_allow_remote !== false) {
$csp_parts[] = $csp_allow_remote;
}
}
$this->header('Content-Security-Policy: ' . implode('; ', $csp_parts));
}
}

/**
* Get a CSP-related value from the config, stripped by surrounding
* whitespace and semicolons (and NUL byte, because it's included in the
* default second argument to trim(), too).
*
* @param $name string The key of the wanted config value
*
* @return string|false
*/
protected function get_csp_value($name)
{
$value = $this->app->config->get($name);
if (is_string($value)) {
return trim($value, "; \n\r\t\v\x00");
}
return false;
}
}
11 changes: 10 additions & 1 deletion program/lib/Roundcube/rcube_config.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ class rcube_config
{
public const DEFAULT_SKIN = 'elastic';

public const DEFAULTS = [
'content_security_policy' => "default-src 'self' data: blob:; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline'",
'content_security_policy_add_allow_remote' => 'img-src *; media-src *; font-src: *; frame-src: *',
];

/** @var string A skin configured in the config file (before being replaced by a user preference) */
public $system_skin = 'elastic';

Expand Down Expand Up @@ -376,8 +381,12 @@ public function get($name, $def = null)
{
if (isset($this->prop[$name])) {
$result = $this->prop[$name];
} else {
} elseif (isset($def)) {
$result = $def;
} elseif (isset(self::DEFAULTS[$name])) {
$result = self::DEFAULTS[$name];
} else {
$result = null;
}

$result = $this->getenv_default('ROUNDCUBE_' . strtoupper($name), $result);
Expand Down
18 changes: 18 additions & 0 deletions tests/Framework/ConfigTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,22 @@ public function test_parse_env()
$this->assertSame([1], invokeMethod($object, 'parse_env', ['[1]', 'array']));
$this->assertSame(['test' => 1], (array) invokeMethod($object, 'parse_env', ['{"test":1}', 'object']));
}

// Test if values in defaults.inc.php and values in rcube_config::DEFAULTS match
public function test_defaults_values(): void
{
// Initialize the variable to avoid warnings.
$config = null;
// Load the values from defaults.inc.php manually (we don't want to
// test the whole loading mechanics of `rcube_config` here).
ob_start();
require realpath(RCUBE_INSTALL_PATH . 'config/defaults.inc.php');
ob_end_clean();

$this->assertIsArray($config); // @phpstan-ignore-line

foreach (\rcube_config::DEFAULTS as $name => $hardcoded_value) {
$this->assertSame($hardcoded_value, $config[$name], "The value for '{$name}' in defaults.inc.php does not match the hardcoded default in rcube_config!");
}
}
}
64 changes: 64 additions & 0 deletions tests/Rcmail/OutputHtmlTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\TestCase;

use function Roundcube\Tests\invokeMethod;

/**
* Test class to test rcmail_output_html class
*/
Expand Down Expand Up @@ -429,4 +431,66 @@ public function test_charset_selector()
$this->assertTrue(strpos($result, '<select name="_charset">') === 0);
$this->assertTrue(strpos($result, '<option value="UTF-8" selected="selected">UTF-8 (Unicode)</option>') !== false);
}

/**
* Test adding the Content-Security-Policy with "safemode" off.
*/
public function test_add_csp_header_default_without_safemode()
{
$headers = $this->run_add_csp_header_and_get_headers(false);

$this->assertCount(1, $headers);
$this->assertSame("Content-Security-Policy: default-src 'self' data: blob:; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline'", $headers[0]);
}

/**
* Test adding the Content-Security-Policy with "safemode" on.
*/
public function test_add_csp_header_default_with_safemode()
{
$headers = $this->run_add_csp_header_and_get_headers(true);

$this->assertCount(1, $headers);
$this->assertSame("Content-Security-Policy: default-src 'self' data: blob:; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline'; img-src *; media-src *; font-src: *; frame-src: *", $headers[0]);
}

/**
* Test adding a custom configured Content-Security-Policy.
*/
public function test_add_csp_header_custom()
{
$rcmail = \rcube::get_instance();
// Some a little strange strings.
$rcmail->config->set('content_security_policy', 'default-src * ; ');
$rcmail->config->set('content_security_policy_add_allow_remote', "; script-src 'unsafe-inline'");

$headers = $this->run_add_csp_header_and_get_headers(true);

$this->assertCount(1, $headers);
$this->assertSame("Content-Security-Policy: default-src *; script-src 'unsafe-inline'", $headers[0]);
}

protected function run_add_csp_header_and_get_headers(bool $safemode): array
{
// Use a subclass to get hands on the headers to be sent. There's not
// other way to do this, unfortunately, since the PHP CLI SAPI silently
// swallows all calls to `header()` (as done in
// `rcube_output::header()`).
// The class `OutputHtmlMock` is mocking a lot more than we need, so we
// rather roll our own.
$output = new class extends \rcmail_output_html {
public $http_headers = [];

#[\Override]
public function header($header, $replace = true)
{
$this->http_headers[] = $header;
}
};

$output->set_env('safemode', $safemode);
invokeMethod($output, 'add_csp_header');

return $output->http_headers;
}
}