DoS of TBB when no Content-Type header and more than 512 bytes of content are sent

Following a user question in #tor where the user couldn't open the URL http://cdimage.debian.org/debian-cd/7.1.0/i386/iso-dvd/MD5SUMS in TBB, I decided to investigate the problem by simulating a webserver with netcat. (The file loads fine in non-TBB Firefox; the problem exists in both TBB beta and alpha, presumably also in stable.) Here are my findings:

• The above resource is delivered without a Content-Type header by cdimage.debian.org.

• Upon retrieving the resource, Firefox displays a blank page and starts consuming 100% CPU (only one core on SMP systems) periodically, backing down for a few seconds every now and then.

• When adding a Content-Type header to the server response, Firefox shows the file in the browser (text/plain) or displays the content type warning dialog (other content type), as expected.

• One can remove all headers (not including of course "HTTP/1.0 200 OK") and the problem will still occur.

• The problem stops occurring once 512 bytes or less of content (without headers and \n\n) are sent. The content will then be displayed as a text file in Firefox.

• There is no significant change on the wire between the two cases -- the reply consists of two TCP packets broken up at the same point.

In a nutshell, service can be denied by crafting a special server response to an ordinary HTTP request. However, because Firefox only consumes 1 core and occasionally backs down shortly, the user will likely be able to recover from the situation by closing the problematic tab.

Trac:
Username: sqrt2

added MikePerry201402R component::torbrowserbutton interview owner::mikeperry priority::medium reporter::sqrt2 resolution::fixed status::closed tbb-crash tbb-usability type::defect labels

This doesn't happen for me with the above link. Is it regularly reproducible for you? Is there a test case that will always trigger it?

Based on the description, it sounds like a mime type sniffing issue. If the mime type is not specified, Firefox will try to infer it based on content. This code is crazy old and crufty and has experienced a number of security issues in the past. It's also possible it has weird interactions with super-slow networks/halted downloads due to bad Tor circuits.

Still, we have not touched that code. So whatever issues are present are likely also in Firefox 17-ESR, and should show up there if we can get a solid repro case. For reference, here are the official FF17.0.9 binaries: https://ftp.mozilla.org/pub/mozilla.org/firefox/releases/17.0.9esr/

All of the above is 100% always reproducible for me (Linux x86_64).

The problem does not occur with the official 17.0.9esr binary.

Trac:
Username: sqrt2

I see the different behavior of a vanilla ESR 17.0.9 and TBB 3.0 as well on my 32bit Debian testing, interesting (although not the 100% CPU consumption).

Trac:
Cc: N/A to g.koppen@jondos.de

Trac:
Keywords: tbb dos content-type deleted, tbb dos content-type Firefox Patch Issues added

Trac:
Keywords: tbb dos content-type Firefox Patch Issues deleted, tbb dos content-type added
Component: Tor bundles/installation to Firefox Patch Issues
Owner: erinn to mikeperry

Trac:
Component: Firefox Patch Issues to TorBrowserButton

Trac:
Status: new to needs_review

Trac:
Status: needs_review to new

Trac:
eab_nonull.diff

NULL

Experienced what seems to be this bug in TBB 3.5 with the following URL:

http://gkall.hobby.nl/cism216.fw

It does not occur in Firefox ESR (the same version as TBB 3.5 uses). Occurs on both Linux and Windows. Checked with Wireshark that no Content-Type header is sent.

Trac:
Username: adrian.halston

Resurrecting some relevant comments from above:

see #9115 for some details about it.

Problem is "return null" from external-app-blocker.js 
Removing it solves everything except spamming of error console with warning, that is why hack with returning null was needed in first place. 
	 	 
Proposed new hack solves noise in console by returning string "text/plain" that code can accept. Code will use "text/plain" anyway for case like this ticket describes. Hopes nothing another will be broken with new hack. Need to test. 

[RFC 2616](http://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html#sec7.2.1): 
Any HTTP/1.1 message containing an entity-body SHOULD include a Content-Type header field defining the media type of that body. If and only if the media type is not given by a Content-Type field, the recipient MAY attempt to guess the media type via inspection of its content and/or the name extension(s) of the URI used to identify the resource. If the media type remains unknown, the recipient SHOULD treat it as type "application/octet-stream".  
 	 
Returning string prevents inspecting of content. And returning "text/plain" violates specification. 
Doubly wrong way to fix bug. 
	 	 
Only choice: Spam or DoS.

Firefox 10ESR had not ability to handle null from Torbutton too, no recursion and almost no CPU eating, but still blank page in result.

Trac:
Keywords: tbb dos content-type deleted, tbb-usability added
Cc: g.koppen@jondos.de to gk

Wild guess: What happens if we return "" instead of NULL or "text/plain"? The key property we want is for the content sniffing to still kick in if the type is empty, rather than hack it to some default type and have behavior change.

return null and return "" show the same behaviour. No more 100% CPU consumption in TBB 3.5, though, just a blank page.

Trac:
Username: sqrt2

Trac:
Resolution: N/A to worksforme
Status: new to closed

Replying to cypherpunks:

Resurrecting some relevant comments from above: Did you ask permission?

The comments were still available, in diff form. I just collected them to make it easier to work on the bug.

Trac:
Resolution: worksforme to N/A
Status: closed to reopened

Linking related #9912 (closed), also about cdimage.debian.org, only about the file being truncated.

Again useful comments from the diffs:

You can't bypass exception logging in JavaScript using null.

{{{
ACString getTypeFromURI (in nsIURI aURI)

And you can't return null, it will be converted to ACString and passed to core as content type. That means you passes non empty content type for any case even if string is empty, and if code nothing knows about this type it going to do something bad. }}}

> you passes non empty content type for any case even if string is empty

Or content type is empty actually, and if firefox was build with debug enabled then
{{{
   if (SniffURI(aRequest)) {
     NS_ASSERTION(!mContentType.IsEmpty(), 
                  "Content type should be known by now.");
     return;
   }

triggered. Test it. }}}

Can you explain any profit to have remotely triggerable hang? Is it worth of suppressing for one logged exception?

I think the right fix here is to remove external-app-blocker.js from Torbutton, and patch the Firefox app launching code to emit our custom confirmation dialog before actually launching the app (or create another observer for this purpose).

Unfortunately, the external app launching code itself is a little hairy and convoluted. The starting points are nsExternalHelperAppService::DoContent() and nsExternalHelperAppService::LoadURI(). It looks like there are still a few entrypoints there to launch external apps that happen before Mozilla tries to present their version of the app launch confirmation dialogs. Unfortunately, some of these points may happen in what appears to be compile-time generated C++ code.

I can also try to bring this to Mozilla's attention to see if they are willing to write a proper fix themselves, since this silent app launching behavior is a longstanding issue in their own confirmation dialog system.

           var call;
           if(params.length) call = "("+params.join().replace(/(?:)/g,function(){return "p"+(++x)})+")";
           else call = "()";
-          if(method == "getTypeFromFile" || method == "getTypeFromExtension" || method == "getTypeFromURI") {
-           // XXX: Due to https://developer.mozilla.org/en/Exception_logging_in_JavaScript
-           // this is necessary to prevent error console noise on the return to C++ code.
-           // It is not technically correct, but as far as I can tell, returning null
-           // here should be equivalent to throwing an error for the codepaths invovled
-           var fun = "(function "+call+"{"+
-              "if (arguments.length < "+wrapped[method].length+")"+
-              "  throw Components.results.NS_ERROR_XPC_NOT_ENOUGH_ARGS;"+
-              "try { return wrapped."+method+".apply(wrapped, arguments); }"+
-              "catch(e) { if(e.result == Components.results.NS_ERROR_NOT_AVAILABLE) return null; else throw e;} })";
-            newObj[method] = eval(fun);
-          } else {
+          {
             var fun = "(function "+call+"{"+
               "if (arguments.length < "+wrapped[method].length+")"+
               "  throw Components.results.NS_ERROR_XPC_NOT_ENOUGH_ARGS;"+

Keep it simple.

Replying to mikeperry:

I think the right fix here is to remove external-app-blocker.js from Torbutton, and patch the Firefox app launching code to emit our custom confirmation dialog before actually launching the app (or create another observer for this purpose).

The observer idea sounds good. This way other extensions and Mozilla itself may use this, too.

Unfortunately, the external app launching code itself is a little hairy and convoluted. The starting points are nsExternalHelperAppService::DoContent() and nsExternalHelperAppService::LoadURI(). It looks like there are still a few entrypoints there to launch external apps that happen before Mozilla tries to present their version of the app launch confirmation dialogs. Unfortunately, some of these points may happen in what appears to be compile-time generated C++ code.

I can also try to bring this to Mozilla's attention to see if they are willing to write a proper fix themselves, since this silent app launching behavior is a longstanding issue in their own confirmation dialog system.

Might at least be interesting to know what they think about it. I was always under the impression that this "feature" was on purpose to save some time: "The user clicks on the resource, hence she wants to have it (be it opened somewhere else or saving it), thus lets already download it in the background before the final decision is made".

Replying to gk:

Replying to mikeperry:

I can also try to bring this to Mozilla's attention to see if they are willing to write a proper fix themselves, since this silent app launching behavior is a longstanding issue in their own confirmation dialog system.

Might at least be interesting to know what they think about it. I was always under the impression that this "feature" was on purpose to save some time: "The user clicks on the resource, hence she wants to have it (be it opened somewhere else or saving it), thus lets already download it in the background before the final decision is made".

I forgot to add "and handle it if there is a (default) handler available".

Trac:
Summary: DoS of TBB 2.4/3.0 when no Content-Type header and more than 512 bytes of content are sent to DoS of TBB when no Content-Type header and more than 512 bytes of content are sent
Keywords: N/A deleted, tbb-crash, interview added

Btw, the right fix here could knock out #1079 (moved) and possibly also #7439 (closed). See #7439 (closed) for suggestions on the message text, and #10482 (closed) for some other usability complaints about the current dialog.

Replying to mikeperry:

Btw, the right fix here could knock out #1079 (moved) and possibly also #7439 (closed). See #7439 (closed) for suggestions on the message text, and #10482 (closed) for some other usability complaints about the current dialog.

I've been thinking about this over the weekend. This issue bugs amany of our users (there are a lot of possible duplicates floating around, there are reports on IRC, etc.) but the right fix won't be available easily. What if we use this ticket as (another) stopgap for the 3.6 release (which is probably within the next two weeks) and doing either a fix a la comment 75 or by hooking the console service to get rid of the error messages. The real fix as mentioned in comment 74 would then be saved for #1079 (moved). This way, we have time to design this properly while avoiding unnecessary browser freezes which can be super annoying.

Ok. You will never hear me argue against expedient crazy stopgap hacks. If you want to do the console filtering and comment:75 (we need both, right?), it sounds good.

Replying to mikeperry:

Ok. You will never hear me argue against expedient crazy stopgap hacks.

That's the spirit!

If you want to do the console filtering and comment:75 (we need both, right?), it sounds good.

We need comment:75 or something like that for the console filtering, yes.

As a work-in-progress report: I spent one day on trying to get the hooking approach to work but failed. While the hooking is working and I can filter the messages properly I encountered websites that are getting my browser crashed reliably with that setup. This happens even if I don't do anything besides hooking the component. So, we need something different here.

There: bug_9901_v4 in my public torbutton repo (what a pain...).

Trac:
Status: reopened to needs_review
Keywords: N/A deleted, MikePerry201401R added

torbutton -b bug_9901_v4 seems to be working, however, when I first tried it, I've come across the following behaviour that I cannot reproduce right now:

While going to <​http://cdimage.debian.org/debian-cd/7.3.0/i386/iso-dvd/> and clicking the MD5SUMS file works, pasting the URL <​http://cdimage.debian.org/debian-cd/7.3.0/i386/iso-dvd/MD5SUMS> to the address bar now opens Startpage with the URL as a search term; not so if I leave out the "http://", however.

I suppose if there is no concern that there could be a race condition or something similar (I //am// currently running at close to 100% CPU on all cores for unrelated reasons), we can credit this to solar flares or whatever seems marginally plausible within the employed philosophical framework.

Trac:
Username: sqrt2

Trac:
Keywords: MikePerry201401R deleted, MikePerry201402R added

There was no need to remove the "New window" log message; bug_9901_v5 is now the branch to review.

For best practice, we need to prefix these functions with something (torbutton_ has been our convention). The reason is because overlay javascript is added into the browser XUL window scope, and we risk collisions if another addon defines something named either handleConsole, consoleObserver.

I have fixed this and merged the branch. It will go into master and it should be in a nightly at https://people.torproject.org/~linus/builds/ shortly to play with. If that goes well, we can tag a new release and push it out in a stable.

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

Replying to mikeperry:

For best practice, we need to prefix these functions with something (torbutton_ has been our convention). The reason is because overlay javascript is added into the browser XUL window scope, and we risk collisions if another addon defines something named either handleConsole, consoleObserver.

Ooops, I fully agree. Thanks for fixing this slip-up.

closed

mentioned in issue #9912 (closed)

mentioned in issue #10482 (closed)

mentioned in issue #10497 (moved)

mentioned in issue #10630 (moved)

mentioned in issue #10631 (moved)

mentioned in issue #10646 (closed)

mentioned in issue #10704 (moved)

mentioned in issue #10852 (closed)

mentioned in issue #10905 (closed)

mentioned in issue #19273 (moved)