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

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

Remaining tasks

- Pick a workaround
- Code Patch & Tests
- Review

User interface changes

- N/A

API changes

- N/A

Data model changes

- N/A

CommentFileSizeAuthor
#89 interdiff_88-89.txt2.04 KBranjith_kumar_k_u
#89 2725255-89.patch9.38 KBranjith_kumar_k_u
#88 interdiff_2725255_87-88.txt717 byteskarishmaamin
#88 2725255-88.patch9.29 KBkarishmaamin
#87 2725255-87.patch9.32 KBSuresh Prabhu Parkala
#83 2725255-83.patch9.37 KBNWOM
#82 2725255-82.patch9.37 KBravi.shankar
#81 2725255-81.patch9.37 KBNWOM
#80 2725255-80.patch9.33 KBNWOM
#76 2725255-76.patch9.49 KBrromore
#69 2725255-69.patch9.45 KBalianov
#69 2725255-69-test-only.patch3.88 KBalianov
#69 interdiff_2725255-62-69.txt903 bytesalianov
#62 interdiff-2725255-62.txt1.32 KBlokapujya
#62 2725255-62.patch9.45 KBlokapujya
#62 2725255-62-test-only.patch2.19 KBlokapujya
#57 unfiltered_data_in_allowed_html_tags-2725255-57-with-fix.patch9.48 KBtatarbj
#57 unfiltered_data_in_allowed_html_tags-2725255-57-test.patch3.86 KBtatarbj
#52 2725255-52.patch9.48 KBtatarbj
#43 2725255-43.patch9.11 KBGrandmaGlassesRopeMan
#43 interdiff-2725255-41-43.txt8.26 KBGrandmaGlassesRopeMan
#41 2725255-41.patch6.36 KBjofitz
#35 2725255-32.patch6.98 KBb_sharpe
#33 2725255-31.patch7.01 KBb_sharpe
#30 2725255-29.patch7.01 KBb_sharpe
#28 2725255-28.patch6.3 KBdroplet
#25 interdiff.txt243 bytesb_sharpe
#25 2725255-parsetags-24.patch2.4 KBb_sharpe
#23 2725255-parsetags-23.patch2.4 KBb_sharpe
#12 functional-js-test-12.patch1.64 KBcmanalansan
#11 functional-js-test.patch1.82 KBcmanalansan
A-no-more-createHTMLDocument.patch641 bytesdroplet
B-use-CKeditor-parser.patch2.4 KBdroplet
#6 restricted HTML no text editor.png78.42 KBvegantriathlete
#6 malicious allowed tag.png140.35 KBvegantriathlete
#6 exploit of malicious tag.png320.2 KBvegantriathlete
Support from Acquia helps fund testing for Drupal Acquia logo

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 » GrandmaGlassesRopeMan
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.

GrandmaGlassesRopeMan’s picture

Assigned: GrandmaGlassesRopeMan » 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.

jofitz’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.

GrandmaGlassesRopeMan’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.

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tatarbj’s picture

I'm planning to bring this issue to the security improvements sprint of DrupalCamp Ruhr this weekend and work on it with sprinters - marking it with the tag of #dcruhr18_SecImproveSprint.
It should be rerolled, tested and checked also does it need a backport to d7 or not.

tstoeckler’s picture

Issue tags: +dcruhr18
alianov’s picture

Assigned: Unassigned » alianov
alianov’s picture

Assigned: alianov » Unassigned
tatarbj’s picture

Assigned: Unassigned » tatarbj
Status: Needs review » Needs work

needs reroll

tatarbj’s picture

Status: Needs work » Needs review
FileSize
9.48 KB

Rerolled patch is attached for 8.6.x

kporras07’s picture

I confirm that the issue still exists on 8.6.x and that the patch in #52 applies cleanly and fix the issue.

tatarbj’s picture

Assigned: tatarbj » Unassigned
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Based on @kporras07 #53 comment, i'm marking it RTBC.

alexpott’s picture

@tatarbj can you upload a test only patch as well as the final patch so we can confirm the tests are testing what we think they are.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

Also the issue summary looks like it needs an update since we did not fork anything.

tatarbj’s picture

As you asked @alexpott:
- summary is updated (should i remove the tag?)
- test-only patch is uploaded, also the patch that is identically the same just like under #53 (only renamed).

Anything else?

Bests,
Balazs.

The last submitted patch, 57: unfiltered_data_in_allowed_html_tags-2725255-57-test.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tatarbj’s picture

Status: Needs review » Reviewed & tested by the community

marking it RTBC in favor of #53.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/filter/tests/src/FunctionalJavascript/FilterIntegrationTest.php
@@ -0,0 +1,104 @@
+  /**
+   * Tests if we can exploit JS injection when setting allowed HTML tags.
+   */
+  public function testJSInjection() {
+    $this->drupalGet('admin/config/content/formats/manage/basic_html');
+    $page = $this->getSession()->getPage();
+
+    // Test that javascript is run when adding an img tag with 'onerror'.
+    $injected_tag = ' <img src=x onerror=jQuery("[for=edit-filters-filter-html-settings-allowed-html]").html("hacked")>';
+    $page->fillField('edit-filters-filter-html-settings-allowed-html', $injected_tag);
+
+    // JavaScript blur event does not work properly on the test engine.
+    // So we reload the page and checking the result.
+    $page->pressButton('Save configuration');
+    $this->drupalGet('admin/config/content/formats/manage/basic_html');
+    $this->assertText('hacked');
+  }

This test has passed on the test only patch so I'm not sure it's testing what we think it is.

lokapujya’s picture

I think @alexpott is right; we weren't testing the exploit. It appeared the test was working because the test-only patch was failing, and the fixed version was passing, but not for the testJSInjection method. I hope I'm understanding the test correctly.

The last submitted patch, 62: 2725255-62-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 62: 2725255-62.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

lokapujya’s picture

Now, testJSInjection fails the test-only test (as it should.) However, it's also failing WITH the patch so the JSInjection isn't fixed yet.

alianov’s picture

Issue tags: +DevDaysLisbon

DDD Lisbon, trying to resolve.

DuaelFr’s picture

I'll try to mentor @alianov and help him moving this issue forward.

tatarbj’s picture

From today midday i'll be also in Lisbon, tell me guys if you need my help, otherwise enjoy DDD :)

alianov’s picture

tatarbj’s picture

Status: Needs work » Needs review

In order to see how the patches are, i'm changing the status of the issue to 'Needs review'.

Status: Needs review » Needs work

The last submitted patch, 69: 2725255-69.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.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.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

rromore’s picture

Rerolling for 8.8.x. Also works with 8.9.x.

rromore’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 76: 2725255-76.patch, failed testing. View results

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

NWOM’s picture

FileSize
9.33 KB

Here is a re-rolled patch for 9.0.x. I also fixed the following line that had a second "+":

++ $injected_tag = ' <img src=x onerror=jQuery("[for=edit-filters-filter-html-settings-allowed-html]").html"hacked")>';

NWOM’s picture

FileSize
9.37 KB

Please ignore the last patch. I had messed up the Tests directory.

ravi.shankar’s picture

Added reroll of patch #81.

NWOM’s picture

FileSize
9.37 KB

Here is a re-roll for D9.1.x

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

maximpodorov’s picture

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Suresh Prabhu Parkala’s picture

Status: Needs work » Needs review
FileSize
9.32 KB

Just a re-roll against the latest 9.4.x.

karishmaamin’s picture

ranjith_kumar_k_u’s picture

Status: Needs review » Needs work

The last submitted patch, 89: 2725255-89.patch, failed testing. View results

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.