[tbb-bugs] #18741 [Applications/Tor Browser]: OCSP and favicon isolation is only partly working in ESR 45

[Tor Bug Tracker & Wiki]

#18741: OCSP and favicon isolation is only partly working in ESR 45
-------------------------------------------------+-------------------------
 Reporter:  gk                                   |          Owner:  tbb-
     Type:  defect                               |  team
 Priority:  High                                 |         Status:
Component:  Applications/Tor Browser             |  needs_review
 Severity:  Major                                |      Milestone:
 Keywords:  ff45-esr, tbb-6.0a5,                 |        Version:
  TorBrowserTeam201604R                          |     Resolution:
Parent ID:                                       |  Actual Points:
 Reviewer:                                       |         Points:
                                                 |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by arthuredelstein):

 * status:  needs_information => needs_review


Comment:

 Replying to [comment:9 mcs]:

 Thanks for the review. Here's a new version with 4 patches:
 https://github.com/arthuredelstein/tor-browser/commits/16326+4.
 Hash: 164431b40788b18b28502804224b54cc5760083b

 > Can you explain why the above patch is needed? Why aren't we passing the
 correct aNode in all cases? I am worried that we will poke around in the
 ancestor elements looking for a "firstparty" attribute in a lot more cases
 now, and I am not sure what the implications are (but I have not run the
 code yet).

 I agree that seems dangerous. So now I'm proposing a change to the
 `ThirdPartyUtil.cpp` code so that it only looks at the immediate node, and
 does not traverse the ancestors.
 Hash: 3553684ab8fa75ac55b930916b7ee06c862c644e

 To compensate for this change, I used a XUL attribute inheritance trick
 that allows the "firstparty" attribute on the `tab` element to propagate
 down to its icon `xul:image`:
 Hash: e4854b02006d5b156c8e40d482b869b904eb1034

 These changes together give the same behavior of favicons, but hopefully
 this arrangement is much less likely to affect other objects in the DOM.

 > > * 8317e098f0b880eded1301fe40e3e9fd1b813fc3 adds network isolation
 testing to our cache isolation regression test patch
 >
 > It would have helped me if there was a comment inside the
 observeChannels() callback that explained how the check worked, e.g.,
 > {{{
 > // All "thirdPartyChild" resources are loaded from example.net, so we
 expect
 > // the first party host to be .com or .org.
 > }}}

 Sorry that wasn't clear. I have added a comment like that in the new
 version.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18741#comment:10>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online