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

Status: RESOLVED FIXED

(Firefox :: Protections UI, defect, P3)

Description

[(no longer active)]

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?

Comment 1

[(no longer active)]

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

Comment 2

[(no longer active)]

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.

Comment 3

[(no longer active)]

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.

Comment 4

[(no longer active)]

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.

Comment 5

[(no longer active)]

Attachment: Bug 1493211 - Part 1: Make all consumers of storage access checks in Gecko declare why they are asking for storage access

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.

Comment 6

[(no longer active)]

Attachment: Bug 1493211 - Part 2: Make the doubly nested iframe checks specific to storage access requests coming from trackers

Depends on D6559

Comment 7

[Andrea Marchesini [:baku]]

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>

Comment 8

[(no longer active)]

(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?

Comment 9

[Andrea Marchesini [:baku]]

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.

Comment 10

[Andrea Marchesini [:baku]]

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

Comment 11

[(no longer active)]

Thanks!  Did you see part 2 too?

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 13

[(no longer active)]

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. :-(

Comment 14

[(no longer active)]

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?

Comment 15

[(no longer active)]

Attachment: Bug 1493211 - Make the doubly nested iframe checks specific to requests coming from trackers

Comment 16

[Andrea Marchesini [:baku]]

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.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b69245da928a
Make the doubly nested iframe checks specific to requests coming from trackers r=baku

Comment 18

[(no longer active)]

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.

Comment 19

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/b69245da928a

Comment 20

[Pascal Chevrel:pascalc]

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.

Comment 21

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d72ddbe97988