From 5127c3fa59567258bbb44ec87449f4b944a4caa7 Mon Sep 17 00:00:00 2001 From: bnjmnm Date: Thu, 28 Apr 2022 10:41:49 -0400 Subject: [PATCH] Issue #3228691 by Wim Leers, lauriii, nod_: Restrict allowed additional attributes to prevent self XSS --- .../config/schema/ckeditor5.schema.yml | 1 + .../SourceEditingPreventSelfXssConstraint.php | 28 ++++ ...itingPreventSelfXssConstraintValidator.php | 121 ++++++++++++++++++ .../tests/src/Kernel/ValidatorsTest.php | 76 +++++++++++ 4 files changed, 226 insertions(+) create mode 100644 core/modules/ckeditor5/src/Plugin/Validation/Constraint/SourceEditingPreventSelfXssConstraint.php create mode 100644 core/modules/ckeditor5/src/Plugin/Validation/Constraint/SourceEditingPreventSelfXssConstraintValidator.php diff --git a/core/modules/ckeditor5/config/schema/ckeditor5.schema.yml b/core/modules/ckeditor5/config/schema/ckeditor5.schema.yml index 3bbda549a71..03b39e4c083 100644 --- a/core/modules/ckeditor5/config/schema/ckeditor5.schema.yml +++ b/core/modules/ckeditor5/config/schema/ckeditor5.schema.yml @@ -85,6 +85,7 @@ ckeditor5.plugin.ckeditor5_sourceEditing: label: 'Allowed Tag' constraints: SourceEditingRedundantTags: [] + SourceEditingPreventSelfXssConstraint: [] # Plugin \Drupal\ckeditor5\Plugin\CKEditor5Plugin\ListPlugin ckeditor5.plugin.ckeditor5_list: diff --git a/core/modules/ckeditor5/src/Plugin/Validation/Constraint/SourceEditingPreventSelfXssConstraint.php b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/SourceEditingPreventSelfXssConstraint.php new file mode 100644 index 00000000000..3bf2e94cc43 --- /dev/null +++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/SourceEditingPreventSelfXssConstraint.php @@ -0,0 +1,28 @@ +allowsNothing() || count($restrictions->getAllowedElements()) > 1) { + return; + } + + // This validation constraint only validates attributes, not tags; so if all + // attributes are allowed (TRUE) or no attributes are allowed (FALSE), + // return early. Only proceed when some attributes are allowed (an array). + $allowed_elements = $restrictions->getAllowedElements(FALSE); + assert(count($allowed_elements) === 1); + $tag = array_key_first($allowed_elements); + $attribute_restrictions = $allowed_elements[$tag]; + if (!is_array($attribute_restrictions)) { + return; + } + + $text_editor = $this->createTextEditorObjectFromContext(); + $text_format_allowed_elements = HTMLRestrictions::fromTextFormat($text_editor->getFilterFormat()) + ->getAllowedElements(); + // Any XSS-prevention related measures imposed by filter plugins are relayed + // through their ::getHtmlRestrictions() return value. The global attribute + // `*` HTML tag allows attributes to be forbidden. + // @see https://html.spec.whatwg.org/multipage/dom.html#global-attributes + // @see \Drupal\ckeditor5\HTMLRestrictions::validateAllowedRestrictionsPhase4() + // @see \Drupal\filter\Plugin\Filter\FilterHtml::getHTMLRestrictions() + $forbidden_attributes = []; + if (array_key_exists('*', $text_format_allowed_elements)) { + $forbidden_attributes = array_keys(array_filter($text_format_allowed_elements['*'], function ($attribute_value_restriction, string $attribute_name) { + return $attribute_value_restriction === FALSE; + }, ARRAY_FILTER_USE_BOTH)); + } + + foreach ($forbidden_attributes as $forbidden_attribute_name) { + // Forbidden attributes not containing wildcards, such as `style`. + if (!self::isWildcardAttributeName($forbidden_attribute_name)) { + if (array_key_exists($forbidden_attribute_name, $attribute_restrictions)) { + $this->context->buildViolation($constraint->message) + ->setParameter('%dangerous_tag', $value) + ->addViolation(); + } + } + // Forbidden attributes containing wildcards such as `on*`. + else { + $regex = self::getRegExForWildCardAttributeName($forbidden_attribute_name); + if (!empty(preg_grep($regex, array_keys($attribute_restrictions)))) { + $this->context->buildViolation($constraint->message) + ->setParameter('%dangerous_tag', $value) + ->addViolation(); + } + } + } + } + + /** + * Checks whether the given attribute name contains a wildcard, e.g. `data-*`. + * + * @param string $attribute_name + * The attribute name to check. + * + * @return bool + * Whether the given attribute name contains a wildcard. + */ + private static function isWildcardAttributeName(string $attribute_name): bool { + assert($attribute_name !== '*'); + return strpos($attribute_name, '*') !== FALSE; + } + + /** + * Computes a regular expression for matching a wildcard attribute name. + * + * @param string $wildcard_attribute_name + * The wildcard attribute name for which to compute a regular expression. + * + * @return string + * The computed regular expression. + */ + private static function getRegExForWildCardAttributeName(string $wildcard_attribute_name): string { + assert(self::isWildcardAttributeName($wildcard_attribute_name)); + return '/^' . str_replace('*', '.*', $wildcard_attribute_name) . '$/'; + } + +} diff --git a/core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php b/core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php index 58ba428236d..52a6c460a92 100644 --- a/core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php +++ b/core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php @@ -13,6 +13,8 @@ use Drupal\KernelTests\KernelTestBase; use Drupal\Tests\SchemaCheckTestTrait; use Symfony\Component\Yaml\Yaml; +// cspell:ignore onhover + /** * @covers \Drupal\ckeditor5\Plugin\Validation\Constraint\ToolbarItemConstraintValidator * @covers \Drupal\ckeditor5\Plugin\Validation\Constraint\ToolbarItemDependencyConstraintValidator @@ -334,6 +336,7 @@ class ValidatorsTest extends KernelTestBase { * @covers \Drupal\ckeditor5\Plugin\Validation\Constraint\ToolbarItemConstraintValidator * @covers \Drupal\ckeditor5\Plugin\Validation\Constraint\ToolbarItemDependencyConstraintValidator * @covers \Drupal\ckeditor5\Plugin\Validation\Constraint\EnabledConfigurablePluginsConstraintValidator + * @covers \Drupal\ckeditor5\Plugin\Validation\Constraint\SourceEditingPreventSelfXssConstraintValidator * @dataProvider providerPair * * @param array $ckeditor5_settings @@ -856,6 +859,79 @@ class ValidatorsTest extends KernelTestBase { ], 'violations' => [], ]; + $self_xss_source_editing = [ + // Dangerous attribute with all values allowed. + '

', + '', + '

', + + // No danger. + '', + + // Dangerous attribute with some values allowed. + '', + '', + + // Also works on wildcard tags. + '<$text-container style>', + ]; + $data['INVALID: SourceEditing plugin configuration: self-XSS detected when using filter_html'] = [ + 'settings' => [ + 'toolbar' => [ + 'items' => [ + 'sourceEditing', + ], + ], + 'plugins' => [ + 'ckeditor5_sourceEditing' => [ + 'allowed_tags' => $self_xss_source_editing, + ], + ], + ], + 'image_upload' => [ + 'status' => FALSE, + ], + 'filters' => [ + 'filter_html' => [ + 'id' => 'filter_html', + 'provider' => 'filter', + 'status' => TRUE, + 'weight' => 0, + 'settings' => [ + 'allowed_html' => '


', + 'filter_html_help' => TRUE, + 'filter_html_nofollow' => TRUE, + ], + ], + ], + 'violations' => [ + 'settings.plugins.ckeditor5_sourceEditing.allowed_tags.0' => 'The following tag in the Source Editing "Manually editable HTML tags" field is a security risk: <p onhover>.', + 'settings.plugins.ckeditor5_sourceEditing.allowed_tags.1' => 'The following tag in the Source Editing "Manually editable HTML tags" field is a security risk: <img on*>.', + 'settings.plugins.ckeditor5_sourceEditing.allowed_tags.2' => 'The following tag in the Source Editing "Manually editable HTML tags" field is a security risk: <blockquote style>.', + 'settings.plugins.ckeditor5_sourceEditing.allowed_tags.4' => 'The following tag in the Source Editing "Manually editable HTML tags" field is a security risk: <a onclick="javascript:*">.', + 'settings.plugins.ckeditor5_sourceEditing.allowed_tags.5' => 'The following tag in the Source Editing "Manually editable HTML tags" field is a security risk: <code style="foo: bar;">.', + 'settings.plugins.ckeditor5_sourceEditing.allowed_tags.6' => 'The following tag in the Source Editing "Manually editable HTML tags" field is a security risk: <$text-container style>.', + ], + ]; + $data['VALID: SourceEditing plugin configuration: self-XSS not detected when not using filter_html'] = [ + 'settings' => [ + 'toolbar' => [ + 'items' => [ + 'sourceEditing', + ], + ], + 'plugins' => [ + 'ckeditor5_sourceEditing' => [ + 'allowed_tags' => $self_xss_source_editing, + ], + ], + ], + 'image_upload' => [ + 'status' => FALSE, + ], + 'filters' => [], + 'violations' => [], + ]; return $data; }