Last Comment Bug 595176 - Midas' inserthtml doesn't seem to work correctly
: Midas' inserthtml doesn't seem to work correctly
Status: RESOLVED DUPLICATE of bug 597784
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: unspecified
: x86 Windows XP
: -- major with 4 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
http://www.miichan.net/main.php
: 595182 595338 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-10 07:29 PDT by omoikane
Modified: 2010-09-21 09:18 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description omoikane 2010-09-10 07:29:24 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ko; rv:1.9.2.9) Gecko/20100824 Firefox/3.6.9 ( .NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ko; rv:1.9.2.9) Gecko/20100824 Firefox/3.6.9 ( .NET CLR 3.5.30729)

When I executed execCommand("inserthtml", false, "<img src = 'foobar.png' originalindex = '160' />"); on Midas' iframe, it inserts only <img src = 'foobar.png' /> and ignores originalindex property. 

Is it intended or sort of bug? 

Reproducible: Always

Steps to Reproduce:
1. create iframe and make it editable
2. run myiframe.document.execCommand("inserthtml", false, "<img src = 'foobar.png' originalindex = '160' />"); on Javascript

Actual Results:  
only inserted <img src = 'foobar.png' />

Expected Results:  
<img src = 'foobar.png' originalindex = '160' />
Comment 1 Matthias Versen [:Matti] 2010-09-10 10:37:20 PDT
*** Bug 595182 has been marked as a duplicate of this bug. ***
Comment 2 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-10 13:44:13 PDT
This is by design.  We match the attribute names against a white-list, and only accept the safe ones, and discard the rest.

*** This bug has been marked as a duplicate of bug 594725 ***
Comment 3 Matthias Versen [:Matti] 2010-09-10 14:35:44 PDT
*** Bug 595338 has been marked as a duplicate of this bug. ***
Comment 4 Martin 2010-09-10 15:21:53 PDT
So what are you saying? Is this the way it's supposed to work? Blocking images from being inserted?
Comment 5 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-10 15:36:12 PDT
(In reply to comment #4)
> So what are you saying? Is this the way it's supposed to work? Blocking images
> from being inserted?

originalindex is not a valid attribute in HTML, therefore you should not rely on it to work.  We can't accept random attributes in the pasted contents.
Comment 6 hans 2010-09-10 15:47:43 PDT
It would be nice if the white list was expanded to include new HTML5 attributes (e.g. 'contenteditable', and even 'data-*'). Unlike event handler attributes they are pretty harmless.
Comment 7 Martin 2010-09-10 16:01:13 PDT
Okay but what attribute is not valid in this string?

'<iframe class="youtube-player" type="text/html" width="430" height="259" src="http://www.youtube.com/embed/dHC2VPjHbWI" frameborder="0"></iframe>'

Not working either.
Comment 8 Franck Stauffer 2010-09-11 01:07:41 PDT
I have to say that I am a bit puzzled by such a move...we ended up asking all our customers to keep on using 3.6.8 because of this. And in the current version of our soft we ended polluting white listed attributes with our data. Adopting this kind of workaround is not a very standard compliant solution to handle our attributes.
Comment 9 Martin 2010-09-11 03:08:09 PDT
Forget about the attributes in my example, it's not even working to insert "<iframe></iframe" without any attributes.

We're also recommending our users to not upgrade.
Comment 10 hans 2010-09-11 16:35:31 PDT
(In reply to comment #9)
> Forget about the attributes in my example, it's not even working to insert
> "<iframe></iframe" without any attributes.

This was explained here:
<https://bugzilla.mozilla.org/show_bug.cgi?id=594725#c3>
The iframe tag is not white listed.
Comment 11 hans 2010-09-11 16:36:37 PDT
To prevent XSS attacks I can understand you need to check tags and attributes. Obviously tags like script, object, iframe and attributes like onmouseover and onwhatever pose a threat. I suppose a black list would be effective to solve this problem. If you use a white list, it should be well maintained. The current list is very arbitrary. Note that document.execCommand implies HTML5 (because of the required designmode and contenteditable attributes), so it makes very little sense to restrict the white list to HTML4 tags and attributes.

Some arbitrary choices in the current white list:
<http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentSink.cpp#1729>

Most new HTML tags are not allowed, but some are (e.g. audio).
Most new attributes are not allowed, but some are (e.g. autocomplete).
Some propietary attributes are allowed (e.g. pointsize).
Lots of obsolete tags and attributes are allowed (e.g. font)

For a list of current HTML5 elements and attributes:
<http://www.whatwg.org/specs/web-apps/current-work/multipage/section-index.html>
Comment 12 Franck Stauffer 2010-09-13 02:14:25 PDT
I couldn't agree more with the previous comment. Indeed security is an issue, but it should not be tackled without taking care of standards and without thinking about the possible side-effects for people developing applications based on Midas. 

At the very least there should be a way for the developer to extended the allow attributes/tags list : it could then be up to the third party application to make sure it prevents XSS and other security flaws. 

Right now, the result for our users is that most of them will have to stick with 3.6.8 as it is quite complicated and costly to fix all the versions we've deployed over the last two years. In the mean time we have to ask them to use 3.6.8 only for our application and invite them to have a second browser for other operations (browsing, webmail, ...) as they won't be able to get all the security fixes they should get for safe browsing...
Comment 13 omoikane 2010-09-13 02:42:39 PDT
Additionally XSS attacks are not *well* defended by these measures because user(attacker) cound send POST Request via firebug(firefox extension for developers), other browsers and homebrew/modified web browser (simply change src code of firefox and recompile for attack should be fine), using old version of browser

These attacks should be defended by server-side (php, jsp, python, etc.) by parsing html content and filter potentially harmful tags and attributes. (PHP 5 provides a nice DOM class very similar to Javascript's DOM, best for detecting harmful tags and attributes (I use whitelist for tags and blacklist for attributes  )

I've routed current measures by changing contents manually by using Javascript's Range and Selection objects (and anyone should rout them by this) so I think it's just confusing users and web developers, so on.
Comment 14 hans 2010-09-13 03:22:17 PDT
(In reply to comment #13)
> Additionally XSS attacks are not *well* defended by these measures because
> user(attacker) cound send POST Request via firebug(firefox extension for
> developers), other browsers and homebrew/modified web browser (simply change
> src code of firefox and recompile for attack should be fine), using old version
> of browser

If the attacker is the user, it isn't an XSS attack. Of course an attacker can send any HTTP request he wants, that is not a browser issue. The point is that the attacker may entice a user to paste something malicious in the browser.

I'm not authorized to access the relevant XSS bug that issued the change in 3.6.9 <https://bugzilla.mozilla.org/show_bug.cgi?id=520189> but I can imagine an attacker may exploit the (currently white listed) src attribute as well. E.g. <img src="/index.cgi?logout=1" /> or something more serious.
Disallowing to copy/paste images very much limits the use of Midas. Patching this seems more complicated than simply white listing attributes and tags.
Comment 15 Matthias Versen [:Matti] 2010-09-21 09:10:12 PDT
*** Bug 598287 has been marked as a duplicate of this bug. ***
Comment 16 :Ehsan Akhgari (busy, don't ask for review please) 2010-09-21 09:18:44 PDT
In fact, we're going to change this behavior in bug 597784, so that inserthtml
doesn't strip anything from its input, the same way that setting innerHTML does
not.  Please follow that bug for the progress of this issue.

*** This bug has been marked as a duplicate of bug 597784 ***

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