Opened 7 months ago

Closed 3 months ago

Last modified 4 weeks ago

#21830 closed defect (fixed)

Copying large text from web console leaks to /tmp

Reported by: gk Owned by: neillm
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-disk-leak, TorBrowserTeam201708R, tbb-backported
Cc: arthuredelstein, brade, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

A user reported using the webconsole copying a large section of de-obfuscated html with torbrowser 6.5.1 resulted in those contents being available in /tmp.

Child Tickets

Change History (17)

comment:1 Changed 7 months ago by gk

Status: newneeds_information

On which operating system is that happening?

Last edited 7 months ago by gk (previous) (diff)

comment:2 Changed 7 months ago by gk

Status: needs_informationassigned

And ideally it would be nice to have steps to reproduce.

comment:3 Changed 7 months ago by cypherpunks

linux
try a google image search and have javascript enabled, keep hitting page down for a while, then go to tools -> web developer -> inspector, go to the opening html element and then right click and select 'copy inner html', then check to see for the turds.
and in my opinion, with the same end result, this is the same as bug #9701 and it felt pointless to rewrite it.

comment:4 Changed 4 months ago by cypherpunks

have you heard the good news? this still occurs in tbb 7

comment:5 Changed 3 months ago by arthuredelstein

Cc: arthuredelstein added

comment:6 Changed 3 months ago by mcs

Cc: brade mcs added

comment:7 Changed 3 months ago by neillm

Owner: changed from tbb-team to neillm

comment:8 Changed 3 months ago by neillm

After inspecting this issue for some time (very easily reproducible by the original reporter's steps), as well as examining the previous solution (#9701) and finding the shortcomings, I propose that this issue can be addressed as follows:

diff --git a/widget/nsTransferable.cpp b/widget/nsTransferable.cpp
index e99d454..976180c 100644
--- a/widget/nsTransferable.cpp
+++ b/widget/nsTransferable.cpp
@@ -39,6 +39,7 @@ Notes to self:
 #include "nsIFile.h"
 #include "nsILoadContext.h"
 #include "mozilla/UniquePtr.h"
+#include "mozilla/Preferences.h"
 
 NS_IMPL_ISUPPORTS(nsTransferable, nsITransferable)
 
@@ -248,6 +249,11 @@ nsTransferable::Init(nsILoadContext* aContext)
 
   if (aContext) {
     mPrivateData = aContext->UsePrivateBrowsing();
+  } else {
+    // without aContext here to provide PrivateBrowsing information,
+    // we defer to the active configured setting
+    mPrivateData =
+      mozilla::Preferences::GetBool("browser.privatebrowsing.autostart");
   }
 #ifdef DEBUG
   mInitialized = true;

There are a number of times a transferable is initialized without a context that it can use to determine if we're in private browsing mode or not (which dictates the value of mPrivateData as seen above). Rather than assuming that we are not by default (the security leak reported), I think the TorBrowser should go with the configured 'privatebrowsing' default (which is on by default, though it can be disabled in the preferences, which resorts to the current/unpatched behavior).

This patch has been applied to tor-browser-52.2.0esr-7.0-1-build1 and tested on Ubuntu 16.04.2 LTS.

comment:9 Changed 3 months ago by gk

Status: assignedneeds_information

Thanks. Did you build a complete new Tor Browser bundle for that (if so, could you make it available for us to test your patch) or just the browser part copying the result over in an already existing Tor Browser? Or did you something else?

comment:10 in reply to:  9 Changed 3 months ago by neillm

Replying to gk:

Thanks. Did you build a complete new Tor Browser bundle for that (if so, could you make it available for us to test your patch) or just the browser part copying the result over in an already existing Tor Browser? Or did you something else?

In the interest of time, I built the gitian-builder/inputs/tor-browser source tree alone for testing and configured the SOCKS5 proxy to localhost:9050, as I ran into unrelated issues doing the tor-browser-bundle/gitian 'make' route.

UPDATE: The tag in the original post should have been tor-browser-52.2.0esr-7.5-1, which may have caused confusion.

Last edited 3 months ago by neillm (previous) (diff)

comment:11 Changed 3 months ago by gk

Keywords: TorBrowserTeam201707R added
Status: needs_informationneeds_review

comment:12 in reply to:  8 ; Changed 3 months ago by arthuredelstein

Replying to neillm:

This patch has been applied to tor-browser-52.2.0esr-7.0-1-build1 and tested on Ubuntu 16.04.2 LTS.

Thanks -- I built with the patch and it worked as described. But the aContext argument of nsTransferable::Init() appears to have only one purpose, which is to check for PBM state. So I wonder if you think it would make sense to change the signature to nsTransferable::Init(bool isPrivateBrowsingMode)? Then perhaps the callers could be modified to provide PBM state, assuming they have that information:

https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsTransferable%3A%3AInit%28nsILoadContext+%2A%29%22

(I'm not sure if this is a practical idea or not, so feel free to disagree.)

comment:13 in reply to:  12 Changed 3 months ago by neillm

Replying to arthuredelstein:

Replying to neillm:

This patch has been applied to tor-browser-52.2.0esr-7.0-1-build1 and tested on Ubuntu 16.04.2 LTS.

Thanks -- I built with the patch and it worked as described. But the aContext argument of nsTransferable::Init() appears to have only one purpose, which is to check for PBM state. So I wonder if you think it would make sense to change the signature to nsTransferable::Init(bool isPrivateBrowsingMode)? Then perhaps the callers could be modified to provide PBM state, assuming they have that information:

https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsTransferable%3A%3AInit%28nsILoadContext+%2A%29%22

(I'm not sure if this is a practical idea or not, so feel free to disagree.)

It's a good idea, but I think practically speaking, it would be much more complicated. The reason I say that is because all of those times where the nsTransferable is initialized without a context, it's because we (likely) don't actually know at that point if it's a private browsing mode or not. After all, if we did, we could have loaded it with the context to begin with (although perhaps there are some lazy cases where it could be used and isn't).

So for those remaining cases (without the context), while we may be able to load the preference default as this patch does, we would have to do it in a lot more places before we know what boolean to pass in to the Init (if modified). Does that make sense?

Other than that, you're right, it appears that it's only used for that reason at this point. Assuming there are no other future uses of that context by the transferable, refactoring could work.

comment:14 Changed 3 months ago by gk

Keywords: TorBrowserTeam201708R added; TorBrowserTeam201707R removed

Moving review tickets to August.

comment:15 Changed 3 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Okay, I took the patch for the alpha (commit 06580161f901dedf60e6dcb9252e7ce6b3e7b37b on tor-browser-52.2.0esr-7.5-1). I agree, when we want to upstream the patch we should try the road Arthur outlined in comment:12.

comment:16 Changed 7 weeks ago by gk

Keywords: tbb-backport added

comment:17 Changed 4 weeks ago by gk

Keywords: tbb-backported added; tbb-backport removed

Taking this for the stable (7.0.6). Cherry-picked to tor-browser-52.3.0esr-7.0-1 (commit 7a1b25245e73051cb724a7eb6a66de18263f88c2).

Note: See TracTickets for help on using tickets.