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
Comment | File | Size | Author |
---|---|---|---|
#89 | interdiff_88-89.txt | 2.04 KB | ranjith_kumar_k_u |
#89 | 2725255-89.patch | 9.38 KB | ranjith_kumar_k_u |
#88 | interdiff_2725255_87-88.txt | 717 bytes | karishmaamin |
#88 | 2725255-88.patch | 9.29 KB | karishmaamin |
#87 | 2725255-87.patch | 9.32 KB | Suresh Prabhu Parkala |
Comments
Comment #2
cmanalansan CreditAttribution: cmanalansan at CGI Federal commentedLooking at this right now at DrupalCon NOLA (LaunchPad extended sprints)
Comment #3
cmanalansan CreditAttribution: cmanalansan at CGI Federal commentedComment #4
cmanalansan CreditAttribution: cmanalansan at CGI Federal commentedComment #5
cmanalansan CreditAttribution: cmanalansan at CGI Federal commentedThis is not exclusively a CKEditor issue (thanks @vegantriathlete).
You can see also this vulnerability by:
<img src=x onerror=alert(1)>
` into "Allowed HTML tags"Comment #6
vegantriathleteComment #7
vegantriathleteComment #8
vegantriathleteComment #9
vegantriathleteComment #10
cmanalansan CreditAttribution: cmanalansan at CGI Federal commentedI have confirmed that both patches prevent this particular vulnerability. Meanwhile, I will be writing the JavaScript functional test.
Comment #11
cmanalansan CreditAttribution: cmanalansan at CGI Federal commentedHitting 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.
Comment #12
cmanalansan CreditAttribution: cmanalansan at CGI Federal commentedWhoops, the patch I previously uploaded was the PHPStorm-generated patch.
Comment #13
andypostComment #15
aburrows CreditAttribution: aburrows as a volunteer commentedWorking on this issue.
cmanalansan - can you please use the patchname suggestion for consistency. e.g. unfiltered_data_in-2725255-15.patch
Thanks :)
Comment #16
droplet CreditAttribution: droplet commentedOffTopic:
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.:)
Comment #17
aburrows CreditAttribution: aburrows as a volunteer commentedNot had time to take at this today, works got in the way.
Comment #18
droplet CreditAttribution: droplet commentedPick a solution is more important than make a Patch & Tests at the moment.
Comment #19
droplet CreditAttribution: droplet commentedRecall 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
Comment #22
droplet CreditAttribution: droplet commentedComment #23
b_sharpe CreditAttribution: b_sharpe at ImageX commentedVery 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.
Comment #25
b_sharpe CreditAttribution: b_sharpe at ImageX commentedGuess I should've fixed the JS coding issues as well :)
Comment #26
b_sharpe CreditAttribution: b_sharpe at ImageX commentedComment #27
droplet CreditAttribution: droplet commentedThanks 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
Comment #28
droplet CreditAttribution: droplet commentedThe patch should be failed. We haven't force to load the ckeditor.js.
Comment #30
b_sharpe CreditAttribution: b_sharpe at ImageX commentedFixes syntax issues and adds CKEDITOR dependency.
Comment #31
b_sharpe CreditAttribution: b_sharpe at ImageX commentedComment #33
b_sharpe CreditAttribution: b_sharpe at ImageX commentedHA, apparently someone was copying namespaces :)
Fatal error: Cannot redeclare class Drupal\Tests\toolbar\FunctionalJavascript\FilterIntegrationTest
Let's try this again...
Comment #35
b_sharpe CreditAttribution: b_sharpe at ImageX commentedRequires editor for
Drupal.FilterHTMLRule()
, also removed screenshot causing test to fail, not sure why that was in there.Comment #36
droplet CreditAttribution: droplet commentedThanks @b_sharpe! It added by me to figure how the filter UI looks in testing env and forgot to remove it.
Comment #37
droplet CreditAttribution: droplet commentedMost 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!
Just noticed that it should check the element existence before $.html or use BODY tag.
Comment #38
nod_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.
Comment #39
GrandmaGlassesRopeManComment #40
alexpottThe 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.
Comment #41
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #42
alexpott@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.
Comment #43
GrandmaGlassesRopeManRerolled with the new JavaScript procedure.
Comment #47
tatarbjI'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.
Comment #48
tstoecklerComment #49
alianov CreditAttribution: alianov commentedComment #50
alianov CreditAttribution: alianov commentedComment #51
tatarbjneeds reroll
Comment #52
tatarbjRerolled patch is attached for 8.6.x
Comment #53
kporras07 CreditAttribution: kporras07 at Manatí commentedI confirm that the issue still exists on 8.6.x and that the patch in #52 applies cleanly and fix the issue.
Comment #54
tatarbjBased on @kporras07 #53 comment, i'm marking it RTBC.
Comment #55
alexpott@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.
Comment #56
alexpottAlso the issue summary looks like it needs an update since we did not fork anything.
Comment #57
tatarbjAs 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.
Comment #59
tatarbjmarking it RTBC in favor of #53.
Comment #60
alexpottThis test has passed on the test only patch so I'm not sure it's testing what we think it is.
Comment #62
lokapujyaI 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.
Comment #65
lokapujyaNow, testJSInjection fails the test-only test (as it should.) However, it's also failing WITH the patch so the JSInjection isn't fixed yet.
Comment #66
alianov CreditAttribution: alianov commentedDDD Lisbon, trying to resolve.
Comment #67
DuaelFrI'll try to mentor @alianov and help him moving this issue forward.
Comment #68
tatarbjFrom today midday i'll be also in Lisbon, tell me guys if you need my help, otherwise enjoy DDD :)
Comment #69
alianov CreditAttribution: alianov as a volunteer commentedupdating as per: https://www.drupal.org/node/2945059
JavascriptTestBase is deprecated in favor of WebDriverTestBase
I confirm the patch #62 works as suggested in 'steps to reproduce'
The test still fails.
Comment #70
tatarbjIn order to see how the patches are, i'm changing the status of the issue to 'Needs review'.
Comment #76
rromore CreditAttribution: rromore at Webspec commentedRerolling for 8.8.x. Also works with 8.9.x.
Comment #77
rromore CreditAttribution: rromore at Webspec commentedComment #80
NWOM CreditAttribution: NWOM commentedHere 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")>';
Comment #81
NWOM CreditAttribution: NWOM commentedPlease ignore the last patch. I had messed up the Tests directory.
Comment #82
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #81.
Comment #83
NWOM CreditAttribution: NWOM commentedHere is a re-roll for D9.1.x
Comment #85
maximpodorov CreditAttribution: maximpodorov commentedThis should be re-checked and re-rolled as #2556069: JS error with elements in "allowed HTML tags" that can't be direct descendants of a div is in core now.
Comment #87
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedJust a re-roll against the latest 9.4.x.
Comment #88
karishmaamin CreditAttribution: karishmaamin at Specbee commentedTried to fix custom code failures.
Comment #89
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commented