1933428 - ThirdPartyUtil doesn't properly determine thirdpartyness for channels initiated by a sandboxed context

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, defect, P2)

Description

[Tim Huang[:timhuang]]

Currently, we don't properly determine the thirdPartyness for channels initiated by a sandboxed context. There are two cases here.

• The channel initiated by the top-level sandboxed context
  In this case, we currently treat the channel as a third party because the top-level sandboxed context uses a null principal. So, any request from it will be considered a third-party request. However, we should consider channels as first-party if they are supposed to inherit the principal from the sandboxed context.
• The channel initiated by an sandboxed iframe
  In this case, we should treat the channel as a third party because the sandboxed iframe is supposed to be considered foreign. But we don't correctly apply the foreign bit in our storage access implementation.

Comment 1

[Tim Huang[:timhuang]]

Attachment: Bug 1933428 - Part 1: Check sandbox flags in ThirdPartyUtil::IsThirdPartyWindow() and ThirdPartyUtil::IsThirdPartyGlobal(). r?bvandersloot

The patch changes the ThirdPartyUtil::IsThirdPartyWindow() and
ThirdPartyUtil::IsThirdPartyGlobal() to consider the sandbox flags when
doing the third-party checks.

Comment 2

[Tim Huang[:timhuang]]

Attachment: Bug 1933428 - Part 2: Merge the mPartitionKey for the CookieJarSettings when loadInfos get merged in the child process. r?bvandersloot

Depends on D230817

Comment 3

[Tim Huang[:timhuang]]

Attachment: Bug 1933428 - Part 3: Recompute partitionKey for top-level documents if required. r?bvandersloot!

The loading principal of any requets that is comming from a top-level
sandboxed context will be a null principal. So, they will be treated as
third-party because a null principal is always consider third-party to
other principals.

However, we need to consider the base domain of the requests. We should
treat the channel as first-party in this case if the channel is comming
from the same base domain as the top-level sandboxed context.

In this patch, we use the precursor principal of the null principal to
check if their base domains match.

Depends on D230818

Comment 4

[Tim Huang[:timhuang]]

Attachment: Bug 1933428 - Part 4: Add a test to ensure we don't hit invalid first-party cookie assertion. r?bvandersloot!

Depends on D230819

Comment 5

[Tim Huang[:timhuang]]

Attachment: Bug 1933428 - Part 5: Consider sandbox flags in AntiTrackingUtils::IsThirdPartyDocument(). r?bvandersloot!

We didn't consider sandboxed flags in
AntiTrackingUtils::IsThirdPartyDocument() check. So, it would
incorrectly consider a sandboxed iframe first party.

This patch fixes this issue.

Depends on D230820

Comment 6

[Tim Huang[:timhuang]]

Attachment: Bug 1933428 - Part 6: Modify the way how we merge cookieJarSettings. r?bvandersloot!

Currently, we replace the values in the merging cookieJarSettings to
merge two cookieJarSettings. This behavior could change
cookieJarSettings that shouldn't be change becasue cookieJarSettings is
passed by pointers.

For example, the partitionKey of the worker private's cookieJarSettings
will be changed after calling importScripts() in ABA context.

To avoid this, this patch changes the merging behavior. The Merge()
function will create a new cookieJarSettings to merge. So we don't change the
existing one.

Depends on D231313

Comment 7

[Tim Huang[:timhuang]]

Attachment: Bug 1933428 - Part 7: Update the test expection of web-platform-test network-partition-key.html. r?bvandersloot!

Depends on D231314

Comment 8

[Tim Huang[:timhuang]]

Attachment: Bug 1933428 - Part 8: Support null principal URIs for ThirdPartyUtil::GetBaseDomain() r?bvandersloot!

Depends on D232631

Comment 9

[Tim Huang[:timhuang]]

Attachment: Bug 1933428 - Part 9: Enforce Session if the cookies is partitioned by a null principal partitionKey. r?valentin!

We are now using a partitionKey created from a null principal if the
top-level context is sandboxed. Because every null principal is unique,
so setting cookies under this partitionKey will end up with cookies that
we cannot delete.

To avoid creating dangling cookies like this, we enforece session
cookies if they are created under a null prinicpal partition key.

Depends on D232632

Comment 10

[Tim Huang[:timhuang]]

Attachment: Bug 1933428 - Part 10: Update the test test_bug1769155.html. r?smaug

We need to update this test because the way we compute the partitionKey
for top-level blob URL is changed. Previously, the blob URL doesn't go
through the Http Channel, so the partitionKey won't be populated.

After we recompute the partitionKey in top-level document, now we
populate the partitionKey according to the document's principal. This
changes breaks this test in xorigin test because the blob url created
under the top-level xorigin domain can no longer be resolved under
top-level mochitest domain.

To fix this test, we create the blob url directly in the mochitest
window, so the blob url can be resolved.

Depends on D232633

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7b1a3832630
Part 1: Check sandbox flags in ThirdPartyUtil::IsThirdPartyWindow() and ThirdPartyUtil::IsThirdPartyGlobal(). r=bvandersloot
https://hg.mozilla.org/integration/autoland/rev/af92deb55302
Part 2: Merge the mPartitionKey for the CookieJarSettings when loadInfos get merged in the child process. r=bvandersloot,cookie-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/9878302a4cb4
Part 3: Recompute partitionKey for top-level documents if required. r=bvandersloot,smaug
https://hg.mozilla.org/integration/autoland/rev/3db923cf4bb8
Part 4: Add a test to ensure we don't hit invalid first-party cookie assertion. r=bvandersloot,anti-tracking-reviewers
https://hg.mozilla.org/integration/autoland/rev/fc400e4ab086
Part 5: Consider sandbox flags in AntiTrackingUtils::IsThirdPartyDocument(). r=bvandersloot,anti-tracking-reviewers
https://hg.mozilla.org/integration/autoland/rev/b122f021875e
Part 6: Modify the way how we merge cookieJarSettings. r=bvandersloot,cookie-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/d31e0480bbd9
Part 7: Update the test expection of web-platform-test network-partition-key.html. r=bvandersloot
https://hg.mozilla.org/integration/autoland/rev/b2cfb42e31bf
Part 8: Support null principal URIs for ThirdPartyUtil::GetBaseDomain() r=bvandersloot
https://hg.mozilla.org/integration/autoland/rev/85aab9235de5
Part 9: Enforce Session if the cookies is partitioned by a null principal partitionKey. r=valentin,cookie-reviewers,anti-tracking-reviewers,bvandersloot
https://hg.mozilla.org/integration/autoland/rev/94c12c68067f
Part 10: Update the test test_bug1769155.html. r=smaug
https://hg.mozilla.org/integration/autoland/rev/688df3cbad90
apply code formatting via Lando

Comment 12

[Sandor Molnar[:smolnar]]

Backed out for causing mochitest failures @ test_iframe_sandbox_popups_inheritance.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/f3e32ec71fc520fb6802b4ae6ca1363dc21f9bb8

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | dom/html/test/test_iframe_sandbox_popups_inheritance.html | Test timed out

Comment 13

[Kershaw Chang [:kershaw]]

(In reply to Sandor Molnar[:smolnar] from comment #12)

Backed out for causing mochitest failures @ test_iframe_sandbox_popups_inheritance.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/f3e32ec71fc520fb6802b4ae6ca1363dc21f9bb8

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | dom/html/test/test_iframe_sandbox_popups_inheritance.html | Test timed out

I'll try to fix this test failure in bug 1940723.

Comment 14

[Tim Huang[:timhuang]]

Attachment: Bug 1933428 - Skip test_iframe_sandbox_popups_inheritance.html in http3 test. r?kershaw!

The test is currently failing in Http3 server test because the http3
server crashes during the test. The test is running fine with http2
server.

Depends on D233096

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d58c186d315
Part 1: Check sandbox flags in ThirdPartyUtil::IsThirdPartyWindow() and ThirdPartyUtil::IsThirdPartyGlobal(). r=bvandersloot
https://hg.mozilla.org/integration/autoland/rev/93109a32cd47
Part 2: Merge the mPartitionKey for the CookieJarSettings when loadInfos get merged in the child process. r=bvandersloot,cookie-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/32e0f0c78f14
Part 3: Recompute partitionKey for top-level documents if required. r=bvandersloot,smaug
https://hg.mozilla.org/integration/autoland/rev/130e48d0087e
Part 4: Add a test to ensure we don't hit invalid first-party cookie assertion. r=bvandersloot,anti-tracking-reviewers
https://hg.mozilla.org/integration/autoland/rev/aa5c2a5c1a4c
Part 5: Consider sandbox flags in AntiTrackingUtils::IsThirdPartyDocument(). r=bvandersloot,anti-tracking-reviewers
https://hg.mozilla.org/integration/autoland/rev/e6150204f99c
Part 6: Modify the way how we merge cookieJarSettings. r=bvandersloot,cookie-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/3380a7392f48
Part 7: Update the test expection of web-platform-test network-partition-key.html. r=bvandersloot
https://hg.mozilla.org/integration/autoland/rev/a67dbc81dcd0
Part 8: Support null principal URIs for ThirdPartyUtil::GetBaseDomain() r=bvandersloot
https://hg.mozilla.org/integration/autoland/rev/b246d3a33f31
Part 9: Enforce Session if the cookies is partitioned by a null principal partitionKey. r=valentin,cookie-reviewers,anti-tracking-reviewers,bvandersloot
https://hg.mozilla.org/integration/autoland/rev/f7ff7967b068
Part 10: Update the test test_bug1769155.html. r=smaug
https://hg.mozilla.org/integration/autoland/rev/7a068fb52af0
Skip test_iframe_sandbox_popups_inheritance.html in http3 test. r=kershaw
https://hg.mozilla.org/integration/autoland/rev/1d0be15990d4
apply code formatting via Lando

Comment 16

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/1d58c186d315
https://hg.mozilla.org/mozilla-central/rev/93109a32cd47
https://hg.mozilla.org/mozilla-central/rev/32e0f0c78f14
https://hg.mozilla.org/mozilla-central/rev/130e48d0087e
https://hg.mozilla.org/mozilla-central/rev/aa5c2a5c1a4c
https://hg.mozilla.org/mozilla-central/rev/e6150204f99c
https://hg.mozilla.org/mozilla-central/rev/3380a7392f48
https://hg.mozilla.org/mozilla-central/rev/a67dbc81dcd0
https://hg.mozilla.org/mozilla-central/rev/b246d3a33f31
https://hg.mozilla.org/mozilla-central/rev/f7ff7967b068
https://hg.mozilla.org/mozilla-central/rev/7a068fb52af0
https://hg.mozilla.org/mozilla-central/rev/1d0be15990d4

Comment 17

[Andra Esanu (needinfo me)]

(In reply to Pulsebot from comment #15)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d58c186d315
Part 1: Check sandbox flags in ThirdPartyUtil::IsThirdPartyWindow() and
ThirdPartyUtil::IsThirdPartyGlobal(). r=bvandersloot
https://hg.mozilla.org/integration/autoland/rev/93109a32cd47
Part 2: Merge the mPartitionKey for the CookieJarSettings when loadInfos get
merged in the child process. r=bvandersloot,cookie-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/32e0f0c78f14
Part 3: Recompute partitionKey for top-level documents if required.
r=bvandersloot,smaug
https://hg.mozilla.org/integration/autoland/rev/130e48d0087e
Part 4: Add a test to ensure we don't hit invalid first-party cookie
assertion. r=bvandersloot,anti-tracking-reviewers
https://hg.mozilla.org/integration/autoland/rev/aa5c2a5c1a4c
Part 5: Consider sandbox flags in AntiTrackingUtils::IsThirdPartyDocument().
r=bvandersloot,anti-tracking-reviewers
https://hg.mozilla.org/integration/autoland/rev/e6150204f99c
Part 6: Modify the way how we merge cookieJarSettings.
r=bvandersloot,cookie-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/3380a7392f48
Part 7: Update the test expection of web-platform-test
network-partition-key.html. r=bvandersloot
https://hg.mozilla.org/integration/autoland/rev/a67dbc81dcd0
Part 8: Support null principal URIs for ThirdPartyUtil::GetBaseDomain()
r=bvandersloot
https://hg.mozilla.org/integration/autoland/rev/b246d3a33f31
Part 9: Enforce Session if the cookies is partitioned by a null principal
partitionKey.
r=valentin,cookie-reviewers,anti-tracking-reviewers,bvandersloot
https://hg.mozilla.org/integration/autoland/rev/f7ff7967b068
Part 10: Update the test test_bug1769155.html. r=smaug
https://hg.mozilla.org/integration/autoland/rev/7a068fb52af0
Skip test_iframe_sandbox_popups_inheritance.html in http3 test. r=kershaw
https://hg.mozilla.org/integration/autoland/rev/1d0be15990d4
apply code formatting via Lando

Perfherder has detected a browsertime performance change from push 1d0be15990d4f5402d25d8f47139ac57083b75c2.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
7%	wikia fcp	linux1804-64-shippable-qr	fission warm webrender	126.61 -> 118.02	Before/After
7%	wikia fcp	linux1804-64-shippable-qr	fission warm webrender	127.11 -> 118.83	Before/After
5%	wikia SpeedIndex	linux1804-64-shippable-qr	fission warm webrender	509.44 -> 484.91	Before/After
5%	wikia PerceptualSpeedIndex	linux1804-64-shippable-qr	fission warm webrender	478.84 -> 455.88	Before/After
5%	wikia largestContentfulPaint	linux1804-64-shippable-qr	fission warm webrender	182.38 -> 174.13	Before/After
...	...	...	...	...	...
3%	cnn loadtime	macosx1470-64-shippable	bytecode-cached fission warm webrender	505.95 -> 489.02	Before/After

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 43424

For more information on performance sheriffing please see our FAQ.