1123480 - Prevent disk caching data leak with nsTransferable selections in private browsing mode

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[MSvB]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 2014112600

Steps to reproduce:

Private browsing mode on, ​navigate a large doc, select Ctrl-A, ls $TMPDIR/clipboardcache*
Private browsing mode off, ​navigage a large doc, select Ctrl-A, ls $TMPDIR/clipboardcache*

The book http://www.gutenberg.org/cache/epub/345/pg345.txt is a suitable test document.


Actual results:

Regardless of private browsing mode (PBM), files named:

clipboardcache
clipboardcache-1
clipboardcache-n

...are written to disk, violating PBM's principle of  disk avoidance.


Expected results:

When private browsing mode is active, nsTransferable logic should not cache data to disk.

The problem has been discovered and solved by Tor Browser developers in:

https://bugs.torproject.org/9701
https://bugs.torproject.org/14255

Comment 1

[MSvB]

Attachment: Proposed solution to data leak in private browsing mode nsTransferable.

Comment 2

[Dave Huseby [:huseby]]

Any idea who we should get to review this?

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Ehsan can maybe review?

Comment 4

[Hema Koka [:hema]]

Looks like ehsan and roc have reviewed changes in that file recently. Try one of them.

Thanks
Hema

Comment 5

[(no longer active)]

Yes, I will take a look at this tomorrow.

Comment 6

[(no longer active)]

Comment on attachment 8551536 [details] [diff] [review]
Proposed solution to data leak in private browsing mode nsTransferable.

Review of attachment 8551536 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot for the fix, it looks great overall!  I have a couple of nits below.

Can you please write a unit test for this as well?  Something like a mochitest <https://developer.mozilla.org/en-US/docs/Mochitest> which opens up a huge page in a private window, uses synthesizeKey("A", {accelKey: true}) to select all and synthesizeKey("C", {accelKey: true}) to copy the selection and makes sure that we don't write a file in /tmp?  dom/tests/mochitest/beacon/test_beaconCookies.html has some boiler plate functions that can help you open a private window from a mochitest, which you can copy into the new test, until we find a good way to share such code across the tests.  Please let me know if you need help.

::: widget/nsTransferable.cpp
@@ +55,5 @@
>  }
>  
>  //-------------------------------------------------------------------------
>  void
> +DataStruct::SetData ( nsISupports* aData, uint32_t aDataLen, bool aIsPrivBrowsing )

Nit: please rename the variable to aIsPrivateData.  Same in the header file as well.

@@ +399,5 @@
>    // first check our intrinsic flavors to see if one has been registered.
>    for (size_t i = 0; i < mDataArray.Length(); ++i) {
>      DataStruct& data = mDataArray.ElementAt(i);
>      if ( data.GetFlavor().Equals(aFlavor) ) {
> +      data.SetData ( aData, aDataLen, mPrivateData);

Nit: please either preserve the space before ), or get rid of all of the spaces around the parenthesis (the latter is better according to our coding style, but this file violates that pretty heavily.)

Comment 7

[MSvB]

Attachment: Stylistic correction (same logic as previous patch) accommodating review requests.

> Same in the header file as well.
>
I've made the name in the header file the same as requested 'aIsPrivateData', not sure if we really want that though.

Comment 8

[MSvB]

Attachment: Unit test construction accommodating review requests.

(In reply to [Away: 1/29-2/20] from comment #6)
> Review of attachment 8551536 [details] [diff] [review]:
> -----------------------------------------------------------------
> Can you please write a unit test for this as well?  Something like a
> mochitest <https://developer.mozilla.org/en-US/docs/Mochitest> which opens
> up a huge page in a private window, uses synthesizeKey("A", {accelKey:
> true}) to select all and synthesizeKey("C", {accelKey: true}) to copy the
> selection and makes sure that we don't write a file in /tmp?
>
The unit test files are revisions:

http://hg.mozilla.org/users/michael_schloh.com/mozilla-cntrl/file/tip/widget/tests/chrome.ini
http://hg.mozilla.org/users/michael_schloh.com/mozilla-cntrl/file/tip/widget/tests/test_bug1123480.xul
http://hg.mozilla.org/users/michael_schloh.com/mozilla-cntrl/file/tip/widget/tests/bug1123480.sjs

...and a corresponding patch msvb1123480-unitest.patch is attached for review.

The synthesizeKey() methods cannot be used due to bugs in the chrome window or docshell 'unreliable' event loop, and maybe other bugs in the test harness as well. The given unit tests work around the problems by using alternative constructs like 'nsIClipboardHelper' and 'nsITransferable'.

Regardless of work arounds, the test logic accuracy is confirmed by the try server:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e19c59f79814
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-e19c59f79814/

Can these nsTransferable corrections and tests please be merged in ESR38 as well as central?

Comment 9

[MSvB]

Attachment: Correct some cross platform hard coded duh mistakes.

Improved (and hopefully finalised) unit tests according to renewed try server results from revision 4ec3c6c89e97 [1][2][3].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ec3c6c89e97
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=984285cd2a95
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e19c59f79814

Comment 10

[Josh Matthews [:jdm]]

Comment on attachment 8559203 [details] [diff] [review]
Correct some cross platform hard coded duh mistakes.

Review of attachment 8559203 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think the commit message needs to include instructions on how to apply the patch, but otherwise this test looks fine to me with some small stylistic changes.

::: widget/tests/chrome.ini
@@ +85,5 @@
>  support-files = window_mouse_scroll_win.html
> +
> +# Privacy relevant
> +[test_bug1123480.xul]
> +support-files = bug1123480.sjs

No need for this if we don't use the content any longer.

::: widget/tests/test_bug1123480.xul
@@ +21,5 @@
> +  for (var Iter = 0; Iter < SmallDataset; Iter++) {
> +      Ipsum += Math.random().toString(36) + ' ';
> +  }
> +
> +  function RunTest() {

Everything under here should be indented by one level.

@@ +24,5 @@
> +
> +  function RunTest() {
> +  // Construct a nsIFile object for access to file methods
> +  Components.utils.import("resource://gre/modules/FileUtils.jsm");
> +  var TurdFile = FileUtils.getFile("TmpD", ["clipboardcache"]);

Let's rename this to clipboardFile.

@@ +47,5 @@
> +  ok(!TurdFile.exists(), "failed to postsanitize the environment");
> +
> +  // Repeat procedure of plain text selection with private browsing enabled
> +  Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
> +  const SJS_URL = "http://example.com/chrome/widget/tests/bug1123480.sjs";

I think this can just be about:blank now.

Comment 11

[MSvB]

Attachment: msvb1123480-commit.patch

Unification of runtime logic and unit test patches integrating review requests.

Comment 12

[Josh Matthews [:jdm]]

There's a try run in comment 9.

Comment 13

[MSvB]

For good measure, a try run [1] has completed and indicated success regarding the comprehensive (program logic and unit test) patch in attachment #8565974 [details] [diff] [review] [2] msvb1123480-commit.patch.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=855cb101b4c3
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8565974

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please make sure you include your committer information in future patches when requesting checkin.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55e4ab92ff94

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55e4ab92ff94