578732 - FireTorrent probably fails to sanitize torrent metadata

Status: RESOLVED FIXED

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

Description

[Wladimir Palant]

FireTorrent shows info about torrents being downloaded in about:torrent?url=... pages. Given that it returns the chrome channel of chrome://firetorrent/content/aboutTorrent.xhtml for these pages they have chrome privileges. The file responsible for updating the pages is components/aboutTorrentProgressListener.js. Looking at the source code, there is much innerHTML usage where textContent would do - with the latter being inherently safer. A particularly concerning code part is this one:

>      while (trackersEnumerator.hasMoreElements()) {
>        var tracker = trackersEnumerator.getNext().QueryInterface(Ci.nsISupportsString);
>
>        trackersHtml += '<tr><td>' + tracker + '</td></tr>';
>      }
>
>      trackersTable.innerHTML = trackersHtml;

trackersEnumerator is implemented in the binary component so I cannot check its implementation. But I strongly doubt that it does any kind of sanitizing. So if the torrent has "http://fakeTracker<img src='dummy' onerror='alert(Components.classes)'>" as one of its trackers it will most likely be able to run JavaScript code in chrome context.

The solution would be to replace innerHTML by textContent where possible and escape HTML entities on all the other occasions.

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

Adding add-on owners.

Comment 2

[Jorge Villalobos [:jorgev] (he/him)]

A new version was submitted today with the same insecure code. I rejected it and pointed the authors to this bug. The addresses I CC'd are probably unused.
Moving to 5.11.9

Comment 3

[Matthew Gertner]

I was traveling and behind on my bugmail. I'll make the change you suggest and resubmit tomorrow.

Comment 4

[Matthew Gertner]

Attachment: Use textContent and escape HTML where necessary

Great find Wladimir! Thanks for taking the time to analyze this. This patch uses textContent where feasible and escapes the HTML otherwise. I've been conservative and escaped all strings that get injected into HTML.

This patch also contains the original fix to FireTorrent 2.0.2, which resolves a crash that occurs on Windows since the out-of-band plugin stuff landed. There's a bit of related makefile stuff in the patch relating to my upgrading to a newer version of Boost.

Finally, I removed the UA string stuff.

I'm not sure what the procedure is for a bug like this. Do I need to get my patch reviewed? Should I use the same version number or a new one for the corrected build?

To accelerate the process I've gone ahead and posted a new version of FireTorrent (2.0.3) with this patch integrated. I'm concerned that our users who have upgraded to Firefox 3.6.6+ are experiencing frequent crashes on Windows, so I want to get this live ASAP. If there's anything I need to do differently to conform with the established procedures, please let me know.

Comment 5

[Matthew Gertner]

Attachment: Same but with version number bumped to 2.0.3

Comment 6

[Wladimir Palant]

Comment on attachment 467337 [details] [diff] [review]
Same but with version number bumped to 2.0.3

I'm not sure what the process is either but I'll simply review this. It is a little hard without having the IDL for mozITorrentPeer. Here is my analysis:

* Almost all instances of innerHTML have been replaced by textContent - good.
* The remaining instances assign the variables filesHTML, trackersHTML and peersHTML to innerHTML. These are all generated dynamically.
* Some of the data inserted into the variables listed above is not escaped:
 - fileNum: numerical (safe)
 - fileSize, totalDone, uSpeed, dSpeed, dTotal, uTotal: numerical result of DownloadUtils.convertByteUnits() (safe)
 - fileSizeUnits, totalDoneUnits, uSpeedUnits, dSpeedUnits, dTotalUnits, uTotalUnits: string result of DownloadUtils.convertByteUnits() coming from chrome://mozapps/locale/downloads/downloads.properties (part of Firefox install, can be considered safe)
 - peer.ip: a string I guess. I guess that this is converted from a numerical IP presentation which would make it safe. If it isn't (e.g. if it could be a DNS-resolved name) this should be escaped.
 - peer.uploadLimit, peer.downloadLimit: a string? Not sure whether these are locally set limits or data coming from the peer as well, probably the latter. I guess that it is received from the peer in numerical form which would make it safe.
 - peer.lastRequestTime, peer.lastActiveTime: I guess that this is set locally which would make it safe.
 - peer.requestTimeout: I guess that this is a numerical value?

Matthew, any comments on my guesses above?

Comment 7

[Matthew Gertner]

(In reply to comment #6)
> Matthew, any comments on my guesses above?

Thanks for the review. Basically yeah, all the fields you mention are numerical. The only exception is peer.id, which is a string. Here's the getter code:

NS_IMETHODIMP
mozTorrentPeer::GetIp(nsAString &aIp)
{
  std::string ip = mPeerInfo.ip.address().to_string();
  aIp = NS_ConvertASCIItoUTF16((const char*)ip.c_str());

  return NS_OK;
}

So I'm going to assume we're good to go.

Comment 8

[Wladimir Palant]

Is that Boost? If so - yes, boost::asio::ip::address::to_string() will simply convert a numerical IP address into a string, no DNS resolution or anything else where an attacker could inject his data. Good enough for me. I suggest you submit the new version for review and let Jorge handle it.

Comment 9

[Brian King [:kinger]]

I've reviewed and approved new versions (2.0.3 Mac and Windows) on AMO.

Comment 10

[Jorge Villalobos [:jorgev] (he/him)]

I've double-checked the add-on, and it looks good.
Resolving this as fixed.

Comment 11

[Wladimir Palant]

I think that this should be public by now.