From d8289ef29d2a09cfaf732a12adf4ef602c4a51dc Mon Sep 17 00:00:00 2001 From: Ralf Becker Date: Tue, 28 Jan 2020 17:14:38 +0100 Subject: [PATCH] stricter CSP policy --- api/src/Framework/Login.php | 4 +- api/src/Header/Content.php | 13 ++-- api/src/Header/ContentSecurityPolicy.php | 78 +++++++++++++++--------- api/src/Html/HtmLawed.php | 2 +- 4 files changed, 59 insertions(+), 38 deletions(-) diff --git a/api/src/Framework/Login.php b/api/src/Framework/Login.php index 3a8cff19c4..bade353db1 100644 --- a/api/src/Framework/Login.php +++ b/api/src/Framework/Login.php @@ -46,7 +46,9 @@ class Login */ function screen($extra_vars, $change_passwd=null) { - Api\Header\ContentSecurityPolicy::add('frame-src', array()); // array() no external frame-sources + // set a more limiting CSP for our login page + Api\Header\ContentSecurityPolicy::add('frame-src', 'none'); + Api\Header\ContentSecurityPolicy::add('media-src', 'none'); //error_log(__METHOD__."() this->template=$this->framework->template, this->template_dir=$this->framework->template_dir, get_class(this)=".get_class($this)); try { diff --git a/api/src/Header/Content.php b/api/src/Header/Content.php index dfcf89a7ea..52ef7e5c9e 100644 --- a/api/src/Header/Content.php +++ b/api/src/Header/Content.php @@ -73,8 +73,8 @@ class Content $length += 5; } } - // mitigate risk of html downloads by using CSP or force download for IE - if (!$force_download && in_array($mime, array('text/html', 'application/xhtml+xml'))) + // mitigate risk of html (or SVG) downloads by using CSP or force download for IE + if (!$force_download && in_array($mime, ['text/html', 'application/xhtml+xml', 'image/svg+xml'])) { // use CSP only for current user-agents/versions I was able to positivly test if (UserAgent::type() == 'chrome' && UserAgent::version() >= 24 || @@ -83,11 +83,8 @@ class Content UserAgent::type() == 'safari' && !UserAgent::mobile() && UserAgent::version() >= 536 || // OS X UserAgent::type() == 'safari' && UserAgent::mobile() && UserAgent::version() >= 9537) // iOS 7 { - $csp = "script-src 'none'"; // forbid to execute any javascript - header("Content-Security-Policy: $csp"); - header("X-Webkit-CSP: $csp"); // Chrome: <= 24, Safari incl. iOS - //header("X-Content-Security-Policy: $csp"); // FF <= 22 - //error_log(__METHOD__."('$options[path]') ".UserAgent::type().'/'.UserAgent::version().(UserAgent::mobile()?'/mobile':'').": using Content-Security-Policy: $csp"); + // forbid to execute any javascript (to be precise anything but images and styles) + ContentSecurityPolicy::header("image-src 'self' data: https:; style-src 'self' 'unsafe-inline' https:; default-src 'none'"); } else // everything else get's a Content-dispostion: attachment, to be on save side { @@ -95,7 +92,7 @@ class Content $force_download = true; } } - // always tell browser to do not sniffing / use our content-type + // always tell browser to do no sniffing / use our content-type header('X-Content-Type-Options: nosniff'); if ($no_content_type) diff --git a/api/src/Header/ContentSecurityPolicy.php b/api/src/Header/ContentSecurityPolicy.php index 6dec18b480..a4b24940a2 100644 --- a/api/src/Header/ContentSecurityPolicy.php +++ b/api/src/Header/ContentSecurityPolicy.php @@ -20,20 +20,28 @@ use EGroupware\Api; class ContentSecurityPolicy { /** - * Additional attributes or urls for CSP beside always added "self" + * Additional attributes or urls for CSP beside always added 'self' for everything not 'none' * * - "script-src 'self' 'unsafe-eval'" allows only self and eval, but forbids inline scripts, onchange, etc * - "connect-src 'self'" allows ajax requests only to self * - "style-src 'self' 'unsafe-inline'" allows only self and inline style, which we need * - "frame-src 'self' manual.egroupware.org" allows frame and iframe content only for self or manual.egroupware.org + * - "manifest-src 'self'" + * - "media-src 'self' data:" + * - "img-src 'self' data: https:" + * - "default-src 'none'" disallows all not explicitly set sources * * @var array */ - private static $sources = array( - 'script-src' => array("'unsafe-eval'"), - 'style-src' => array("'unsafe-inline'"), + private static $sources = array( // our dhtmlxcommon version (not the current) uses eval, + 'script-src' => array("'unsafe-eval'"), // sidebox javascript links, et2_widget_date / jQueryUI datepicker, maybe more + 'style-src' => array("'unsafe-inline'"), // eTemplate styles and custom framework colors 'connect-src' => null, // NOT array(), to allow setting no default connect-src! 'frame-src' => null, // NOT array(), to allow setting no default frame-src! + 'manifest-src'=> ["'self'"], + 'media-src' => ["data:"], + 'img-src' => ["data:", "https:"], + 'default-src' => ["'none'"], // disallows all not explicit set sources! ); /** @@ -41,9 +49,8 @@ class ContentSecurityPolicy * * Calling this method with an empty array for frame-src, sets no defaults but "'self'"! * - * @param string|array $set =array() URL (incl. protocol!) * @param string $source valid CSP source types like 'script-src', 'style-src', 'connect-src', 'frame-src', ... - * @param string|array $attrs 'unsafe-eval' and/or 'unsafe-inline' (without quotes!) or URL (incl. protocol!) + * @param string|array $attrs 'unsafe-eval', 'unsafe-inline' (without quotes!), full URLs or protocols (incl. colon!) */ public static function add($source, $attrs) { @@ -53,12 +60,14 @@ class ContentSecurityPolicy if (in_array($source, ['frame-src', 'connect-src']) && !isset($attrs)) { $attrs = []; - // no permssion / user-run-rights check for connect-src - if (($app_additional = Api\Hooks::process('csp-'.$source, [], $source === 'connect-src'))) + // for regular (non login) pages, call hook allowing apps to add additional frame- and connect-src + if (basename($_SERVER['PHP_SELF']) !== 'login.php' && + // no permission / user-run-rights check for connect-src + ($app_additional = Api\Hooks::process('csp-'.$source, [], $source === 'connect-src'))) { - foreach($app_additional as $addtional) + foreach($app_additional as $app => $additional) { - if ($addtional) $attrs = array_unique(array_merge($attrs, $addtional)); + if ($additional) $attrs = array_unique(array_merge($attrs, $additional)); } } } @@ -81,13 +90,11 @@ class ContentSecurityPolicy /** * Set Content-Security-Policy attributes for script-src: 'unsafe-eval' and/or 'unsafe-inline' * - * Using CK-Editor currently requires both to be set :( - * * Old pre-et2 apps might need to call Api\Headers::script_src_attrs(array('unsafe-eval','unsafe-inline')) * * EGroupware itself currently still requires 'unsafe-eval'! * - * @param string|array $set =array() 'unsafe-eval' and/or 'unsafe-inline' (without quotes!) or URL (incl. protocol!) + * @param string|array $set 'unsafe-eval', 'unsafe-inline' (without quotes!), full URLs or protocols (incl. colon!) */ public static function add_script_src($set=null) { @@ -99,7 +106,7 @@ class ContentSecurityPolicy * * EGroupware itself currently still requires 'unsafe-inline'! * - * @param string|array $set =array() 'unsafe-inline' (without quotes!) and/or URL (incl. protocol!) + * @param string|array $set 'unsafe-eval', 'unsafe-inline' (without quotes!), full URLs or protocols (incl. colon!) */ public static function add_style_src($set=null) { @@ -109,7 +116,7 @@ class ContentSecurityPolicy /** * Set Content-Security-Policy attributes for connect-src: * - * @param string|array $set =array() URL (incl. protocol!) + * @param string|array $set 'unsafe-eval', 'unsafe-inline' (without quotes!), full URLs or protocols (incl. colon!) */ public static function add_connect_src($set=null) { @@ -121,8 +128,7 @@ class ContentSecurityPolicy * * Calling this method with an empty array sets no frame-src, but "'self'"! * - * @param string|array $set =array() URL (incl. protocol!) - * @return string with attributes eg. "'unsafe-inline'" + * @param string|array $set 'unsafe-eval', 'unsafe-inline' (without quotes!), full URLs or protocols (incl. colon!) */ public static function add_frame_src($set=null) { @@ -136,27 +142,43 @@ class ContentSecurityPolicy */ public static function send() { - self::add('connect-src', null); // set defaults for connect-src (no run rights checked) - self::add('frame-src', null); // set defaults for frame-src + self::add('connect-src', null); // set defaults for connect-src (no run rights checked) + self::add('frame-src', null); // set defaults for frame-src + + // force default-src 'none' + self::$sources['default-src'] = ["'none'"]; $policies = array(); - foreach(self::$sources as $source => $urls) - { - $policies[] = "$source 'self' ".implode(' ', $urls); + foreach (self::$sources as $source => $urls) { + // for 'none' remove source, as we use "default-src 'none'" + if (in_array("'none'", $urls)) { + if ($source !== 'default-src') continue; + } + // automatic add 'self', if not 'none' + elseif (!in_array("'self'", $urls)) { + array_unshift($urls, "'self'"); + } + $policies[] = "$source " . implode(' ', $urls); } - $csp = implode('; ', $policies); - - //$csp = "default-src * 'unsafe-eval' 'unsafe-inline'"; // allow everything + self::header(implode('; ', $policies)); + } + /** + * Send a CSP header with given policy + * + * @param {string} $csp + */ + public static function header($csp) + { $user_agent = UserAgent::type(); $version = UserAgent::version(); - // recommendaton ist to not send regular AND deprecated headers together, as they can cause unexpected behavior - if ($user_agent == 'chrome' && $version < 25 || $user_agent == 'safari' && $version < 7) + // recommendation is to not send regular AND deprecated headers together, as they can cause unexpected behavior + if ($user_agent === 'chrome' && $version < 25 || $user_agent === 'safari' && $version < 7) { header("X-Webkit-CSP: $csp"); // Chrome: <= 24, Safari incl. iOS } - elseif ($user_agent == 'firefox' && $version < 23 || $user_agent == 'msie') // Edge is reported as 'edge'! + elseif ($user_agent === 'firefox' && $version < 23 || $user_agent === 'msie') // Edge is reported as 'edge'! { header("X-Content-Security-Policy: $csp"); } diff --git a/api/src/Html/HtmLawed.php b/api/src/Html/HtmLawed.php index 0b30bd945f..20cd3003c9 100644 --- a/api/src/Html/HtmLawed.php +++ b/api/src/Html/HtmLawed.php @@ -90,7 +90,7 @@ class HtmLawed // tidy eats away even some wanted whitespace, so we switch it off; // we used it for its compacting and beautifying capabilities, which resulted in better html for further processing 'tidy'=>0, - 'elements' => "* -script -meta", + 'elements' => "* -script -meta -object", 'deny_attribute' => 'on*', 'schemes'=>'href: file, ftp, http, https, mailto, tel, phone; src: cid, data, file, ftp, http, https; *:file, http, https', 'hook_tag' =>"hl_my_tag_transform",