[tor-bugs] #24398 [Applications/Tor Browser]: Plugin-container exhausts memory leading to thrash and/or crash on Linux

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#24398: Plugin-container exhausts memory leading to thrash and/or crash on Linux
-------------------------------------+-------------------------------------
     Reporter:  gk                   |      Owner:  tbb-team
         Type:  defect               |     Status:  new
     Priority:  Medium               |  Milestone:
    Component:  Applications/Tor     |    Version:
  Browser                            |   Keywords:  tbb-crash,
     Severity:  Normal               |  TorBrowserTeam201711R
Actual Points:                       |  Parent ID:
       Points:                       |   Reviewer:
      Sponsor:                       |
-------------------------------------+-------------------------------------
 We got a nice bugreport from a cypherpunk on our tbb-dev mailing list
 (https://lists.torproject.org/pipermail/tbb-dev/2017-November/000673.html)
 with a patch up for review (which I'll attach in a minute) (the problem
 seems to be caused by our workaround for #24052):
 {{{
 STR
 ---

 1. Run one of the platforms affected by the recent "tormoil" vulnerability
 (I
    tested this on a GNU/Linux distro).

 2. Under Tor Browser 7.0.10's installation directory, create
 `Browser/TorBrowser/Data/Browser/profile.default/chrome/userContent.css`
    with the following content (you can use whatever you want here, just
 adjust
    the next steps accordingly):

      body {
        background-color: blue !important;
        color: white !important;
      }

 3. Start Tor Browser.

 4. Confirm that the content background looks blue.

 5. Right click the blue background on an empty spot (body element) and
 choose
    "Inspect Element".

 6. Wait until the Inspector window shows up.  Observe memory consumption
 of
    process "plugin-container" continually rise.

 Analysis
 --------

 I think this regression is caused by the workaround [0] for the recent
 critical
 vulnerability [1], but it has exposed what looks to me (not being privy to
 the
 details of "tormoil") like another bug, this one in javascript code.

   0: https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-
 browser-52.5.0esr-7.0-1&id=643117230bb3402c997f065980db1eec19c7a6ed
   1: https://trac.torproject.org/projects/tor/ticket/24052

 `newChannelForURL` in DevToolsUtils.js (packed inside
 `Browser/browser/omni.ja`) will recursively call itself when
 `NetUtil.newChannel` raises an exception, prepending "file://" to the
 input
 URL:

   /**
    * Opens a channel for given URL. Tries a bit harder than
 NetUtil.newChannel.
    *
    * @param {String} url - The URL to open a channel for.
    * @param {Object} options - The options object passed to @method fetch.
    * @return {nsIChannel} - The newly created channel. Throws on failure.
    */
   function newChannelForURL(url, { policy, window, principal }) {
     var securityFlags =
 Ci.nsILoadInfo.SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL;

     let uri;
     try {
       uri = Services.io.newURI(url, null, null);
     } catch (e) {
       // In the xpcshell tests, the script url is the absolute path of the
 test
       // file, which will make a malformed URI error be thrown. Add the
 file
       // scheme to see if it helps.
       uri = Services.io.newURI("file://" + url, null, null);
     }

     [...]

     try {
       return NetUtil.newChannel(channelOptions);
     } catch (e) {
       // In xpcshell tests on Windows,
 nsExternalProtocolHandler::NewChannel()
       // can throw NS_ERROR_UNKNOWN_PROTOCOL if the external protocol
 isn't
       // supported by Windows, so we also need to handle the exception
 here if
       // parsing the URL above doesn't throw.
       return newChannelForURL("file://" + url, { policy, window, principal
 });
     }
   }

 With the mentioned patch, the call to `NetUtil.newChannel` raises an
 exception.  This results in infinite recursion coupled with rapid (though
 linear) memory consumption.  Thus, `plugin-container` will, in just a few
 seconds, exhaust all available memory.

 Partial fix
 -----------

 AFAICT, the current patch for "tormoil" is just a hurried stop-gap
 workaround
 and as such it is acceptable, and somewhat expected, for it to cause
 other,
 less severe, kinds of breakage.  However, ISTM that the code in
 `newChannelForURL` is buggy regardless: the recursion has no (evident)
 termination condition.

 The comment before the recursive call says that it is needed due to some
 tests
 for which `newChannel` can raise `NS_ERROR_UNKNOWN_PROTOCOL`.  So maybe it
 should only catch the exception (and make the recursive call) if the error
 is
 that one and `url` does not already start with "file://".  The attached
 patch
 does this.  It does not fix the regression (the Developer Toolbox still
 fails
 to show the appropriate styles and stylesheets), but at least fixes the
 DoS
 caused by the runaway memory allocation.
 }}}

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/24398>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs