Closed Bug 1319773 Opened 8 years ago Closed 7 years ago

Issues signing in on Soundcloud using Firefox with FPI

Categories

(Core :: DOM: Security, defect, P1)

Product:
Core
Component:
DOM: Security
Version:
53 Branch
Platform:
All
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- unaffected
firefox54 --- verified

People

(Reporter: bmaris, Assigned: timhuang)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [tor][domsecurity-active][dfpi-ok])

Attachments

(5 files)

Screenshot showing the issue
53.50 KB, image/png
Details
WIP - Making the WrapperFactory ignores the First Party Domain of the originAttributes when rewrap the wrapper.
11.26 KB, patch
Details | Diff | Splinter Review
Bug 1319773 - Part 1: Add a SubsumesConsideringDomainIgnoringFPD in BasePrincipal.
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1319773 - Part 2: Add a pref 'privacy.firstparty.isolate.restrict_opener_access' which controls the access of window.opener for different first party domain.
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1319773 - Part 3: Making the WrapperFactory ignores the First Party Domain of the originAttributes when rewrapping the wrapper.
59 bytes, text/x-review-board-request
mrbkap
: review+
Details
Attached image Screenshot showing the issue — 
[Note]:
- Prerequisites:
Enable perf "privacy.firstparty.isolate"
Disable perf "network.predictor.enabled"
Disable perf "network.predictor.enable-prefetch"

[Affected versions]:
- latest Nightly 53.0a1

[Affected platforms]:
- Ubuntu 16.04 32bit

[Steps to reproduce]:
1. Visit Soundcloud.com
2. Click sign-in
3. Select Continue with Facebook
4. Enter credentials and complete the login process displayed in the new opened pop-up

5. Click Continue with Google
6. Enter credentials and complete the login process displayed in the new opened pop-up

[Expected result]:
- Firefoxx successfully completes the login process using Facebook and Google.

[Actual result]:
- Facebook: After login a blank page is displayed in the new opened pop-up and the log-in using Facebook social network is a success.
- Google: After the login form is completed, nothing happens, user is not signed in.

[Regression range]:
- This is not a regression, It's still an experimental feature not enabled by default in any official build.

[Additional notes]:
- Screenshot attached showing the issue on Facebook.
- Screencast showing the issue on Google: https://dl.dropboxusercontent.com/u/109148197/First%20Party%20Isolation/Soundcloud%20google.mp4

- I should mention that using TOR Browser there is a different behavior. 
Since social networks are not loaded at all, I used a normal login using an email. Each time I finish the login process, I'm logged out immediately. Here is a screencast:
https://dl.dropboxusercontent.com/u/109148197/First%20Party%20Isolation/Tor%20soundcloud.mp4
Has Regression Range: --- → no
Has STR: --- → yes
QA Whiteboard: [qe-fpi]
Someone needs to look into this and figure out what the behavior is supposed to be.  It's either a P1 to fix or a won't fix (since the second window is a new first party context).
Flags: needinfo?(etseng)
Priority: -- → P1
Whiteboard: [tor] → [tor][domsecurity-active]
Arthur: is this site expected to work with first party isolation? That is, is the design such that it can handle popups like this or is this a known broken pattern?
Flags: needinfo?(arthuredelstein)
That's not Ethan's email, let's change to right one(ettseng@mozilla.com).
Flags: needinfo?(etseng) → needinfo?(ettseng)
Assignee: nobody → tihuang
(In reply to Daniel Veditz [:dveditz] from comment #2)
> Arthur: is this site expected to work with first party isolation? That is,
> is the design such that it can handle popups like this or is this a known
> broken pattern?

I think we would need to investigate how this particular authorization flow is implemented. But in principle I think window.opener.location.href should be writable even though FPI is enabled. So a login popup should be able to do, for example,
window.opener.location.href = https://soundcloud.com/login?provider=google&access=token

If, on the other hand, an access token is being passed via third-party cookies or similar, then FPI would break that mechanism.
Flags: needinfo?(arthuredelstein)
(In reply to Tanvi Vyas [:tanvi] from comment #1)
> Someone needs to look into this and figure out what the behavior is supposed
> to be.  It's either a P1 to fix or a won't fix (since the second window is a
> new first party context).

Tim will investigate the root cause, and we'll make the decision based on Arthur's advice in comment 4.
Flags: needinfo?(ettseng)
The reason why Soundcloud login fails is because the facebook window has a different first party domain from the Soundcloud one. During the login process, the facebook window will try to access certain resources in the Soundcloud window through the window.opener. However, the facebook window cannot access these resources since their first party domains are different. So, the access will be blocked.

Although, why Tor browser can log in successfully is because Tor browser's implementation of the first party isolation doesn't tweak the nsIPrincipal(Maybe they do, I don't know). So, the facebook window can still access certain resources, that have the same origin with facebook, within the Soundcloud window.
https://artines1.github.io/openerTracker.html

Above link demonstrates what I said in comment 6. This link has a hidden iframe which records the URL and there are two <a> tags allow you to open a tab or a window which has a different first party than the 'github.io' but has the same one as the hidden iframe. Then the opened tab/window will fetch information from the hidden iframe through the window.opener.

For FPI with firefox, this link won't work, but this will work for Tor browser.
Doesn't this basically mean Tor has a bug? It lets firstPartyDomain pages communication with each others without using postMessage or other cross-domain communication methods.

Or if not, then it is rather unclear to me what behavior is expected with first party domain.
The idea is that all browser identifier sources are scoped to the first party domain. I have not tested it but looking at the example in comment 7 it seems to me there is nothing special about `document.referrer`. I.e. `document.cookie` would probably work as well, reading the cookie(s) out and allowing it to pass them on for usage in a different first party context. If that's the case, yes, then Tor Browser has a bug and the behavior witnessed with FPI on is the one intended.
Arthur, what do you think about comment 9?
Flags: needinfo?(arthuredelstein)
This WIP patch makes the behavior of window.opener consistent with Tor.
Here, we'd like to make our behavior is consistent with Tor in terms of this login problem for now. But with further discussion, this could be changed since this breaks first party isolation, based on comment 9, even though it allows certain websites can log in successfully.
(In reply to Georg Koppen from comment #9)
> The idea is that all browser identifier sources are scoped to the first
> party domain. I have not tested it but looking at the example in comment 7
> it seems to me there is nothing special about `document.referrer`. I.e.
> `document.cookie` would probably work as well, reading the cookie(s) out and
> allowing it to pass them on for usage in a different first party context. If
> that's the case, yes, then Tor Browser has a bug and the behavior witnessed
> with FPI on is the one intended.

I think I agree with Georg here. This behavior is too permissive in Tor Browser.
Flags: needinfo?(arthuredelstein)
Per offline discussion, we decided to make the behavior more restricted, which means we follow the current behavior which firefox now has. At meanwhile, we will watch the response of Tor's users, if they don't like this behavior, which may break more web, we could change this behavior, so we will still make these patches to get review, but not land them. If we decide to make the window.opener more non-restricted, them we can apply these patches.
Another idea (from Richard Barnes):
1. Make these patches being reviewed and landed.
2. Make a pref to enable/disable the effect of these patches.

Then the Tor Browser can simply switch the pref instead of uplifting patches based on their decision.

I think this is a good idea.  The only concern is we might not have enough time to finish this work before the coming merge date (Jan 23, 2017).
(In reply to Ethan Tseng [:ethan] from comment #17)
> Another idea (from Richard Barnes):
> 1. Make these patches being reviewed and landed.
> 2. Make a pref to enable/disable the effect of these patches.
> 
> Then the Tor Browser can simply switch the pref instead of uplifting patches
> based on their decision.
> 
> I think this is a good idea.  The only concern is we might not have enough
> time to finish this work before the coming merge date (Jan 23, 2017).

I agree with that being a good idea. I think I would not worry too much about the deadline as we can backport the patch if needed I guess.
I'm a little confused here -- all of the comments seem to suggest that we don't want these patches so why request review? I guess I could review the patch as "does what it claims to" but I'd rather hold off until we know that we have to take it. I agree with Olli in comment 8: our current behavior seems to be correct and I'd only change it if we absolutely have to.
(In reply to Blake Kaplan (:mrbkap) from comment #19)
> I'm a little confused here -- all of the comments seem to suggest that we
> don't want these patches so why request review? I guess I could review the
> patch as "does what it claims to" but I'd rather hold off until we know that
> we have to take it. I agree with Olli in comment 8: our current behavior
> seems to be correct and I'd only change it if we absolutely have to.

Hi Blake,

Sorry for the confusion.  Some contexts are missing because they're discussed in project meetings.
This is basically a trade-off between correctness and availability.

Tor Browser will use our implementation on First Party Isolation (double keying) to replace theirs starting from ESR 52.  Although we argue that our implementation is correct, in reality it will create a lot of Web breakage for Tor Browser, which we are not sure yet whether Tor users would accept.

So Tim's patch is a workaround in order to make Firefox+FPI behave consistently with current Tor Browser.  The proposal to add a pref (in comment 17) is to make this temporary solution more flexible that Tor Browser can easily experiment the impact and user feedback.  If in the future Tor Project thinks it's fine with taking the correct and more restrictive isolation, we can then remove this workaround from our code base.
Status: NEW → ASSIGNED
Tim, since Barnes, Georg, and I all agree to make a new pref, you should cancel the current review request and re-flag it when you have the new patches.
I will add a pref, which is called 'privacy.firstparty.isolate.restrict_opener_access', with default value 'true'. This pref controls the restriction of the access of window.opener when first party domain is enabled. If it is true, then the access of window.opener across different FPDs within the same origin will be treated as a cross-origin access. Otherwise, it is the same-origin access.

In fact, this does not only affect the access of window.opener, but every access which is protected by JS wrappers.
Comment on attachment 8827908 [details]
Bug 1319773 - Part 2: Add a pref 'privacy.firstparty.isolate.restrict_opener_access' which controls the access of window.opener for different first party domain.

https://reviewboard.mozilla.org/r/105508/#review107824

::: caps/BasePrincipal.cpp:299
(Diff revision 2)
> +  static bool sRestrictedOpenerAccess = false;
> +  static bool sCachedRestrictedAccessPref = false;
> +  if (!sCachedRestrictedAccessPref) {
> +    sCachedRestrictedAccessPref = true;
> +    Preferences::AddBoolVarCache(&sRestrictedOpenerAccess,
> +                                 "privacy.firstparty.isolate.restrict_opener_access");

Preferences is main-thread only.
You should assert it.
Attachment #8827908 - Flags: review?(amarchesini) → review+
I need to see how you use these patches. Are you going to change how we manage the opening of the window?
Flags: needinfo?(tihuang)
Comment on attachment 8829335 [details]
Bug 1319773 - Part 3: Making the WrapperFactory ignores the First Party Domain of the originAttributes when rewrapping the wrapper.

https://reviewboard.mozilla.org/r/106450/#review108056

I think we should be aiming to remove this pref in the future. Having prefs for these low-level security decisions gives me an uncomfortable feeling in my stomach. Let's figure out which configuration we can get away with and settle on that.

::: js/xpconnect/wrappers/AccessCheck.cpp:62
(Diff revision 1)
>  
>  // Same as above, but considering document.domain.
>  bool
>  AccessCheck::subsumesConsideringDomain(JSCompartment* a, JSCompartment* b)
>  {
>      nsIPrincipal* aprin = GetCompartmentPrincipal(a);

Should we assert here that IsRestrictOpenerAccessForFPI is false (and the opposite in AccessCheck::subsumesConsideringDomainIgnoringFPD)? It looks we're changing all callers of subsumesConsideringDomain and I think we should protect against future callers choosing the wrong one.
Attachment #8829335 - Flags: review?(mrbkap) → review+
(In reply to Andrea Marchesini [:baku] from comment #27)
> I need to see how you use these patches. Are you going to change how we
> manage the opening of the window?

No, these patches will not change the way we open the window. These patches will only be used to change how JS wrapper factory consider a same-origin. Tor browser doesn't consider first party domain when wrapping a JS wrapper, so window.opener can be accessed like a same origin window even they are in different first party domains. This behavior is a problem for isolation, but it allows certain login process can work correctly. 

FireFox's first party isolation doesn't have this issue since the JS wrapper factory will consider all originAttributes including first party domain. Arthur and I agree that we should use Firefox's isolation, but it will incur more breakages of Web, like the login problem which was described in this bug. So we would keep a more restricted isolation on JS wrapper for now but leave a flexibility here that Tor can change this behavior by a pref when their users complain a lot.
Flags: needinfo?(tihuang)
Comment on attachment 8827907 [details]
Bug 1319773 - Part 1: Add a SubsumesConsideringDomainIgnoringFPD in BasePrincipal.

https://reviewboard.mozilla.org/r/105506/#review109830
Attachment #8827907 - Flags: review?(amarchesini) → review+
Try looks good.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c08cb388baee
Part 1: Add a SubsumesConsideringDomainIgnoringFPD in BasePrincipal. r=baku
https://hg.mozilla.org/integration/autoland/rev/05e1a37cd829
Part 2: Add a pref 'privacy.firstparty.isolate.restrict_opener_access' which controls the access of window.opener for different first party domain. r=baku
https://hg.mozilla.org/integration/autoland/rev/d7f24c6a2fdb
Part 3: Making the WrapperFactory ignores the First Party Domain of the originAttributes when rewrapping the wrapper. r=mrbkap
Keywords: checkin-needed
Blocks: 1336439
Blocks: 1336440
Blocks: 1336441
Blocks: 1336442
Blocks: 1336458
Blocks: 1336460
Blocks: 1336461
Blocks: 1336462
Blocks: 1336463
Blocks: 1336464
Blocks: 1336465
Blocks: 1336466
Bogdan, could you help to verify websites with this pref?
Flags: needinfo?(bogdan.maris)
(In reply to Tim Huang[:timhuang] from comment #40)
> Bogdan, could you help to verify websites with this pref?

Sure. I'll have a look ASAP. Leaving the needinfo on me just as a reminder.
Using latest Nightly 54.0a1 with 'privacy.firstparty.isolate.restrict_opener_access' set to 'false' I verified that the login process works as expected but the pop-up opened for Facebook credentials stays opened with blank content (same issue in normal Firefox build) but that's another issue. 
Also removing tracking status for 53 since this does not affect normal users that use Fx without FPI.
Status: RESOLVED → VERIFIED
status-firefox53: affected → unaffected
status-firefox54: fixed → verified
Flags: needinfo?(bogdan.maris)
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #42)
> Using latest Nightly 54.0a1 with
> 'privacy.firstparty.isolate.restrict_opener_access' set to 'false' I
> verified that the login process works as expected but the pop-up opened for
> Facebook credentials stays opened with blank content (same issue in normal
> Firefox build) but that's another issue. 

Bogdan, thanks for the verification work!
I don't see tests added in this bug?
Depends on: 1339336
Blocks: 1345961
Looks like this didn't land in 52, so I assume we don't have feedback from Tor users whether/how privacy.firstparty.isolate.restrict_opener_access:true|false affects breakage rates?

I'm considering adding a privacy.firstparty.isolate variant to the privacy shield study [1].


Should I do a single variation:

privacy.firstparty.isolate:true + privacy.firstparty.isolate.restrict_opener_access:true (Tor defaults)

Or 2 variations:

privacy.firstparty.isolate:true + privacy.firstparty.isolate.restrict_opener_access:true (Tor defaults)
privacy.firstparty.isolate:true + privacy.firstparty.isolate.restrict_opener_access:false (new combination)


Note: the more variations we have, the longer the study takes to complete ... up to and including never. :/


[1] https://github.com/mozilla/shield-study-privacy/issues/46
Whiteboard: [tor][domsecurity-active] → [tor][domsecurity-active][dfpi-ok]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: