Background information

This was originally reported as a private security issue, but has been approved for handling in the public queue by the Drupal Security Team.

Problem/Motivation

block.admin.js passes the value of the data-element attribute to $() without sanitizing it:

      const $input = $(once('block-filter-text', 'input.block-filter-text'));
      const $table = $($input.attr('data-element'));

This is dangerous because $() accepts either a selector or raw HTML.

Steps to reproduce

This could be exploited if an attacker can insert this payload on /admin/structure/block :

<input class="block-filter-text" data-element="&lt;img src=x onerror=alert()&gt;">

Proposed resolution

Using document.querySelector() would probably be safer.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3607797

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:

About issue forks

Comments

prudloff created an issue. See original summary.

prudloff opened merge request !16208

prudloff’s picture

prudloff commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Ran the test locally since can't in the pipeline right now

JavaScript condition met:
typeof window.XSS == 'undefined'
Failed asserting that false is true.
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php:161
/var/www/html/core/modules/block/tests/src/FunctionalJavascript/BlockXssTest.php:46

Change itself seems pretty minimal and makes sense to run through all so LGTM.

No additional feedback.

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

One comment on the MR. I think we can move the test assertions to Drupal\Tests\block\FunctionalJavascript\BlockFilterTest::testBlockFilter and save one test bootstrap.

brandonlira made their first commit to this issue’s fork.

brandonlira’s picture

brandonlira commented
Status: Needs work » Needs review

Addressed the review feedback by moving the XSS coverage into BlockFilterTest::testBlockFilter() and removing the separate BlockXssTest. The test still uses the block_test fixture and asserts that window.XSS remains undefined after loading the block admin page.

Please let me know if any further changes are needed.

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented
Status: Needs review » Reviewed & tested by the community

Looks good, did a fresh check after the last changes and I think this is good to go.

My first thoughts was to wonder if we still needed this to be jQuery, but changing that would be a bigger change than we should introduce in this issue.

  • longwave committed 7587cc07 on 11.4.x 
    fix: #3607797 Potential XSS in block.admin.js

By: prudloff
By:...

  • longwave committed 15e81137 on 11.x 
    fix: #3607797 Potential XSS in block.admin.js

By: prudloff
By:...

  • longwave committed 6d0b1224 on main 
    fix: #3607797 Potential XSS in block.admin.js

By: prudloff
By:...

  • longwave committed a2a9a82e on 10.6.x 
    fix: #3607797 Potential XSS in block.admin.js

By: prudloff
By:...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: main » 10.6.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 6d0b12247f4 to main and 15e811374ff to 11.x and 7587cc077b6 to 11.4.x. Thanks!

I also thought this might not backport cleanly, but turns out it does, so backported to 10.6.x as a security improvement.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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