Problem/Motivation

The Xss::filter() method does not remove srcdoc attributes from iframe tags.
This attribute can be used to inject JS into the page.
The XSS filter removes other dangerous attributes like onclick or onload, so I think it should also remove srcdoc.

Xss::filter() does not allow iframe tags by default, so it needs to be called explicitly with iframe in the $allowed_html_tags parameter to be vulnerable.
You could argue that this is a misuse of the XSS filter, but I think it is an easy mistake to make and it could benefit from this hardening.

This was originally logged as a private issue to the security team, but was cleared to be moved to the public queue.

Steps to reproduce

If someone calls this:

\Drupal\Component\Utility\Xss::filter('<iframe srcdoc="&lt;script&gt;alert(document.cookie)&lt;/script&gt;"></iframe>', ['iframe'])

They could expect a safe iframe tag to be returned but instead the returned HTML could contain malicious JS.

Proposed resolution

Remove srcdoc attributes in Xss::attributes().

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3511566

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

prudloff created an issue. See original summary.

prudloff’s picture

Issue summary: View changes
Status: Active » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Reading up on srcdoc on https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/srcdoc and the example

iframe.srcdoc = `<!doctype html><p>Hello World!</p>`;
document.body.appendChild(iframe);

Shows HTML being used so would agree it probably should be stripped. Not sure if this needs a CR? Will let committer decide

  • catch committed 17be2e1b on 10.5.x
    Issue #3511566 by prudloff, smustgrave: Remove srcdoc attributes in Xss...

  • catch committed 540351dc on 11.1.x
    Issue #3511566 by prudloff, smustgrave: Remove srcdoc attributes in Xss...
catch’s picture

Version: 11.x-dev » 10.5.x-dev
Status: Reviewed & tested by the community » Fixed

Don't think this needs a CR, just a bug fix for me.

Committed/pushed to 11.x, 11.1.x and 10.5.x, thanks!

  • catch committed 26caa6b1 on 11.x
    Issue #3511566 by prudloff, smustgrave: Remove srcdoc attributes in Xss...

catch’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.