Issue #3228691 by Wim Leers, lauriii, nod_: Restrict allowed additional...

Issue #3228691 by Wim Leers, lauriii, nod_: Restrict allowed additional attributes to prevent self XSS

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:

core/modules/ckeditor5/src/Plugin/Validation/Constraint/SourceEditingPreventSelfXssConstraint.php

+<?php
+
+declare(strict_types = 1);
+
+namespace Drupal\ckeditor5\Plugin\Validation\Constraint;
+
+use Symfony\Component\Validator\Constraint;
+
+/**
+ * For disallowing Source Editing configuration that allows self-XSS.
+ *
+ * @Constraint(
+ *   id = "SourceEditingPreventSelfXssConstraint",
+ *   label = @Translation("Source Editing should never allow self-XSS.", context = "Validation"),
+ * )
+ *
+ * @internal
+ */
+class SourceEditingPreventSelfXssConstraint extends Constraint {
+
+  /**
+   * When Source Editing is configured to allow self-XSS.
+   *
+   * @var string
+   */
+  public $message = 'The following tag in the Source Editing "Manually editable HTML tags" field is a security risk: %dangerous_tag.';
+
+}

core/modules/ckeditor5/src/Plugin/Validation/Constraint/SourceEditingPreventSelfXssConstraintValidator.php

+<?php
+
+declare(strict_types = 1);
+
+namespace Drupal\ckeditor5\Plugin\Validation\Constraint;
+
+use Drupal\ckeditor5\HTMLRestrictions;
+use Symfony\Component\Validator\Constraint;
+use Symfony\Component\Validator\ConstraintValidator;
+use Symfony\Component\Validator\Exception\UnexpectedTypeException;
+
+/**
+ * Ensures Source Editing cannot be configured to allow self-XSS.
+ *
+ * @internal
+ */
+class SourceEditingPreventSelfXssConstraintValidator extends ConstraintValidator {
+
+  use TextEditorObjectDependentValidatorTrait;
+
+  /**
+   * {@inheritdoc}
+   *
+   * @throws \Symfony\Component\Validator\Exception\UnexpectedTypeException
+   *   Thrown when the given constraint is not supported by this validator.
+   */
+  public function validate($value, Constraint $constraint) {
+    if (!$constraint instanceof SourceEditingPreventSelfXssConstraint) {
+      throw new UnexpectedTypeException($constraint, __NAMESPACE__ . '\SourceEditingPreventSelfXssConstraint');
+    }
+    if (empty($value)) {
+      return;
+    }
+
+    $restrictions = HTMLRestrictions::fromString($value);
+    // @todo Remove this early return in
+    //   https://www.drupal.org/project/drupal/issues/2820364. It is only
+    //   necessary because CKEditor5ElementConstraintValidator does not run
+    //   before this, which means that this validator cannot assume it receives
+    //   valid values.
+    if ($restrictions->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) . '$/';
+  }
+
+}

core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php

@@ -13,6 +13,8 @@
 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 @@ public function provider(): array {
    * @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 @@ public function providerPair(): array {
       ],
       'violations' => [],
     ];
+    $self_xss_source_editing = [
+      // Dangerous attribute with all values allowed.
+      '<p onhover>',
+      '<img on*>',
+      '<blockquote style>',
+
+      // No danger.
+      '<marquee>',
+
+      // Dangerous attribute with some values allowed.
+      '<a onclick="javascript:*">',
+      '<code style="foo: bar;">',
+
+      // 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' => '<p onhover style> <br> <img on*> <blockquote style> <marquee> <a onclick="javascript:*"> <code style="foo: bar;">',
+            '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: <em class="placeholder">&lt;p onhover&gt;</em>.',
+        'settings.plugins.ckeditor5_sourceEditing.allowed_tags.1' => 'The following tag in the Source Editing "Manually editable HTML tags" field is a security risk: <em class="placeholder">&lt;img on*&gt;</em>.',
+        'settings.plugins.ckeditor5_sourceEditing.allowed_tags.2' => 'The following tag in the Source Editing "Manually editable HTML tags" field is a security risk: <em class="placeholder">&lt;blockquote style&gt;</em>.',
+        'settings.plugins.ckeditor5_sourceEditing.allowed_tags.4' => 'The following tag in the Source Editing "Manually editable HTML tags" field is a security risk: <em class="placeholder">&lt;a onclick=&quot;javascript:*&quot;&gt;</em>.',
+        'settings.plugins.ckeditor5_sourceEditing.allowed_tags.5' => 'The following tag in the Source Editing "Manually editable HTML tags" field is a security risk: <em class="placeholder">&lt;code style=&quot;foo: bar;&quot;&gt;</em>.',
+        'settings.plugins.ckeditor5_sourceEditing.allowed_tags.6' => 'The following tag in the Source Editing "Manually editable HTML tags" field is a security risk: <em class="placeholder">&lt;$text-container style&gt;</em>.',
+      ],
+    ];
+    $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;
   }