Prototype image cache url isolation
There is a suspicious comment in imgLoader::LoadImage() about ignoring CacheKey, which we use for domain isolation of the cache. We need to test sourcing the same image url from two different url bar domains to make sure the cache is not shared between them.
Activity
Your suspicion seems to be correct :(.
My test setup and results:
I used a simple html file containing the Google image mentioned in #5715 (closed) and the latest TBB (otherwise I had to patch the JonDoFox-JS as well as we currently avoid the cache for all 3rd party requests) on Linux. The file was hosted on svn.jondos.de and on anonymous-proxy-servers.net. After clearing the cache I loaded the file on svn.jondos.de in the first tab and afterwards the file on anonymous-proxy-servers.net in the second tab. about:cache showed me only the image cache entry with the svn.jondos.de domain. I repeated the same experiment and looked at the request/response headers as well. about:cache gave me the same results. Additionally, the captured headers showed no request for the image in tab 2. Therefore, I would conclude that the cached image is indeed used despite being loaded by a different domain.
-
Take these URLs: https://anonym-surfen.de/ImageTest.html and https://anonymous-proxy-servers.net/en/ImageTest.html.
Ok, I started looking into this more and it would seem that the "cacheKey" argument to imgLoader::LoadImage is often null.. Elsewhere in the imgLoader, the actual cache key is constructed directly from the URI without even a channel available, so we can't use nsHttpChannel::AssembleCacheKey() to get the expected cacheKey.
I think this might mean that several functions in the non-critical paths of the image loader will have to become O(N), to be able to continue to operate on uri strings and retain API compatibility. Those functions will just search over the cache and return/remove the first matching URI, isolated or not. We'll need to double check these functions for cross-domain info leaks, though.. I think the only dangerous one in that regard is imgLoader::FindEntryProperties().
For the actual cached image storage and retrieval in imgLoader::LoadImage() and imgLoader::LoadImageWithChannel(), we'll have to do our best to construct a url domain-isolated cacheKey using either the referer uri, the channel, the notificationCallbacks, or who knows what, depending upon availability.
In short, this is going to be a huge messy pile of pain. The only good thing is that the image caching code hasn't changed since ~2001. Our patch probably won't generate too many conflicts at that rate of code change.
This is probably going to take like a week to get right. :/
Trac:
Points: N/A to 20
Actualpoints: N/A to 2-
Some more useful tidbits: nsIHttpChannelInternal actually has a documentURI getter and setter that seems to provide the top-level url that was used during a load. It gets set in nsDocShell, so it may not be present for all image loads, but we can at least test it out in imgLoader::LoadImageWithChannel(). If the channel fails to QI and/or the getter fails, I suppose we could just bypass the cache (perhaps with an Error Console warn somehow or something?).
Additionally, it appears that the documentURI in imgLoader::LoadImage() might actually be what we need when it is actually non-null, but we should test nested iframes and/or inspect the callers more closely to be sure. Perhaps we can also bypass cache and log if that parameter is null.
Trac:
Actualpoints: 2 to 4 -
Trac:
ImageCache-Notes.diff -
See also the patch in #3246 (moved) to verify documentURI assumptions.
Trac:
Actualpoints: 4 to 6 -
Replying to mikeperry:
See also the patch in #3246 (moved) to verify documentURI assumptions.
Ok, so this patch actually relies on nsCookiePermission::GetOriginatingURI(), which extracts the top-level window URI directly from a channel using the loadgroup's notification callbacks and like 4 or 5 other codepaths.. Surprisingly, it does not use nsIHttpChannelInternal.documentURI, even though it QI's nsIHttpChannelInternal to check for an unrelated sketchy bypass of the third party cookie pref..
-
Trac:
ImageCache-Notes3.diff -
Trac:
ImageCache-Notes5.diff -
Making progress, slowly but surely. Still probably a couple more days work left before actually isolating the damn image cache, though... :/.
After writing a new interface to reliably get the top-level URL for an arbitrary third party nsIDocument, I discovered mozIThirdPartyUtil. It looks like it could be useful, but is still not exactly what we need here. Probably want to move my helper there, instead?
Trac:
Actualpoints: 9 to 14 -
Replying to mikeperry:
After writing a new interface to reliably get the top-level URL for an arbitrary third party nsIDocument, I discovered mozIThirdPartyUtil. It looks like it could be useful, but is still not exactly what we need here. Probably want to move my helper there, instead? I think so. I stumbled over it while adapting Dan Witte's cookie double-key patch to recent mozilla-central versions. And, yes, creating an own, central utility including helper methods to solve third-party questions for all parts of Gecko/Necko that need them seems quite reasonable to me.
-
Trac:
ImageCache-Notes7.diff -
-
gk: Can you take that patch for a spin? I tested your two urls as well as a load of www.google.com, and with that patch, I now have 3 entries in about:cache, bound to the domain cacheKey, as expected. I also watched the google logo slowly load in each tab. Hurray!
The patch itself still needs some cleanup, though.
Trac:
Actualpoints: 17 to 20 -
Replying to mikeperry:
gk: Can you take that patch for a spin? Yes, I'll do that on the weekend (hopefully). Do you develop using mozilla-central or the next stable (i.e. current beta) of Firefox or...?
-
Bleh. I just found a crash bug in debug builds for this patch, but to satisfy my crazy work accounting fetish, I opened #6539 (closed) for that crash issue so I can close this ticket.
Trac:
Summary: Fix image cache url isolation to Prototype image cache url isolation
Resolution: N/A to fixed
Status: new to closed
Actualpoints: 20 to 21 mentioned in issue #6458 (moved)
mentioned in issue #6539 (closed)
mentioned in issue #6564 (closed)
mentioned in issue #20680 (moved)
