Last Comment Bug 604332 - Security changes for copy/paste of html contents into contentEditable breaks TinyMCE
: Security changes for copy/paste of html contents into contentEditable breaks ...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Editor (show other bugs)
: unspecified
: x86 Windows Vista
: -- normal (vote)
: mozilla2.0b7
Assigned To: :Ehsan Akhgari (busy, don't ask for review please)
:
Mentors:
Depends on: 619287
Blocks: CVE-2010-2769
  Show dependency treegraph
 
Reported: 2010-10-14 04:01 PDT by Spocke
Modified: 2010-12-15 15:30 PST (History)
7 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.13-fixed
.16-fixed


Attachments
Patch (v1) (4.79 KB, patch)
2010-10-15 09:00 PDT, :Ehsan Akhgari (busy, don't ask for review please)
roc: review+
roc: approval2.0+
christian: approval1.9.2.13+
christian: approval1.9.1.16+
Details | Diff | Review

Description Spocke 2010-10-14 04:01:45 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 ( .NET CLR 3.5.30729; .NET4.0E)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 ( .NET CLR 3.5.30729; .NET4.0E)

Copy/paste of images breaks in TinyMCE since the custom attributes with the _mce_ prefix gets removed. This is a regression and isn't an issue on other browsers it will break existing systems out there that uses TinyMCE as it's rich text editor.

Reproducible: Always

Steps to Reproduce:
See this bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=596300
Actual Results:  
Custom attributes gets removed.

Expected Results:  
Custom attributes should be retained.
Comment 1 :Ehsan Akhgari (busy, don't ask for review please) 2010-10-14 11:44:06 PDT
One option here is to white-list custom attributes in _mce "namespace", but I'm not sure if that's the cleanest solution.  roc, bz, what do you guys think?
Comment 2 Boris Zbarsky [:bz] 2010-10-14 11:48:40 PDT
Are there other common editor widgets we need to worry about?

Or put another way, won't that be entrenching the market position of TinyMCE and stifling competition?
Comment 3 :Ehsan Akhgari (busy, don't ask for review please) 2010-10-14 12:17:18 PDT
(In reply to comment #2)
> Are there other common editor widgets we need to worry about?
> 
> Or put another way, won't that be entrenching the market position of TinyMCE
> and stifling competition?

Well, yes, there are other editor widgets.  To address this problem in general, I still don't see any other way besides just allowing custom attributes... :(
Comment 4 Spocke 2010-10-14 14:13:49 PDT
I think the best way would be to keep browser vendor attributes starting with "_" it would solve CKEditor as well since they use _ck_<something> if I remember correctly. Also it would be really nice if it was possible to extract the unsanitized html code using the dataTransfer object that would be a feature in Firefox that would be useful for everyone. Something like event.dataTransfer.getData('text/html'); but the problem with that method is that we then would need people to upgrade their TinyMCE installations to use that new logic. Having access to the raw HTML would then make it up to the developers to secure it?
Comment 5 :Ehsan Akhgari (busy, don't ask for review please) 2010-10-14 16:17:27 PDT
(In reply to comment #4)
> I think the best way would be to keep browser vendor attributes starting with
> "_" it would solve CKEditor as well since they use _ck_<something> if I
> remember correctly.

White-listing attribute names beginning with "_" might be an option to consider.

> Also it would be really nice if it was possible to extract
> the unsanitized html code using the dataTransfer object that would be a feature
> in Firefox that would be useful for everyone. Something like
> event.dataTransfer.getData('text/html'); but the problem with that method is
> that we then would need people to upgrade their TinyMCE installations to use
> that new logic. Having access to the raw HTML would then make it up to the
> developers to secure it?

Why would you need this functionality?
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-10-14 19:49:14 PDT
Let's white-list _* for now. But we should work on getting editor widgets to use data- attributes so we can stop whitelisting _*; it's not entirely safe.
Comment 7 Sacha Varma 2010-10-15 06:33:14 PDT
I don't pretend to understand the security issues around preserving custom attributes, but:

Is allowing one class of custom attributes (those beginning with an underscore) consistent with the security considerations that led you to strip off all custom attributes? (In other words, could it lead to an exploit.)
Comment 8 :Ehsan Akhgari (busy, don't ask for review please) 2010-10-15 08:44:31 PDT
(In reply to comment #7)
> Is allowing one class of custom attributes (those beginning with an underscore)
> consistent with the security considerations that led you to strip off all
> custom attributes? (In other words, could it lead to an exploit.)

In theory, yes, it could.  In practice, we're only worried about attribute names which have the potential of running JS code in other browsers.  There aren't any such attributes which begin with "_" to the best of my knowledge.
Comment 9 :Ehsan Akhgari (busy, don't ask for review please) 2010-10-15 09:00:43 PDT
Created attachment 483492 [details] [diff] [review]
Patch (v1)
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-10-15 13:56:31 PDT
Comment on attachment 483492 [details] [diff] [review]
Patch (v1)

It's actually possible that other browsers have (or will have) vendor attributes starting with _ that might be unsafe. But this will have to do for now.
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2010-10-18 11:03:04 PDT
http://hg.mozilla.org/mozilla-central/rev/378f617276a8
Comment 12 Mike 2010-10-20 07:50:58 PDT
my editor does not use _underscore for all of its attributes, so its still busted on copy/paste.
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2010-10-20 13:32:20 PDT
(In reply to comment #12)
> my editor does not use _underscore for all of its attributes, so its still
> busted on copy/paste.

You should consider switching to HTML5 data-* attributes.
Comment 14 Mike 2010-10-20 13:39:29 PDT
I did, but I'm reiterating what someone said earlier that your giving preference to the "_attribute" and the editors that use those rather than "attribute". Its a slippery slope your riding when you exclude "attribute" over "_attribute". Eventually some browser, somewhere, will implement that.
Comment 16 :Ehsan Akhgari (busy, don't ask for review please) 2010-10-20 13:41:04 PDT
(In reply to comment #14)
> I did, but I'm reiterating what someone said earlier that your giving
> preference to the "_attribute" and the editors that use those rather than
> "attribute". Its a slippery slope your riding when you exclude "attribute" over
> "_attribute". Eventually some browser, somewhere, will implement that.

See comment 6, comment 8 and comment 10 please.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-10-20 14:01:52 PDT
Yes. To be clear: all editor authors should start transitioning to data-* attributes immediately. At some point we will start stripping all unknown, non data-* attributes.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-10-20 14:23:24 PDT
And yes, I agree this issue needs a lot more visibility. Ehsan, can you put together a post for Blizzard's hacks blog?
Comment 19 :Ehsan Akhgari (busy, don't ask for review please) 2010-10-20 14:36:53 PDT
(In reply to comment #18)
> And yes, I agree this issue needs a lot more visibility. Ehsan, can you put
> together a post for Blizzard's hacks blog?

Sure.

Note You need to log in before you can comment on or make changes to this bug.