Prevent TorBrowser from creating clipboardcache turds
Firefox dumps these wonderful UTF-16 files right into /tmp if you happen to highlight and copy a significantly large amount of text, this does not seem that ideal for security reasons.
Activity
Trac:
Username: michael-
DEFENSE OF SIMPLISTIC SOLUTION (IN ATTACHMENT 'msvb-9701.diff') TO THIS PROBLEM
Q1) Why didn't you make the same change in other parts of tor-browser? A1) Because there is exactly one place where the data in question is written to a disk cache, and it is in the SetData method of the nsTransferable interface.
Q2) Won't other blocks try to read from the disk cache instead of mData? A2) No, since they test mData for existence before testing for a cache file.
Q3) Won't the mData memory buffer be exhausted with excessive clipboard data? In other words isn't the proposed solution vulnerable to a buffer overflow? A3) Only a reference to the actual data is copied avoiding a buffer overflow. A3 Oberservation 1) While it's not well understood how the underlying browser logic handles memory problems when populating aData, any such testing falls into a larger scope of DOM/Dragdrop/ and other more generic storage questions (and bugs.) A3 Oberservation 2) It would seem from the patched code that a valid case for caching clipboard data to disk exists. Unfortunately, no documentation exists to support this. Worse, the upstream source code repository doesn't reflect any earlier file history to gain insight.
Q4) You mean neither writing nor reading of data can lead to a memory overflow? A4) Yes, the data is in aData from the beginning. Regardless of whether the data is later stored to a disk cache or not, it was present in memory before this solution changed any storage allocation logic. A4 Oberservation 3) Investigation is needed to better understand why the upstream author chose to disk cache in the first place.
Q5) What are the security implications of this bug report? A5) This bug report (wether applying the proposed solution or not) implies memory conditions which could lead to a buffer overflow and root compromise of a system running the tor-browser software. Note that this potential (probably very low) exists equally whether the proposed solution is applied (tor-browser is patched) or not, that is the proposed solution carries no added risk.
Q6) Has the tor-browser been tested thoroughly for the named buffer overflow? A6) Security analysis of tor-browser without applying the proposed solution falls outside the scope of this bug report, so it hasn't been carried out. Furthermore, the proposed solution carries no added security risk.
Q7) Where did the value kLargeDatasetSize come from? A7) Nobody knows, since the very first revisions of nsTransferable.cpp and nsTransferable.h contain the logic and constant value.
Q8) What about implementing memory chunking for large data sets? A8) This would require alternative storage, probably defeating the purpose of disk avoidance by design. It would also open a large can of worms due to Firefox's portable nature as the paste interface of underlying platforms behaves differently, requiring several chunking implementations. Lastly, it is out of the scope of the corresponding bug report.
Q9) What is the mathematical proof behind the proposed solution defense? A9) Assuming that all web pages (DOM) are stored entirely in virtual memory, and that any web page consumes at most 1/2 available virtual memory, and that selected or copied data consumes at most 100% DOM representation, ...the proposed solution carries no side effects while correcting the otherwise violated premise of disk avoidance by design.
Q10) What documentation was referred to while engineering the proposed solution? A10 https://developer.mozilla.org/en-US/docs/Using_the_Clipboard/ https://wiki.mozilla.org/Firefox/Projects/ClipboardAPI https://bugzilla.mozilla.org/show_bug.cgi?id=407983 http://kb.mozillazine.org/Clipboard_not_working http://kb.mozillazine.org/Memory_Leak
Q11) In which source context was the proposed solution engineered? A11) After cloning tor-browser and checking out tor-browser-24.5.0esr-1-build4
Q12) How was the proposed solution built? A12) By carrying out the command 'cd tor-browser && make -f client.mk' After gaining confidence by QA, TBB packages were built for all variants.
Q13) What was tested to assure the quality (QA) of this change? A13) The binary tor-browser/obj-x86_64-unknown-linux-gnu/dist/bin/firefox was launched using code in tor-browser/obj-x86_64-unknown-linux-gnu/toolkit/library/libxul.so After that, similar launches were carried out on Mac OSX 10.9 AMD64 and Windows 8.1 AMD64 platforms. The launches were observed for symptoms of the clipboard turd bug and found to be free of this problem once the proposed solution was applied. All platforms are affected by this bug.
Q14) Were only tor-browser builds tested for the platforms in question? A14) No, the Tor Browser Bundle was created for all platforms (32-bit EN variants and 64-bit Linux EN) and tested as well.
Q15) What was not tested to assure the quality of this change? A15) Unit tests were not constructed using the ClipboardHelper and Clipboard XPCOM interfaces in order to fully guarantee correctness. This can optionally be carried out on request by TPI. A15 Oberservation 4) Does a tor-browser or Firefox test harness exist?
Trac:
Username: michael 
Here is the commit log (CVS revision 1.23) that shows when the concept of writing to disk first landed in the Mozilla code:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/widget/src/xpwidgets/nsTransferable.cpp&rev=1.23
Unfortunately, this is ancient history and there is no reference to a bug number. And the check in comment does not say why they changed things to write clipboard data to disk rather than keep it in memory. My best guess is that they just wanted to ensure that memory usage stayed as low as possible.
I think your proposed fix is OK. It might be better to add a preference or a compile-time method to disable the WriteCache() code (Mozilla would be more like to accept a patch that uses such an approach). For example, the kLargeDatasetSize value could be retrieved from a preference and a zero value could mean "never write to disk."
-
DataStruct::WriteCache and nsIOutputStream::Write don't deallocate the memory in aData. If you look closely, nsMemory::Free(buff) only frees the 'buff' memory that was deep copied from aData by nsPrimitiveHelpers::CreateDataFromPrimitive.
Before proposing a preference, it might be wise to do a real world test to confirm this. If confirmed it makes no sense producing the cache disk files under any conditions (even the stock Firefox) whatsoever.
Trac:
Username: michael 
Replying to mcs:
I think your proposed fix is OK. It might be better to add a preference or a compile-time method to disable the WriteCache() code (Mozilla would be more like to accept a patch that uses such an approach). For example, the kLargeDatasetSize value could be retrieved from a preference and a zero value could mean "never write to disk."
That would be one approach. But I'd be in favor of another one: binding it to the state of the Private Browser Mode: if we are in PBM we don't write to disk otherwise we do as I think the current behavior is just a violation of the Private Browsing Mode paradigm (at least as we would like to have it).
I tested the patch on all platforms and it is working fine.
-
Replying to gk:
That would be one approach. But I'd be in favor of another one: binding it to the state of the Private Browser Mode: if we are in PBM we don't write to disk otherwise we do as I think the current behavior is just a violation of the Private Browsing Mode paradigm (at least as we would like to have it).
Tying this to PBM sounds perfect to me.
-
Replying to michael:
DataStruct::WriteCache and nsIOutputStream::Write don't deallocate the memory in aData. If you look closely, nsMemory::Free(buff) only frees the 'buff' memory that was deep copied from aData by nsPrimitiveHelpers::CreateDataFromPrimitive.
Before proposing a preference, it might be wise to do a real world test to confirm this. If confirmed it makes no sense producing the cache disk files under any conditions (even the stock Firefox) whatsoever.
Unfortunately, a limited [1] black box test has confirmed the opposite of the hypothesis based on aData block code analysis. Basically, a prestine TBB 3.6.1 (which creates clipboardcache files) consumes no new memory when selecting >1Mo text:
pmap `pgrep firefox` >pmap1.txt echo 'Select text now...' pmap `pgrep firefox` >pmap2.txt diff pmap1.txt pmap2.txtTesting a modified (with msvb-9701.diff) tor-browser build reveals additional memory use:
diff pmap3.txt pmap4.txt 4a5 > 00007f2962700000 131072K rw--- [ anon ] 649c650 < total 771608K --- > total 902680KTrac:
Username: michael -
Replying to mikeperry:
So should we just go with this hack for now then? It seems like yes, while we wait to see what Mozilla says about the deeper issue?
The test (above) was quick and dirty, but suggests increased memory consumption. Mozilla will probably not accept a change that increases footprint by 30 Mo when selecting large text blocks.
I'll implement Georg's suggestion of changing this logic only when in Private Browser Mode.
Trac:
Username: michael -
The most recent patch improves on this solution by implementing the idea (from TBB meetings) to disable turds conditionally according to privacy preference.
Trac:
Username: michael -
What speaks against the solution in comment:13? Isolating possible (cache) identifiers to the URL bar domain is one thing but the clipboard cache is of a different kind I think. Moreover, I still think this is a real bug in Firefox's PBM (seen as a do-not-store-things-to-disk-mode) and should get fixed there directly.
-
Replying to gk:
What speaks against the solution in comment:13?
If you mean, why did I implement the condition according to privacy.thirdparty.isolate rather than NS_UsePrivateBrowsing(aChannel), it's because the nsTransferable code block dealing with disk caching has no chance at knowing in which channel it's operating. So it can't know if Private Browsing Mode is active.
Isolating possible (cache) identifiers to the URL bar domain is one thing but the clipboard cache is of a different kind I think.
I see your point and agree. Lets figure out:
- How hard it would be to implement the channel driven PBM approach.
- If we want to go this distance at giving something so obvious (for Tor) a condition.
Moreover, I still think this is a real bug in Firefox's PBM (seen as a do-not-store-things-to-disk-mode) and should get fixed there directly.
I'm not sure, since Firefox in PBM doesn't have as strong a requirement as Tor to not store things to disk. Mozilla would probably rather support old hardware by reducing memory consumption.
Is there some specification that you're referring to where Mozilla emphasizes disk avoidance?
Trac:
Username: michael -
void DataStruct::SetData ( nsISupports* aData, uint32_t aDataLen )After new trials like:
nsCOMPtr<nsIChannel> aChannel(do_QueryInterface(aData, &rv)); nsCOMPtr<nsIDOMWindow> aWin(do_QueryInterface(aData, &rv)); nsCOMPtr<nsIDOMChromeWindow> aChromeWin(do_QueryInterface(aData, &rv)); nsCOMPtr<nsILoadContext> aLoadContext(do_QueryInterface(aData, &rv));...in every case rv == NS_NOINTERFACE, with the exception of:
nsCOMPtr<nsISupportsString> aSuppStr(do_QueryInterface(aData, &rv));So the recent suggestions to 'get a top level chrome window' or to 'reverse engineer the JavaScript PrivateBrowsingUtils module' seem misguided. Maybe Firefox runtime exposes undocumented window/document/channel globals?
Trac:
Username: michael -
Regarding msvb9701-283f7c6.patch, preemptive consideration of the question is:
Won't this break binary compatibility causing dependent code to crash at runtime?
Answer: Yes, but only if the dependent code uses the new method ^1^ while calling into the old (not overloaded) vtable. Or possibly NS_ERROR_OBJECT_IS_IMMUTABLE is returned from dependent code.
^1^ Assuming XPCOM/GCC works like classic COM/VC++.
Furthermore, the interface signature (struct DataStruct) in question has no corresponding IDL entry, so it hopefully won't be called from foreign (non Mozilla) components at all?
Trac:
Username: michael
Status: needs_information to needs_review
Keywords: TorBrowserTeam201501 deleted, TorBrowserTeam201501R added -
Kathy and I took a quick look at msvb9701-283f7c6.patch. I am glad you found a way to make this approach work!
I don't think we have to worry about non-Mozilla components calling DataStruct::SetData(); it looks like that method is only called from within nsTransferable.cpp (I am not sure why it is not declared inside nsTransferable.cpp instead of in the header). Is that your conclusion as well?
I would feel better if the old DataStruct::SetData() call that does not accept a aIsPrivBrowsing parameter was removed entirely (to avoid any chance that the wrong call could be made). In fact, there is another call later in nsTransferable::SetTransferData() that should be modified to use the new method as well.
One nit: You should be able to just access the mPrivateData member directly instead of bothering with a call to GetIsPrivateData().
-
Replying to mcs:
I don't think we have to worry about non-Mozilla components calling DataStruct::SetData(); it looks like that method is only called from within nsTransferable.cpp [...] Is that your conclusion as well?
Yes, SetData is not defined in IDL and only nsTransferable objects call DataStruct::SetData(). So Arthur and I think it's okay to change the arguments by adding a PBM flag.
Furthermore, with no IDL entries, there's no XPCOM transport to get interfaces, so my attempt to avoid foreign code crashing by adding an overloaded method might not help that much.
(I am not sure why it is not declared inside nsTransferable.cpp instead of in the header).
...or why DataStruct is a independent class, rather than a private member of nsTransferable?
I would feel better if the old DataStruct::SetData() call that does not accept a aIsPrivBrowsing parameter was removed entirely (to avoid any chance that the wrong call could be made).
Assuming the overloaded method is a NOOP there's no tradeoff to worry about, so thanks for the tip.
I've deleted the original DataStruct::SetData() definition and implementation not including the bool PBM flag.
DataStruct::SetData ( nsISupports*, uint32_t);DataStruct::SetData ( nsISupports*, uint32_t, bool);In fact, there is another call later in nsTransferable::SetTransferData() that should be modified to use the new method as well.
Opla, good catch. Now the second (of two) call has the relevant PBM aware logic as well.
One nit: You should be able to just access the mPrivateData member directly instead of bothering with a call to GetIsPrivateData().
I used the accessor so that the mPrivateData type could be internally changed in a future FF release (typical raison d'etre for accessors,) but either way is fine. If we have a convention of direct member manipulation I'll change the logic to match.
Do we/should I?
Trac:
Username: michael -
Replying to michael:
I used the accessor so that the mPrivateData type could be internally changed in a future FF release (typical raison d'etre for accessors,) but either way is fine. If we have a convention of direct member manipulation I'll change the logic to match.
Do we/should I?
I do not know that we have a policy, but in this case things are simple enough (and you are in the same class), so direct access seems OK to me. And it will simplify the patch a little since you can avoid some error checking that is not needed.
-
The revised patch looks OK. My only nit is to avoid checks like boolVar == false. Please change the line that uses that kind of test to:
if ((aDataLen > kLargeDatasetSize) && !aIsPrivBrowsing) {
(I think a simple ! test is more consistent with code elsewhere in nsTransferable.cpp and with other Mozilla code).
Have you tested the fix? I am not sure how difficult it would be to add automated tests to ensure that the fix does not break in the future, but Mozilla will want them. That could be addressed in a separate bug though.
-
Replying to mcs:
... boolVar == false. Please change the line that uses that kind of test to:
if ((aDataLen > kLargeDatasetSize) && !aIsPrivBrowsing) {
Good idea, I like the more consistent syntax better as well. It's in the latest patch post.
Have you tested the fix?
Blackbox
I've high level tested [1][2] without covering all code paths, like when unregistered flavor handling causes recursion or a format converter to manipulate the flavor (see around line 410.) I can nevertheless try to force all code paths on request. There might be no natural (or possible?) use case for some conditions however, like selecting millions of kLargeDatasetSize bytes from a about: URI.
[1] PBM on, navigate a large doc, select Ctrl-A, ls $TMPDIR/clipboardcache* [2] PBM off, navigage a large doc, select Ctrl-A, ls $TMPDIR/clipboardcache*
Control flow
On another note, DataStruct::GetData() is left unmodified as my tests conclude that that logic can only read from disk if no selection exists in memory.
Implicit tests
An unconditional state of disk avoidance has been in Tor Browser releases [3] since tor-browser-24.5.0esr-1.
[3] https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-24.5.0esr-1&id=8088761c
I am not sure how difficult it would be to add automated tests to ensure that the fix does not break in the future, but Mozilla will want them.
Good idea regardless of Mozilla, like a xpcshell test for regression.
That could be addressed in a separate bug though.
Yes, better for clarity and code reuse. The new requirement is described in #14255 (moved).
Trac:
Username: michael -
The revised fix has been merged into tor-browser-31.4.0esr-4.5-1.
Revert old fix: https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-31.4.0esr-4.5-1&id=ba4eca38fdad352c564719ff49aaa8c87e8a8ded Apply new fix: https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-31.4.0esr-4.5-1&id=453b4ee4b2cd1c3dd22ee2040b38f09d26e8a60f
I hope this was the correct (cleanest and clearest) way to handle this with git.
Trac:
Resolution: N/A to fixed
Keywords: TorBrowserTeam201501R deleted, TorBrowserTeam201501 added
Status: needs_review to closed -
We committed a follow up fix after a standalone test build failed on Mac OS:
We did not attempt a Gitian-based build; hopefully all is well.
-
A corresponding report with hopes of merging to ESR38 is on Bugzilla.
Trac:
Username: michael -
Trac:
Username: michael -
Replying to mcs:
We committed a follow up fix after a standalone test build failed on Mac OS:
Thanks, the patch block in question was improperly overwritten during upload.
The most recent patch correctly matches your follow up fix as well as the improperly overwritten block.
We did not attempt a Gitian-based build; hopefully all is well.
A 32 hour gitian-build is under way and will be build time and high level run time tested accordingly.
Trac:
Username: michael -
Replying to myself:
Replying to mcs:
We did not attempt a Gitian-based build; hopefully all is well.
A 32 hour gitian-build is under way and will be build time and high level run time tested accordingly.
A local nightly gitian-build including our patch completed and high level testing validated the logic in question.
Trac:
Username: michael -
Replying to cypherpunks:
i had this happen using the webconsole copying a large section of de-obfuscated html with torbrowser 6.5.1
Please don't reopen tickets that got closed two years ago and open new ones instead, thanks. I did so for this issue: #21830 (moved) and have a question for you on this ticket.
Trac:
Resolution: N/A to fixed
Status: reopened to closed mentioned in issue #12620 (moved)
mentioned in issue #14255 (moved)
