Bug: plugin-container exhausts memory leading to thrash and/or crash
(Re-sending after subscribing to the list.) 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.0es... 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. Cheers. PS: How about fixing your bug tracker? It's impossible to register an account. Bloody google captchas? Free labour for google from the tor community courtesy of tor inc., isn't that lovely (never mind the fact that they require javascript, and even then never actually work). Ridiculous.
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.0es... 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.
participants (2)
-
astian -
Georg Koppen