727216 - Security code review for Instant messaging in Thunderbird

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Keeping the security notes in here, since some of them may be sensitive to InstantBird. 

PT Notes:

1. Untrusted text appears to be parsed by the HTML parser prior to sanitization. Although the DOM is cleaned prior to being displayed on the page, there may be a risk that script execution could happen during parsing (I think).
In http://lxr.instantbird.org/instantbird/source/chat/modules/imContentSink.jsm the cleanupImMarkup function (line 369) takes untrusted text (aText), and then adds this to an innerhtml attribute.
It then walks the created element remove DOM nodes which are not allowed. 

This might be a problem, since assigning to .innerHTML engage the browser parser. <script> tags I dont think will get executed (not sure...) but img tags will get evaluated, and could result in inducing a client request, or script excection.

2. - Only some attribute and style values are whitelisted (i.e. some are not restricted). The attack surface is small, but this may lead to issues. The developer has updated code to sanitize these, and this updated code is currently under review.

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Further notes from dchan:


1. Typing chrome:// as a message in instantbird results in the error
Invalid chrome URI: /
* This may be a result of the auto-linkification that instantbird attempts with mozTXTToHTMLConv [2][3]

2. You can cause an exception in instantbird by sending the following from Adium with raw HTML. The client doesn't crash
%_html{<a href="abcd">foo</a>}
* Double-clicking on this link in instantbird gives (it won't be autolinked)
Error: uncaught exception: [Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService2.newURI]"  nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)"  location: "JS frame :: chrome://instantbird/content/convbrowser.xml :: onxblclick :: line 844"  data: no]

3. The code in mozTXTToHTMLConv parses strings "unsafely" and has other bugs...
* There isn't much bounds checking
1259   for (PRInt32 i = 0; i < lengthOfInString;)
1260   {
1261     if (aInString[i] == '<')  // html tag
1262     {
1263       PRUint32 start = PRUint32(i);
1264       if (nsCRT::ToLower((char)aInString[PRUint32(i) + 1]) == 'a')
1265            // if a tag, skip until </a>
1266       {
1267         i = aInString.Find("</a>", true, i);
1268         if (i == kNotFound)
1269           i = lengthOfInString;
1270         else
1271           i += 4;
1272       }
1273       else if (aInString[PRUint32(i) + 1] == '!' && aInString[PRUint32(i) + 2] == '-' &&
1274         aInString[PRUint32(i) + 3] == '-')

* L1273 reads outside the string if i + 3 > lengthOfInString... though maybe some nsString magic happens...

* L1263 - L167 are wrong
* If the input is "<afoo other stuff </a>", the whole string will be consumed since the character after '<' is 'a' and there is a "</a>"

* Sending a message with the contents
http://www.google.com>
results in the following equivalent HTML
<a href="http://www.google.com>/">http://www.google.com&gt;</a>;

* Note that > was pulled into the URL and a semicolon appears out of nowhere


I haven't filed any bugs for these issues, but did find this really old bug.
https://bugzilla.mozilla.org/show_bug.cgi?id=19313

We should see if there is some other newer txt-to-html interface they could use.

Regards,
David

[1] - http://www.adiumxtras.com/index.php?a=xtras&xtra_id=1260
[2] - http://lxr.instantbird.org/instantbird/source/chat/content/convbrowser.xml#381
[3] - http://mxr.mozilla.org/mozilla-central/source/netwerk/streamconv/converters/mozTXTToHTMLConv.cpp

Comment 2

[Daniel Veditz [:dveditz]]

multiple issues in a single bug rarely all get fixed correctly. Let's keep this as as meta bug and clone off "depends on" bugs for each actual security issue.

Comment 3

[Florian Quèze [:florian]]

(In reply to Paul Theriault [:pauljt] from comment #0)

> 2. - Only some attribute and style values are whitelisted (i.e. some are not
> restricted). The attack surface is small, but this may lead to issues. The
> developer has updated code to sanitize these, and this updated code is
> currently under review.

In the most permissive mode, the attributes which aren't filtered based on a whitelist are: "face", "color", and "size" on the "font" tag. The CSS attributes which are allowed with unfiltered values are: 'color', 'font', 'font-family', 'font-size', 'font-style', 'font-weight' and 'text-decoration'.
Do you see a problem with any of these?

Comment 4

[Paul Theriault [:pauljt] (no longer reading bugmail)]

> In the most permissive mode, the attributes which aren't filtered based on a
> whitelist are: "face", "color", and "size" on the "font" tag. The CSS
> attributes which are allowed with unfiltered values are: 'color', 'font',
> 'font-family', 'font-size', 'font-style', 'font-weight' and
> 'text-decoration'.
> Do you see a problem with any of these?

No I don't think so - I was worried about url() or something obscure that lead to a network request, but I can't think of any situation.

Also, from Boris' comment on bug 728093, it sounds like DomParser is a safe approach (once that is available), so I think that just leaves David's comments, which I think boils down to "We should see if there is some other newer txt-to-html interface they could use."

David - did you have anything further to add on this? Were these issues in new code, or bugs in existing code?

Comment 5

[Florian Quèze [:florian]]

(In reply to Paul Theriault [:pauljt] from comment #1)

> 2. You can cause an exception in instantbird by sending the following from
> Adium with raw HTML. The client doesn't crash
> %_html{<a href="abcd">foo</a>}

You can send raw HTML to an Instantbird conversation using the /raw command, so you don't need Adium and a plugin to test :). For example:
/raw a<b>b</b>c

> * Double-clicking on this link in instantbird gives (it won't be autolinked)
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService2.newURI]"  nsresult:
> "0x804b000a (NS_ERROR_MALFORMED_URI)"  location: "JS frame ::
> chrome://instantbird/content/convbrowser.xml :: onxblclick :: line 844" 
> data: no]

This is a trivial coding error that I've fixed (https://hg.instantbird.org/instantbird/rev/46f3a0c1a1ea). The code was just for some reason expecting a tags without href attribute to have an undefined value for the href property, but the value is an empty string.

Comment 6

[Florian Quèze [:florian]]

(In reply to Paul Theriault [:pauljt] from comment #4)

> Also, from Boris' comment on bug 728093, it sounds like DomParser is a safe
> approach (once that is available)

I emailed Henri (who fixed bug 102699) to have a confirmation that DOMParser isn't supposed to start requests for images.

> David's
> comments, which I think boils down to "We should see if there is some other
> newer txt-to-html interface they could use."

mozTXTToHTMLConv (whose usage should be avoided if I understood comment 1 correctly) has currently 2 different use cases:

- convert plain text strings to HTML, ie. escape HTML special characters. Is there a better alternative for this?

- detect links in messages, and create <a> tags for them. I'm aware of several cases where mozTXTToHTMLConv worked poorly for that, so we have discussed several times reimplementing the whole thing in JS. An idea (but nobody has started implementing it as it's a lot of work) was to follow the reference implementation published by Twitter (it is in Ruby, and mostly uses regexps: https://github.com/twitter/twitter-text-rb/blob/master/lib/regex.rb#L227 https://github.com/twitter/twitter-text-rb/blob/master/lib/extractor.rb#L152).

Comment 7

[Florian Quèze [:florian]]

We are trying to have IM in Thunderbird 13, which means we are doing our best to land the code in comm-central Monday so that it's picked up in the aurora merge of March 13.

None of the protocol plugins we currently have in the patch send HTML as received into the conversation (twitter doesn't supports formatting; our XMPP plugin doesn't handle formatted messages yet; and formatted IRC messages are parsed by our JS code (the formatting characters are special non printable characters that our own code converts to HTML tags; the HTML special characters in the rest of the message is escaped).

So I don't think the HTML sanitization is a real security concern for the patch as we have it now. The reason I was concerned about it is that we intended to release at the same time as Thunderbird with IM an add-on providing libpurple protocol plugins, and I don't trust that code to be doing sane escaping/sanitization. However, it seems we aren't going to release that add-on at this time (the work on packaging libpurple in an add-on hasn't started).


Then, the remaining issue here seems to be the usage of mozTXTToHTMLConv. As I pointed out in comment 6, we use it for 2 completely different things. Would you feel better if we used a regexp instead of this component for the first use case (escaping HTML special characters)?

Comment 8

[David Chan [:dchan]]

(In reply to Florian Quèze from comment #7)
> Then, the remaining issue here seems to be the usage of mozTXTToHTMLConv. As
> I pointed out in comment 6, we use it for 2 completely different things.
> Would you feel better if we used a regexp instead of this component for the
> first use case (escaping HTML special characters)?

I'm not aware of a converter that can be used for both cases. Paranoid fragment sink has been replaced by the HTML parser in bug 482909. This can be used to convert a string into text/html [1]. The interface will only parse and not execute any of the code in the fragments.

I asked Dan about linkfication and he suggested looking at Chatzilla. Chatzilla performs some regex linkification of chat [2]. However you would have to iterate the content again and possibly linkify the contents of Node.textContent. [3] Though there are some quirks with Node.textContent e.g.
<div>foo<span>bar</span></div>
The textContent would be foobar with no space.

That may result in the wrong thing being linkified.


I agree that for now the risk of using mozTXTToHTMLConv isn't that high for the initial feature release mentioned in comment 7. I don't think a regex would change my concerns much.

Dan, Paul: Any concerns about shipping as is and then migrating to DOMParser at a later time?


[1] - https://developer.mozilla.org/en/nsIDOMParser
[2] - http://hg.mozilla.org/chatzilla/file/cc9596e8780c/xul/content/mungers.js#l150
[3] - https://developer.mozilla.org/En/DOM/Node.textContent

Comment 9

[Paul Theriault [:pauljt] (no longer reading bugmail)]

No, given the mitigations above it would seem ok to me. I guess we just need to make sure this is tracked for when HTML support is added.