Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.STR as reported:
1. Login to account
2. Visit following URL: /admin/structure/block/block-content/manage/basic/fields
3. Click Manage fields and then edit tab
4. Name label as Body"></iframe><img/src="x"/onerror="alert(document.domain)"/><"
5. Save the settings
6. Now click back edit tab XSS alert will pop-up
Note from pwolanin: attack string can be simplified to:
Body<img/src="x"/onerror="alert(document.domain)"/>
This appears to occur via the ckeditor markup that's injected into the page, not any of the original markup. I see this in the console:
GET http://drupal-8.local:8083/admin/structure/block/block-content/manage/basic/fields/x 404 (Not Found)
Here's the attack in the manipulated page via Chrome inspector:
I can also reproduce on path: /admin/structure/comment/manage/comment/fields/comment.comment.comment_body
so it's not specific to block content.
Reported via Drupal 8 security bug bounty
https://tracker.bugcrowd.com/submissions/999c269994c4384d59e40d8dd5a6f21...
credit to:
https://www.drupal.org/u/g1n1
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | xss_on_field_edit_form-2512478-10.patch | 737 bytes | googletorp |
| Screen Shot 2015-06-25 at 11.31.24 PM.png | 215.22 KB | pwolanin | |
Comments
Comment #1
larowlanI thought we didn't consider exploits that required escalated privs as exploits?
Manage fields is an elevated perm.
Comment #2
dawehnerNo question, this issue should be fixed.
It is true that these permissions are restricted, see
\Drupal\field_ui\FieldUiPermissions::fieldPermissionsso given that I think this is major.
On the other hand we need to keep in mind that these permissions are often granted to less administrative users, for example in order to configure entity forms.
Comment #3
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedSince this is accessible via several routes where permissions are not documented as the dangerous one that control the site, and suggests there may be other XSS leaks in ckeditor, I think it's critical unless a committer triages it down.
Comment #4
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedper: https://www.drupal.org/drupal8-security-bounty there are specific permissions we consider elevated. not all admin permissions
Comment #5
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedComment #6
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedComment #7
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedComment #8
dawehnerTo be clear, its a restricted permission, but its not listed on the list of problematic permissions ...
Comment #9
G1N1 CreditAttribution: G1N1 commentedThanks for the discussion on this issue. I reported this bug.
Comment #10
googletorp CreditAttribution: googletorp as a volunteer commentedI made a patch for this that seems to fix the issue.
Since the contents of the label has already been escaped,
.htmlis the correct usage, since.textundo the escaping.Comment #11
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedThanks - good find. Do we need to check the other uses of .text() ?
I see in some cases we make that safe like:
Comment #12
googletorp CreditAttribution: googletorp as a volunteer commentedjQuery does a lot of escaping built in, so using
.text()is in itself not a problem. The issue arises if.html()is used at a later point to insert the value again some other place on the page. This is pretty much what CKEditor does through it's own templating system, which is what caused the problem in the first place.I tried to investigate our usage of
.html(), there are some places where.text()would be better suited, but I couldn't find a place where it would cause a security issue. In some places it hard to know which should be used, since they are used in some internal functions. Using.html()gives the flexibility that you can actually insert HTML which might be useful/needed in some cases.I guess the best option is to test for security issues and treat them on a case by case basis.
Comment #13
Fabianx CreditAttribution: Fabianx for Drupal Association commentedRTBC per #12
Comment #14
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented@Fabianx - did you manually test?
Comment #15
nod_Manually tested. RTBC+1
Comment #16
Fabianx CreditAttribution: Fabianx for Drupal Association commentedTrain of thought:
Maybe we should indeed instead of relying on the feature of using .html() have checkPlain run always via .text() instead as a pattern.
.text() is probably like strip_tags in PHP.
Comment #17
alexpottThe fix looks good and works. I don't think we should override jQuery's .text() and mkae it go through Drupal.checkPlain() - that'd be a massive javascript API change at this point.
Committed f1314cc and pushed to 8.0.x. Thanks!