Closed Bug 1493211 Opened 6 years ago Closed 6 years ago

network.cookie.cookieBehavior == 4 breaks serviceworker interception for doubly(+) nested iframes

Categories

(Firefox :: Protections UI, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 + fixed
firefox64 + fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

This is a tricky situation, I'm not sure how to fix it.

Turning on this pref breaks lots of SW tests, for example test_csp_upgrade-insecure_intercept.html.

The problem is that the test page itself runs inside an iframe.  So the page to embedded (https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/dom/serviceworkers/test/test_csp_upgrade-insecure_intercept.html#28) runs as a doubly nested iframe.

This causes nsGlobalWindowInner::GetTopLevelStorageAreaPrincipal() to return null (https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/dom/base/nsGlobalWindowInner.cpp#6290) but nsGlobalWindowInner::GetTopLevelPrincipal() will return non-null.  Then this check <https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/dom/serviceworkers/ServiceWorkerInterceptController.cpp#41> will fail here: <https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/toolkit/components/antitracking/AntiTrackingCommon.cpp#794>.

I'm not sure how to fix this, without breaking the storage case that we care about...

Andrea, any ideas?
Flags: needinfo?(amarchesini)
Adding asuth mostly for visibility since this is breaking service workers, but neither the bug nor the fix is really about service workers per se, so no need to be alarmed...  :-)
Blocks: 1480780
Keywords: regression
So it turns out that removing this condition fixes this, and it doesn't break anything else that I can see...  And I honestly don't understand why this check is required, this is one of those checks that I wasn't sure about which I was telling you about earlier today, Andrea.

So, I guess I'm gonna remove it? :D  I'll send you a patch and you can reject it if you don't agree with the approach.
Actually, after posting this comment I thought about something, perhaps the fact that all of our tests pass when I remove this show that we don't have enough test coverage, so I checked our test coverage data, and indeed, we don't have test coverage :-(

<https://codecov.io/gh/mozilla/gecko-dev/src/92be42b8f356e0f6f55b29fc3f19556325d97e1d/toolkit/components/antitracking/AntiTrackingCommon.cpp#L793>

So I think the main issue here is that sometimes we need to do the doubly nested iframe check, and sometimes that is not needed.
OK, I believe I have landed on the right fix for this bug finally.  The fix is to make the doubly nested iframe checks specific to storage access requests coming from trackers only, not other things.  Patches coming up.
I will use eStorage in part 2, but having the more detailed data about
who our caller is may be useful for other needs in the future, and was
not too hard to do.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Priority: -- → P3
About nested iframes, we haven't decided if we want to support them or not. Currently we support scenarios where site.com embeds iframe(tracker.com). We don't support: site.com embedding iframe(foobar.com) embedding (tracker.com) - maybe foobar.com is a tracker too.

I think we should support them, extending our permission name with each nested iframe origin. In the previous scenario the permission name, stored on site.com, should be: 3rdPartyStorage^foobar.com^tracker.com^<somethig else here>
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini [:baku] from comment #7)
> About nested iframes, we haven't decided if we want to support them or not.
> Currently we support scenarios where site.com embeds iframe(tracker.com). We
> don't support: site.com embedding iframe(foobar.com) embedding (tracker.com)
> - maybe foobar.com is a tracker too.
> 
> I think we should support them, extending our permission name with each
> nested iframe origin. In the previous scenario the permission name, stored
> on site.com, should be: 3rdPartyStorage^foobar.com^tracker.com^<somethig
> else here>

I don't see what problem supporting this additional case solves (what about the case where there are two non-tracking parents?  what about three/four/five?) but regardless, we shouldn't change our behavior *in this bug* since I would like to backport it to 63.

The purpose of this is only to fix the regression in comment 0 and to allow bug 1492563 to land, not anything else.  How does this sound?
Flags: needinfo?(amarchesini)
Comment on attachment 9011167 [details]
Bug 1493211 - Part 1: Make all consumers of storage access checks in Gecko declare why they are asking for storage access

Andrea Marchesini [:baku] has approved the revision.
Attachment #9011167 - Flags: review+
> The purpose of this is only to fix the regression in comment 0 and to allow
> bug 1492563 to land, not anything else.  How does this sound?

Sure. it makes sense.
Flags: needinfo?(amarchesini)
Thanks!  Did you see part 2 too?
:ehsan, thanks for cc'ing me on this bug.  In fact, this is directly relevant to me as I pursue fixing bug 1487227 which is partially due to the asymmetry between StorageAllowedForChannel and StorageAllowedForWindow when anti-tracking gets involved.  (It's partially also due to a bug where we fail to clear controller status.  I'm currently dealing with adding test coverage; it's very possible this bug is directly related to my failures to repro the diagnostic assert in my new tests, so it's quite timely!)

I worry that the StoragePurpose value is moving us further down the road to more asymmetry.  As I view it, the question that all of the storage-related APIs are asking are:
What storage partition is this client allowed to access, if any?

Right now, the answer is the tuple of (StorageAllowedFor(...), Principal), where we're constantly re-computing StorageAllowedFor(...) and latching it in a few cases.  (Specifically, the StoragePurpose "eOther" case in WorkerPrivate.cpp latches it for Workers, and various APIs effectively latch a value of "true" once they instantiate their bindings/whatever.)

As I understand :baku's desire, ideally this could be somewhat decoupled from the Principal so that the Storage Access API could change the partition in use by a client.


The StoragePurpose field seems to move us farther from that goal.  Also, if I understand things correctly, the answer to the question for nested iframes right now is "no storage partition should be used".  The patch seems to be a hack to disable this rule for ServiceWorkers so that the existing tests work, but I don't think this is actually the behavior you want in non-test-cases.  Hack-wise, it seems like you'd be better off just letting the "dom.serviceWorkers.testing.enabled" pref be the thing that breaks your tracking protection logic.  Or add an additional pref in all those places that disables tracking protection so we can have some tests where it is enabled.

If you're looking for something even hackier than that, GetTopLevelStorageAreaPrincipal() uses IsTopLevelWindow() uses GetScriptableTop() which is impacted by "mozbrowser".  Maybe the SW tests' iframes could be hacked up with "mozbrowser" annotations?


I may be misunderstanding, however.  Is there some documentation that provides a rough flow chart of the decision making process about when storage should be allowed?  Other than reading the code, https://wiki.mozilla.org/Security/Tracking_protection#Code_walkthrough is what I've found thus far, but it doesn't get into things like iframes.
Firstly, the problem here is not that this breaks our tests.  The issue is that this really breaks interception of doubly nested iframes, as well as anything else in Gecko that relies on these storage checks for nested iframes.  Our tests just helped uncover this issue, but this isn't at all about our tests only.  So your suggestion on using the dom.serviceWorkers.testing.enabled pref is missing this important point, I believe.

Another general point: Our storage checks determine two things:

a) Whether an origin should have access to storage, and if so, what are the lifetimes and persistency constraints of the storage.

b) Whether an origin should be allowed to communicate with other contexts from the same origin.

We generally don't determine anything about the origin's storage paritition as part of these storage checks, although the origin's storage, if granted, may be partitioned transparently.

So as I was typing this, it occurred to me that the approach taken in part 2 may not be quite right, actually.  My initial idea was to apply the doubly nested restrictions only to the storage APIs, but looking at the enum values again, they don't parition the callers across the (a)/(b) categories above very well, since for example eDOMCacheAPI is still storage, and eServiceWorkersAPI is both storage and communication.

You may have noted that the part 2 patch is doing two things at the same time, one is moving the tracker check before the nesting level check (to make that path match the window case) and one is applying the aPurpose==eStorage check.  Maybe the desired behavior here is to actually apply the interception restriction to doubly nested iframes if they are coming from trackers, and only do the first thing that the part 2 patch here is doing?

I guess my biggest difficulty here is figuring out what the heck the desired behavior we want to have here is. :-(
Flags: needinfo?(amarchesini)
We talked on IRC and decided to do the following:

(In reply to :Ehsan Akhgari from comment #13)
> Maybe the desired behavior here is to actually
> apply the interception restriction to doubly nested iframes if they are
> coming from trackers, and only do the first thing that the part 2 patch here
> is doing?
Flags: needinfo?(amarchesini)
Attachment #9011167 - Attachment is obsolete: true
Attachment #9011168 - Attachment is obsolete: true
Comment on attachment 9011966 [details]
Bug 1493211 - Make the doubly nested iframe checks specific to requests coming from trackers

Andrea Marchesini [:baku] has approved the revision.
Attachment #9011966 - Flags: review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b69245da928a
Make the doubly nested iframe checks specific to requests coming from trackers r=baku
Comment on attachment 9011966 [details]
Bug 1493211 - Make the doubly nested iframe checks specific to requests coming from trackers

Approval Request Comment
[Feature/Bug causing the regression]: Bug fix in new feature
[User impact if declined]: Turning on the new anti-tracking pref breaks interception by service workers in doubly nested iframes.
[Is this code covered by automated tests?]: Yes, but on m-c.
[Has the fix been verified in Nightly?]: Just landed.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Small fix in code that is preffed off by default.
[String changes made/needed]: None.
Attachment #9011966 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/b69245da928a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Comment on attachment 9011966 [details]
Bug 1493211 - Make the doubly nested iframe checks specific to requests coming from trackers

Low risk patch for a feature preffed off by default, approved for 63 beta 10, thanks.
Attachment #9011966 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: