[tbb-dev] Bug: plugin-container exhausts memory leading to thrash and/or crash

[Georg Koppen]

Hi!

astian:
> STR
> ---

Thanks for the detailed bug report and the patch. That's really
appreciated. I've created
https://trac.torproject.org/projects/tor/ticket/24398 for your report.

I am sorry that you had issues with out bug tracker. One thing you might
be interested in is that we don't require an account to be created
before commenting/reporting bugs. You can use the "cypherpunks" account
for that. See: https://trac.torproject.org/projects/tor "Unofficial
Documentation" for more about that.

Georg

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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.torproject.org/pipermail/tbb-dev/attachments/20171124/3622c70d/attachment.sig>