Opened 3 months ago

Closed 2 months ago

#24398 closed defect (fixed)

Plugin-container exhausts memory leading to thrash and/or crash on Linux

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-crash, TorBrowserTeam201712R
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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.

Child Tickets

Attachments (1)

tor-browser-runaway-memory-allocation.patch (626 bytes) - added by gk 3 months ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 3 months ago by gk

I gave the STR a quick test and for some reason I did not have a plugin-container process to begin with. So, we might need slightly adapted STR for testing the patch.

comment:2 Changed 3 months ago by gk

Status: newneeds_review

comment:3 Changed 3 months ago by gk

Keywords: TorBrowserTeam201712R added; TorBrowserTeam201711R removed

Moving review tickets over to December

comment:4 Changed 2 months ago by mcs

Kathy and I do not always see the memory growth, but when running with --verbose` we see many lines like the following:

newChannelForURL@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:567:12

and then a few like this:

console.error: Protocol error (unknownError): too much recursion

So I think we are reproducing the bug. We did not do anything special; if you load a page you should have a plugin-container process (assuming multiprocess mode is working).

We are going to rebuild with the proposed fix in place and see if the problem goes away.

comment:5 Changed 2 months ago by mcs

r=brade, r=mcs
We confirmed that the patch avoids the recursion problem. Because the devtools code cannot load file URLs such as the userContent.css stylesheet, things still do not work correctly... but at least we avoid infinite recursion.

comment:6 Changed 2 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Applied to tor-browser-52.5.2esr-7.5-2 (as commit cef2fe28a238d7e445d7c5b4292bfe27c1b71bca). Thanks!

Note: See TracTickets for help on using tickets.