From ea00c044d3fbf43bdf27de6106b004c81fc97547 Mon Sep 17 00:00:00 2001 From: Matias Griese Date: Tue, 17 Nov 2020 18:54:03 +0200 Subject: [PATCH] Added XSS detection to all forms (use `check_xss: false` to disable it per field) --- CHANGELOG.md | 1 + system/languages/en.yaml | 7 +- system/src/Grav/Common/Data/Blueprint.php | 5 +- .../src/Grav/Common/Data/BlueprintSchema.php | 21 ++++-- system/src/Grav/Common/Data/Validation.php | 69 +++++++++++++++++++ system/src/Grav/Framework/Flex/FlexObject.php | 4 +- 6 files changed, 96 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 027574098..c3bf52a25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +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) 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/languages/en.yaml b/system/languages/en.yaml index 7c351e649..7a8a68c00 100644 --- a/system/languages/en.yaml +++ b/system/languages/en.yaml @@ -94,9 +94,10 @@ GRAV: YR_PLURAL: yrs DEC_PLURAL: decs FORM: - VALIDATION_FAIL: Validation failed: - INVALID_INPUT: Invalid input in - MISSING_REQUIRED_FIELD: Missing required field: + VALIDATION_FAIL: 'Validation failed:' + INVALID_INPUT: 'Invalid input in' + MISSING_REQUIRED_FIELD: 'Missing required field:' + XSS_ISSUES: "Potential XSS issues detected in '%s' field" MONTHS_OF_THE_YEAR: ['January', 'February', 'March', 'April', 'May', 'June', 'July', 'August', 'September', 'October', 'November', 'December'] DAYS_OF_THE_WEEK: ['Monday', 'Tuesday', 'Wednesday', 'Thursday', 'Friday', 'Saturday', 'Sunday'] YES: "Yes" diff --git a/system/src/Grav/Common/Data/Blueprint.php b/system/src/Grav/Common/Data/Blueprint.php index 7755f9e54..a2f08dac2 100644 --- a/system/src/Grav/Common/Data/Blueprint.php +++ b/system/src/Grav/Common/Data/Blueprint.php @@ -260,14 +260,15 @@ class Blueprint extends BlueprintForm * Validate data against blueprints. * * @param array $data + * @param array $options * @return void * @throws RuntimeException */ - public function validate(array $data) + public function validate(array $data, array $options = []) { $this->initInternals(); - $this->blueprintSchema->validate($data); + $this->blueprintSchema->validate($data, $options); } /** diff --git a/system/src/Grav/Common/Data/BlueprintSchema.php b/system/src/Grav/Common/Data/BlueprintSchema.php index b06cb6478..44708ce9e 100644 --- a/system/src/Grav/Common/Data/BlueprintSchema.php +++ b/system/src/Grav/Common/Data/BlueprintSchema.php @@ -26,6 +26,9 @@ class BlueprintSchema extends BlueprintSchemaBase implements ExportInterface { use Export; + /** @var array */ + protected $filter = ['validation' => true, 'check_xss' => true]; + /** @var array */ protected $ignoreFormKeys = [ 'title' => true, @@ -57,13 +60,14 @@ class BlueprintSchema extends BlueprintSchemaBase implements ExportInterface * Validate data against blueprints. * * @param array $data + * @param array $options * @throws RuntimeException */ - public function validate(array $data) + public function validate(array $data, array $options = []) { try { $validation = $this->items['']['form']['validation'] ?? 'loose'; - $messages = $this->validateArray($data, $this->nested, $validation === 'strict'); + $messages = $this->validateArray($data, $this->nested, $validation === 'strict', $options['check_xss'] ?? true); } catch (RuntimeException $e) { throw (new ValidationException($e->getMessage(), $e->getCode(), $e))->setMessages(); } @@ -141,16 +145,18 @@ class BlueprintSchema extends BlueprintSchemaBase implements ExportInterface * @param array $data * @param array $rules * @param bool $strict + * @param bool $xss * @return array * @throws RuntimeException */ - protected function validateArray(array $data, array $rules, bool $strict) + protected function validateArray(array $data, array $rules, bool $strict, bool $xss = true) { $messages = $this->checkRequired($data, $rules); foreach ($data as $key => $child) { $val = $rules[$key] ?? $rules['*'] ?? null; $rule = is_string($val) ? $this->items[$val] : null; + $checkXss = $xss; if ($rule) { // Item has been defined in blueprints. @@ -160,11 +166,14 @@ class BlueprintSchema extends BlueprintSchemaBase implements ExportInterface } $messages += Validation::validate($child, $rule); + } elseif (is_array($child) && is_array($val)) { // Array has been defined in blueprints. $messages += $this->validateArray($child, $val, $strict); + $checkXss = false; + } elseif ($strict) { - // Undefined/extra item. + // Undefined/extra item in strict mode. /** @var Config $config */ $config = Grav::instance()['config']; if (!$config->get('system.strict_mode.blueprint_strict_compat', true)) { @@ -173,6 +182,10 @@ class BlueprintSchema extends BlueprintSchemaBase implements ExportInterface user_error(sprintf('Having extra key %s in your data is deprecated with blueprint having \'validation: strict\'', $key), E_USER_DEPRECATED); } + + if ($checkXss) { + $messages += Validation::checkSafety($child, $rule ?: ['name' => $key]); + } } return $messages; diff --git a/system/src/Grav/Common/Data/Validation.php b/system/src/Grav/Common/Data/Validation.php index ad5448ce6..7fa8eca2d 100644 --- a/system/src/Grav/Common/Data/Validation.php +++ b/system/src/Grav/Common/Data/Validation.php @@ -13,8 +13,12 @@ use ArrayAccess; use Countable; use DateTime; use Grav\Common\Grav; +use Grav\Common\Language\Language; +use Grav\Common\Security; +use Grav\Common\User\Interfaces\UserInterface; use Grav\Common\Utils; use Grav\Common\Yaml; +use Grav\Framework\Flex\Interfaces\FlexObjectInterface; use Traversable; use function count; use function is_array; @@ -92,6 +96,71 @@ class Validation return $messages; } + /** + * @param mixed $value + * @param array $field + */ + public static function checkSafety($value, array $field) + { + $messages = []; + + if (!($field['check_xss'] ?? true)) { + return $messages; + } + $name = ucfirst($field['label'] ?? $field['name'] ?? 'UNKNOWN'); + + $user = Grav::instance()['user'] ?? null; + $xss_whitelist = Grav::instance()['config']->get('security.xss_whitelist', 'admin.super'); + + // Get language class. + /** @var Language $language */ + $language = Grav::instance()['language']; + + if (!static::authorize($xss_whitelist, $user)) { + if (is_string($value)) { + $violation = Security::detectXss($value); + if ($violation) { + $messages[$name][] = $language->translate(['GRAV.FORM.XSS_ISSUES', $language->translate($name)], null, true); + } + } elseif (is_array($value)) { + $violations = Security::detectXssFromArray($value, $name); + if ($violations) { + $messages[$name][] = $language->translate(['GRAV.FORM.XSS_ISSUES', $language->translate($name)], null, true); + } + } + } + + return $messages; + } + + /** + * Checks user authorisation to the action. + * + * @param string|string[] $action + * @param UserInterface|null $user + * @return bool + */ + public static function authorize($action, UserInterface $user = null) + { + if (!$user) { + return false; + } + + $action = (array)$action; + foreach ($action as $a) { + // Ignore 'admin.super' if it's not the only value to be checked. + if ($a === 'admin.super' && count($action) > 1 && $user instanceof FlexObjectInterface) { + continue; + } + + if ($user->authorize($a)) { + return true; + } + } + + return false; + } + /** * Filter value against a blueprint field definition. * diff --git a/system/src/Grav/Framework/Flex/FlexObject.php b/system/src/Grav/Framework/Flex/FlexObject.php index 2a271f021..449b117d4 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); + $blueprint->validate($elements, ['check_xss' => 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()]); + $blueprint->validate($test + ['storage_key' => $this->getStorageKey(), 'timestamp' => $this->getTimestamp()], ['check_xss' => false]); $data = $blueprint->filter($data, true, true); // Finally update the object.