From 33d98114ba2a2a86325cffd37fb7f3e7ceacda9f Mon Sep 17 00:00:00 2001 From: Andy Miller Date: Sat, 29 Sep 2018 21:24:21 -0600 Subject: [PATCH 1/9] XSS enhancements --- .gitignore | 1 + system/src/Grav/Common/Utils.php | 35 +++++++++++++------------------- 2 files changed, 15 insertions(+), 21 deletions(-) diff --git a/.gitignore b/.gitignore index f20ae570e..e8c09ea3c 100644 --- a/.gitignore +++ b/.gitignore @@ -42,3 +42,4 @@ tests/_output/* tests/_support/_generated/* tests/cache/* tests/error.log +/system/templates/testing diff --git a/system/src/Grav/Common/Utils.php b/system/src/Grav/Common/Utils.php index 53c67e7ab..2de360683 100644 --- a/system/src/Grav/Common/Utils.php +++ b/system/src/Grav/Common/Utils.php @@ -280,7 +280,7 @@ abstract class Utils * their content. * * @param string $string The string to run XSS detection logic on - * @return boolean True if the given `$string` may contain XSS, false otherwise. + * @return boolean|string Type of XSS vector if the given `$string` may contain XSS, false otherwise. * * Copies the code from: https://github.com/symphonycms/xssfilter/blob/master/extension.driver.php#L138 */ @@ -311,30 +311,23 @@ abstract class Utils // Strip whitespace characters $string = preg_replace('!\s!u','', $string); - // Set the patterns we'll test against - $patterns = [ - // Match any attribute starting with "on" or xmlns - '#(<[^>]+[\x00-\x20\"\'\/])(on|xmlns)[^>]*>?#iUu', + // Get XSS rules from security configuration + $xss_rules = Grav::instance()['config']->get('security.xss_rules'); - // Match javascript:, livescript:, vbscript:, mocha:, feed: and data: protocols - '!((java|live|vb)script|mocha|feed|data):(\w)*!iUu', + // Iterate over rules and return label if fail + foreach ((array) $xss_rules as $rule) { + if ($rule['enabled'] === true) { + $label = $rule['label']; + $regex = $rule['regex']; - // Match -moz-bindings - '#-moz-binding[\x00-\x20]*:#u', - - // Match style attributes - '#(<[^>]+[\x00-\x20\"\'\/])style=[^>]*>?#iUu', - - // Match potentially dangerous tags - '#]*>?#ui' - ]; - - foreach ($patterns as $pattern) { - // Test both the original string and clean string - if (preg_match($pattern, $string) || preg_match($pattern, $orig)) { - return true; + if ($label && $regex) { + if (preg_match($regex, $string) || preg_match($regex, $orig)) { + return $label; + } + } } } + return false; } /** From 5b787d56e668600dda3d8d5484ff5df8bd40dc18 Mon Sep 17 00:00:00 2001 From: Andy Miller Date: Sat, 29 Sep 2018 21:24:58 -0600 Subject: [PATCH 2/9] Add default XSS security config --- system/blueprints/config/security.yaml | 49 ++++++++++++++++++++++++++ system/config/security.yaml | 22 ++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 system/blueprints/config/security.yaml create mode 100644 system/config/security.yaml diff --git a/system/blueprints/config/security.yaml b/system/blueprints/config/security.yaml new file mode 100644 index 000000000..c0f4085b9 --- /dev/null +++ b/system/blueprints/config/security.yaml @@ -0,0 +1,49 @@ +title: PLUGIN_ADMIN.SECURITY + +form: + validation: loose + fields: + + security_section: + type: section + title: XSS Security + underline: true + + xss_whitelist: + type: selectize + size: large + label: Whitelist Permissions + help: Users with these permissions will skip the XSS rules when saving content + placeholder: 'admin.super' + classes: fancy + validate: + type: commalist + + + xss_rules: + type: list + label: Rules + help: Be careful when tweaking these rules, a broken regex will break things badly! + classes: compact + fields: + .label: + type: text + label: Label + validate: + required: true + .enabled: + type: toggle + label: Rule Enabled + highlight: 1 + options: + 1: PLUGIN_ADMIN.YES + 0: PLUGIN_ADMIN.NO + default: true + validate: + type: bool + .regex: + type: text + label: Regex + validate: + required: true + diff --git a/system/config/security.yaml b/system/config/security.yaml new file mode 100644 index 000000000..69b44dd80 --- /dev/null +++ b/system/config/security.yaml @@ -0,0 +1,22 @@ +xss_whitelist: [admin.super] # Whitelist of user access that should 'skip' XSS checking +xss_rules: # Array of XSS tests to run through + - + label: On-Events + enabled: true + regex: '#(<[^>]+[\x00-\x20\"''\/])(on|xmlns)[^>]*>?#iUu' + - + label: JavaScript + enabled: true + regex: '!((java|live|vb)script|mocha|feed|data):(\w)*!iUu' + - + label: Moz-Binding + enabled: true + regex: '#-moz-binding[\x00-\x20]*:#u' + - + label: 'Style Attributes' + enabled: false + regex: '#(<[^>]+[\x00-\x20\"''\/])style=[^>]*>?#iUu' + - + label: 'Dangerous Tags' + enabled: true + regex: '#]*>?#ui' From 17a371d86a6cc17e69cf80be306df78361c46517 Mon Sep 17 00:00:00 2001 From: Andy Miller Date: Sat, 29 Sep 2018 21:37:01 -0600 Subject: [PATCH 3/9] lang stuff --- system/blueprints/config/security.yaml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/system/blueprints/config/security.yaml b/system/blueprints/config/security.yaml index c0f4085b9..5309238b8 100644 --- a/system/blueprints/config/security.yaml +++ b/system/blueprints/config/security.yaml @@ -6,14 +6,14 @@ form: security_section: type: section - title: XSS Security + title: PLUGIN_ADMIN.XSS_SECURITY underline: true xss_whitelist: type: selectize size: large - label: Whitelist Permissions - help: Users with these permissions will skip the XSS rules when saving content + label: PLUGIN_ADMIN.XSS_WHITELIST_PERMISSIONS + help: PLUGIN_ADMIN.XSS_WHITELIST_PERMISSIONS_HELP placeholder: 'admin.super' classes: fancy validate: @@ -22,18 +22,18 @@ form: xss_rules: type: list - label: Rules - help: Be careful when tweaking these rules, a broken regex will break things badly! + label: PLUGIN_ADMIN.XSS_RULES + help: PLUGIN_ADMIN.XSS_RULES_HELP classes: compact fields: .label: type: text - label: Label + label: PLUGIN_ADMIN.XSS_RULE_LABEL validate: required: true .enabled: type: toggle - label: Rule Enabled + label: PLUGIN_ADMIN.ENABLED highlight: 1 options: 1: PLUGIN_ADMIN.YES @@ -43,7 +43,7 @@ form: type: bool .regex: type: text - label: Regex + label: PLUGIN_ADMIN.XSS_RULE_REGEX validate: required: true From 7abe01ed8cb65f2d4ac884fd64e2a6449c2899a8 Mon Sep 17 00:00:00 2001 From: Andy Miller Date: Sun, 30 Sep 2018 00:10:04 -0600 Subject: [PATCH 4/9] vertical style --- system/blueprints/config/security.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/system/blueprints/config/security.yaml b/system/blueprints/config/security.yaml index 5309238b8..582157968 100644 --- a/system/blueprints/config/security.yaml +++ b/system/blueprints/config/security.yaml @@ -22,6 +22,7 @@ form: xss_rules: type: list + style: vertical label: PLUGIN_ADMIN.XSS_RULES help: PLUGIN_ADMIN.XSS_RULES_HELP classes: compact From e69d6cefeed9fe3b0782a30363cc01988adcdba6 Mon Sep 17 00:00:00 2001 From: Andy Miller Date: Sun, 30 Sep 2018 00:10:44 -0600 Subject: [PATCH 5/9] ordering --- system/blueprints/config/security.yaml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/system/blueprints/config/security.yaml b/system/blueprints/config/security.yaml index 582157968..c99e2effd 100644 --- a/system/blueprints/config/security.yaml +++ b/system/blueprints/config/security.yaml @@ -32,6 +32,11 @@ form: label: PLUGIN_ADMIN.XSS_RULE_LABEL validate: required: true + .regex: + type: text + label: PLUGIN_ADMIN.XSS_RULE_REGEX + validate: + required: true .enabled: type: toggle label: PLUGIN_ADMIN.ENABLED @@ -42,9 +47,4 @@ form: default: true validate: type: bool - .regex: - type: text - label: PLUGIN_ADMIN.XSS_RULE_REGEX - validate: - required: true From 1709eb038c211f4de5fb1362bf46c380aa1959d2 Mon Sep 17 00:00:00 2001 From: Andy Miller Date: Sun, 30 Sep 2018 15:24:01 -0600 Subject: [PATCH 6/9] Fix for array method --- system/src/Grav/Common/Utils.php | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/system/src/Grav/Common/Utils.php b/system/src/Grav/Common/Utils.php index 2de360683..807080750 100644 --- a/system/src/Grav/Common/Utils.php +++ b/system/src/Grav/Common/Utils.php @@ -266,12 +266,16 @@ abstract class Utils if (\is_array($value)) { $list[] = static::detectXssFromArray($value, $prefix . $key . '.'); } - if (static::detectXss($value)) { - $list[] = [$prefix . $key]; + if ($result = static::detectXss($value)) { + $list[] = [$prefix . $key => $result]; } } - return array_merge(...$list); + if (!empty($list)) { + return array_merge(...$list); + } + + return $list; } /** From 451ec49d9c5fec03aa4f8c90c3299f56a9142b26 Mon Sep 17 00:00:00 2001 From: Andy Miller Date: Sun, 30 Sep 2018 17:45:45 -0600 Subject: [PATCH 7/9] refactor --- system/src/Grav/Common/Security.php | 130 ++++++++++++++++++ system/src/Grav/Common/Utils.php | 81 ----------- .../src/Grav/Console/Cli/SecurityCommand.php | 86 ++++++++++++ 3 files changed, 216 insertions(+), 81 deletions(-) create mode 100644 system/src/Grav/Common/Security.php create mode 100644 system/src/Grav/Console/Cli/SecurityCommand.php diff --git a/system/src/Grav/Common/Security.php b/system/src/Grav/Common/Security.php new file mode 100644 index 000000000..b2f647110 --- /dev/null +++ b/system/src/Grav/Common/Security.php @@ -0,0 +1,130 @@ +routes(); + + $list = []; + + foreach ($routes as $route => $path) { + + $messager && $messager([ + 'type' => 'progress', + 'percentage' => false, + 'complete' => false + ]); + + try { + $page = $pages->get($path); + // call the content to load/cache it + $header = $page->header(); + $content = $page->content(); + + $data = ['header' => $header, 'content' => $content]; + $results = Security::detectXssFromArray($data); + + if (!empty($results)) { + $list[$route] = $results; + } + + } catch (\Exception $e) { + continue; + } + } + + dump($list); + } + + /** + * @param array $array Array such as $_POST or $_GET + * @param string $prefix Prefix for returned values. + * @return array Returns flatten list of potentially dangerous input values, such as 'data.content'. + */ + public static function detectXssFromArray(array $array, $prefix = '') + { + $list = []; + + foreach ($array as $key => $value) { + if (\is_array($value)) { + $list[] = static::detectXssFromArray($value, $prefix . $key . '.'); + } + if ($result = static::detectXss($value)) { + $list[] = [$prefix . $key => $result]; + } + } + + if (!empty($list)) { + return array_merge(...$list); + } + + return $list; + } + + /** + * Determine if string potentially has a XSS attack. This simple function does not catch all XSS and it is likely to + * return false positives because of it tags all potentially dangerous HTML tags and attributes without looking into + * their content. + * + * @param string $string The string to run XSS detection logic on + * @return boolean|string Type of XSS vector if the given `$string` may contain XSS, false otherwise. + * + * Copies the code from: https://github.com/symphonycms/xssfilter/blob/master/extension.driver.php#L138 + */ + public static function detectXss($string) + { + // Skip any null or non string values + if (null === $string || !\is_string($string) || empty($string)) { + return false; + } + + // Keep a copy of the original string before cleaning up + $orig = $string; + + // URL decode + $string = urldecode($string); + + // Convert Hexadecimals + $string = (string)preg_replace_callback('!(&#|\\\)[xX]([0-9a-fA-F]+);?!u', function($m) { + return \chr(hexdec($m[2])); + }, $string); + + // Clean up entities + $string = preg_replace('!(�+[0-9]+)!u','$1;', $string); + + // Decode entities + $string = html_entity_decode($string, ENT_NOQUOTES, 'UTF-8'); + + // Strip whitespace characters + $string = preg_replace('!\s!u','', $string); + + // Get XSS rules from security configuration + $xss_rules = Grav::instance()['config']->get('security.xss_rules'); + + // Iterate over rules and return label if fail + foreach ((array) $xss_rules as $rule) { + if ($rule['enabled'] === true) { + $label = $rule['label']; + $regex = $rule['regex']; + + if ($label && $regex) { + if (preg_match($regex, $string) || preg_match($regex, $orig)) { + return $label; + } + } + } + } + + return false; + } +} diff --git a/system/src/Grav/Common/Utils.php b/system/src/Grav/Common/Utils.php index 807080750..5de4fe7b1 100644 --- a/system/src/Grav/Common/Utils.php +++ b/system/src/Grav/Common/Utils.php @@ -253,87 +253,6 @@ abstract class Utils return $date_formats; } - /** - * @param array $array Array such as $_POST or $_GET - * @param string $prefix Prefix for returned values. - * @return array Returns flatten list of potentially dangerous input values, such as 'data.content'. - */ - public static function detectXssFromArray(array $array, $prefix = '') - { - $list = []; - - foreach ($array as $key => $value) { - if (\is_array($value)) { - $list[] = static::detectXssFromArray($value, $prefix . $key . '.'); - } - if ($result = static::detectXss($value)) { - $list[] = [$prefix . $key => $result]; - } - } - - if (!empty($list)) { - return array_merge(...$list); - } - - return $list; - } - - /** - * Determine if string potentially has a XSS attack. This simple function does not catch all XSS and it is likely to - * return false positives because of it tags all potentially dangerous HTML tags and attributes without looking into - * their content. - * - * @param string $string The string to run XSS detection logic on - * @return boolean|string Type of XSS vector if the given `$string` may contain XSS, false otherwise. - * - * Copies the code from: https://github.com/symphonycms/xssfilter/blob/master/extension.driver.php#L138 - */ - public static function detectXss($string) - { - // Skip any null or non string values - if (null === $string || !\is_string($string) || empty($string)) { - return false; - } - - // Keep a copy of the original string before cleaning up - $orig = $string; - - // URL decode - $string = urldecode($string); - - // Convert Hexadecimals - $string = (string)preg_replace_callback('!(&#|\\\)[xX]([0-9a-fA-F]+);?!u', function($m) { - return \chr(hexdec($m[2])); - }, $string); - - // Clean up entities - $string = preg_replace('!(�+[0-9]+)!u','$1;', $string); - - // Decode entities - $string = html_entity_decode($string, ENT_NOQUOTES, 'UTF-8'); - - // Strip whitespace characters - $string = preg_replace('!\s!u','', $string); - - // Get XSS rules from security configuration - $xss_rules = Grav::instance()['config']->get('security.xss_rules'); - - // Iterate over rules and return label if fail - foreach ((array) $xss_rules as $rule) { - if ($rule['enabled'] === true) { - $label = $rule['label']; - $regex = $rule['regex']; - - if ($label && $regex) { - if (preg_match($regex, $string) || preg_match($regex, $orig)) { - return $label; - } - } - } - } - - return false; - } /** * Truncate text by number of characters but can cut off words. * diff --git a/system/src/Grav/Console/Cli/SecurityCommand.php b/system/src/Grav/Console/Cli/SecurityCommand.php new file mode 100644 index 000000000..5e3568447 --- /dev/null +++ b/system/src/Grav/Console/Cli/SecurityCommand.php @@ -0,0 +1,86 @@ +setName("security") + ->addArgument( + 'xss', + InputArgument::OPTIONAL, + 'Perform XSS security checks on all pages' + + ) + ->setDescription("Capable of running XSS Security checks") + ->setHelp('The security runs various security checks on your Grav site'); + + $this->source = getcwd(); + } + + /** + * @return int|null|void + */ + protected function serve() + { + $this->progress = new ProgressBar($this->output); + $this->progress->setFormat('Archiving %current% files [%bar%] %elapsed:6s% %memory:6s%'); + + /** @var Grav $grav */ + $grav = Grav::instance(); + + $grav['config']->init(); + $grav['debugger']->enabled(false); + $grav['twig']->init(); + $grav['pages']->init(); + + $results = Security::detectXssFromPages($grav['pages'], [$this, 'output']); + + print_r($results); + + } + + /** + * @param $args + */ + public function output($args) + { + switch ($args['type']) { + case 'message': + $this->output->writeln($args['message']); + break; + case 'progress': + if ($args['complete']) { + $this->progress->finish(); + } else { + $this->progress->advance(); + } + break; + } + } + +} + From fb98ca7b19934b0bcf499bdf2c22fe0d8286bbfe Mon Sep 17 00:00:00 2001 From: Andy Miller Date: Sun, 30 Sep 2018 18:34:53 -0600 Subject: [PATCH 8/9] Added a new Security CLI command --- bin/grav | 1 + system/src/Grav/Common/Security.php | 20 ++++-- .../src/Grav/Console/Cli/SecurityCommand.php | 63 +++++++++++++------ 3 files changed, 60 insertions(+), 24 deletions(-) diff --git a/bin/grav b/bin/grav index faa8846f7..e36f0cc1c 100755 --- a/bin/grav +++ b/bin/grav @@ -41,5 +41,6 @@ $app->addCommands(array( new \Grav\Console\Cli\ClearCacheCommand(), new \Grav\Console\Cli\BackupCommand(), new \Grav\Console\Cli\NewProjectCommand(), + new \Grav\Console\Cli\SecurityCommand(), )); $app->run(); diff --git a/system/src/Grav/Common/Security.php b/system/src/Grav/Common/Security.php index b2f647110..9b345cb18 100644 --- a/system/src/Grav/Common/Security.php +++ b/system/src/Grav/Common/Security.php @@ -11,24 +11,32 @@ namespace Grav\Common; class Security { - public static function detectXssFromPages($pages, callable $messager = null) + public static function detectXssFromPages($pages, callable $status = null) { $routes = $pages->routes(); + // Remove duplicate for homepage + unset($routes['/']); + $list = []; +// // This needs Symfony 4.1 to work +// $status && $status([ +// 'type' => 'count', +// 'steps' => count($routes), +// ]); + foreach ($routes as $route => $path) { - $messager && $messager([ + $status && $status([ 'type' => 'progress', - 'percentage' => false, - 'complete' => false ]); try { $page = $pages->get($path); + // call the content to load/cache it - $header = $page->header(); + $header = (array) $page->header(); $content = $page->content(); $data = ['header' => $header, 'content' => $content]; @@ -43,7 +51,7 @@ class Security } } - dump($list); + return $list; } /** diff --git a/system/src/Grav/Console/Cli/SecurityCommand.php b/system/src/Grav/Console/Cli/SecurityCommand.php index 5e3568447..3361d44ce 100644 --- a/system/src/Grav/Console/Cli/SecurityCommand.php +++ b/system/src/Grav/Console/Cli/SecurityCommand.php @@ -13,12 +13,10 @@ use Grav\Common\Security; use Grav\Console\ConsoleCommand; use Symfony\Component\Console\Helper\ProgressBar; use Symfony\Component\Console\Input\InputArgument; +use Symfony\Component\Console\Style\SymfonyStyle; class SecurityCommand extends ConsoleCommand { - /** @var string $source */ - protected $source; - /** @var ProgressBar $progress */ protected $progress; @@ -29,13 +27,7 @@ class SecurityCommand extends ConsoleCommand { $this ->setName("security") - ->addArgument( - 'xss', - InputArgument::OPTIONAL, - 'Perform XSS security checks on all pages' - - ) - ->setDescription("Capable of running XSS Security checks") + ->setDescription("Capable of running various Security checks") ->setHelp('The security runs various security checks on your Grav site'); $this->source = getcwd(); @@ -46,34 +38,69 @@ class SecurityCommand extends ConsoleCommand */ protected function serve() { - $this->progress = new ProgressBar($this->output); - $this->progress->setFormat('Archiving %current% files [%bar%] %elapsed:6s% %memory:6s%'); + /** @var Grav $grav */ $grav = Grav::instance(); + $grav['uri']->init(); $grav['config']->init(); $grav['debugger']->enabled(false); + $grav['streams']; + $grav['plugins']->init(); + $grav['themes']->init(); + + $grav['twig']->init(); $grav['pages']->init(); - $results = Security::detectXssFromPages($grav['pages'], [$this, 'output']); + $this->progress = new ProgressBar($this->output, (count($grav['pages']->routes()) - 1)); + $this->progress->setFormat('Scanning %current% pages [%bar%] %percent:3s%% %elapsed:6s%'); + $this->progress->setBarWidth(100); - print_r($results); + $io = new SymfonyStyle($this->input, $this->output); + $io->title('Grav Security Check'); + + $output = Security::detectXssFromPages($grav['pages'], [$this, 'outputProgress']); + + $io->newline(2); + + if (!empty($output)) { + + $counter = 1; + foreach ($output as $route => $results) { + + $results_parts = array_map(function($value, $key) { + return $key.': \''.$value . '\''; + }, array_values($results), array_keys($results)); + + $io->writeln($counter++ .' - ' . $route . ' → ' . implode(', ', $results_parts) . ''); + } + + $io->error('Security Scan complete: ' . count($output) . ' potential XSS issues found...'); + + } else { + $io->success('Security Scan complete: No issues found...'); + } + + $io->newline(1); } /** * @param $args */ - public function output($args) + public function outputProgress($args) { switch ($args['type']) { - case 'message': - $this->output->writeln($args['message']); + case 'count': + $steps = $args['steps']; + $freq = intval($steps > 100 ? round($steps / 100) : $steps); + $this->progress->setMaxSteps($steps); + $this->progress->setRedrawFrequency($freq); break; case 'progress': - if ($args['complete']) { + if (isset($args['complete']) && $args['complete']) { $this->progress->finish(); } else { $this->progress->advance(); From f40c6a8617f1b765cfa20d262bd6947759c7d9cc Mon Sep 17 00:00:00 2001 From: Andy Miller Date: Sun, 30 Sep 2018 18:37:42 -0600 Subject: [PATCH 9/9] Changelog updated --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c6c08018..4ec3c0027 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,8 @@ 1. [](#new) * Added `Deprecated` tab to DebugBar to catch future incompatibilities with later Grav versions * Added deprecation notices for features which will be removed in Grav 2.0 - * Added `Utils::detectXssFromArray()` and `Utils::detectXss()` methods + * Added new `bin/grav security` command to scan for security issues (XSS currently) + * Added new `Security` class for Grav security functionality * Added `onHttpPostFilter` event to allow plugins to globally clean up XSS in the forms and tasks 1. [](#bugfix) * Allow `$page->slug()` to be called before `$page->init()` without breaking the page