Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#16620 closed defect (fixed)

Transform window.name handling into Firefox patch

Reported by: mikeperry Owned by: mcs
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-torbutton-conversion, TorBrowserTeam201510R
Cc: brade, gk, arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor: SponsorU

Description

Right now, we reset window.name in Torbutton in torbutton_check_progress(). We should rewrite this as a direct Firefox patch, as per our SponsorU Torbutton conversion deliverable.

Child Tickets

Change History (22)

comment:1 Changed 2 years ago by mikeperry

Keywords: TorBrowserTeam201509 added

comment:2 Changed 2 years ago by gk

Keywords: TorBrowserTeam201510 added; TorBrowserTeam201509 removed
Owner: changed from tbb-team to mcs
Sponsor: SponsorU
Status: newassigned

comment:3 Changed 2 years ago by mcs

Cc: brade added
Keywords: TorBrowserTeam201510R added; TorBrowserTeam201510 removed
Severity: Normal
Status: assignedneeds_review

Kathy and I cleaned up the "clear window.name" patch that is attached to this Mozilla bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=444222

We believe it will work well for Tor Browser. The browser changes are here:

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug16620-01&id=3ed7c9bbecd6354543914bc78cf4cb13ab4d09fe

and the Torbutton changes are here:

https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug16620-01&id=09e3e25c5464b6f2b22bdc84d2b9dbab65bec0a3

Please review.

comment:4 Changed 2 years ago by mcs

Cc: gk arthuredelstein added

comment:5 Changed 2 years ago by arthuredelstein

(Sorry for the delay in reviewing.)

I built and tested the C++ patch and it seems to be working as intended.

Instead of

+    nsCOMPtr<nsIDocShellTreeItem> item(this);
+    nsCOMPtr<nsIScriptGlobalObject> sgo = do_GetInterface(item);
+    nsCOMPtr<nsIScriptObjectPrincipal> sop(do_QueryInterface(sgo));

would it be possible to use

+    nsCOMPtr<nsIDocShellTreeItem> item(this);
+    nsCOMPtr<nsIScriptObjectPrincipal> sop(do_QueryInterface(item));

?

As an experiment, I browsed to https://www.torproject.org, opened the page's JS console and entered window.name = "test". Then I navigated to https://trac.torproject.org. I noticed that window.name was reset to an empty string. This behavior is different from our isolation policy for caches, DOM storage, favicons, etc, where we isolate by base domain. Might we want to use ThirdPartyUtil::GetBaseDomain instead of CheckSameOriginURI, so that www.torproject.org and trac.torproject.org are allowed to share data via window.name?

Last edited 2 years ago by arthuredelstein (previous) (diff)

comment:6 in reply to:  5 ; Changed 2 years ago by gk

Replying to arthuredelstein:

As an experiment, I browsed to https://www.torproject.org, opened the page's JS console and entered window.name = "test". Then I navigated to https://trac.torproject.org. I noticed that window.name was reset to an empty string. This behavior is different from our isolation policy for caches, DOM storage, favicons, etc, where we isolate by base domain. Might we want to use ThirdPartyUtil::GetBaseDomain instead of CheckSameOriginURI, so that www.torproject.org and trac.torproject.org are allowed to share data via window.name?

Not sure what "navigated" in this context means. Are you saying the patch behaves differently than specified in 4.5.12 of our design document?

comment:7 in reply to:  5 ; Changed 2 years ago by brade

Keywords: TorBrowserTeam201510 added; TorBrowserTeam201510R removed
Status: needs_reviewneeds_information

Replying to arthuredelstein:

(Sorry for the delay in reviewing.)

I built and tested the C++ patch and it seems to be working as intended.

Thanks, but unfortunately Mark and I discovered that our patch has some problems. Specifically, window.name is being cleared when a new tab/window is opened. This means that, effectively, you cannot set window.name via a call to window.open(). We discovered this while working on automated tests and will revise the patch to include some special-case handling for about:blank (which the JS code we are replacing has). Sorry for the premature review request.

would it be possible to use

+    nsCOMPtr<nsIDocShellTreeItem> item(this);
+    nsCOMPtr<nsIScriptObjectPrincipal> sop(do_QueryInterface(item));

?

No, QI is not the same things as get_Interface. While do_QueryInterface() returns a variant of the same object, do_GetInterface() typically returns a completely different object. The implementations of those two methods are different for docShell objects.

As an experiment, I browsed to https://www.torproject.org, opened the page's JS console and entered window.name = "test". Then I navigated to https://trac.torproject.org. I noticed that window.name was reset to an empty string. This behavior is different from our isolation policy for caches, DOM storage, favicons, etc, where we isolate by base domain. Might we want to use ThirdPartyUtil::GetBaseDomain instead of CheckSameOriginURI, so that www.torproject.org and trac.torproject.org are allowed to share data via window.name?

That is a good question. Mark and I tried to create a patch that Mozilla might accept. In https://bugzilla.mozilla.org/show_bug.cgi?id=444222, there is some support for the "always clear window.name when switching origins" approach that our patch implements. Of course other opinions are expressed there as well.

Replying to gk:

Not sure what "navigated" in this context means. Are you saying the patch behaves differently than specified in 4.5.12 of our design document?

Not to answer for Arthur, but I think the new behavior is slightly different. The TB design doc says "we reset the window.name property of tabs in Torbutton every time we encounter a blank Referer." Our patch clears it every time the security context changes, which means whenever the origin changes. This means that our new patch is even more aggressive about clearing window.name. According to the following comment, this is OK:
https://bugzilla.mozilla.org/show_bug.cgi?id=444222#c71
But there is a chance that we will introduce compatibility issues with some web sites who use (abuse?) window.name to pass info across domains. Opinions?

comment:8 in reply to:  7 Changed 2 years ago by gk

Status: needs_informationassigned

Replying to brade:

Replying to gk:

Not sure what "navigated" in this context means. Are you saying the patch behaves differently than specified in 4.5.12 of our design document?

Not to answer for Arthur, but I think the new behavior is slightly different. The TB design doc says "we reset the window.name property of tabs in Torbutton every time we encounter a blank Referer." Our patch clears it every time the security context changes, which means whenever the origin changes. This means that our new patch is even more aggressive about clearing window.name. According to the following comment, this is OK:
https://bugzilla.mozilla.org/show_bug.cgi?id=444222#c71
But there is a chance that we will introduce compatibility issues with some web sites who use (abuse?) window.name to pass info across domains. Opinions?

How hard would it be to have the current behavior implemented? Especially given all the other things that still need to be done until Oct 31? If that sounds like too much work I think cleaning up the current approach and shipping it in the alpha series for a while at least would be fine.

comment:9 in reply to:  6 ; Changed 2 years ago by arthuredelstein

Replying to gk:

Replying to arthuredelstein:

As an experiment, I browsed to https://www.torproject.org, opened the page's JS console and entered window.name = "test". Then I navigated to https://trac.torproject.org. I noticed that window.name was reset to an empty string. This behavior is different from our isolation policy for caches, DOM storage, favicons, etc, where we isolate by base domain. Might we want to use ThirdPartyUtil::GetBaseDomain instead of CheckSameOriginURI, so that www.torproject.org and trac.torproject.org are allowed to share data via window.name?

Not sure what "navigated" in this context means. Are you saying the patch behaves differently than specified in 4.5.12 of our design document?

Now I've read 4.5.12. :) I tried some more experiments, each of which I started by going to https://www.torproject.org and entering window.name = "test"; in the content page JS console, then browsing to either https://trac.torproject.org (trac) or https://www.internetdefenseleague.org (idl), either by entering the latter address in the URL bar or clicking on a link ("Tor Wiki" or the IDL logo). Here are the results:

BrowserTargetMethod"Referer" in headerwindow.name
1.Firefox 41.0.2tracClicked link"https://www.torproject.org/"Retained
2.Firefox 41.0.2tracEntered in URL barNoneRetained
3.Firefox 41.0.2idlClicked link"https://www.torproject.org/"Retained
4.Firefox 41.0.2idlEntered in URL barNoneRetained
5.Chrome 43.0tracClicked link"https://www.torproject.org/"Retained
6.Chrome 43.0tracEntered in URL barNoneRetained
7.Chrome 43.0idlClicked link"https://www.torproject.org/"Retained
8.Chrome 43.0idlEntered in URL barNoneSet to empty
9.Tor Browser 5.0.3tracClicked link"https://www.torproject.org/"Retained
10.Tor Browser 5.0.3tracEntered in URL barNoneSet to empty
11.Tor Browser 5.0.3idlClicked link"https://www.torproject.org/"Retained
12.Tor Browser 5.0.3idlEntered in URL barNoneSet to empty
13.Patch in comment:3tracClicked link"https://www.torproject.org/"Set to empty
14.Patch in comment:3tracEntered in URL barNoneSet to empty
15.Patch in comment:3idlClicked link"https://www.torproject.org/"Set to empty
16.Patch in comment:3idlEntered in URL barNoneSet to empty

If we reset window.name when the Referer is blank, as in section 4.5.12 and TBB 5.0.3, we are differing from the behavior of vanilla Firefox and Chrome in (2, 4 and 6). If we isolate by first party base domain (like DOM storage and caching), then our behavior would be different from (3, 4 and 7).

I guess it's not entirely clear to me what the best choice is. Chrome's behavior seems possibly the least disruptive choice. It pains me that content can observe what third-party links a user clicks on and can send data to the third-party site, but as explained in the Tor Browser Design document, there are ways besides window.name to pass on that information via a link click.

On the other hand, as Mark and Kathy point out, what Mozilla is willing to accept is a big consideration.

Last edited 2 years ago by arthuredelstein (previous) (diff)

comment:10 in reply to:  9 Changed 2 years ago by mcs

Replying to arthuredelstein:

Now I've read 4.5.12. :) I tried some more experiments, each of which I started by going to https://www.torproject.org and entering window.name = "test"; in the content page JS console, then browsing to either https://trac.torproject.org (trac) or https://www.internetdefenseleague.org (idl), either by entering the latter address in the URL bar or clicking on a link ("Tor Wiki" or the IDL logo). Here are the results:
...

Thanks for doing this!

I guess it's not entirely clear to me what the best choice is. Chrome's behavior seems possibly the least disruptive choice. It pains me that content can observe what third-party links a user clicks on and can send data to the third-party site, but as explained in the Tor Browser Design document, there are ways besides window.name to pass on that information via a link click.

On the other hand, as Mark and Kathy point out, what Mozilla is willing to accept is a big consideration.

I suspect it will be difficult to get Mozilla to accept any change (due to potential breakage of sites). But I don't really know.

For test case 8, it isn't clear what criteria Chrome used to decide to clear window.name. Maybe it is comparing top-level domains? We would need to look at the Chromium code and try to match Google's behavior.

Since Kathy and I are in favor of breaking this link between windows more aggressively, we prefer our current patch's behavior or the existing TB 5.0.3 behavior. Kathy and I will look at how difficult it would be to create a C++ patch that mimics the 5.0.3 behavior.

comment:11 Changed 2 years ago by mcs

Keywords: TorBrowserTeam201510R added; TorBrowserTeam201510 removed
Status: assignedneeds_review

Kathy and I completed work on a new browser patch that clears window.name when there is no referrer (like the Torbutton JS code we are removing does). We also added some automated tests. Please review:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug16620-02&id=d3d2126f72b729d88d261bfd2efff0ae3e070e22

comment:12 Changed 2 years ago by gk

Arthur, could you take a look at this patch (too)? We should include that into the alpha if we can.

comment:13 Changed 2 years ago by arthuredelstein

I read over the code and ran the mochitest. Looks good to me.

comment:14 Changed 2 years ago by gk

Could you try testing with http://www.thomasfrank.se/sessvarsTestPage1.html? I am currently recompiling my build to be absolutely sure I tested your patches but it seems your patch does not handle this testcase (see #3414 for context).

There seem to be in fact two issues:

1) If I understand this correctly then caching might bypass the protections in your patch.
2) But even if I disable caching and disable sending the Referer header your code behaves differently than the one in 5.0.3.

(I think I made sure I used a Torbutton version with your Torbutton patch applied to, too).

comment:15 Changed 2 years ago by gk

Status: needs_reviewneeds_revision

Yeah, seems still to be not working. :(

comment:16 in reply to:  14 ; Changed 2 years ago by mcs

Replying to gk:

Could you try testing with http://www.thomasfrank.se/sessvarsTestPage1.html? I am currently recompiling my build to be absolutely sure I tested your patches but it seems your patch does not handle this testcase (see #3414 for context).

There seem to be in fact two issues:

1) If I understand this correctly then caching might bypass the protections in your patch.
2) But even if I disable caching and disable sending the Referer header your code behaves differently than the one in 5.0.3.

For the http://www.thomasfrank.se/sessvarsTestPage1.html page (which I assume is issue 2 above), the problem is that our patch clears window.name too soon. That page installs an unload event handler that re-saves its "session variables" to window.name after we clear it. We are working on a new patch that fixes this problem by relocating our code that clears window.name.

But can you explain more about issue 1? Is the concern that pages loaded from the cache will not cause window.name to be cleared? Do you have a test case? (if not, Kathy and I will come up with one).

comment:17 in reply to:  16 Changed 2 years ago by gk

Replying to mcs:

Replying to gk:

Could you try testing with http://www.thomasfrank.se/sessvarsTestPage1.html? I am currently recompiling my build to be absolutely sure I tested your patches but it seems your patch does not handle this testcase (see #3414 for context).

There seem to be in fact two issues:

1) If I understand this correctly then caching might bypass the protections in your patch.
2) But even if I disable caching and disable sending the Referer header your code behaves differently than the one in 5.0.3.

For the http://www.thomasfrank.se/sessvarsTestPage1.html page (which I assume is issue 2 above), the problem is that our patch clears window.name too soon. That page installs an unload event handler that re-saves its "session variables" to window.name after we clear it. We are working on a new patch that fixes this problem by relocating our code that clears window.name.

But can you explain more about issue 1? Is the concern that pages loaded from the cache will not cause window.name to be cleared? Do you have a test case? (if not, Kathy and I will come up with one).

Yes, that was the concern. I don't have a special test case just http://www.thomasfrank.se/sessvarsTestPage1.html. I saw no network requests and hence no Referers that could influence the behavior while testing which gave me the general idea. This is no issue in our current Torbutton patch and the behavior I saw might solely be due to the unload event handler.

comment:18 Changed 2 years ago by mcs

Here is a revised patch:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug16620-03&id=cd8e63be6a6be9377fd81ab8fbfd2ee2230cfc65

We relocated the code that clears window.name so it runs after the previous page has received its unload event. We also added a mochitest that tries to use an unload event listener to reinstate the window.name value (similar to what http://www.thomasfrank.se/sessvarsTestPage1.html does). We did not see a problem even when pages were loaded from the cache.

Please review.

comment:19 in reply to:  18 Changed 2 years ago by arthuredelstein

Replying to mcs:

Here is a revised patch:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug16620-03&id=cd8e63be6a6be9377fd81ab8fbfd2ee2230cfc65

We relocated the code that clears window.name so it runs after the previous page has received its unload event. We also added a mochitest that tries to use an unload event listener to reinstate the window.name value (similar to what http://www.thomasfrank.se/sessvarsTestPage1.html does). We did not see a problem even when pages were loaded from the cache.

Please review.

I read over the code and mochitest source and they look good to me. I ran the mochitest and everything passed.

I also experimented with http://www.thomasfrank.se/sessvarsTestPage1.html -- it seems clicking on links or the "submit" button results in a Referer; entering the URL in the URL bar does not, and window.name seems to follow the rules correctly (getting wiped when there is no Referer). So I think this patch is good.

comment:20 Changed 2 years ago by gk

Resolution: fixed
Status: needs_revisionclosed

Alright, looks good to me as well. Just a nit: s/bug # 16620/bug 16620/. I'll merge it though now to get the alpha going.

comment:21 in reply to:  20 ; Changed 2 years ago by mcs

Replying to gk:

Alright, looks good to me as well. Just a nit: s/bug # 16620/bug 16620/. I'll merge it though now to get the alpha going.

Thanks. Should Kathy and I create an additional patch to fix that?

comment:22 in reply to:  21 Changed 2 years ago by gk

Replying to mcs:

Replying to gk:

Alright, looks good to me as well. Just a nit: s/bug # 16620/bug 16620/. I'll merge it though now to get the alpha going.

Thanks. Should Kathy and I create an additional patch to fix that?

Nah. What I should have done is just making a fixup commit _before_ rebasing but I had already done everything and was only waiting for Arthur's feedback when I say the typo. And then I was to tired to undo that and just hit the "Please!! Build the Alpha!!1!"-button :/. I'll keep it in mind and do The Right Thing before rebasing the next time. :)

Note: See TracTickets for help on using tickets.