Opened 3 years ago

Closed 3 years ago

#18741 closed defect (fixed)

OCSP and favicon isolation is only partly working in ESR 45

Reported by: gk Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Major Keywords: ff45-esr, tbb-6.0a5, TorBrowserTeam201604R
Cc: arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by gk)

We might need a fixup patch for our OCSP and favicon isolation in ESR45. If one takes https://dist.torproject.org as an example URL I can see things like

[01-01 00:00:00] Torbutton INFO: tor SOCKS: https://dist.torproject.org/favicon.ico via torproject.org:0
[01-01 00:00:00] Torbutton INFO: tor SOCKS: https://dist.torproject.org/favicon.ico via --NoFirstPartyHost-chrome-browser.xul--:0

and

[01-01 00:00:00] Torbutton INFO: tor SOCKS: http://ocsp.digicert.com/ via torproject.org:0
[01-01 00:00:0=] Torbutton INFO: tor SOCKS: http://ocsp.digicert.com/ via --nofirstpartyhost-chrome-browser.xul--:0

in the log output. Note the differing nofirstpartyhost-chrome-browser.xul and Nofirstpartyhost-chrome-browser.xul.

Child Tickets

Change History (14)

comment:1 Changed 3 years ago by gk

Description: modified (diff)
Summary: OCSP and Favicon isolation is only partly working in ESR 45OCSP and favicon isolation is only partly working in ESR 45

comment:2 Changed 3 years ago by arthuredelstein

Keywords: tbb-6.0a5 added

comment:3 Changed 3 years ago by arthuredelstein

Keywords: TorBrowserTeam201604R added
Status: newneeds_review

comment:4 Changed 3 years ago by gk

Hm... I still see the issue in a test build applied with the patch it seems.

comment:5 in reply to:  4 Changed 3 years ago by gk

Replying to gk:

Hm... I still see the issue in a test build applied with the patch it seems.

On a Linux box in case this matters.

comment:6 Changed 3 years ago by gk

Reading comment:11:ticket:16326 it seems we want to have this commit anyway. What broke without it?

comment:7 in reply to:  6 Changed 3 years ago by arthuredelstein

Replying to gk:

Reading comment:11:ticket:16326 it seems we want to have this commit anyway. What broke without it?

The favicon first-party cache isolation test was not working. My patch fixed the cache isolation, but I realize now that the TorButton error message you reported in the comment:description indicates a problem with network isolation. I have written a patch that I think will fix both cache and network isolation, which I will post today after testing.

comment:8 Changed 3 years ago by arthuredelstein

OK, here's the new branch. I tested on Ubuntu and got all favicon and OCSP requests running through the first party domain:
https://github.com/arthuredelstein/tor-browser/commits/16326+3
Note there are three commits here.

  • 483bd1684d437f0e03743b9573990240d77e8acc adds a fix for #16326
  • 4117c6b544e4fba93d192262aeabc0be4f38c4d7 fixes favicon cache and network isolation
  • 8317e098f0b880eded1301fe40e3e9fd1b813fc3 adds network isolation testing to our cache isolation regression test patch

comment:9 in reply to:  8 ; Changed 3 years ago by mcs

Status: needs_reviewneeds_information

Replying to arthuredelstein:

OK, here's the new branch. I tested on Ubuntu and got all favicon and OCSP requests running through the first party domain:
https://github.com/arthuredelstein/tor-browser/commits/16326+3
Note there are three commits here.

  • 483bd1684d437f0e03743b9573990240d77e8acc adds a fix for #16326
  • 4117c6b544e4fba93d192262aeabc0be4f38c4d7 fixes favicon cache and network isolation

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

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

comment:10 in reply to:  9 ; Changed 3 years ago by arthuredelstein

Status: needs_informationneeds_review

Replying to 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.

comment:11 in reply to:  10 ; Changed 3 years ago by gk

Replying to arthuredelstein:

Replying to 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)

comment:12 in reply to:  11 ; Changed 3 years ago by mcs

Replying to gk:

[snip]
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.

Yes, r=brade and r=mcs if you remove the extra IsElement() check.

comment:13 in reply to:  12 ; Changed 3 years ago by arthuredelstein

Replying to mcs:

Replying to gk:

[snip]
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.

Yes, r=brade and r=mcs if you remove the extra IsElement() check.

Here's the new branch with this nit fixed. I am currently running a 3-platform build and will also run mochitests on linux when it's ready. I have already run the mochitest on OS X and it all passes.

https://github.com/arthuredelstein/tor-browser/commits/16326+5
hash: e8765c0bd920734cb1bcfa5fe00c77f45a7c9a2d

comment:14 in reply to:  13 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to arthuredelstein:

Replying to mcs:

Replying to gk:

[snip]
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.

Yes, r=brade and r=mcs if you remove the extra IsElement() check.

Here's the new branch with this nit fixed. I am currently running a 3-platform build and will also run mochitests on linux when it's ready. I have already run the mochitest on OS X and it all passes.

Sounds good to me. Let me know if anything breaks. Otherwise I am closing this and putting this in the alpha to get the build going. This is fixed in commit 1f16ce85bc145032a6954a2b8ae81875c6ec1dbf, 9c9b94f764495c8500093622c611029883badc97 and e6b06c64d7a2294a0f3def31d8e51acd751f40c9.

Note: See TracTickets for help on using tickets.