Problem/Motivation

In the "Allowed HTML tags" of Filter settings (eg. admin/config/content/formats/manage/basic_html), it's saving & display RAW user inputs. It allowed to hijack anything into tags.

http://cgit.drupalcode.org/drupal/tree/core/modules/filter/filter.filter...

You can see this vulnerability by:
1. Go to admin/config/content/formats/manage/basic_html
2. Paste `<img src=x onerror=alert(1)>` into "Allowed HTML tags"
3. Save or blur input
4. Edit again (executed on blur directly)

Proposed resolution

A. Use document.implementation.createHTMLDocument. See background details: https://github.com/jquery/jquery/issues/2965
Pros: Simple

BUG: #2865842: Parsing of allowed HTML tags (for integration with Text Editor configuration) throws JS error when adding <tbody> element

B. Fork CKEditor parser. (below patch just as demo, not the final fork copy)
Pros: (It may able to share same data structures with CKEditor config.)

Remaining tasks

- Pick a workaround
- Code Patch & Tests
- Review

User interface changes

- N/A

API changes

- N/A

Data model changes

- N/A

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

droplet created an issue. See original summary.

cmanalansan’s picture

Issue tags: +neworleans2016

Looking at this right now at DrupalCon NOLA (LaunchPad extended sprints)

cmanalansan’s picture

Issue summary: View changes
cmanalansan’s picture

Status: Active » Needs review
cmanalansan’s picture

This is not exclusively a CKEditor issue (thanks @vegantriathlete).

You can see also this vulnerability by:

  1. Go to admin/config/content/formats/manage/restricted_html (note: the text editor here is "none")
  2. Paste `<img src=x onerror=alert(1)>` into "Allowed HTML tags"
  3. Save or blur input
  4. Edit again (executed on blur directly)
vegantriathlete’s picture

vegantriathlete’s picture

Issue summary: View changes
vegantriathlete’s picture

Issue summary: View changes
vegantriathlete’s picture

cmanalansan’s picture

I have confirmed that both patches prevent this particular vulnerability. Meanwhile, I will be writing the JavaScript functional test.

cmanalansan’s picture

Hitting a wall with the functional test, submitting what I have so far. Regardless of what method we go with, this should aim to cover either case.

cmanalansan’s picture

Status: Needs review » Needs work
FileSize
1.64 KB

Whoops, the patch I previously uploaded was the PHPStorm-generated patch.

andypost’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: functional-js-test-12.patch, failed testing.

aburrows’s picture

Assigned: Unassigned » aburrows

Working on this issue.
cmanalansan - can you please use the patchname suggestion for consistency. e.g. unfiltered_data_in-2725255-15.patch
Thanks :)

droplet’s picture

OffTopic:
Always get a patch from online and you never have to remember what that is. Don't trust your memory. Novice Docs should change that suggestion. If you have mess folder offline, tidy it up instead. Anyone can help me get this feedback back.:)

aburrows’s picture

Assigned: aburrows » Unassigned

Not had time to take at this today, works got in the way.

droplet’s picture

Status: Needs work » Needs review

Pick a solution is more important than make a Patch & Tests at the moment.

droplet’s picture

Recall for feedback. To me, I'm ready for either solution.

I just recheck the policy, is it a Critical issue?
https://www.drupal.org/node/45111

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

droplet’s picture

Issue summary: View changes
b_sharpe’s picture

Very much in favor of "B" (I've attached rolled against current 8.3 Dev) as it also solves issues with table tags: #2865842: Parsing of allowed HTML tags (for integration with Text Editor configuration) throws JS error when adding <tbody> element

I also agree patching the issue and getting committed is more important than a test currently due to the XSS nature of the bug.

Status: Needs review » Needs work

The last submitted patch, 23: 2725255-parsetags-23.patch, failed testing.

b_sharpe’s picture

Guess I should've fixed the JS coding issues as well :)

b_sharpe’s picture

Status: Needs work » Needs review
droplet’s picture

Thanks b_sharpe.

So option "A" won't work because there's another bug. And CKEDITOR.htmlParser is more well-tested I believed.

The remaining task is:

Should we fork it or just reuse ckeditor.js? This is almost 100% loaded on that page with standard installation profile and admin only. I think to reuse it is fine.

the source code of this function:
http://docs.ckeditor.com/source/htmlparser.html#CKEDITOR-htmlParser

droplet’s picture

The patch should be failed. We haven't force to load the ckeditor.js.

Status: Needs review » Needs work

The last submitted patch, 28: 2725255-28.patch, failed testing.

b_sharpe’s picture

Fixes syntax issues and adds CKEDITOR dependency.

b_sharpe’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 30: 2725255-29.patch, failed testing.

b_sharpe’s picture

Status: Needs work » Needs review
FileSize
7.01 KB

HA, apparently someone was copying namespaces :)

Fatal error: Cannot redeclare class Drupal\Tests\toolbar\FunctionalJavascript\FilterIntegrationTest

Let's try this again...

Status: Needs review » Needs work

The last submitted patch, 33: 2725255-31.patch, failed testing.

b_sharpe’s picture

Status: Needs work » Needs review
FileSize
6.98 KB

Requires editor for Drupal.FilterHTMLRule(), also removed screenshot causing test to fail, not sure why that was in there.

droplet’s picture

removed screenshot causing test to fail, not sure why that was in there.

Thanks @b_sharpe! It added by me to figure how the filter UI looks in testing env and forgot to remove it.

droplet’s picture

Assigned: Unassigned » drpal
Issue summary: View changes

Most of the code & the tests coded by me. I'm not the right person to review this patch. Let's heard some feedback from another 2 JS Maintainers also. And then filter.module maintainers.

Thanks All!

+++ b/core/modules/filter/tests/src/FunctionalJavascript/FilterIntegrationTest.php
@@ -0,0 +1,104 @@
+    $injected_tag = ' <img src=x onerror=jQuery("[for=edit-filters-filter-html-settings-allowed-html]").html("hacked")>';

Just noticed that it should check the element existence before $.html or use BODY tag.

nod_’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs subsystem maintainer review

Patch fixes the issue. Looks good to me.

CKEditor is a big file to load just for that but it's what ckeditor does best so it makes sense to me.

drpal’s picture

Assigned: drpal » Unassigned
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

The patch needs a reroll for ES6 see https://www.drupal.org/node/2815083 - this will only affect the 8.4.x version of the patch (where this has to be fixed first). Also I ran FilterIntegrationTest without the fixes to filter.filter_html.admin.js and \Drupal\Tests\filter\FunctionalJavascript\FilterIntegrationTest::testJSInjection() passed so this test is not really testing the exploit outlined in the issue summary.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.36 KB

Re-rolled.

alexpott’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs review » Needs work

@Jo Fitzgerald - see https://www.drupal.org/node/2815083 for more about the ES6 move. You need to apply the changes to core/modules/filter/filter.filter_html.admin.es6.js and then compile the js.

drpal’s picture

Status: Needs work » Needs review
FileSize
8.26 KB
9.11 KB

Rerolled with the new JavaScript procedure.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.