diff --git a/CHANGELOG.md b/CHANGELOG.md index 749f920bc..dc55aac84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ * Allow `JsonFormatter` options to be passed as a string * Hide Flex Pages frontend configuration (not ready for production use) * Improve Flex configuration: gather views together in blueprint - * Added XSS detection to all forms (use `check_xss: false` to disable it per field) + * Added XSS detection to all forms. See [documentation](http://learn.grav.local/17/forms/forms/form-options#xss-checks) 1. [](#bugfix) * *Menu Visibility Requires Access* Security option setting wrong frontmatter [login#265](https://github.com/getgrav/grav-plugin-login/issues/265) * Accessing page with unsupported file extension (jpg, pdf, xsl) will use wrong mime type [#3031](https://github.com/getgrav/grav/issues/3031) diff --git a/system/src/Grav/Common/Data/BlueprintSchema.php b/system/src/Grav/Common/Data/BlueprintSchema.php index e5c0d2389..dd06f8d69 100644 --- a/system/src/Grav/Common/Data/BlueprintSchema.php +++ b/system/src/Grav/Common/Data/BlueprintSchema.php @@ -27,7 +27,7 @@ class BlueprintSchema extends BlueprintSchemaBase implements ExportInterface use Export; /** @var array */ - protected $filter = ['validation' => true, 'check_xss' => true]; + protected $filter = ['validation' => true, 'xss_check' => true]; /** @var array */ protected $ignoreFormKeys = [ @@ -67,7 +67,7 @@ class BlueprintSchema extends BlueprintSchemaBase implements ExportInterface { try { $validation = $this->items['']['form']['validation'] ?? 'loose'; - $messages = $this->validateArray($data, $this->nested, $validation === 'strict', $options['check_xss'] ?? true); + $messages = $this->validateArray($data, $this->nested, $validation === 'strict', $options['xss_check'] ?? true); } catch (RuntimeException $e) { throw (new ValidationException($e->getMessage(), $e->getCode(), $e))->setMessages(); } diff --git a/system/src/Grav/Common/Data/Validation.php b/system/src/Grav/Common/Data/Validation.php index c69fb1918..9f1d43940 100644 --- a/system/src/Grav/Common/Data/Validation.php +++ b/system/src/Grav/Common/Data/Validation.php @@ -12,6 +12,7 @@ namespace Grav\Common\Data; use ArrayAccess; use Countable; use DateTime; +use Grav\Common\Config\Config; use Grav\Common\Grav; use Grav\Common\Language\Language; use Grav\Common\Security; @@ -105,26 +106,45 @@ class Validation $messages = []; $type = $field['validate']['type'] ?? $field['type'] ?? 'text'; - if ($type === 'unset' || !($field['check_xss'] ?? true)) { + $options = $field['xss_check'] ?? []; + if ($options === false || $type === 'unset') { return $messages; } + if (!is_array($options)) { + $options = []; + } + $name = ucfirst($field['label'] ?? $field['name'] ?? 'UNKNOWN'); + /** @var UserInterface $user */ $user = Grav::instance()['user'] ?? null; - $xss_whitelist = Grav::instance()['config']->get('security.xss_whitelist', 'admin.super'); + /** @var Config $config */ + $config = Grav::instance()['config']; + + $xss_whitelist = $config->get('security.xss_whitelist', 'admin.super'); // Get language class. /** @var Language $language */ $language = Grav::instance()['language']; if (!static::authorize($xss_whitelist, $user)) { + $defaults = Security::getXssDefaults(); + $options += $defaults; + $options['enabled_rules'] += $defaults['enabled_rules']; + if (!empty($options['safe_protocols'])) { + $options['invalid_protocols'] = array_diff($options['invalid_protocols'], $options['safe_protocols']); + } + if (!empty($options['safe_tags'])) { + $options['dangerous_tags'] = array_diff($options['dangerous_tags'], $options['safe_tags']); + } + if (is_string($value)) { - $violation = Security::detectXss($value); + $violation = Security::detectXss($value, $options); if ($violation) { $messages[$name][] = $language->translate(['GRAV.FORM.XSS_ISSUES', $language->translate($name)], null, true); } } elseif (is_array($value)) { - $violations = Security::detectXssFromArray($value, $name); + $violations = Security::detectXssFromArray($value, "{$name}.", $options); if ($violations) { $messages[$name][] = $language->translate(['GRAV.FORM.XSS_ISSUES', $language->translate($name)], null, true); } diff --git a/system/src/Grav/Common/Security.php b/system/src/Grav/Common/Security.php index 9790e548f..ce15bf8c2 100644 --- a/system/src/Grav/Common/Security.php +++ b/system/src/Grav/Common/Security.php @@ -11,6 +11,7 @@ namespace Grav\Common; use enshrined\svgSanitize\Sanitizer; use Exception; +use Grav\Common\Config\Config; use Grav\Common\Page\Pages; use function chr; use function count; @@ -119,18 +120,22 @@ class Security * Detect XSS in an array or strings such as $_POST or $_GET * * @param array $array Array such as $_POST or $_GET + * @param array|null $options Extra options to be passed. * @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 = '') + public static function detectXssFromArray(array $array, string $prefix = '', array $options = null) { - $list = []; + if (null === $options) { + $options = static::getXssDefaults(); + } + $list = []; foreach ($array as $key => $value) { if (is_array($value)) { - $list[] = static::detectXssFromArray($value, $prefix . $key . '.'); + $list[] = static::detectXssFromArray($value, $prefix . $key . '.', $options); } - if ($result = static::detectXss($value)) { + if ($result = static::detectXss($value, $options)) { $list[] = [$prefix . $key => $result]; } } @@ -149,15 +154,34 @@ class Security * their content. * * @param string|null $string The string to run XSS detection logic on - * @return bool|string Type of XSS vector if the given `$string` may contain XSS, false otherwise. + * @param array|null $options + * @return string|null 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) + public static function detectXss($string, array $options = null): ?string { // Skip any null or non string values if (null === $string || !is_string($string) || empty($string)) { - return false; + return null; + } + + if (null === $options) { + $options = static::getXssDefaults(); + } + + $enabled_rules = (array)($options['enabled_rules'] ?? null); + $dangerous_tags = (array)($options['dangerous_tags'] ?? null); + if (!$dangerous_tags) { + $enabled_rules['dangerous_tags'] = false; + } + $invalid_protocols = (array)($options['invalid_protocols'] ?? null); + if (!$invalid_protocols) { + $enabled_rules['invalid_protocols'] = false; + } + $enabled_rules = array_filter($enabled_rules, static function ($val) { return !empty($val); }); + if (!$enabled_rules) { + return null; } // Keep a copy of the original string before cleaning up @@ -168,7 +192,7 @@ class Security // Convert Hexadecimals $string = (string)preg_replace_callback('!(&#|\\\)[xX]([0-9a-fA-F]+);?!u', function ($m) { - return \chr(hexdec($m[2])); + return chr(hexdec($m[2])); }, $string); // Clean up entities @@ -180,19 +204,13 @@ class Security // Strip whitespace characters $string = preg_replace('!\s!u', '', $string); - $config = Grav::instance()['config']; - - $dangerous_tags = array_map('preg_quote', array_map("trim", $config->get('security.xss_dangerous_tags'))); - $invalid_protocols = array_map('preg_quote', array_map("trim", $config->get('security.xss_invalid_protocols'))); - $enabled_rules = $config->get('security.xss_enabled'); - // Set the patterns we'll test against $patterns = [ // Match any attribute starting with "on" or xmlns 'on_events' => '#(<[^>]+[[a-z\x00-\x20\"\'\/])(\son|\sxmlns)[a-z].*=>?#iUu', // Match javascript:, livescript:, vbscript:, mocha:, feed: and data: protocols - 'invalid_protocols' => '#(' . implode('|', $invalid_protocols) . '):.*?#iUu', + 'invalid_protocols' => '#(' . implode('|', array_map('preg_quote', $invalid_protocols, ['#'])) . '):.*?#iUu', // Match -moz-bindings 'moz_binding' => '#-moz-binding[a-z\x00-\x20]*:#u', @@ -201,13 +219,12 @@ class Security 'html_inline_styles' => '#(<[^>]+[a-z\x00-\x20\"\'\/])(style=[^>]*(url\:|x\:expression).*)>?#iUu', // Match potentially dangerous tags - 'dangerous_tags' => '#]*>?#ui' + 'dangerous_tags' => '#]*>?#ui' ]; - // Iterate over rules and return label if fail - foreach ((array) $patterns as $name => $regex) { - if ($enabled_rules[$name] === true) { + foreach ($patterns as $name => $regex) { + if (!empty($enabled_rules[$name])) { if (preg_match($regex, $string) || preg_match($regex, $orig)) { return $name; } @@ -216,4 +233,16 @@ class Security return false; } + + public static function getXssDefaults(): array + { + /** @var Config $config */ + $config = Grav::instance()['config']; + + return [ + 'enabled_rules' => $config->get('security.xss_enabled'), + 'dangerous_tags' => array_map('trim', $config->get('security.xss_dangerous_tags')), + 'invalid_protocols' => array_map('trim', $config->get('security.xss_invalid_protocols')), + ]; + } } diff --git a/system/src/Grav/Framework/Flex/FlexObject.php b/system/src/Grav/Framework/Flex/FlexObject.php index 449b117d4..171a4571f 100644 --- a/system/src/Grav/Framework/Flex/FlexObject.php +++ b/system/src/Grav/Framework/Flex/FlexObject.php @@ -134,7 +134,7 @@ class FlexObject implements FlexObjectInterface, FlexAuthorizeInterface if ($validate) { $blueprint = $this->getFlexDirectory()->getBlueprint(); - $blueprint->validate($elements, ['check_xss' => false]); + $blueprint->validate($elements, ['xss_check' => false]); $elements = $blueprint->filter($elements, true, true); } @@ -576,7 +576,7 @@ class FlexObject implements FlexObjectInterface, FlexAuthorizeInterface $test = $blueprint->mergeData($elements, $data); // Validate and filter elements and throw an error if any issues were found. - $blueprint->validate($test + ['storage_key' => $this->getStorageKey(), 'timestamp' => $this->getTimestamp()], ['check_xss' => false]); + $blueprint->validate($test + ['storage_key' => $this->getStorageKey(), 'timestamp' => $this->getTimestamp()], ['xss_check' => false]); $data = $blueprint->filter($data, true, true); // Finally update the object.