438641 - XSS vulnerability in MDC article history view

Status: RESOLVED FIXED

(developer.mozilla.org Graveyard :: General, defect)

Description

[Brandon Sterne (:bsterne)]

The title parameter is vulnerable to script/HTML injection.  Recommend using htmlentities() to encode output.

Also, it appears there is some regex-based filtering currently employed, but it can be defeated:

The input string "<b>foo</b>" is filtered and output as "B>", but the input string "<b>foo<//b>" is filtered and output as "<b>foo</b>".

Regex filters for XSS mitigation are very hard to get right and it is much simpler to use either character whitelists or htmlentities.

Comment 1

[Eric Shepherd [:sheppy]]

In what situation is the regex filtering thing happening?  I'm working with MindTouch to get these issues addressed and they need clarification.

Comment 2

[Brandon Sterne (:bsterne)]

I can't confirm that regexes are being used because I haven't looked at the source code for this page.  I was assuming they were, though, because of the example filtering I mentioned above.  By adding an extra forward-slash to the closing tag, the filter only strips the extra slash.

All the vendor needs to know is that an encoding step needs to be added to whatever the current filtering routine is.  htmlentities() is a standard way of doing this.  This will protect us from script/HTML injection, even when some characters pass through the other filtering steps.

If you can point me to the source code for this page, I can provide a better analysis of the root cause of this bug.

Comment 3

[Eric Shepherd [:sheppy]]

A response from Roy at MindTouch:

Your tester will need to clarify where the behavior is changing - FCKeditor itself will not allow for the double slashed closing tag to be rendered - you can test this behavior by creating a new page, switching to HTML source mode, and try adding the double slash - it gets converted into an HTML comment, and the proper HTML tag is closed off at the end of the document. 

The XHTML parser is written in C# - this is not a simple case of adding an htmlentities() encoding to the document, since it is not PHP which is doing any content parsing. The tester will need to clarify whether they are doing these types of changes directly in the editor (in which case you'd have to dive into FCKeditor and Firefox), or if they're saving and somehow getting this output (in which case they'd have to dive into the C# code). A specific repro case would be immensely helpful. 

Comment 4

[Brandon Sterne (:bsterne)]

(In reply to comment #3)
> A response from Roy at MindTouch:
> 
> Your tester will need to clarify where the behavior is changing

My comment regarding regexes being used for filtering was speculation and was perhaps a red herring...

Roy, I see that you are copied now on this bug.  The sample URL I posted when I filed the bug is a clear XSS and should hopefully help you identify the root cause.

Comment 5

[Eric Shepherd [:sheppy]]

This turns out to be a skin problem.  There's an updated skin that needs to be installed to fix this; the URL for the update is:

http://mozilla.focalcurve.com/mdc/Mozilla-Dekiskin-r5.2.zip

The only changed file is /skins/mozilla/Mozilla.php

Jeremy, if you can install that, that should resolve this one.

Comment 6

[Yvan Boily [:ygjb][:yvan]]

Adding keywords to bugs for metrics, no action required.  Sorry about bugmail spam.

Comment 7

[Will Kahn-Greene [:willkg] ET needinfo? me]

For bugs that are resolved, we remove the security flag. These haven't had their flag removed, so I'm removing it now.