GitLab

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 (moved)):

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.