Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#9901 closed defect (fixed)

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

Reported by: sqrt2 Owned by: mikeperry
Priority: Medium Milestone:
Component: TorBrowserButton Version:
Severity: Keywords: tbb-usability, interview, tbb-crash, MikePerry201402R
Cc: gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

Attachments (1)

eab_nonull.diff (3 bytes) - added by cypherpunks 6 years ago.
NULL

Download all attachments as: .zip

Change History (47)

comment:1 Changed 6 years ago by mikeperry

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/

comment:2 Changed 6 years ago by sqrt2

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.

comment:3 Changed 6 years ago by gk

Cc: g.koppen@… added

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).

comment:4 Changed 6 years ago by cypherpunks

Keywords: Firefox Patch Issues added

comment:5 Changed 6 years ago by cypherpunks

Component: Tor bundles/installationFirefox Patch Issues
Keywords: Firefox Patch Issues removed
Owner: changed from erinn to mikeperry

comment:6 Changed 6 years ago by cypherpunks

Component: Firefox Patch IssuesTorBrowserButton
Last edited 6 years ago by cypherpunks (previous) (diff)

comment:7 Changed 6 years ago by cypherpunks

Last edited 6 years ago by cypherpunks (previous) (diff)

comment:8 Changed 6 years ago by cypherpunks

Status: newneeds_review
Last edited 6 years ago by cypherpunks (previous) (diff)

comment:9 Changed 6 years ago by cypherpunks

Status: needs_reviewnew
Last edited 6 years ago by cypherpunks (previous) (diff)

comment:10 Changed 6 years ago by cypherpunks

Last edited 6 years ago by cypherpunks (previous) (diff)

comment:11 Changed 6 years ago by cypherpunks

Last edited 6 years ago by cypherpunks (previous) (diff)

Changed 6 years ago by cypherpunks

Attachment: eab_nonull.diff added

NULL

comment:12 Changed 6 years ago by adrian.halston

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.

comment:13 Changed 6 years ago by gk

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. 
[http://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html#sec7.2.1 RFC 2616]: 
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.

comment:14 Changed 6 years ago by gk

Cc: gk added; g.koppen@… removed
Keywords: tbb-usability added; tbb dos content-type removed

comment:15 Changed 6 years ago by cypherpunks

Last edited 6 years ago by cypherpunks (previous) (diff)

comment:16 Changed 6 years ago by mikeperry

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.

comment:17 Changed 6 years ago by sqrt2

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

comment:18 Changed 6 years ago by cypherpunks

Last edited 6 years ago by cypherpunks (previous) (diff)

comment:63 Changed 6 years ago by cypherpunks

Last edited 6 years ago by cypherpunks (previous) (diff)

comment:64 Changed 6 years ago by cypherpunks

Resolution: worksforme
Status: newclosed
Last edited 6 years ago by cypherpunks (previous) (diff)

comment:65 in reply to:  63 Changed 6 years ago by gk

Resolution: worksforme
Status: closedreopened

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.

comment:66 Changed 6 years ago by cypherpunks

Last edited 6 years ago by cypherpunks (previous) (diff)

comment:67 Changed 6 years ago by cypherpunks

Last edited 6 years ago by cypherpunks (previous) (diff)

comment:68 Changed 6 years ago by cypherpunks

Last edited 6 years ago by cypherpunks (previous) (diff)

comment:69 Changed 6 years ago by cypherpunks

Last edited 6 years ago by cypherpunks (previous) (diff)

comment:70 Changed 6 years ago by cypherpunks

Last edited 6 years ago by cypherpunks (previous) (diff)

comment:71 Changed 6 years ago by cypherpunks

Last edited 6 years ago by cypherpunks (previous) (diff)

comment:72 Changed 6 years ago by dcf

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

comment:73 Changed 6 years ago by gk

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?

comment:74 Changed 6 years ago by 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).

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.

comment:75 Changed 6 years ago by cypherpunks

           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.

comment:76 in reply to:  74 ; Changed 6 years ago by gk

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".

Last edited 6 years ago by gk (previous) (diff)

comment:77 in reply to:  76 Changed 6 years ago by gk

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".

comment:78 Changed 6 years ago by mikeperry

Keywords: interview tbb-crash added
Summary: DoS of TBB 2.4/3.0 when no Content-Type header and more than 512 bytes of content are sentDoS of TBB when no Content-Type header and more than 512 bytes of content are sent

comment:79 Changed 6 years ago by mikeperry

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

Last edited 6 years ago by mikeperry (previous) (diff)

comment:80 in reply to:  79 Changed 6 years ago by gk

Replying to mikeperry:

Btw, the right fix here could knock out #1079 and possibly also #7439. See #7439 for suggestions on the message text, and #10482 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. This way, we have time to design this properly while avoiding unnecessary browser freezes which can be super annoying.

comment:81 Changed 6 years ago by mikeperry

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.

comment:82 in reply to:  81 Changed 6 years ago by gk

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.

comment:83 Changed 6 years ago by gk

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.

comment:84 Changed 6 years ago by gk

Keywords: MikePerry201401R added
Status: reopenedneeds_review

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

comment:85 Changed 6 years ago by sqrt2

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.

comment:86 Changed 6 years ago by mikeperry

Keywords: MikePerry201402R added; MikePerry201401R removed

comment:87 Changed 6 years ago by gk

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

comment:88 Changed 6 years ago by 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.

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.

comment:89 Changed 6 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed

comment:90 in reply to:  88 Changed 6 years ago by gk

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.

Note: See TracTickets for help on using tickets.