Copying large text from web console leaks to /tmp
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.
Activity
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 (moved) and it felt pointless to rewrite it.
After inspecting this issue for some time (very easily reproducible by the original reporter's steps), as well as examining the previous solution (#9701 (moved)) 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.
Trac:
Username: 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.
Trac:
Username: neillm 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
aContextargument ofnsTransferable::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 tonsTransferable::Init(bool isPrivateBrowsingMode)? Then perhaps the callers could be modified to provide PBM state, assuming they have that information:(I'm not sure if this is a practical idea or not, so feel free to disagree.)
-
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
aContextargument ofnsTransferable::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 tonsTransferable::Init(bool isPrivateBrowsingMode)? Then perhaps the callers could be modified to provide PBM state, assuming they have that information:(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.
Trac:
Username: neillm
