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

CommentFileSizeAuthor
#10 xss_on_field_edit_form-2512478-10.patch737 bytesgoogletorp
Screen Shot 2015-06-25 at 11.31.24 PM.png215.22 KBpwolanin

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue summary: View changes

I thought we didn't consider exploits that required escalated privs as exploits?
Manage fields is an elevated perm.

dawehner’s picture

dawehner
Primary language German
commented
Priority: Critical » Major

No question, this issue should be fixed.

It is true that these permissions are restricted, see \Drupal\field_ui\FieldUiPermissions::fieldPermissions
so 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.

pwolanin’s picture

pwolanin commented
Priority: Major » Critical
Issue summary: View changes

Since 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.

pwolanin’s picture

pwolanin commented

per: https://www.drupal.org/drupal8-security-bounty there are specific permissions we consider elevated. not all admin permissions

pwolanin’s picture

pwolanin commented
Issue tags: +D8 Security Bounty, +D8 Accelerate
pwolanin’s picture

pwolanin commented
Title: XSS on field edit form via label field » XSS on field edit form via label field via ckeditor
pwolanin’s picture

pwolanin commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
commented

To be clear, its a restricted permission, but its not listed on the list of problematic permissions ...

G1N1’s picture

G1N1 commented

Thanks for the discussion on this issue. I reported this bug.

googletorp’s picture

googletorp commented
Status: Active » Needs review
Issue tags: +Needs manual testing
StatusFileSize
new xss_on_field_edit_form-2512478-10.patch737 bytes

I made a patch for this that seems to fix the issue.

Since the contents of the label has already been escaped, .html is the correct usage, since .text undo the escaping.

pwolanin’s picture

pwolanin commented

Thanks - good find. Do we need to check the other uses of .text() ?

I see in some cases we make that safe like:

 vals.push(Drupal.checkPlain($(this).text()))
googletorp’s picture

googletorp commented

jQuery 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.

fabianx’s picture

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

RTBC per #12

pwolanin’s picture

pwolanin commented

@Fabianx - did you manually test?

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue tags: -Needs manual testing +JavaScript

Manually tested. RTBC+1

fabianx’s picture

fabianx commented

Train 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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

The 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!

  • alexpott committed f1314cc on 8.0.x 
    Issue #2512478 by googletorp, pwolanin, G1N1: XSS on field edit form via...

Status: Fixed » Closed (fixed)

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