[tor-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:
-------------------------------------------------+-------------------------

Comment (by gk):

 Replying to [comment:10 arthuredelstein]:
 > 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.

 Sounds like a good approach we could test in the alpha. One nit:

 There is no need for an additional `node->IsElement()` check as you are
 landing in the `if`-block only after the `node->IsElement()` check passes.

 Otherwise looks good to me. (I assume you made sure the patch builds for
 all platforms and fixes the issue and the tests are passing; I currently
 don't have the means to verify that)

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