870179 - Privacy is leaked by BackgroundPageThumbs thumbnails that are captured while a private-browsing session is active

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Drew Willcoxon :adw]

If you open a private window, log in to a site, and then use BackgroundPageThumbs to capture that site, your logged-in page is shown in the thumbnail.  The thumbnail is not removed when you close the private window.

The problem is that BackgroundPageThumbs uses a private-browsing docshell, and there's no concept of multiple, concurrent PB sessions.  The temporary pool of caches and storage used in the PB window is the same as the one used by BackgroundPageThumbs.

An easy thing to do would be to ignore all captures while a PB session is active.  So what if PB sessions don't produce thumbnails, which would necessarily be temporary anyway?  It doesn't look like the new-tab page is used for PB windows in desktop Firefox.  I don't know if thumbnails are currently used anywhere else.

Ideally BackgroundPageThumbs could use a separate temporary pool, but that's probably a tall order since it's not how PB mode is designed.  Every bit of state that's impacted by private browsing would need to be updated to recognize the possibility of multiple, concurrent PB sessions.

Comment 1

[Josh Matthews [:jdm]]

Avoiding capturing thumbnails for sites in private windows seems totally reasonable to me, but you'll want to capture them if permanent private browsing mode is activated.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Hrm, seems obvious in hindsight that this would be a problem, but I hadn't thought about this at all.

Not capturing thumbnails when there is a private session active seems like perhaps a tolerable workaround in the short term, but it really handicaps the ability to rely more on BackgroundPageThumbs for thumbnailing in the future.

I don't really understand comment 1 - this problem can be triggered by attempting to thumbnail pages that are not in private windows (e.g. if a site is open in both private and non-private windows). I don't think there's an easy way to know whether a given site is _also_ loaded in a private browsing window, and any such solution seems like it would be somewhat fragile. Additionally, I don't think we ever want to capture thumbnails at all in permanent private browsing mode.

It seems like maybe we could address this by using some form of LOAD_ANONYMOUS, as we originally discussed in bug 841495, in addition to the private browsing (to avoid persistence of data).

(Random only vaguely related thought: how does the background thumbnail service deal with pages that trigger prompts, either through e.g. alert() or for HTTP auth, etc?)

Comment 3

[Drew Willcoxon :adw]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #2)
> (Random only vaguely related thought: how does the background thumbnail
> service deal with pages that trigger prompts, either through e.g. alert() or
> for HTTP auth, etc?)

The first, non-remote version set nsIDocShell.allowAuth, but I forgot to set it when I moved to a remote browser.  I also forgot to set allowPlugins.  Whoops.

I tried capturing a page that shows an alert.  The capture completes successfully, but it causes this error:

Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWindowWatcher.openWindow]
Source File: file:///Users/adw/mi/obj-debug/dist/NightlyDebug.app/Contents/MacOS/components/nsPrompter.js
Line: 379

If that's because the browser is remote, off-screen, or what, I don't know.

I also tried capturing a page requiring authorization.  Setting allowAuth = false on the remote docshell in the content script didn't stop an auth dialog from appearing out of nowhere.  When I closed it, below it was a tiny, little, chromeless window containing chrome://global/content/mozilla.xhtml, which is used to host the thumbnail browser.

Comment 4

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #3)
> I also tried capturing a page requiring authorization.  Setting allowAuth =
> false on the remote docshell in the content script didn't stop an auth
> dialog from appearing out of nowhere.

Nor did hardcoding docShell->SetAllowAuth(false) in TabChild, the thought being that setting it right after the docshell's created might make a difference.  Guess allowAuth doesn't do what its name and idl comment make it sound like it might?

Comment 5

[Drew Willcoxon :adw]

Filed bug 875157 for allowAuth.

Comment 6

[Drew Willcoxon :adw]

Attachment: wallpaper patch

This is a simple wallpaper patch that Gavin and I talked about.  It ignores captures made while any private windows are open.  Ultimately it's not the right fix, but we think it's OK for now while we figure out the right fix.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 753485 [details] [diff] [review]
wallpaper patch

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

::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm
@@ +45,5 @@
> +      // logged in in the thumbnail browser.  A crude way to avoid capturing
> +      // sites in this situation is to refuse to capture at all when any private
> +      // windows are open.  See bug 870179.
> +      if (options.onDone)
> +        Services.tm.mainThread.dispatch(options.onDone.bind(options, url), 0);

Is that more readable? Don't know, up to you :)

Services.tm.mainThread.dispatch(() => options.onDone(url), 0);

@@ +312,5 @@
> +/**
> + * Returns true if there are any private windows.
> + */
> +function isPrivateBrowsingActive() {
> +  let wins = Services.ww.getWindowEnumerator();

I think we can restrict the search to browser windows:

let wins = Services.wm.getEnumerator("navigator:browser");

Comment 8

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6c5b1cc1f59

I'm adding [leave open] rather than filing a new bug with basically the same content to find the right solution, but if anyone disagrees feel free close and file.

(In reply to Tim Taubert [:ttaubert] from comment #7)
> Is that more readable? Don't know, up to you :)

No. :-)

> I think we can restrict the search to browser windows:

Not sure if you saw my comment in #fx-team:

> re: restricting search to navigator:browser windows, i had the same
> thought initially, but maybe in the future there will be other types
> of private windows. filtering on navigator:browser isn't any less
> expensive anyway: it's O(number of windows) either way.

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f6c5b1cc1f59

Comment 10

[:Gavin Sharp [email: [e-mail]]]

I think we should file a new bug - tracking multiple landings in one leaves a quite confusing bug history.

Comment 11

[Drew Willcoxon :adw]

Filed bug 875986.