Copying large text from web console leaks to /tmp

REOPENED
Assigned to

Status

()

enhancement
P2
normal
REOPENED
2 years ago
3 days ago

People

(Reporter: arthur, Assigned: arthur)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tor 21830])

Attachments

(2 attachments, 1 obsolete attachment)

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.
Component: Developer Tools: Console → Widget
Product: Firefox → Core
Here's an approach that simply avoids writing to /tmp if we aren't sure we're in a private browsing context.
Attachment #8950121 - Flags: review?(josh)
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.
Attachment #8950121 - Flags: review?(josh) → review+
(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.
Attachment #8950121 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e19b58d6494
Stop nsTransferable writing to /tmp in PB windows r=jdm
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9e19b58d6494
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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).
Flags: needinfo?(arthuredelstein)
Blocks: 335545
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?
Flags: needinfo?(josh)
Sorry I didn't reply earlier! I'd be OK with backing this out while we work out the issue.
Flags: needinfo?(arthuredelstein)
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 ?
Flags: needinfo?(josh) → needinfo?(arthuredelstein)
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.
Flags: needinfo?(arthuredelstein)
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.
Assignee: nobody → arthuredelstein
Status: RESOLVED → REOPENED
Depends on: 1396224
Keywords: leave-open
Resolution: FIXED → ---
Target Milestone: mozilla60 → ---
Priority: -- → P1
Comment on attachment 8956753 [details]
Bug 1433030 - Initialize nsTransferable::mPrivateData with false

https://reviewboard.mozilla.org/r/225718/#review231672
Attachment #8956753 - Flags: review?(josh) → review+
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/87a34b1b1ad7
Initialize nsTransferable::mPrivateData with false r=jdm
Priority: P1 → P2

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

Flags: needinfo?(jmathies)
Status: REOPENED → RESOLVED
Closed: Last year6 months ago
Flags: needinfo?(jmathies)
Keywords: leave-open
Resolution: --- → DUPLICATE
Duplicate of bug: 1396224

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?

Flags: needinfo?(rob)

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
Flags: needinfo?(rob)

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.

Sorry, I meant bug 1396224 :)

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.

Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---

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.

You need to log in before you can comment on or make changes to this bug.