595176 - Midas' inserthtml doesn't seem to work correctly

Status: RESOLVED DUPLICATE of bug 597784

(Core :: DOM: Editor, defect)

Description

[omoikane]

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 2

[(no longer active)]

This is by design.  We match the attribute names against a white-list, and only accept the safe ones, and discard the rest.

Comment 4

[Martin]

So what are you saying? Is this the way it's supposed to work? Blocking images from being inserted?

Comment 5

[(no longer active)]

(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]

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]

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]

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]

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]

(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]

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]

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]

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]

(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 16

[(no longer active)]

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.