Tor Bug Tracker & Wiki: Ticket #9701: Prevent TorBrowser from creating clipboardcache turds

priority, type changed; keywords set; version deleted

arma — Sun, 08 Sep 2013 19:09:16 GMT

keywords tbb-disk-leak added
priority changed from minor to normal
version Tor: 0.2.3.25 deleted
type changed from enhancement to defect

keywords changed

mikeperry — Fri, 24 Jan 2014 06:59:24 GMT

keywords interview added

cc set

gk — Wed, 26 Mar 2014 09:44:23 GMT

cc gk added

michael — Mon, 12 May 2014 08:57:07 GMT

Amending this ticket to state that neither cutting nor copying is particularly relevant. While cut/copy does trigger additional turds to be written, a simple text selection (with no cut/copy at all) causes the symptom in question.

cc changed

michael — Mon, 12 May 2014 08:57:51 GMT

cc michael added

attachment set

michael — Wed, 14 May 2014 23:39:27 GMT

attachment set to msvb-9701.diff

Proposed solution removing affected code region.

michael — Wed, 14 May 2014 23:41:53 GMT

This bug affects all platforms served by the Tor Browser Bundle (Linux (IA32|AMD64), OSX IA32, Win32 IA32.) Although the degree is different, in that a simple selection >1Mo only causes temp file leakage in the Linux builds.

michael — Wed, 14 May 2014 23:43:45 GMT

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?

status changed

michael — Wed, 14 May 2014 23:45:03 GMT

status changed from new to needs_review

keywords changed

gk — Thu, 15 May 2014 06:48:27 GMT

keywords GeorgKoppen201405 added

mcs — Thu, 15 May 2014 19:03:46 GMT

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

michael — Fri, 16 May 2014 16:48:22 GMT

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.

keywords changed

mikeperry — Thu, 22 May 2014 16:06:47 GMT

keywords MikePerry201405R added

gk — Fri, 23 May 2014 13:23:34 GMT

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.

mcs — Fri, 23 May 2014 14:40:57 GMT

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.

keywords changed

mikeperry — Thu, 05 Jun 2014 10:14:00 GMT

keywords MikePerry201406R added; MikePerry201405R removed

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?

michael — Fri, 06 Jun 2014 05:54:54 GMT

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

Testing 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           902680K

michael — Fri, 06 Jun 2014 06:00:06 GMT

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.

keywords changed

erinn — Fri, 25 Jul 2014 17:30:29 GMT

keywords tbb-firefox-patch added

component changed

erinn — Fri, 25 Jul 2014 17:58:55 GMT

component changed from Firefox Patch Issues to Tor Browser

status, keywords changed; resolution set

mikeperry — Tue, 05 Aug 2014 03:49:47 GMT

status changed from needs_review to closed
keywords MikePerry201406R removed
resolution set to fixed

Hrmm. I am going to call this closed. We can decide how we want to handle the Private Browsing Mode isolation when we review our patches for upstreaming after the FF31 rebase.

michael — Tue, 06 Jan 2015 21:10:24 GMT

The most recent patch improves on this solution by implementing the idea (from TBB meetings) to disable turds conditionally according to privacy preference.

status, keywords changed; resolution deleted

michael — Tue, 06 Jan 2015 21:11:47 GMT

status changed from closed to reopened
keywords TorBrowserTeam201501 added
resolution fixed deleted

keywords, status changed

michael — Tue, 06 Jan 2015 21:12:24 GMT

keywords TorBrowserTeam201501R added; TorBrowserTeam201501 removed
status changed from reopened to needs_review

gk — Wed, 07 Jan 2015 08:35:57 GMT

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.

michael — Mon, 12 Jan 2015 19:44:39 GMT

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:

1) How hard it would be to implement the channel driven PBM approach. 2) 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?

keywords, status changed

gk — Tue, 13 Jan 2015 09:39:50 GMT

keywords TorBrowserTeam201501 added; GeorgKoppen201405 TorBrowserTeam201501R removed
status changed from needs_review to needs_revision

michael — Thu, 15 Jan 2015 01:32:14 GMT

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?

status changed

michael — Thu, 15 Jan 2015 01:33:08 GMT

status changed from needs_revision to needs_information

keywords, status changed

michael — Thu, 15 Jan 2015 15:18:29 GMT

keywords TorBrowserTeam201501R added; TorBrowserTeam201501 removed
status changed from needs_information to needs_review

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 ¹ while calling into the old (not overloaded) vtable. Or possibly NS_ERROR_OBJECT_IS_IMMUTABLE is returned from dependent code.

¹ 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?

mcs — Thu, 15 Jan 2015 15:39:43 GMT

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

michael — Thu, 15 Jan 2015 20:29:02 GMT

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?

mcs — Thu, 15 Jan 2015 22:06:15 GMT

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.

cc changed

mcs — Fri, 16 Jan 2015 15:38:46 GMT

cc mcs brade added

mcs — Fri, 16 Jan 2015 16:10:44 GMT

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.

michael — Fri, 16 Jan 2015 18:42:11 GMT

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.

keywords, status changed; resolution set

mcs — Mon, 19 Jan 2015 21:15:56 GMT

keywords TorBrowserTeam201501 added; TorBrowserTeam201501R removed
status changed from needs_review to closed
resolution set to fixed

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.

mcs — Mon, 19 Jan 2015 21:49:24 GMT

We committed a follow up fix after a standalone test build failed on Mac OS:

https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-31.4.0esr-4.5-1&id=a78a094886091a90e5f4e13c0579f9ad730ea8b5

We did not attempt a Gitian-based build; hopefully all is well.

michael — Tue, 20 Jan 2015 00:42:46 GMT

A corresponding report with hopes of merging to ESR38 is on Bugzilla.

attachment set

michael — Tue, 20 Jan 2015 11:24:53 GMT

attachment set to msvb9701-283f7c6.patch

Correction of overrun preexisting patch block including nsTransferable header change.

michael — Tue, 20 Jan 2015 11:33:30 GMT

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.

michael — Thu, 22 Jan 2015 11:57:12 GMT

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.