Closed Bug 1108017 Opened 10 years ago Closed 9 years ago

Loosen third party checks (tracking protection breaks some facebook apps)

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: mmc, Assigned: mmc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Loosen third-party restrictions for tracking protection checks
9.25 KB, patch
sworkman
: review+
Details | Diff | Splinter Review 
Catalin reported this to me over email.
Catalin, can you give me some examples?
Blocks: tp-breakage
Flags: needinfo?(cvarga)
Francois, can you take a look please?
Assignee: nobody → francois
Flags: needinfo?(francois)
Looking at the lets_fish game, the following resources get blocked:

https://www.googletagservices.com/tag/js/gpt.js
https://stats.g.doubleclick.net/dc.js
https://s-static.ak.facebook.com/connect/xd_arbiter/6Dg4oLkBbYq.js?version=41#channel=f23ca0fa66dd582&origin=https%3A%2F%2Ffish-fb.apps.tensquaregames.com
https://www.facebook.com/connect/ping?client_id=328061820606223&domain=fish-fb.apps.tensquaregames.com&origin=5&redirect_uri=https%3A%2F%2Fs-static.ak.facebook.com%2Fconnect%2Fxd_arbiter%2F6Dg4oLkBbYq.js%3Fversion%3D41%23cb%3Df1972899224820c%26domain%3Dfish-fb.apps.tensquaregames.com%26origin%3Dhttps%253A%252F%252Ffish-fb.apps.tensquaregames.com%252Ff23ca0fa66dd582%26relation%3Dparent&response_type=token%2Csigned_request%2Ccode&sdk=joey
https://www.facebook.com/plugins/like.php?app_id=328061820606223&channel=https%3A%2F%2Fs-static.ak.facebook.com%2Fconnect%2Fxd_arbiter%2F6Dg4oLkBbYq.js%3Fversion%3D41%23cb%3Dfef607678b254%26domain%3Dfish-fb.apps.tensquaregames.com%26origin%3Dhttps%253A%252F%252Ffish-fb.apps.tensquaregames.com%252Ff23ca0fa66dd582%26relation%3Dparent.parent&container_width=220&href=https%3A%2F%2Fapps.facebook.com%2Flets_fish%2F&locale=en_US&sdk=joey&send=false&show_faces=false&width=200
https://www.facebook.com/plugins/like_box.php?app_id=328061820606223&channel=https%3A%2F%2Fs-static.ak.facebook.com%2Fconnect%2Fxd_arbiter%2F6Dg4oLkBbYq.js%3Fversion%3D41%23cb%3Df1231e13deccf6c%26domain%3Dfish-fb.apps.tensquaregames.com%26origin%3Dhttps%253A%252F%252Ffish-fb.apps.tensquaregames.com%252Ff23ca0fa66dd582%26relation%3Dparent.parent&container_width=720&header=true&height=700&href=https%3A%2F%2Fwww.facebook.com%2Fpages%2FLets-fish-Community%2F463938473619424&locale=en_US&sdk=joey&show_faces=false&stream=true&width=720

The URL of the page is https://apps.facebook.com/328061820606223/ but it contains an iframe which loads https://fish-fb.apps.tensquaregames.com/sns/common/swf/fisherking_loader.swf:835fee2233129d3ac5fe6db770383215

In order to make this work under Request Policy, I had to whitelist the following:

  tensquaregames.com -> facebook.com

So I guess the problem is that the iframe has tensquaregames.com as a first party and then loads facebook.com which is blocked in third-party contexts.
Flags: needinfo?(francois)
That makes sense. I think we can loosen our third party checks to fix this problem.

From nsChannelClassifier::ShouldEnableTrackingProtection

    bool isThirdParty = true;
    (void)thirdPartyUtil->IsThirdPartyChannel(aChannel, nullptr, &isThirdParty);

Instead of passing nullptr for the second URI, we should pass the URI of the top window for the channel.
The obvious "fix" which I mention in comment 7 doesn't work, because on first navigation to facebook.com the top window URI is the previous URI. So we need to do this only for non-location bar loads.
Assignee: francois → mmc
Status: NEW → ASSIGNED
Oops, bzexport resets the assignee. Anyway, the patch above is not quite right. It turns out that IsThirdPartyChannel does all sorts of checks with the channel's window that we might want to skip, so comparing the channel URI and the top window URI with IsThirdPartyURI is more straightforward. It works in all cases except for top-window loads, because the window URI at that point is irrelevant.

However, I'm not sure checking LOAD_DOCUMENT_INITIAL_URI is quite the right solution. If you try out the patch with NSPR_LOG_MODULES=nsChannelClassifier:5,thirdPartyUtil:5 it skips tracking protection checks on fatratgames on the FB app "Stick Ride". I'm not sure we want to do that. It doesn't seem to be set for all iframes, though -- the test page at http://people.mozilla.org/~mchew/test_tp.html seems to work fine.
Assignee: mmc → francois
Comment on attachment 8578779 [details] [diff] [review]
Loosen third-party restrictions for tracking protection checks

Review of attachment 8578779 [details] [diff] [review]:
-----------------------------------------------------------------

I think the new logic works. The following would be allowed:

  facebook.com -> game.com (iframe) -> facebook.com (sub-resource)

even if facebook.com is on the blocklist.

I initially thought that this would incorrectly block same-origin sub-resources in iframes:

  facebook.com -> game.com (iframe) -> game.com (sub-resource)

but in this case, if game.com were on the blocklist, then not only would the sub-resource be blocked, but the whole iframe would be blocked too, so it doesn't matter.

::: netwerk/base/nsChannelClassifier.cpp
@@ +81,5 @@
> +    // that situation we want to avoid comparing channelURI to the window, since
> +    // what's in the window will be replaced as the document loads.
> +    if (flags & nsIChannel::LOAD_INITIAL_DOCUMENT_URI) {
> +#ifdef DEBUG
> +      nsCString topspec;

unused variable
Attachment #8578779 - Flags: feedback+
Attachment #8578779 - Attachment is obsolete: true
Assignee: francois → mmc
Attachment #8580805 - Flags: review?(sworkman)
Summary: tracking protection breaks some facebook apps → Loosen third party checks (tracking protection breaks some facebook apps)
Comment on attachment 8580805 [details] [diff] [review]
Loosen third-party restrictions for tracking protection checks

Review of attachment 8580805 [details] [diff] [review]:
-----------------------------------------------------------------

Just to be clear, this patch changes TP so that:
1. Top-level loads are ignored (via the flag LOAD_INITIAL_DOCUMENT_URI)
2. Third party loads are checked; a URI is considered third party if it does not share the same base domain as the top-level window load.

Looks fine, but can you check what happens if a link is in an iframe and the load is targeted? Is LOAD_INITIAL_DOCUMENT_URI set there as well, or is that really only for the top-level window load? It's not clear to me from nsIChannel.idl whether it's just for top-level loads.

::: dom/base/ThirdPartyUtil.cpp
@@ +181,5 @@
>  ThirdPartyUtil::IsThirdPartyChannel(nsIChannel* aChannel,
>                                      nsIURI* aURI,
>                                      bool* aResult)
>  {
> +  LOG(("ThirdPartyUtil::IsThirdPartyChannel"));

nit: These logs don't have a unique ID to match them. Can you add in a %p for aChannel.get() or something like that? It'll be easier for scanning logs later.
Attachment #8580805 - Attachment is obsolete: true
Attachment #8580805 - Flags: review?(sworkman)
Comment on attachment 8581942 [details] [diff] [review]
Loosen third-party restrictions for tracking protection checks

Review of attachment 8581942 [details] [diff] [review]:
-----------------------------------------------------------------

This avoids use of LOAD_INITIAL_DOCUMENT_URI, which was triggering for some(?) iframes. It is ugly, but it seems to work. There is some extraneous logging in the ThirdPartyUtil file that I will remove.
Attachment #8581942 - Flags: review?(sworkman)
Comment on attachment 8581942 [details] [diff] [review]
Loosen third-party restrictions for tracking protection checks

Review of attachment 8581942 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsChannelClassifier.cpp
@@ +99,5 @@
> +    rv = aChannel->GetURI(getter_AddRefs(chanURI));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    thirdPartyUtil->IsThirdPartyURI(chanURI, topWinURI, &isThirdParty);
> +    bool isThirdPartyChannel = true;
> +    thirdPartyUtil->IsThirdPartyChannel(aChannel, nullptr, &isThirdPartyChannel);

The logic for IsThirdPartyURI seems to be included in IsThirdPartyChannel if you pass in a uri rather than nullptr. Am I missing something about the logic that means you need to call IsThirdPartyURI as well?
The only reason to call IsThirdPartyChannel is to detect the case where it's a toplevel page load (and the call to IsThirdPartyURI is invoked with the old window's URI). I tried fixing this with LOAD_DOCUMENT_INITIAL_URI and friends but it turns out that IsThirdPartyChannel already does these checks. If there's a cleaner way to do it, please let me know.
Basically, when looking at IsThirdPartyChannel, it looks like passing in topWinURI might work:

thirdPartyUtil->IsThirdPartyChannel(aChannel, topWinURI, &isThirdPartyChannel);

But if you've tried it and it doesn't fit this use case, then I'll approve it :)
(In reply to Steve Workman [:sworkman] (please use needinfo) from comment #21)
> Basically, when looking at IsThirdPartyChannel, it looks like passing in
> topWinURI might work:
> 
> thirdPartyUtil->IsThirdPartyChannel(aChannel, topWinURI,
> &isThirdPartyChannel);
> 
> But if you've tried it and it doesn't fit this use case, then I'll approve
> it :)

Unfortunately it doesn't work for top-level loads, because topWinURI is stale :( That's why I was mucking about with the LOAD_DOCUMENT_URI flags.
Also, it obviously needs better comments.
Comment on attachment 8581942 [details] [diff] [review]
Loosen third-party restrictions for tracking protection checks

Review of attachment 8581942 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Monica!

::: netwerk/base/nsChannelClassifier.cpp
@@ +93,5 @@
>      }
>  
> +    // Third party checks don't work for chrome:// URIs in mochitests, so just
> +    // default to isThirdParty = true
> +    bool isThirdParty = true;

s/isThirdParty/isThirdPartyURI/ just to differentiate from isThirdPartyChannel.
Attachment #8581942 - Flags: review?(sworkman) → review+
QA Contact: mwobensmith
Yes, thanks. Matt, please play lots of Facebook games when you QA this, and check that no facebook domains appear in the security console as "The resource at <blah> is blocked because tracking protection is enabled".
https://hg.mozilla.org/mozilla-central/rev/0c7bcfab694c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: