Closed Bug 1123480 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Widget, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: mozilla, Assigned: mozilla)

References

Details

Attachments

(1 file, 4 obsolete files)

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
Status: UNCONFIRMED → NEW
Component: Untriaged → Widget
Ever confirmed: true
OS: Linux → All
Product: Firefox → Core
Hardware: x86_64 → All
Attachment #8551536 - Flags: review?(vladimir)
Any idea who we should get to review this?
Flags: needinfo?(sstamm)
Flags: needinfo?(hkoka)
Ehsan can maybe review?
Flags: needinfo?(sstamm) → needinfo?(ehsan)
Looks like ehsan and roc have reviewed changes in that file recently. Try one of them.

Thanks
Hema
Flags: needinfo?(hkoka)
Yes, I will take a look at this tomorrow.
Attachment #8551536 - Flags: review?(vladimir) → review?(ehsan)
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.)
Attachment #8551536 - Flags: review?(ehsan) → feedback+
Flags: needinfo?(ehsan)
> 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.
Attachment #8551536 - Attachment is obsolete: true
(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/michael@schloh.com-e19c59f79814/

Can these nsTransferable corrections and tests please be merged in ESR38 as well as central?
Attachment #8558858 - Flags: review?(ehsan)
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
Attachment #8558858 - Attachment is obsolete: true
Attachment #8558858 - Flags: review?(ehsan)
Attachment #8559203 - Flags: review?(josh)
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.
Attachment #8559203 - Flags: review?(josh) → review+
Unification of runtime logic and unit test patches integrating review requests.
Attachment #8565974 - Flags: checkin+
There's a try run in comment 9.
Keywords: checkin-needed
Attachment #8565974 - Flags: checkin+
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
Attachment #8553038 - Attachment is obsolete: true
Attachment #8559203 - Attachment is obsolete: true
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
Assignee: nobody → mozilla
https://hg.mozilla.org/mozilla-central/rev/55e4ab92ff94
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
See Also: → 1396224
You need to log in before you can comment on or make changes to this bug.