874349 - caching of instantiated webfonts should not leak out of private browsing context

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jonathan Kew [:jfkthame]]

In bug 816483, we added a global cache of platform font objects instantiated from @font-face downloads, particularly to improve the user experience when the same fonts are used on successive pages. This avoids the need to re-fetch data (either from the network or from cache) when loading the new page, and to instantiate a new platform font, which may be a fairly expensive operation.

However, this represents a possible privacy leak, as pages might be able to determine (e.g. by a timing attack) whether particular fonts have already been loaded by the browser, even if that load occurred in a private-browsing window.

To solve this, we could include the WindowID of the window for which the font was loaded (if it is a Private Browsing window) as part of the key used for the cache entry. Then fonts loaded by a PB window will still be cached for use in new pages loaded within that same window, but any other window that calls for the same fonts will need to re-load them from the network and instantiate (and cache) its own copies, so it won't gain any clues about what had been loaded in the PB window.

ehsan/jdm: is the WindowID an appropriate identifier to use here to determine whether the cached fonts may be re-used for a new load, or do we have some other kind of "private-browsing context token" that should be used?

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: include window ID of private-browsing window in webfont cache key

Something like this, maybe?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 752056 [details] [diff] [review]
include window ID of private-browsing window in webfont cache key

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

Code looks fine. I don't know if the WindowID is the right thing to use here though.

Comment 3

[(no longer active)]

Comment on attachment 752056 [details] [diff] [review]
include window ID of private-browsing window in webfont cache key

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

Hmm, why would we use the window ID?  It's OK to cache fonts coming from a private tab to be used in another private tab, whether in the same or a different window.  That is how the memory cache works in private mode, for example.  So I think it's OK to just use a boolean flag as part of the cache key.

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #3)
> Comment on attachment 752056 [details] [diff] [review]
> include window ID of private-browsing window in webfont cache key
> 
> Review of attachment 752056 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm, why would we use the window ID?  It's OK to cache fonts coming from a
> private tab to be used in another private tab, whether in the same or a
> different window.  That is how the memory cache works in private mode, for
> example.  So I think it's OK to just use a boolean flag as part of the cache
> key.

Hmm. Ok, if that's how private-browsing stuff works.... I don't really know anything about it, but it seemed to me like you wouldn't want "leakage" between private-browsing windows, either - so if I open two separate PB windows, the site(s) I load in one of them shouldn't be able to detect anything about my activity in the other.

But if that's not a goal, then sure, we can just use a boolean flag and not bother with the ID.

Comment 5

[(no longer active)]

(In reply to comment #4)
> Hmm. Ok, if that's how private-browsing stuff works.... I don't really know
> anything about it, but it seemed to me like you wouldn't want "leakage" between
> private-browsing windows, either - so if I open two separate PB windows, the
> site(s) I load in one of them shouldn't be able to detect anything about my
> activity in the other.
> 
> But if that's not a goal, then sure, we can just use a boolean flag and not
> bother with the ID.

FWIW, our goal is to make it hard for the owner of a web site to be able to tell whether or not you're in private browsing mode, and such an attack would be possible with this bug if you've visited the website in regular mode before, and your fix is right on, but since we don't make any attempt to separate private windows elsewhere, it wouldn't make sense to do it here!  :-)

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: cached webfonts that were loaded in private browsing mode should not leak out to normal windows

Here's a version that only checks whether we're in PB mode or not, but doesn't distinguish between multiple PB windows. Also listens for the last-pb-context-exited notification and discards all private-mode entries, so they won't still be hanging around if a new PB window is opened. (Patch written on top of bug 862222, which will land first.)

Comment 7

[(no longer active)]

Comment on attachment 752496 [details] [diff] [review]
cached webfonts that were loaded in private browsing mode should not leak out to normal windows

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

Great!

::: gfx/thebes/gfxUserFontSet.cpp
@@ +488,5 @@
>                  fe->mFeatureSettings.AppendElements(aProxyEntry->mFeatureSettings);
>                  fe->mLanguageOverride = aProxyEntry->mLanguageOverride;
> +                // for src:local() fonts, we don't care about private-browsing mode
> +                // as there's no issue of caching resources; they're just available
> +                // all the time

Same nit.

::: gfx/thebes/gfxUserFontSet.h
@@ +61,5 @@
>      nsString          mRealName;  // original fullname from the font resource
>      uint32_t          mSrcIndex;  // index in the rule's source list
>      uint32_t          mFormat;    // format hint for the source used, if any
>      uint32_t          mMetaOrigLen; // length needed to decompress metadata
> +    bool              mPrivate;   // whether font belongs to private-browsing mode

Nit: "a private window"

@@ +255,5 @@
>  
>          // Return the gfxFontEntry corresponding to a given URI and principal,
> +        // and the features of the given proxy, or nullptr if none is available.
> +        // The aPrivate flag is set for private-browsing requests, so we can
> +        // avoid leaking fonts cached in PB mode out to normal windows.

Nit: s/private-browsing requests/requests coming from private windows/
     s/PB mode/private windows/

@@ +356,5 @@
>              // The font entry MUST notify the cache when it is destroyed
>              // (by calling Forget()).
>              gfxFontEntry          *mFontEntry;
> +
> +            // Whether this font was loaded from private-browsing mode.

Same nit!

::: layout/style/nsFontFaceLoader.h
@@ +82,5 @@
>                                      const gfxFontFaceSrc *aFontFaceSrc,
>                                      uint8_t* &aBuffer,
>                                      uint32_t &aBufferLength);
>  
> +  virtual bool GetPrivateBrowsing();

MOZ_OVERRIDE.

Comment 8

[Jonathan Kew [:jfkthame]]

Pushed to inbound, with nits fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56f0f5a122ae

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/56f0f5a122ae