1433030 - Copying large text from web console leaks to /tmp

Status: REOPENED

(Core :: Widget, enhancement, P3)

Description

[Arthur Edelstein [:arthur]]

Please see the original Tor ticket:
https://trac.torproject.org/21830

Currently we are using the following patch,
https://torpat.ch/21830
but in Firefox it would probably be better to use Private Browsing state rather than the "browser.privatebrowsing.autostart" pref.

Comment 1

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1433030-Stop-nsTransferable-writing-to-tmp-in-PB.patch

Here's an approach that simply avoids writing to /tmp if we aren't sure we're in a private browsing context.

Comment 2

[Arthur Edelstein [:arthur]]

try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d0b50b91e7d649aabb9f16180dce85739ed2c1b

Comment 3

[Josh Matthews [:jdm]]

Comment on attachment 8950121 [details] [diff] [review]
0001-Bug-1433030-Stop-nsTransferable-writing-to-tmp-in-PB.patch

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

::: widget/tests/test_bug1123480.xul
@@ +37,5 @@
>      const gClipboardHelper = Components.classes["@mozilla.org/widget/clipboardhelper;1"].getService(Components.interfaces.nsIClipboardHelper);
>      gClipboardHelper.copyString(Ipsum);
>  
> +    // Undefined private browsing mode should cache large selections to disk
> +    ok(!clipboardFile.exists(), "correctly saved memory by caching to disk");

The comment and assertion message don't match what we're testing.

Comment 4

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1433030-Stop-nsTransferable-writing-to-tmp-in-PB.patch

(In reply to Josh Matthews [:jdm] from comment #3)
> Comment on attachment 8950121 [details] [diff] [review]
> 0001-Bug-1433030-Stop-nsTransferable-writing-to-tmp-in-PB.patch
> [snip]
> The comment and assertion message don't match what we're testing.

Thanks for the review. Fixed the comment and assertion message.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e19b58d6494
Stop nsTransferable writing to /tmp in PB windows r=jdm

Comment 6

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/9e19b58d6494

Comment 7

[Rob Wu [:robwu]]

Can this patch be backed out? It causes a regression: now the clipboard is never cached to the disk.

To test (on Linux, but the problem is probably platform-independent):
1. Create a big file: python -c 'print("x x "*500000)' > /tmp/big.txt
2. Open the following test case:
firefox-nightly /tmp/big 'data:text/html,<textarea style="width:90vw;height:90vh">'
3. In the first tab (with the repeated "x x"): Select all, copy text (Ctrl-A, Ctrl-C).
4. Go the second tab (input field): Ctrl-V.
   (This works as expected: The text is pasted.)
5. Ctrl-Z to empty the input field.
6. Remove /tmp/clipboardcache*
7. Ctrl-V again

Expected (Nightly 60.0a1 2018-01-26): Nothing should be pasted because the file that backs the clipboard is removed.
Actual   (Nightly 60.0a1 2018-02-24): The data is pasted (indicating that the in-memory data is used).

I discovered this issue when I was reviving my patches for bug 335545.

This regression might be related to the nsITransferable initialization + IPC issue that I pointed to at the bottom of bug 1396224.

Arthur/TOR, you might want to follow bug 1396224, which describes the bug that you are trying to solve here (plus more details about the cause of the bug).

Comment 8

[Rob Wu [:robwu]]

Now the patches for bug 335545 have landed, the patch cannot be backed out by a revert (because the unit test has been modified).

Given the lack of reply from the patch author, I'll direct the question from comment 7 to the reviewer:
Josh, can/should the mPrivateData flag default to false again until bug 1396224 is fixed?

Comment 9

[Arthur Edelstein [:arthur]]

Sorry I didn't reply earlier! I'd be OK with backing this out while we work out the issue.

Comment 10

[Rob Wu [:robwu]]

Thanks for the answer Arthur. I will submit a patch to flip the boolean again.

Do you want to keep this bug open in order to flip the default again in the future, or should this be marked as a duplicate of bug 1396224 ?

Comment 11

[Arthur Edelstein [:arthur]]

I'd be inclined to keep this bug open for continuity, since I imagine the final patch will be similar to the one I introduced before.

Comment 12

[Rob Wu [:robwu]]

Alright. Anticipating the revert, I have re-opened the bug, assigned it to you and added the leave-open label so that the bug is not automatically closed when my revert lands.

Comment 13

[Rob Wu [:robwu]]

Attachment: Bug 1433030 - Initialize nsTransferable::mPrivateData with false

Initialize mPrivateData to false by default again, because the
change that changed the default value from true to false, i.e.
https://hg.mozilla.org/mozilla-central/rev/9e19b58d6494
prevented the clipboard from being backed by the filesystem,
as shown at https://bugzilla.mozilla.org/show_bug.cgi?id=1433030#c7

Review commit: https://reviewboard.mozilla.org/r/225718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225718/

Comment 14

[Josh Matthews [:jdm]]

Comment on attachment 8956753 [details]
Bug 1433030 - Initialize nsTransferable::mPrivateData with false

https://reviewboard.mozilla.org/r/225718/#review231672

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/87a34b1b1ad7
Initialize nsTransferable::mPrivateData with false r=jdm

Comment 16

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/87a34b1b1ad7

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jimm, maybe it's time to close this bug?

Comment 19

[Alex Catarineu (Tor Browser dev)]

I could not reproduce the /tmp leaks in PBM with current Firefox versions (>=67), but still there are several places initializing nsTransferables with null context, so the possibility of the /tmp leaks might still open. Do you think it would be fine to set mPrivateData again to true (reverting the revert) now, or are the regressions still an issue?

Comment 20

[Rob Wu [:robwu]]

Regressions are still an issue. We're already avoiding the clipboard cache in content processes because of https://hg.mozilla.org/mozilla-central/rev/7a7f203680a8, but the parent process is still creating clipboard files. The proper way to fix this issue is described in bug 1396224. And when that bug is fixed, changing the default (whether true or false) shouldn't matter any more. I'll take a quick look at that bug, since it seems that nobody else would work on it otherwise.

How are you testing that there are no /tmp leaks? Did you watch the creation and removing of file (descriptors) in /tmp?
Just checking whether there are files in /tmp is not a complete test, because the file is created, and immediately deleted after getting a file handle, starting from the fix to bug 335545. Although you won't see files, data can still be written to the disk, which is a privacy risk for Tor.

Here is an example when I put text on the PRIMARY clipboard (i.e. follow comment 7, but put data on the clipboard by selecting the text).

$ inotifywait -m /tmp/
Setting up watches.
Watches established.
/tmp/ CREATE mozilla-temp-1322242136
/tmp/ OPEN mozilla-temp-1322242136
/tmp/ CLOSE_WRITE,CLOSE mozilla-temp-1322242136
/tmp/ OPEN mozilla-temp-1322242136
/tmp/ DELETE mozilla-temp-1322242136
/tmp/ MODIFY mozilla-temp-1322242136
/tmp/ CREATE mozilla-temp-430423343
/tmp/ OPEN mozilla-temp-430423343
/tmp/ CLOSE_WRITE,CLOSE mozilla-temp-430423343
/tmp/ OPEN mozilla-temp-430423343
/tmp/ DELETE mozilla-temp-430423343
/tmp/ MODIFY mozilla-temp-430423343

(when I release the clipboard)
/tmp/ CLOSE_WRITE,CLOSE mozilla-temp-1322242136
/tmp/ CLOSE_WRITE,CLOSE mozilla-temp-430423343

Comment 21

[Alex Catarineu (Tor Browser dev)]

True, I did not check with inotify, so then the change I saw w.r.t. to previous versions was that now temp files are deleted and kept open. Thanks for taking a look at bug 335545, that one should allow us to drop this patch for Tor Browser.

Comment 22

[Alex Catarineu (Tor Browser dev)]

Sorry, I meant bug 1396224 :)

Comment 23

[Alex Catarineu (Tor Browser dev)]

I can still reproduce this after bug 1396224 was fixed, so I think this should not be a duplicate of that one.

An easy way to reproduce is to do the same steps as in bug 1396224 to create and open the big file, then open Web Console, select the <html> element and copy the internal HTML.

Comment 24

[Rob Wu [:robwu]]

The issue from comment 23 happens because the string is copied without considering the context of the invocation, and consequently the (default) non-private context is used to copy the string: https://searchfox.org/mozilla-central/rev/da855d65d1fbdd714190cab2c46130f7422f3699/widget/nsClipboardHelper.cpp#71

Which is called via copyLongHTMLString in devtools/client/inspector/markup/markup.js
(there are many other callers of .copyString( and a few callers of .copyStringToClipboard( with the same problem.)

An easy way to fix this leakage is by calling trans->SetIsPrivateData(true); in nsClipboardHelper.cpp after trans->Init(nullptr). The upside of this is that this bug is fixed; the downside is that long strings that are copied with this helper are fully kept in memory (instead of cached to the disk) until the item is removed from the clipboard.

A more involved way to fix this is to somehow pass the privatebrowsing state to the clipboard helper function.

Comment 25

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months and this bug has priority 'P2'.
:spohl, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 26

[Stephen A Pohl [:spohl]]

Rob, could you clarify if there is still a need to keep this bug open today?

Comment 27

[Rob Wu [:robwu]]

Assuming that Tor doesn't want to persist any unnecessary/private state to disk, there is still a bug.
See comment 20 for verification steps using inotifywait,
and see comment 24 for Searchfox search links to affected code (e.g. the copy link to clipboard functionality at https://searchfox.org/mozilla-central/rev/2c4522f76b23b024d9f29e140d41042853874c07/browser/base/content/nsContextMenu.js#1964-1967 ).

The referenced code in nsClipboardHelper.cpp in comment 24 has been updated by bug 1730194 to include a conditional call to trans->SetIsPrivateData(true);, which partly matches with what I suggested there. Before resolving this bug, that condition should be expanded to ensure that no clipboard data is persisted to disk on Tor (again, this assumes that a goal of Tor is to avoid writing such data to disk).