Last Comment Bug 596300 - Element Attributes dropped in DesignMode/ContentEditable sections
: Element Attributes dropped in DesignMode/ContentEditable sections
Status: RESOLVED FIXED
: dataloss, regression, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: 1.9.2 Branch
: x86 All
: -- major with 26 votes (vote)
: mozilla2.0b7
Assigned To: :Ehsan Akhgari (busy, don't ask for review please)
:
Mentors:
: 594725 595152 605722 (view as bug list)
Depends on: 597784 598090 598105
Blocks: 424615 606117 CVE-2010-2769 613130
  Show dependency treegraph
 
Reported: 2010-09-14 11:07 PDT by gamecockguy2003
Modified: 2013-04-18 19:50 PDT (History)
31 users (show)
ehsan: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+
.11+
.11-fixed
.14+
.14-fixed


Attachments
Simple testcase (1006 bytes, text/html)
2010-09-17 08:29 PDT, Alfonso Martinez
no flags Details
Shows that moving an element results in custom and script attributes being dropped. (1.96 KB, text/html)
2010-09-17 08:39 PDT, gamecockguy2003
no flags Details
execCommand() issue demo (639 bytes, text/html)
2010-09-17 08:56 PDT, amouchinski
no flags Details
Fix (5.80 KB, patch)
2010-09-24 14:04 PDT, :Ehsan Akhgari (busy, don't ask for review please)
bzbarsky: review+
christian: approval1.9.2.11+
christian: approval1.9.1.14+
Details | Diff | Review

Description gamecockguy2003 2010-09-14 11:07:22 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.3 (KHTML, like Gecko) Chrome/6.0.472.55 Safari/534.3
Build Identifier: Firefox/3.6.9

This seems to have been caused by the fix for http://www.mozilla.org/security/announce/2010/mfsa2010-62.html. 

When a document is in DesignMode or if a section of a document is marked as ContentEditable, drag-and-drop of an element within the editable area results in some attributes being dropped from the element.

I've noticed two categories of attributes that are dropped:

1) Script-like attributes such as onmouseover. This clearly seems related to the bug mentioned above. While the change fixes an XSS issue for items dropped in from another browser, it breaks the ability to move an item within its own document without having its attributes removed.

2) Expando attributes. If you have an element with a non-standard (invalid) attribute on an element, then it will be dropped after you drag and then drop the element. This did not occur before upgrading from 3.6.8 to 3.6.9. Even if you use a custom Document Type Definition to define the new attributes, they will be dropped after a drag-and-drop action inside the contenteditable section or in a designmode document.

This creates significant problems for browser-based HTML editors like TinyMCE, etc

Reproducible: Always

Steps to Reproduce:
1.Create an HTML document with a DIV marked as contenteditable="true" and some content inside the DIV.
2.Include an IMG tag with an "onmouseover" attribute and possible a fake attribute such as "test"
3.View the page and notice that the attributes are on the element
4.Drag and drop the image to a different location in the DIV.
5.Notice that the element no longer has either the "test" or "onmouseover" attributes.
Actual Results:  
The IMG element has lost the two attributes.

Expected Results:  
The image element exists at its new location in the DIV with all of its original attributes.

This seems related to the fix for http://www.mozilla.org/security/announce/2010/mfsa2010-62.html.
Comment 1 amouchinski 2010-09-15 10:21:46 PDT
This issue seems to affect documents with custom namespaces declared when calling execCommand():

1. custom tags are completely blocked:
document.execCommand("insertHTML", false, '<custom:tag ... />')
-- inserts nothing

2. custom attributes are blocked as well:
document.execCommand("insertHTML", false, '<a href="..." custom:attr="...">...</a>')
-- inserts just <a href="...">...</a>
Comment 2 adek 2010-09-16 02:38:34 PDT
Confirmed. I'm also having big problems with it. 
My clients are complaining a lot.

My WYSIWYG editor's features stopped working.

Like someone said before. There is no simple workaround about this problem. I've got hope Firefox developers will take a look with it.
Comment 3 Jonathan BRAUN 2010-09-16 02:45:12 PDT
Confirmed
Problems with CKEditor WYSIWYG Editor...

No easy workaround... Webapp does not work on firefox because of this...
Comment 4 Standa Opichal 2010-09-16 04:54:41 PDT
Quite common practice to use custom namespace attributes in WYSIWYG Editors. The one I have see used (and broken by this) is the YUI2 simpleditor.

As stated above, there is no easy workaround and the current state breaks a lot of web pages.
Comment 5 amouchinski 2010-09-16 14:23:56 PDT
It looks like the issue persists in FF 3.6.10 and affects many users of all major rich text editors, including TinyMCE, CKEditor, YUI2, Movable Type, etc. Can we increase the priority of this ticket?
Comment 6 gamecockguy2003 2010-09-16 17:06:14 PDT
Updating importance based on comments above
Comment 7 Rocing Chan 2010-09-16 20:17:55 PDT
It looks like the issue persists in FF 3.6.10 and affects many users of all major rich text editors, including TinyMCE, CKEditor, YUI2, Movable Type, etc. Can we increase the priority of this ticket?
Comment 8 adek 2010-09-17 06:01:19 PDT
>Can we increase the priority of this ticket?

I think we need more votes on this bug and maybe somebody will look at it. I'd like to know as soon as possible what can I do with my clients, should I wait or if the won't fix it - I need to change scripts or change browser...
Comment 9 Anthony Ricaud (:rik) 2010-09-17 06:54:38 PDT
Could you provide a testcase?
Comment 10 Alfonso Martinez 2010-09-17 08:29:59 PDT
Created attachment 476264 [details]
Simple testcase

This simple testcase shows how the three attributes "onclick", "data-custom" and "testing" are removed from a link.
It works correctly in 3.6.8, it's broken in 3.6.9 and 3.6.10 and 4.0b5.
Works fine also in Chrome 6, Safari 5
Comment 11 gamecockguy2003 2010-09-17 08:39:20 PDT
Created attachment 476267 [details]
Shows that moving an element results in custom and script attributes being dropped.

Adding a test case that illustrates custom attributes and script attributes are dropped when moving an element inside of an editable div or designmode=on document.
Comment 12 amouchinski 2010-09-17 08:56:59 PDT
Created attachment 476272 [details]
execCommand() issue demo

This example demonstrates the issue with execCommand().
In FF 3.6.9 custom tags and attributes are ignored.
Comment 13 Daniel Veditz [:dveditz] 2010-09-17 11:22:26 PDT
The regressing fix for bug 520189 landed on the 1.9.1 branch  -- can this be reproduced in 3.5.12 too?
Comment 14 gamecockguy2003 2010-09-17 11:44:04 PDT
(In reply to comment #13)
> The regressing fix for bug 520189 landed on the 1.9.1 branch  -- can this be
> reproduced in 3.5.12 too?

Couldn't find 3.5.12, but I did just verify this issue occurs on 3.5.13.  This is consistent with the theory that it is related to the fix for 520189.
Comment 15 Florian Schmitt 2010-09-20 02:53:44 PDT
Is there a list of "valid" elements and attributes? The canvas element seems to get stripped, too, but i suppose it to be a valid element, since firefox supports it for years.
Comment 16 dave batiste 2010-09-20 07:24:14 PDT
Looks like 3.5.13 is also affected.  It looks like is also affects aria attributes.
Comment 17 Boris Zbarsky [:bz] 2010-09-20 08:39:37 PDT
> Is there a list of "valid" elements and attributes?

As of today, the allowed tags are http://hg.mozilla.org/mozilla-central/file/b439e73f33b7/content/base/src/nsContentSink.cpp#l1732 and the allowed attributes are http://hg.mozilla.org/mozilla-central/file/b439e73f33b7/content/base/src/nsContentSink.cpp#l1819

Probably worth filing separate bugs on adding aria attributes (which may be safe to leave) and canvas (which I think should also be safe) to the whitelists.
Comment 18 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-20 13:48:17 PDT
Sorry for not commenting here sooner.  Here's a rundown summary on how I'm planning to address this issue:

1. For HTML5 elements and attributes, I filed bug 598090.
2. For HTML5 data-* attributes, I filed bug 598105.
3. For custom attributes, my initial reaction was that we should just drop them for security purposes.  Boris, what do you think?
4. I'm also beginning to think that we should maybe not use the sanitizing fragment content sink for execCommand("inserthtml"), since page script can already inject whatever it wants in that case, using things like innerHTML.  The original idea in bug 520189 was protecting paste and drop operations, and I might have got carried away there.  :-)  Boris, if you agree, let me know and I'll file a bug for that.

As far as I can tell, these four issues should cover everything that this bug is about, right?
Comment 20 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-20 13:52:57 PDT
(In reply to comment #18)
> 4. I'm also beginning to think that we should maybe not use the sanitizing
> fragment content sink for execCommand("inserthtml"), since page script can
> already inject whatever it wants in that case, using things like innerHTML. 

One case in point is bug 597784, FWIW.
Comment 21 Boris Zbarsky [:bz] 2010-09-20 14:02:36 PDT
I agree on item 3.  For item 4... it's probably more web-compatible to allow pages to shoot themselves in the foot there. :(
Comment 22 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-20 14:09:06 PDT
(In reply to comment #21)
> I agree on item 3.  For item 4... it's probably more web-compatible to allow
> pages to shoot themselves in the foot there. :(

By "agree on item 3", do you mean that you agree that we should continue dropping those attributes, or that we should allow them?
Comment 23 Boris Zbarsky [:bz] 2010-09-20 14:15:40 PDT
I think we should probably keep dropping them for now and see how things go.
Comment 24 gamecockguy2003 2010-09-20 14:20:43 PDT
(In reply to comment #18)
> Sorry for not commenting here sooner.  Here's a rundown summary on how I'm
> planning to address this issue:
> 
> 1. For HTML5 elements and attributes, I filed bug 598090.
> 2. For HTML5 data-* attributes, I filed bug 598105.
> 3. For custom attributes, my initial reaction was that we should just drop them
> for security purposes.  Boris, what do you think?
> 4. I'm also beginning to think that we should maybe not use the sanitizing
> fragment content sink for execCommand("inserthtml"), since page script can
> already inject whatever it wants in that case, using things like innerHTML. 
> The original idea in bug 520189 was protecting paste and drop operations, and I
> might have got carried away there.  :-)  Boris, if you agree, let me know and
> I'll file a bug for that.
> 
> As far as I can tell, these four issues should cover everything that this bug
> is about, right?

For item 3, it would seem that if an attribute is "custom" (not recognized by Firefox) that there wouldn't be a security risk of Firefox interpreting and executing anything based on that unrecognized attribute.

Other commonly used browsers, similar to previous versions of Firefox, leave these attributes as they are, and many WYSIYG implementations rely on this.  Dropping this functionality without warning is going to be a serious pain for a lot of developers, and will remove the value of resolving this bug for many that have been following it.
Comment 25 Boris Zbarsky [:bz] 2010-09-20 14:25:13 PDT
> that there wouldn't be a security risk of Firefox interpreting 

Right.  The security risk is that of Firefox being used as a vector for delivering exploits targeted at other browsers.
Comment 26 adek 2010-09-20 14:31:51 PDT
> Other commonly used browsers, similar to previous versions of Firefox, leave
> these attributes as they are, and many WYSIYG implementations rely on this. 
> Dropping this functionality without warning is going to be a serious pain for a
> lot of developers, and will remove the value of resolving this bug for many
> that have been following it.

Please, don't change something which was working almost from the beginning.

I agree with @gamecockguy2003 in 100%!
Comment 27 dave batiste 2010-09-20 14:41:08 PDT
I tend to agree.  It is up to other browsers to protect against the exploits you mention.  I don't think this is the concern of FF.  A malicious user can just as easily craft some HTML using a text editor and upload it with these "custom" attributes.  

Is there no solution that can meet your security concerns that does not involve such a major breaking change?
Comment 28 Boris Zbarsky [:bz] 2010-09-20 14:44:49 PDT
> Please, don't change something which was working almost from the beginning.

We've done that before, e.g. when we implemented a popup blocker.  The question is the tradeoff.

> It is up to other browsers to protect against the exploits you mention.

How?  The issue is if HTML5 adds a new scripting feature, some other browser implements it, and Firefox doesn't.  Then from our point of view the attribute is unknown and we'd leave it, and it would run script in the other browser.  Whereas if we also implemented the feature we'd not allow the attribute since it runs script... and then it wouldn't run script in either browser.

That's the fundamental issue with blacklist vs whitelist: blacklisting means that as soon as some new stuff gets added you end up insecure, whereas whitelisting means that when new stuff gets added it doesn't work until you add it to your whitelist and that completely unknown stuff just doesn't work.

> A malicious user can just as easily craft some HTML using a text editor and
> upload it with these "custom" attributes.  

Can they?  A lot of places where contenteditable is used do not allow arbitrary file uploads.

> Is there no solution that can meet your security concerns that does not
> involve such a major breaking change?

We clearly haven't thought of one yet; otherwise we wouldn't be asking for trouble like this, obviously.
Comment 29 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-20 14:56:19 PDT
(In reply to comment #24)
> Other commonly used browsers, similar to previous versions of Firefox, leave
> these attributes as they are, and many WYSIYG implementations rely on this. 
> Dropping this functionality without warning is going to be a serious pain for a
> lot of developers, and will remove the value of resolving this bug for many
> that have been following it.

Editor widgets should move to using data-* attributes if they want to attach custom data to elements which survive copying and pasting.
Comment 30 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-20 14:58:35 PDT
(In reply to comment #27)
> Is there no solution that can meet your security concerns that does not involve
> such a major breaking change?

Unfortunately there doesn't seem to be.  Please note that while we could choose to ignore the security of users using other browsers and only care about Firefox users, we decided to go against that convenient choice and try to protect as many users as possible, even if they're not using Firefox.

As I said in comment 29, a work-around is to use data-* attributes, which are specified by the HTML5 spec, and are guaranteed to work in any HTML5 compliant browser.
Comment 31 gamecockguy2003 2010-09-20 15:01:41 PDT
(In reply to comment #29)
> (In reply to comment #24)
> > Other commonly used browsers, similar to previous versions of Firefox, leave
> > these attributes as they are, and many WYSIYG implementations rely on this. 
> > Dropping this functionality without warning is going to be a serious pain for a
> > lot of developers, and will remove the value of resolving this bug for many
> > that have been following it.
> 
> Editor widgets should move to using data-* attributes if they want to attach
> custom data to elements which survive copying and pasting.

Agreed that this is the proper solution going forward.  Unfortunately, it means many broken clients for lots of companies in the meantime.  Ideally, this would be phased out rather than being disabled without notice.
Comment 32 adek 2010-09-20 15:03:00 PDT
Please, correct me if I'm wrong. 

You will allow custom attributes: data-*

But what about attributes like "onclick"? 
Will you still drop them?
Comment 33 gamecockguy2003 2010-09-20 15:07:43 PDT
(In reply to comment #28)
> > A malicious user can just as easily craft some HTML using a text editor and
> > upload it with these "custom" attributes.  
> 
> Can they?  A lot of places where contenteditable is used do not allow arbitrary
> file uploads.
> 

You are correct that they probably can't upload files.  However, usually it isn't the HTML of the page being itself that is submitted to the server in one of these editors.  Typically it will be serialized and submitted through a HIDDEN input or some other mechanism.  This means that a user could create a request that would submit whatever HTML they want.

My point is that this change does not reduce the need to sanitize the HTML that comes from a content-editable section.  As a result, this change does little in that regard, but does a lot in terms of breaking existing solutions until developers can scramble to get hotfixes out.
Comment 34 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-20 15:24:17 PDT
(In reply to comment #21)
> I agree on item 3.  For item 4... it's probably more web-compatible to allow
> pages to shoot themselves in the foot there. :(

Morphed byug 597784 to deal with that.
Comment 35 dave batiste 2010-09-20 15:25:44 PDT
I respect that you are trying to protect as many users as possible, even if they are not FF users, but there will always be cracks to plug that are well outside of your control.

The methods for loading content into a system are determined by the system.  Ultimately, filtering should be done at display-time; this has not changed, and nor will it with these changes.
Comment 36 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-20 15:26:07 PDT
(In reply to comment #32)
> Please, correct me if I'm wrong. 
> 
> You will allow custom attributes: data-*

Starting from tomorrow's mozilla-central nightly, yes.  That patch will also be backported to the next dot-release of 3.5 and 3.6.

> But what about attributes like "onclick"? 
> Will you still drop them?

Yes, because they can result in code execution.
Comment 37 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-20 15:28:24 PDT
(In reply to comment #33)
> My point is that this change does not reduce the need to sanitize the HTML that
> comes from a content-editable section.  As a result, this change does little in
> that regard, but does a lot in terms of breaking existing solutions until
> developers can scramble to get hotfixes out.

This is not the rationale behind this change.  I'm afraid you need to wait until bug 520189 becomes publicly visible to know more about the security risk involved, and why we chose to fix things this way.
Comment 38 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-20 15:30:30 PDT
(In reply to comment #35)
> I respect that you are trying to protect as many users as possible, even if
> they are not FF users, but there will always be cracks to plug that are well
> outside of your control.

If we were to accept this as a principle, we wouldn't fix any security holes, on the grounds that there will always be cracks to plug that are out of our control!  This is not how security works.

> The methods for loading content into a system are determined by the system. 
> Ultimately, filtering should be done at display-time; this has not changed, and
> nor will it with these changes.

As I said, the fix to bug 520189 protects users against a real security threat which cannot be prevented by any kind of filtering at display-time.  I'm afraid that I cannot provide more details about this at this time.  :-(
Comment 39 gamecockguy2003 2010-09-20 15:38:21 PDT
(In reply to comment #37)
> (In reply to comment #33)
> > My point is that this change does not reduce the need to sanitize the HTML that
> > comes from a content-editable section.  As a result, this change does little in
> > that regard, but does a lot in terms of breaking existing solutions until
> > developers can scramble to get hotfixes out.
> 
> This is not the rationale behind this change.  I'm afraid you need to wait
> until bug 520189 becomes publicly visible to know more about the security risk
> involved, and why we chose to fix things this way.

I understand.  I suppose it will be more clear when that becomes public knowledge.  Though I will say that if that isn't the rationale, I can't foresee any reason to be concerned about attributes unknown to Firefox that are scripting attributes in other browsers.
Comment 40 dave batiste 2010-09-20 19:22:58 PDT
> If we were to accept this as a principle, we wouldn't fix any security holes,
> on the grounds that there will always be cracks to plug that are out of our
> control!  This is not how security works.

No.  I meant that it should not be the concern of FF to protect against exploits in other browsers.
 
> As I said, the fix to bug 520189 protects users against a real security threat
> which cannot be prevented by any kind of filtering at display-time.  I'm afraid
> that I cannot provide more details about this at this time.  :-(

I look forward to hearing more about it.
Comment 41 jonbell2 2010-09-20 19:40:49 PDT
Maybe I'm mistaken but:

"Security researcher Paul Stone reported that when an HTML selection containing JavaScript is copy-and-pasted or dropped onto a document with designMode enabled the JavaScript will be executed within the context of the site where the code was dropped. A malicious site could leverage this issue in an XSS attack by persuading a user into taking such an action and in the process running malicious JavaScript within the context of another site."

That suggests the security vulnerability is about scripts executing *in Firefox* -- not another browser.  So if that's the issue here, I don't think unknown tags would be an issue.   But I guess we'll see.
Comment 42 Alfonso Martinez 2010-09-21 00:38:30 PDT
We are only guessing until 520189 is made public, but here are my thoughts:

Doing this kind of changes means that deployed system from years ago are failing with new Firefoxes and the only solution (without spending time and money changing the html editor to a new one unreleased that is able to work correctly) is to switch browsers. 

My personal idea about what that bug is about has two options:
1. People is tricked to copy/dnd a malicious content into their editor. That content includes scripts/event handlers. Then it's saved and when a visitor comes it's executed in the context of the main site.
This can be fixed if the site is using something like http://htmlpurifier.org/ (and it should be using it already)

2. People is tricked to copy/dnd a malicious content into their editor. That content includes scripts/event handlers. Those scripts or handlers are executed right away. This happens only if the site is using contentEditable so scripts aren't stoped in the page.

To avoid those problems, the editor widget could use the clipboardData (sorry, I don't have time right now to check the proper names and urls, so I might be mixing things but I think that you'll understand what I mean) in the onPaste or onDrop events (maybe dataTransfer?). If the editor can read properly that data then it can apply whatever sanitation it needs on the data and the call "insertHTML" with something that has been checked. Of course, the html available from that clipboard should be the real HTML, not something already sanitized or changed in any way.

In order to help with that sanitation, IE8 introduced something like "toSafeHtml" and Firefox should create such a function using the whitelisting that it seems that it's currently being applied automatically. That way every updated script can use the latest security features available in the browser.

The popup blocker has always (IIRC) shown a button or any warning to the user explaining that something has been blocked and allowing with to whitelist that use. In this case it might be harder because most of the people won't be able to recognize if something is safe or not, but it could be at least a preference so it can be changed in about:config without changing browsers.

Also, a new execCommand can be introduced "safeClipboard" that toggles this automatic feature from strict to "previous behavior".

When the user drags and drop some element inside the editor, the last thing that he expects is that some sanitation is applied to that operation and the element loses some of its attributes or contents, so any dnd should verify if the source and destination is the same document and skip any processing of the data in that case. Also, copy and paste should behave in a similar way, maybe storing an extra clipboard flavor with the url and raw html of the source and checking that flavor before pasting it (that means don't apply this extra sanitation and also preserving the url of img, links... without turning them into absolute urls)
Comment 43 gamecockguy2003 2010-09-21 06:47:14 PDT
Has there been any consideration on this issue in regard to Document Type Definitions?  The whole point of DTD's is to define what attributes are allowed on what tags.  The idea that a browser would come in after that with its own list of safe attributes seems like the wrong solution.  It eliminates a lot of the extensibility goodness of a DTD.

Ideally, the DTD itself would be able to define if a given attribute in the context of a given tag is "safe" or "notsafe".
Comment 44 Jonathan BRAUN 2010-09-21 06:53:44 PDT
I've tried the latest build for 1.6.x version of firefox and attributes like data-* are still dropped by the execCommand 'inserthtml'...

Is this normal because some here said earlier it has been done...
Comment 45 gamecockguy2003 2010-09-21 08:06:57 PDT
(In reply to comment #44)
> I've tried the latest build for 1.6.x version of firefox and attributes like
> data-* are still dropped by the execCommand 'inserthtml'...
> 
> Is this normal because some here said earlier it has been done...

Can you confirm if they are working with drag/drop?
Comment 46 Jonathan BRAUN 2010-09-21 08:14:19 PDT
The simpleTestCase provided still doesn't work neither with friefox 3.6.11beta nor with firefox 4.0beta (nightly build from 20th september)
Comment 47 Boris Zbarsky [:bz] 2010-09-21 09:04:47 PDT
Can we _please_ have some reading comprehension?  Comment 36 is very clear about when data-* can be expected to work and neither of the builds mentioned in comment 46 is it.
Comment 48 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-21 09:14:21 PDT
Guys, please calm down!  Let's keep this bug focused on the issue it was filed for.

In case you have not looked through the dependency list, here's a summary of the matters so far:

* Bug 598090 updated our whitelist to the safe elements from the latest HTML5 draft spec.  It has landed on mozilla-central, which means that it's available starting from yesterday's nightlies, and we will take it on branches as well, which means that it will be available in the next release of Firefox 3.5 and 3.6.

* Bug 598105 added support for whitelisting HTML5 data-* attributes, which means that the custom attributes will be preserved if the author of the web page chooses to use the recommended way of storing them, which is data-* attributes.  This bug too has landed on mozilla-central, which means that it's available starting from yesterday's nightlies, and we will take it on branches as well, which means that it will be available in the next release of Firefox 3.5 and 3.6.

* With bug 598105, I think the need of web developers in preserving custom attributes will be satisfied without risking the security of the users of other browsers.  Right now I think that this could be the best middle-ground.

* The on* event attributes can allow code execution, and therefore will continue to be stripped off during paste/drop operations.  Therefore, that part of comment 0 is WONTFIX.

* I've also attached a patch to bug 597784 to not use the paranoid fragment sink, which means that the HTML code inserted using execCommand("inserthtml") _will not_ be subject to any white-list, as was the case before bug 520189.  The patch is currently waiting for review, and will be included in the next release of Firefox 3.5, 3.6 and 4.0.

* The only controversial issue at this point is whether we should preserve custom attributes or not.  I've explained our position towards this issue, but I'm still trying to think of ways that we can switch to accepting those attributes without risking the safety of users of other browsers.  If someone can think of such a solution, I'd be interested to hear their thoughts.
Comment 49 jonbell2 2010-09-21 09:25:44 PDT
"* The only controversial issue at this point is whether we should preserve
custom attributes or not.  I've explained our position towards this issue, but
I'm still trying to think of ways that we can switch to accepting those
attributes without risking the safety of users of other browsers.  If someone
can think of such a solution, I'd be interested to hear their thoughts."


I suppose we can't be much help with that solution because of the undisclosed bug 520189.  I'll take your word for it, but at this point I don't see how drag-and-drop interaction within Firefox poses a security threat to another browser.  As gamecockguy said, the only way I see that posing a threat is if the resulting HTML is later posted to the webserver, not sanitized, and later sent to another browser from the webserver.  In that situation, the threat is still there because you could post dangerous HTML regardless of the drag-and-drop feature, so I don't see how this adds security -- maybe a tiny bit of obfuscation?     However, if there's some other potential threat for *other  browsers* described in 520189, then good luck in finding a resolution internally, but we're working blind here and I don't suppose could be of much help.
Comment 50 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-21 16:04:21 PDT
*** Bug 594725 has been marked as a duplicate of this bug. ***
Comment 51 Jonathan BRAUN 2010-09-22 00:11:48 PDT
Attributes data-* work on this night's mozilla central's build of 4.0b7.

Maybe i didn't check on the right repository yesterday (checked on the 4.0b7 but not mozilla central's, apologies...)
Comment 52 Alfonso Martinez 2010-09-22 08:28:32 PDT
I think that any drag&drop or clipboard operation done within the same document shouldn't be affected by any filtering. There's no risk of injection as that code is already in the document and the user just wants to move it.

And my mention yesterday about toSafeHTML was meant to say toStaticHTML as mentioned in bug 443564. It would be good if the current sanitation method is exposed that way so any page can opt-in to use it.
Comment 53 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-22 16:39:07 PDT
I think bug 597784 should be marked to block branches instead of this bug...
Comment 54 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-22 16:40:42 PDT
(In reply to comment #53)
> I think bug 597784 should be marked to block branches instead of this bug...

Well, this proves that I'm blind!
Comment 55 Mike 2010-09-23 07:28:09 PDT
"* The only controversial issue at this point is whether we should preserve
custom attributes or not.  I've explained our position towards this issue, but
I'm still trying to think of ways that we can switch to accepting those
attributes without risking the safety of users of other browsers.  If someone
can think of such a solution, I'd be interested to hear their thoughts."

I would just like to say that there IS a work-around to this and its quite easy to do. I'm not going to post how to do it though, because I disagree with you on the issue... and frankly I need my editor to work. To me, the solution is to just not do a blanket strip of all custom tags. This breaks almost every editor on the market right now.
Comment 56 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-23 12:29:53 PDT
(In reply to comment #55)
> "* The only controversial issue at this point is whether we should preserve
> custom attributes or not.  I've explained our position towards this issue, but
> I'm still trying to think of ways that we can switch to accepting those
> attributes without risking the safety of users of other browsers.  If someone
> can think of such a solution, I'd be interested to hear their thoughts."
> 
> I would just like to say that there IS a work-around to this and its quite easy
> to do. I'm not going to post how to do it though, because I disagree with you
> on the issue... and frankly I need my editor to work. To me, the solution is to
> just not do a blanket strip of all custom tags. This breaks almost every editor
> on the market right now.

Well, if you have a workaround and you don't post it here because you disagree with me, the chances of the situation getting better here will be much lower.  If it's a good workaround, I'm really eager to hear about it.
Comment 57 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-09-23 21:02:34 PDT
It does seem to me that dragging HTML within the same editable region would be safe. I think we should do that.
Comment 58 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-23 21:33:28 PDT
(In reply to comment #57)
> It does seem to me that dragging HTML within the same editable region would be
> safe. I think we should do that.

Should I piggy back this bug for this in order to use the blocking flag or file another bug and nominate it for blocking?
Comment 59 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-09-23 21:35:34 PDT
Rereading comment #0, I think that's exactly what this bug is about.
Comment 60 klein 2010-09-24 09:24:16 PDT
I found this bug after hours of debugging - I can not believe that you did such a fundamental change whitout considering the effect for all WYSIWYG editors.
Our CMS depends on CKEDITOR or FCKEditor and on custom attributes.

This code could be inserted with InsertHtml before the security "fix":
<img width="220" height="120" src="http://myserver/jpg2/animal02.jpg" newobject="1" fileid="6918" objectid="5043" />

After this "fix" my code is reduced to this:
<img width="220" height="120" src="../jpg2/arena.jpg" />

So not only all custom attributes are gone, also my img src was converted from absolute to relative format.

Please rethink this "fix". I would love to hear about the workaround!
Comment 61 klein 2010-09-24 09:37:06 PDT
>* I've also attached a patch to bug 597784 to not use the paranoid fragment
>sink, which means that the HTML code inserted using execCommand("inserthtml")
>_will not_ be subject to any white-list, as was the case before bug 520189. 
>The patch is currently waiting for review, and will be included in the next
>release of Firefox 3.5, 3.6 and 4.0.

If I understood this comment correctly, this means that execCommand("inserthtml") will be fixed (reverted to the old functionality) with this patch?
Comment 62 Thomas Lehmann 2010-09-24 09:39:39 PDT
Hey fellow. Im a programmer who's using FCKEditor (2.6.2, we can't upgrade that
fast) too. I created an workaround for the InsertHtml() "issue". I'll mail it
to you. I just won't to post it here as I don't want to risk, that our Mozilla
colleagues get noticed about the "backdoor". :x

The workaround will not fix the drag&drop issue. That one I bypassed by
prohibiting drag/drop of threatened (placeholder) images, for now.

As far as I can see, the drag&drop issue will be rolled back within the next
versions soon. I hope so.
Comment 63 Spocke 2010-09-24 09:55:51 PDT
I suggest that the inserthtml command shouldn't be sanitized at all. And drop and paste should only be sanitized when it comes from an external source not within the same document. The sanitation should only include: event attributes like onclick, IE CSS expressions and javascript: urls in links and images everything else like custom attributes should remain intact.

But the contents should be sanitized on the server anyway to avoid XSS attacks by bots so I doesn't really matter much that the browser is secure here. Other browsers doesn't address this a.f.a.i.k so it seems a bit overkill to add this feature to Firefox.
Comment 64 Boris Zbarsky [:bz] 2010-09-24 10:11:22 PDT
I suggest that everyone READ THE BUG before commenting.  It was decided that insertHTML shouldn't sanitize three days ago, and it says so in comment 48.  Now can people please stop spamming the bug they haven't read?
Comment 65 Thomas Lehmann 2010-09-24 10:23:44 PDT
I fully agree that sanitation is a thing that should be done by the RTE-Framework or the user of such an framework, not by the browser.

I can live with it, as I have a workaround that replaces the InsertHtml command.

When it comes to ignoring/sanitizing non-standard attributes and element, I think this is something that should phased out in a major release after introducing data- attributes. So RTE-Framework programmers and their users have enough time to migrate. 

Making such an heavy change within a minor release is crazy.

Sanitizing on drag/drop is unnecessary when dragging elements within the same document because, ideally, they have been cleaned up before. Sanitizing content that has been dragged into the document from elsewhere might be OK and consistend with the InsertHtml change.

My personal attitude on the whole topic is completely different: the editor should just block the Javascript engine and plugins from executing interactive content when existing within an editable document. Than the browser *may* provide an (discussed above) passive method that cleans up content and it's up to the programmer to filter malicious code. But as I understood the above discussion it appears to that the Firefox developers want to rescue the world (wide web). :)

I think, blocking/removing interactive content within an editable document is a big regression in Web 2.0.
Comment 66 Mike 2010-09-24 11:16:57 PDT
I also solved the drag and drop problem. So if need be, theres a solution for that as well. I have only shown this to 1 other person. I believe they have decided however (if im reading correctly) to not strip out custom attributes if drag and drop occurs within the same document (Comment 57, shortly after my first post). This seems reasonable enough to me.
Comment 67 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-24 14:04:26 PDT
Created attachment 478395 [details] [diff] [review]
Fix

Here's the fix, still working on the test...
Comment 68 Boris Zbarsky [:bz] 2010-09-24 19:15:24 PDT
Comment on attachment 478395 [details] [diff] [review]
Fix

r=me
Comment 69 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-27 09:15:53 PDT
So, according to Neil, it's currently not possible to test drag and drop in the editor, because we can't raise systemgroup events from privileged Javascript code.

So, I'm going to land this without a test, and set in-litmus? and in-testsuite? flags on the bug.
Comment 70 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-27 10:48:12 PDT
http://hg.mozilla.org/mozilla-central/rev/e460eac1db7a
Comment 71 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-27 14:25:37 PDT
*** Bug 595152 has been marked as a duplicate of this bug. ***
Comment 73 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-27 15:16:45 PDT
Bustage fix:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/147a84a5ceea

(got the bug # wrong in the commit message...)
Comment 74 Mark Banner (:standard8) 2010-09-28 01:18:23 PDT
This got backed out from 1.9.2 along with bug 597784 as there were persisting test failures.
Comment 75 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-28 11:14:52 PDT
Relanded:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/23cfc084d4a9
Comment 76 amouchinski 2010-09-30 14:21:16 PDT
> The patch is currently waiting for review, and will be included in the next
release of Firefox 3.5, 3.6 and 4.0.

Ehsan, any idea when the next 3.6 release should be expected? Thanks.
Comment 77 amouchinski 2010-09-30 14:26:58 PDT
Found the answer ;)
https://wiki.mozilla.org/Releases
Comment 78 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-30 14:32:35 PDT
(In reply to comment #76)
> > The patch is currently waiting for review, and will be included in the next
> release of Firefox 3.5, 3.6 and 4.0.
> 
> Ehsan, any idea when the next 3.6 release should be expected? Thanks.

https://wiki.mozilla.org/Releases/Firefox_3.6.11
Comment 79 hoch 2010-10-13 04:25:01 PDT
I just tested this with 3.6.11 build2 and while the issue with Drag and Drop appeares to be solved, the error is still reproducible by copy pasting an element (img in my case) inside a contendEditable div.
Comment 80 :Ehsan Akhgari (busy, don't ask for review please) 2010-10-13 07:43:55 PDT
Please see comment 48 and 57.
Comment 81 Spocke 2010-10-13 08:34:11 PDT
If it removes custom attributes from paste as it currently does in the nightly it will break existing products like TinyMCE. We can release a patch for TinyMCE to solve this using data- attributes. However a huge amount of existing systems like Wordpress, Drupal and Joomla just to name a few will break since they will then be using an older version of TinyMCE.

Since we use custom attributes like _mce_<something> could you at least add the possibility to retain prefixed attributes like this so it handles browser vendor specific attributes like _moz_<something> or _webkit_<something> as well and in this case our custom ones?
Comment 82 :Ehsan Akhgari (busy, don't ask for review please) 2010-10-13 09:09:22 PDT
(In reply to comment #81)
> If it removes custom attributes from paste as it currently does in the nightly
> it will break existing products like TinyMCE. We can release a patch for
> TinyMCE to solve this using data- attributes. However a huge amount of existing
> systems like Wordpress, Drupal and Joomla just to name a few will break since
> they will then be using an older version of TinyMCE.
> 
> Since we use custom attributes like _mce_<something> could you at least add the
> possibility to retain prefixed attributes like this so it handles browser
> vendor specific attributes like _moz_<something> or _webkit_<something> as well
> and in this case our custom ones?

Would you mind filing another bug for this, so that we can discuss what a good solution would look like there?  Thanks!
Comment 83 Spocke 2010-10-14 04:05:25 PDT
I added a new bug report and added it as a dependency to this one since it's related: https://bugzilla.mozilla.org/show_bug.cgi?id=604332
Comment 84 :Ehsan Akhgari (busy, don't ask for review please) 2010-10-20 11:48:12 PDT
*** Bug 605722 has been marked as a duplicate of this bug. ***

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