476851 - FireFTP extension allows XSS into chrome through manipulated server responses

Status: RESOLVED FIXED

(addons.mozilla.org :: Security, defect)

Description

[Wladimir Palant]

I didn't verify this but from the code the picture seems to be the following:

* FireFTP maintains a message log in the global variable gLogQueue
* Occasionally, HTML code is added to this variable through appendLog() function (content/js/etc/log.js). No escaping is performed, in particular the message is unescaped
* One of the callers is readControl() method in content/js/connection/controlSocket.js that will pass some of the FTP server's error messages directly to appendLog()
* Once per second the function UIUpdate() in content/js/etc/UIUpdate.js will add gLogQueue to gCmdlogBody.innerHTML. gCmdlogBody is the body of a frame in fireftp.xul that loaded blank.html (a chrome document).

Consequently, if the server error message contains something like |<img src=d onerror="alert(Components.classes)">|, this HTML code will be inserted into a chrome document and the script will run with chrome privileges - access to Components.classes and full control over the browser.

For reference: https://developer.mozilla.org/En/Displaying_web_content_in_an_extension_without_security_issues

Comment 1

[Mime Čuvalo]

Yikes.  Thanks for the heads up, Wladmir.  I will work on a fix this weekend.

Comment 2

[Mime Čuvalo]

So, I went through the document that Wladmir provided.

 - tried setting type="content" on the <browser> element.  Then read that this is ignored - doh!
 - tried adding the <browser> element dynamically with the type attribute but that didn't seem to work at all
 - using a data: uri would not work in my case
 - tried setting frame.docShell.allowJavascript = false (along with the associated ones) onLoad but I still received the alert(Components.classes) security issue.  Not sure what's up with that.
 - also tried nsIScriptableUnescapeHTML but that scrubbed out too much information.


In the end, what seems to "just work" is doing the following on server returned responses:

msg.replace(/</g, '&lt;').replace(/>/g, '&gt;');

Would that suffice?  I'm guessing that's a naive way to fix it - but I'm struggling to see how this should properly be fixed otherwise.  It seems there should just be a simple attribute you can set on the <browser> element to disable access - that would be easiest at least from my perspective.

Comment 3

[Mime Čuvalo]

ping?

adding Wladimir to the cc list as it doesn't seem he's on this thread...

Comment 4

[Wladimir Palant]

I get mails automatically because I reported that bug.

So your entire extension UI is in the content browser already and you cannot use most protection mechanisms. That's too bad...

There is at least one thing that you can do - escape HTML code in log messages. Use a function like this one:

function escapeHTML(str)
{
  return str.replace(/&/g, "&").replace(/"/g, """)
            .replace(/</g, "&lt;").replace(/>/g, "&gt;");
}

And apply it in all cases where you append data to the log that is not part of your extension, e.g.:

this.observer.onAppendLog("&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;" + escapeHTML(outputData), 'output', "info");

That's lots of changes and you have to be careful that you don't miss one. Which is why having a fail-safe would make lots of sense - but from what I can tell, disabling JavaScript and plugins in the docshell is the only thing you can do with your setup. I tested and something like this should work:

  <iframe id="cmdlog"/>

  [...]

  var frame = document.getElementById("cmdlog");
  frame.docShell.allowJavaScript = false;
  frame.docShell.allowPlugins = false;
  frame.setAttribute("src", "blank.html");

Any JavaScript in blank.html won't execute. Note that blank.html is loaded *after* JavaScript is disabled.

(In reply to comment #2)
>  - also tried nsIScriptableUnescapeHTML but that scrubbed out too much
> information.

That's because you use inline styles and non-standard attributes. How about replacing this:

"<div type='" + type + "' style='display:" + (type != "error" && gLogErrorMode ? "none" : "block") + "' " + "class='" + css + "'>"

By:

"<div class='" + css + " type_" + type + "'>"

That will define the type as an additional class. If you also set gLogErrorMode as onlyErrors attribute on the <html> element in blank.html you can add a static CSS rule like:

[onlyErrors="true"]:root div:not(.type_error)
{
  display: none;
}

Comment 5

[Mime Čuvalo]

Attachment: patch

Attached is my proposed fix.

I can't disable javascript as the document does need to use javascript from time.  This patch uses nsIScriptableUnescapeHTML technique but only applies it to untrusted content.

You may look at the entire patch but basically it does this:

  if (untrusted) {
    var dummyElement = document.createElement('DIV');
    var domNode = gUnescapeHTMLService.parseFragment(message, false, null, dummyElement);
    var s = new XMLSerializer();
    message = s.serializeToString(domNode);
  }

Does that seem like an acceptable solution?  Is there anything I'm missing?

By the way, Wladimir, you're awesome, you've been very helpful in all of this.

Comment 6

[Wladimir Palant]

For untrusted content, using escapeHTML() function from above would have been sufficient - I don't think you expect HTML code there. The issue is still that you don't have a generic fail-safe mechanism, if you set a wrong boolean flag somewhere you are in trouble again (yes, humans make mistakes from time to time).

Why do you need JavaScript *inside* the document? From what I can tell, you only use it for the body event handlers in blank.html and for the welcome message in loadUnload.js (click event handler). All those event handlers can be moved to the <iframe> element, then they will run independently of whether JavaScript is allowed in blank.html (for the load message you will have to check event.target of the click but that shouldn't be a big deal either).

<iframe type="content" src="blank.html" onmousemove="this.contentDocument.getElementById('mouseY').textContent = event.screenY;"/>

Comment 7

[Rey Bango]

Mime, what is the status of this?

Comment 8

[Mime Čuvalo]

(In reply to comment #6)
> For untrusted content, using escapeHTML() function from above would have been
> sufficient - I don't think you expect HTML code there.

Ok, my new patch will be to just do:
message = message.replace(/</g, "&lt;").replace(/>/g, "&gt;").replace(/"/g, "&quot;");
for untrusted messages

> Why do you need JavaScript *inside* the document? From what I can tell, you
> only use it for the body event handlers in blank.html and for the welcome
> message in loadUnload.js (click event handler). All those event handlers can be
> moved to the <iframe> element, then they will run independently of whether
> JavaScript is allowed in blank.html (for the load message you will have to
> check event.target of the click but that shouldn't be a big deal either).
> 
> <iframe type="content" src="blank.html"
> onmousemove="this.contentDocument.getElementById('mouseY').textContent =
> event.screenY;"/>

The javascript is needed inside the document to track the mouse when highlighting the tree.  I tried your alternative method of putting the handlers on the iframe but it didn't work.  The event simply does not fire enough to give the mechanism that it updates sufficient data.


Rey, I'm going to release an updated version tonight if that's alright with you then.  Let me know if there is some procedure you want me to follow or to hold off.

Comment 9

[Mime Čuvalo]

(In reply to comment #6)
> The issue is still that
> you don't have a generic fail-safe mechanism, if you set a wrong boolean flag
> somewhere you are in trouble again (yes, humans make mistakes from time to
> time).

Also, just to ease your fears.  The only points of entry that the server has as far as appending the log is in ftpController.js and fxp.js (as seen in the patch).

Comment 10

[Mime Čuvalo]

version 1.0.4 has been released to address the issue.  Let me know if there is anything else needed to be done.  Thanks again for the help!

Comment 11

[Wil Clouser [:clouserw]]

marking fixed based on comment 10

Comment 12

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

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

Comment 13

[Wladimir Palant]

I think that this should be public by now.