This affects Drupal 7 and Drupal 8, but can be public since the exploit requires the "Administer permissions" permission and possible XSS via roles names is already know and public, such as: https://www.drupal.org/node/316136

Reported by alexpott
https://tracker.bugcrowd.com/submissions/326439e4f113e9ecec884ee26681155...

STR:

1. Install minimal profile
2. Create a role with the label: Body<img/src="x"/onerror="alert(document.domain)"/>
3. Visit admin/structure/block/manage/stark_tools and select the Body role. This will execute the javascript. Save the block. And then the javascript will execute when anyone else visits the page to administer the block.

This is also broken in Drupal 7. It is caused by using .text() in Javascript. The problem is in Drupal.behaviors.blockSettingsSummary

Found via https://www.drupal.org/node/2512478 so other uses of text() should be examined as well.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
549 bytes
1.23 KB

D7 and D8 patches were submitted by alexpott on bugcrowd.

pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

alexpott’s picture

It's a shame we can't test stuff like this.

pwolanin’s picture

Well, I assume we will eventually with behat, but not yet.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, looks great.

Yes, too bad BrowserTestBase cannot execute Javascript currently.

alexpott’s picture

As I provided this patch to bugcrowd ticking the credit box.

  • xjm committed a9136f2 on 8.0.x
    Issue #2513646 by pwolanin, alexpott: Role name is unescaped on block...
xjm’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +JavaScript

As a major bugfix and security improvement, this issue is a prioritized change during the beta. Committed and pushed to 8.0.x. Thanks!

Is there any place (in security or JS best practices) that we could highlight the importance of the difference between these two jQuery methods? As @Wim Leers described, from the naming alone, it's easy to mix them up.

Moving to 7.x for the backport.

naveenvalecha’s picture

Status: Patch (to be ported) » Needs review
FileSize
585 bytes

..

Status: Needs review » Needs work

The last submitted patch, 11: d7-block-admin-xss.patch, failed testing.

Status: Needs work » Needs review
Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC as well

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

That patch is changing the content type setting, not the role setting, so it doesn't fix the bug.

I'm not sure if we need this change on the content type setting too (probably not since they appear to be going through filter_xss_admin() rather than check_plain()).

David_Rothstein’s picture

I'm not sure if we need this change on the content type setting too (probably not since they appear to be going through filter_xss_admin() rather than check_plain()).

Either way it's not a security issue there either, since content type names require "administer content types" to set which is also a restricted permission.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.23 KB

What was wrong with the D7 patch in #2 from alexpott?

Reposting his patch.

David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

That one looks good. I did some testing and there are situations where it's needed for the content types too, so marking RTBC.

Elsewhere in core, Drupal.checkPlain() is used for this. For the roles that might be a cleaner way to do it, but for the content types it wouldn't work (at least if we believe they actually are allowed to contain HTML; that honestly might just be a separate bug in the server-side code) so using .html() consistently throughout the file seems reasonable.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

  • David_Rothstein committed d3e0221 on 7.x
    Issue #2513646 by pwolanin, naveenvalecha, alexpott: Role name is...

Status: Fixed » Closed (fixed)

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