Potential XSS vulnerability in colorbox and lazy load formatters

Created on 30 January 2025, 3 months ago

Problem/Motivation

This module has a potential XSS vulnerability because it injects the value of a data attribute directly into the DOM without sanitizing it.

Steps to reproduce

You can see this vulnerability by:
1. Enabling the module
2. Create a node type with a video embed field and add the field to a view mode with the "Colorbox Modal" formatter.
3. On the same node type, add a CKE field with a text format that allows "a" tags with "class" and "data-video-embed-field-modal" attributes.
4. As a user that can create nodes, publish a node containing this HTML in the CKE field:

<p><a class="video-embed-field-launch-modal" data-video-embed-field-modal="&lt;img src=x onerror=&apos;alert(`XSS`);&apos;&gt;">Click me!</a></p>

5. If another user clicks the link, it will trigger the injected JS.
This is mitigated by the fact the attacker must be able to create an HTML tag with the "data-video-embed-field-modal" attribute and the attacked user needs to click on the link.

The "Video (lazy load)" formatter is also vulnerable to a similar attack but with a different class and attribute:

<p><a class="video-embed-field-lazy" data-video-embed-field-lazy="&lt;img src=x onerror=&apos;alert(`XSS`);&apos;&gt;">Click me!</a></p>

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

🐛 Bug report
Status

Active

Version

2.0

Component

Code

Created by

🇪🇸Spain plopesc Valladolid

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @plopesc
  • 🇫🇷France mably

    Hi @plopesc, how are we supposed to sanitize it exactly?

    It looks like it could be anything in there, here is the code adding the attribute in the LazyLoad field formatter for example:

          $element[$delta] = [
            '#type' => 'container',
            '#attributes' => [
              'data-video-embed-field-lazy' => (string) $this->renderer->render($videos[$delta]),
              'class' => ['video-embed-field-lazy'],
    
  • 🇫🇷France prudloff Lille

    DOMPurify (https://github.com/cure53/DOMPurify) could be used for this, but it would add a new dependency.

  • 🇫🇷France mably

    Shouldn't it be fixed on CKEditor side preferably?

  • 🇫🇷France prudloff Lille

    It's true that this can be mitigated by not allowing the data-video-embed-field-modal in CKE config.
    However, there are valid reasons to allow data-* attributes and they are not dangerous by themselves.

    video_embed_field turns a safe attribute into dangerous markup so it should be this module's responsibility to ensure the markup is safe.

  • 🇫🇷France mably

    Looks like it would be a huge architectural change then, as the embed rendered code could have some valid javascript code in it.

    What would be the best way to do that then? Load the rendered code using Ajax?

  • 🇫🇷France prudloff Lille

    I see several ways to fix this:

    • Sanitize the value of data-video-embed-field-modal with DOMPurify before using it. But if the embed can contain legitimate JS this would remove it.
    • Insert the embed somewhere in the page (in a div tag with display:none; for example) and only have a selector in the data attribute.
    • Render the embed in a drupalSettings variable and use this variable to generate the dialog.
    • Load the embed with AJAX.
  • 🇫🇷France mably

    Thanks for the suggestions.

    So one of these solutions will have to be implemented before we merge back the CKEditor plugins into the 3.x dev branch.

  • 🇫🇷France GaëlG Lille, France

    gaëlg made their first commit to this issue’s fork.

  • 🇮🇳India abhishek@kumar

    Proposed Fix
    Sanitize the data-* attribute values before injecting them into the DOM. Possible approaches:

    • Use Drupal\Component\Utility\Html::escape() to sanitize the input.
    • Strip unwanted HTML/JS before processing the attribute.
    • Use data-* safely by ensuring values are treated as plain text, not HTML.

    Next Steps

    • Patch needed: A developer should submit a fix that properly escapes the data-* attributes.
    • Testing: Verify the fix prevents XSS while maintaining functionality.
    • Security advisory: Since this is public, a SA may be needed if the risk is deemed high enough.
  • Merge request !80Fix XSS vulnerability #3503383 → (Open) created by GaëlG
  • Pipeline finished with Failed
    19 days ago
    Total: 162s
    #469983
  • 🇫🇷France GaëlG Lille, France

    This seems to work. Please check it works the same as before, I'm not very used to these lazyload and modal features.

    abhishek@kumar: no offense, your comment seems spammy/generated by an LLM. Please avoid adding verbose comments which do not add value to the existing issue.

  • 🇫🇷France GaëlG Lille, France

    FieldOutputTest seems to need an update to reflect the new way it works.

  • Pipeline finished with Failed
    19 days ago
    Total: 170s
    #470001
Production build 0.71.5 2024