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.
Comment | File | Size | Author |
---|---|---|---|
#17 | 2513646-D7-2.patch | 1.23 KB | pwolanin |
#11 | d7-block-admin-xss.patch | 585 bytes | naveenvalecha |
#2 | d7.block-admin-xss-do-not-test.patch | 1.23 KB | pwolanin |
#2 | d8.block-admin-xss.patch | 549 bytes | pwolanin |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedComment #2
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedD7 and D8 patches were submitted by alexpott on bugcrowd.
Comment #3
pwolanin CreditAttribution: pwolanin as a volunteer commentedComment #4
pwolanin CreditAttribution: pwolanin as a volunteer commentedComment #5
alexpottIt's a shame we can't test stuff like this.
Comment #6
pwolanin CreditAttribution: pwolanin as a volunteer commentedWell, I assume we will eventually with behat, but not yet.
Comment #7
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC, looks great.
Yes, too bad BrowserTestBase cannot execute Javascript currently.
Comment #8
alexpottAs I provided this patch to bugcrowd ticking the credit box.
Comment #10
xjmAs 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.
Comment #11
naveenvalecha..
Comment #14
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC as well
Comment #15
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThat 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()).
Comment #16
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedEither 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.
Comment #17
pwolanin CreditAttribution: pwolanin as a volunteer commentedWhat was wrong with the D7 patch in #2 from alexpott?
Reposting his patch.
Comment #18
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThat 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.
Comment #19
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedCommitted to 7.x - thanks!