962365 - Isolate/double-key the image cache to first party URI

Status: RESOLVED DUPLICATE of bug 1270680

(Core :: Graphics: ImageLib, defect, P2)

Description

[Mike Perry]

Attachment: Double-key the image cache (WIP)

As part of the third party tracking protections in Tor Browser, we patched the image cache to support double-keying, to allow it to be isolated to the url bar domain.

We did this prior to the multi-window private browsing support. Not sure if the approach used to isolate the image cache for PBM can be extended to all first parties (with an additional PBM vs non-PBM flag), or if our more invasive approach is still the way to go.

The patch is against FF24ESR and has been deployed in Tor Browser for a while now.

Comment 1

[Arthur Edelstein [:arthur]]

Here's a link that tracks the latest version of the Tor Browser patch:
https://torpat.ch/6539
and here are the regression tests:
https://torpat.ch/13749

Comment 2

[Dave Huseby [:huseby]]

Attachment: adding origin attributes to ImageCacheKey

This patch adds origin attributes to the ImageCacheKey class.  Now I just need to fix up the callers.

Comment 3

[Dave Huseby [:huseby]]

Attachment: mostly done image cache isolation

this is mostly done.  i just need to figure out how to get the origin attributes in one specific place...right where it fails to compile.  i'm going to need some review on the principal i chose to use in another place.  i marked it with //XXX(huseby).

Comment 4

[Dave Huseby [:huseby]]

Attachment: image cache isolation, compiles and probably works.

this patch probably works.

Comment 5

[Dave Huseby [:huseby]]

Attachment: image cache isolation

compiles and doesn't crash.  this ensures that all calls to LoadImage have a principal associated with the load so that we can isolate image caching by origin attributes.

Comment 6

[Dave Huseby [:huseby]]

Attachment: image isolation with origin attributes

Arthur,

This is my first pass at origin attribute based image cache isolation.  This should demonstrate how origin attributes can be used for this kind of thing.  ATM, the unit tests are failing but that's probably a test bug.  Let me know what you think.

--dave

Comment 7

[Arthur Edelstein [:arthur]]

(In reply to Dave Huseby [:huseby] from comment #6)
> Created attachment 8739908 [details] [diff] [review]
> image isolation with origin attributes
> 
> Arthur,
> 
> This is my first pass at origin attribute based image cache isolation.  This
> should demonstrate how origin attributes can be used for this kind of thing.
> ATM, the unit tests are failing but that's probably a test bug.  Let me know
> what you think.
> 
> --dave

I'm very excited to see this. :)

Have you tried Tor Browser's "cache" regression test here:
https://torpat.ch/13749
You can change the `suffixes` part to
+    suffixes = ["img.png", "object.png", "embed.png"];
so it will be testing image caching only.

>diff --git a/toolkit/system/gnome/nsAlertsIconListener.cpp b/toolkit/system/gnome/nsAlertsIconListener.cpp
>index 154630d..894f434 100644
[snip]
>+  nsCOMPtr<nsIScriptSecurityManager> ssm = nsContentUtils::GetSecurityManager();
>+  NS_ENSURE_TRUE(ssm, NS_ERROR_FAILURE);
>+  nsCOMPtr<nsIPrincipal> systemPrincipal;
>+  ssm->GetSystemPrincipal(getter_AddRefs(systemPrincipal));
>+  NS_ENSURE_TRUE(systemPrincipal, NS_ERROR_FAILURE);
>+
>   nsresult rv = il->LoadImageXPCOM(imageUri, nullptr, nullptr,
>-                                   NS_LITERAL_STRING("default"), nullptr, nullptr,
>-                                   this, nullptr,
>+                                   NS_LITERAL_STRING("default"), 
>+                                   systemPrincipal, nullptr, this, nullptr,
>                                    aInPrivateBrowsing ? nsIRequest::LOAD_ANONYMOUS :
>                                                         nsIRequest::LOAD_NORMAL,
>                                    nullptr, 0 /* use default */,
>                                    getter_AddRefs(mIconRequest));

Using the system principal here means that alert images aren't isolated to first party, correct? Is there a way to get the content principal? I think we didn't manage to do this yet in Tor Browser, but it would be good to fix that. See https://trac.torproject.org/15569

>diff --git a/widget/cocoa/nsMenuItemIconX.mm b/widget/cocoa/nsMenuItemIconX.mm
>index 1247f82..2c7bc7e26 100644
>--- a/widget/cocoa/nsMenuItemIconX.mm
>+++ b/widget/cocoa/nsMenuItemIconX.mm
>@@ -304,19 +304,25 @@ nsMenuItemIconX::LoadIcon(nsIURI* aIconURI)
[snip]
>+  nsCOMPtr<nsIScriptSecurityManager> ssm = nsContentUtils::GetSecurityManager();
>+  NS_ENSURE_TRUE(ssm, NS_ERROR_FAILURE);
>+  nsCOMPtr<nsIPrincipal> systemPrincipal;
>+  ssm->GetSystemPrincipal(getter_AddRefs(systemPrincipal));
>+  NS_ENSURE_TRUE(systemPrincipal, NS_ERROR_FAILURE);
>+
>   nsresult rv = loader->LoadImage(aIconURI, nullptr, nullptr,
>                                   mozilla::net::RP_Default,
>-                                  nullptr, loadGroup, this,
>+                                  systemPrincipal, loadGroup, this,
>                                   nullptr, nsIRequest::LOAD_NORMAL, nullptr,
>                                   nsIContentPolicy::TYPE_INTERNAL_IMAGE, EmptyString(),
>                                   getter_AddRefs(mIconRequest));

Here as well -- is it not dangerous to use the system principal here? I'm worried that this icon could come from content and needs to be isolated to first party.  In the Tor Browser version of this patch (https://torpat.ch/6539), Kathy and Mark used the pre-existing `document` variable in this function to obtain the first-party.

Comment 8

[Dave Huseby [:huseby]]

unassigned from my list so that tim, yoshi, or jonathan can pick this up.

Comment 9

[Dave Huseby [:huseby]]

tanvi,

i just want to double check with you that the image cache isolation with origin attributes has already landed.  if so, then this bug is redundant/unnecessary because once the firstPartyDomain origin attribute patch lands, then first party domain isolation of image cache will come for free.

Comment 10

[Jonathan Hao (inactive) [:jhao]]

Yes, it has landed in bug 1270680.

Comment 11

[Ethan Tseng [:ethan]]

So this bug should be resolved already.
Bug 1264572 will verify this.