Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#5742 closed defect (fixed)

Prototype image cache url isolation

Reported by: mikeperry Owned by: mikeperry
Priority: High Milestone:
Component: Firefox Patch Issues Version:
Severity: Keywords: tbb-linkability, MikePerry201207
Cc: gk, michael Actual Points: 21
Parent ID: Points: 20
Reviewer: Sponsor:

Description

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.

Child Tickets

Attachments (5)

ImageCache-Notes.diff (5.4 KB) - added by mikeperry 7 years ago.
Some specific notes on where to butcher the src
ImageCache-Notes3.diff (9.3 KB) - added by mikeperry 7 years ago.
More notes. Also, these notes actually compile. So we got that going for us. Which is nice.
ImageCache-Notes5.diff (18.1 KB) - added by mikeperry 7 years ago.
More logging + notes. Confirms suspicion that most arguments to LoadImage are lies
ImageCache-Notes7.diff (25.9 KB) - added by mikeperry 7 years ago.
Implement mozIThirdPartyUtil.getFirstPartyURI. Use it in the requiste places.
ImageCache-SeemsToWork.diff (35.6 KB) - added by mikeperry 7 years ago.
Holy crap, it actually seems to work. I'm getting good at this stuff.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 7 years ago by gk

Your suspicion seems to be correct :(.

My test setup and results:

I used a simple html file containing the Google image mentioned in #5715 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.

comment:2 Changed 7 years ago by mikeperry

Summary: Verify image cache url isolationFix image cache url isolation
Type: taskdefect

Thanks a lot gk. If you can paste the test pages here, it might save me a little time when trying to fix the thing.

comment:3 Changed 7 years ago by mikeperry

Keywords: tbb-linkability added

comment:5 Changed 7 years ago by mikeperry

Keywords: MikePerry201206 added

comment:6 Changed 7 years ago by mikeperry

Keywords: MikePerry201207 added; MikePerry201206 removed

comment:7 Changed 7 years ago by mikeperry

Actual Points: 2
Points: 20

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. :/

comment:8 Changed 7 years ago by mikeperry

Actual Points: 24

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.

Changed 7 years ago by mikeperry

Attachment: ImageCache-Notes.diff added

Some specific notes on where to butcher the src

comment:9 Changed 7 years ago by mikeperry

Actual Points: 46

See also the patch in #3246 to verify documentURI assumptions.

comment:10 in reply to:  9 Changed 7 years ago by mikeperry

Replying to mikeperry:

See also the patch in #3246 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..

Changed 7 years ago by mikeperry

Attachment: ImageCache-Notes3.diff added

More notes. Also, these notes actually compile. So we got that going for us. Which is nice.

comment:11 Changed 7 years ago by mikeperry

Actual Points: 69

Yeah that's right, I'm loggin with fprintf in that patch. Stand back or I'll cut you. I'm srs.

Changed 7 years ago by mikeperry

Attachment: ImageCache-Notes5.diff added

More logging + notes. Confirms suspicion that most arguments to LoadImage are lies

comment:12 Changed 7 years ago by mikeperry

Actual Points: 914

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?

comment:13 in reply to:  12 Changed 7 years ago by gk

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.

Changed 7 years ago by mikeperry

Attachment: ImageCache-Notes7.diff added

Implement mozIThirdPartyUtil.getFirstPartyURI. Use it in the requiste places.

comment:14 Changed 7 years ago by mikeperry

Actual Points: 1417

Ok, the attached patch should implement the API mozIThirdPartyUtil.getFirstPartyURI(). It seems to work, but I still need to alter the image loader to actually use the first party URI that gets passed in.

Changed 7 years ago by mikeperry

Attachment: ImageCache-SeemsToWork.diff added

Holy crap, it actually seems to work. I'm getting good at this stuff.

comment:15 Changed 7 years ago by mikeperry

Actual Points: 1720

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.

comment:16 Changed 7 years ago by mikeperry

Also, who knows what this thing does to the 5 frameworks worth of tests in the image/test subdir.. I'm not even sure how to run all of those...

comment:17 in reply to:  15 Changed 7 years ago by gk

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...?

comment:18 Changed 7 years ago by mikeperry

Actual Points: 2021
Resolution: fixed
Status: newclosed
Summary: Fix image cache url isolationPrototype image cache url isolation

Bleh. I just found a crash bug in debug builds for this patch, but to satisfy my crazy work accounting fetish, I opened #6539 for that crash issue so I can close this ticket.

comment:19 Changed 4 years ago by michael

Cc: michael added
Note: See TracTickets for help on using tickets.