Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

consider including a "cross-site ancestor chain" bit in the storage key #25

Open
wanderview opened this issue Oct 6, 2021 · 12 comments

Comments

@wanderview
Copy link

Currently service workers have poor SameSite cookie protections because its "site for cookies" is simply set to the origin:

https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis#section-5.2.2.2

In contrast, documents take into account the top-level-site and the ancestor chain when computing "site for cookies":

https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis#section-5.2.1

This is problematic because it means adding a service worker to a site can reduce the safety of SameSite cookies.

With storage partitioning we have the opportunity to fix this. We already plan to include top-level site in the storage key which will allow us to include it in the "site for cookies" computation for service worker. We lack any ancestor chain information, however.

The ancestor chain is important for "site for cookies" because it helps protect against clickjacking attacks. To extend this protection to service workers we propose:

Include a "cross-site ancestor chain" bit in the storage key. This bit would be true if there are any sites between the current context and the top-level context that are cross-site to the current context. So it would be true for A -> B -> C or A -> B -> A. It would be false for A -> A or A -> B.

With this bit in the storage key it would permit us to compute a "site for cookies" value for service workers that is equivalent to any document controlled by that service worker.

This was discussed at the recent service worker virtual F2F: w3c/ServiceWorker#1604.

@annevk
Copy link
Collaborator

annevk commented Oct 7, 2021

Just to be clear on the implications here:

  1. Since it's part of the storage key, it would affect an entire storage shelf. I.e., service workers, localStorage, etc.
  2. It would mean that documents that have synchronous script access (in A1 -> B -> A2, this would go for A1 and A2) no longer observe the same storage shelf so you have to be careful where you peek into storage.

The benefit of this is that sites can adopt service workers easily without ending up in a worse security situation. That makes me think it's worth it, despite the fact that 2 above is something I had been trying to avoid.

@wanderview
Copy link
Author

Are there known use cases for synchronous scripting in this scenario? Could we look to make synchronous scripting also dependent on the cross-site entries in the ancestor chain?

@annevk
Copy link
Collaborator

annevk commented Oct 7, 2021

In theory it's possible, but I'm not sure what the compat story for that is. Would it be a new agent cluster essentially or something in between? (We'd also have to untangle the window name targeting algorithm presumably, but that needs solving anyway.)

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 19, 2021
This CL adds a number of new cases to the service worker SameSite
cookies test.  The cases break down into two general types:

1. Cases where A1 frames B frames A2, and then A2 calls window.open()
   to an A origin URL.
2. Cases where A1 frames B frames A2, and then A2 sets the location
   to an A origin URL.

For (1) we expect SameSite strict cookies to be sent because
window.open() creates a top-level context that will have a populated
site-for-cookies and the initiator is same-origin (regardless of the
cross-site ancestor chain).

For (2) we expect only SameSite=None cookies to be sent.  This is
because setting the location results in a navigation to an A1->B->A3
nested frame with an empty site-for-cookies.

We currently fail the passthrough and change-request cases for (2).
We plan to fix this as part of storage partitioning with an ancestor
chain bit in the StorageKey.  See:

privacycg/storage-partitioning#25

This CL also includes some minor cleanup of the WPT test and associated
resources.

Bug: 1115847
Change-Id: I9002e60a271ae95d1d702068d44b30bd0e33b5dc
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 22, 2021
This CL adds a number of new cases to the service worker SameSite
cookies test.  The cases break down into two general types:

1. Cases where A1 frames B frames A2, and then A2 calls window.open()
   to an A origin URL.
2. Cases where A1 frames B frames A2, and then A2 sets the location
   to an A origin URL.

For (1) we expect SameSite strict cookies to be sent because
window.open() creates a top-level context that will have a populated
site-for-cookies and the initiator is same-origin (regardless of the
cross-site ancestor chain).

For (2) we expect only SameSite=None cookies to be sent.  This is
because setting the location results in a navigation to an A1->B->A3
nested frame with an empty site-for-cookies.

We currently fail the passthrough and change-request cases for (2).
We plan to fix this as part of storage partitioning with an ancestor
chain bit in the StorageKey.  See:

privacycg/storage-partitioning#25

This CL also includes some minor cleanup of the WPT test and associated
resources.

Bug: 1115847
Change-Id: I9002e60a271ae95d1d702068d44b30bd0e33b5dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3277058
Reviewed-by: Steven Bingler <bingler@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#944293}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Nov 23, 2021
This CL adds a number of new cases to the service worker SameSite
cookies test.  The cases break down into two general types:

1. Cases where A1 frames B frames A2, and then A2 calls window.open()
   to an A origin URL.
2. Cases where A1 frames B frames A2, and then A2 sets the location
   to an A origin URL.

For (1) we expect SameSite strict cookies to be sent because
window.open() creates a top-level context that will have a populated
site-for-cookies and the initiator is same-origin (regardless of the
cross-site ancestor chain).

For (2) we expect only SameSite=None cookies to be sent.  This is
because setting the location results in a navigation to an A1->B->A3
nested frame with an empty site-for-cookies.

We currently fail the passthrough and change-request cases for (2).
We plan to fix this as part of storage partitioning with an ancestor
chain bit in the StorageKey.  See:

privacycg/storage-partitioning#25

This CL also includes some minor cleanup of the WPT test and associated
resources.

Bug: 1115847
Change-Id: I9002e60a271ae95d1d702068d44b30bd0e33b5dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3277058
Reviewed-by: Steven Bingler <bingler@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#944293}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 23, 2021
This CL adds a number of new cases to the service worker SameSite
cookies test.  The cases break down into two general types:

1. Cases where A1 frames B frames A2, and then A2 calls window.open()
   to an A origin URL.
2. Cases where A1 frames B frames A2, and then A2 sets the location
   to an A origin URL.

For (1) we expect SameSite strict cookies to be sent because
window.open() creates a top-level context that will have a populated
site-for-cookies and the initiator is same-origin (regardless of the
cross-site ancestor chain).

For (2) we expect only SameSite=None cookies to be sent.  This is
because setting the location results in a navigation to an A1->B->A3
nested frame with an empty site-for-cookies.

We currently fail the passthrough and change-request cases for (2).
We plan to fix this as part of storage partitioning with an ancestor
chain bit in the StorageKey.  See:

privacycg/storage-partitioning#25

This CL also includes some minor cleanup of the WPT test and associated
resources.

Bug: 1115847
Change-Id: I9002e60a271ae95d1d702068d44b30bd0e33b5dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3277058
Reviewed-by: Steven Bingler <bingler@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#944293}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 30, 2021
…rviceWorkers with nested frames., a=testonly

Automatic update from web-platform-tests
Add WPT tests for SameSite cookies in ServiceWorkers with nested frames.

This CL adds a number of new cases to the service worker SameSite
cookies test.  The cases break down into two general types:

1. Cases where A1 frames B frames A2, and then A2 calls window.open()
   to an A origin URL.
2. Cases where A1 frames B frames A2, and then A2 sets the location
   to an A origin URL.

For (1) we expect SameSite strict cookies to be sent because
window.open() creates a top-level context that will have a populated
site-for-cookies and the initiator is same-origin (regardless of the
cross-site ancestor chain).

For (2) we expect only SameSite=None cookies to be sent.  This is
because setting the location results in a navigation to an A1->B->A3
nested frame with an empty site-for-cookies.

We currently fail the passthrough and change-request cases for (2).
We plan to fix this as part of storage partitioning with an ancestor
chain bit in the StorageKey.  See:

privacycg/storage-partitioning#25

This CL also includes some minor cleanup of the WPT test and associated
resources.

Bug: 1115847
Change-Id: I9002e60a271ae95d1d702068d44b30bd0e33b5dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3277058
Reviewed-by: Steven Bingler <bingler@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#944293}

--

wpt-commits: bfe98d4bc019420281dae737739cb3982010fbc0
wpt-pr: 31690
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Nov 30, 2021
…rviceWorkers with nested frames., a=testonly

Automatic update from web-platform-tests
Add WPT tests for SameSite cookies in ServiceWorkers with nested frames.

This CL adds a number of new cases to the service worker SameSite
cookies test.  The cases break down into two general types:

1. Cases where A1 frames B frames A2, and then A2 calls window.open()
   to an A origin URL.
2. Cases where A1 frames B frames A2, and then A2 sets the location
   to an A origin URL.

For (1) we expect SameSite strict cookies to be sent because
window.open() creates a top-level context that will have a populated
site-for-cookies and the initiator is same-origin (regardless of the
cross-site ancestor chain).

For (2) we expect only SameSite=None cookies to be sent.  This is
because setting the location results in a navigation to an A1->B->A3
nested frame with an empty site-for-cookies.

We currently fail the passthrough and change-request cases for (2).
We plan to fix this as part of storage partitioning with an ancestor
chain bit in the StorageKey.  See:

privacycg/storage-partitioning#25

This CL also includes some minor cleanup of the WPT test and associated
resources.

Bug: 1115847
Change-Id: I9002e60a271ae95d1d702068d44b30bd0e33b5dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3277058
Reviewed-by: Steven Bingler <bingler@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#944293}

--

wpt-commits: bfe98d4bc019420281dae737739cb3982010fbc0
wpt-pr: 31690
@annevk
Copy link
Collaborator

annevk commented May 13, 2022

In the last call @bvandersloot-mozilla mentioned Mozilla was in principle on board with this. @johnwilander has Apple looked at this?

A point that @johannhof brought up that I think is quite important is offering consistency to web developers. I don't think it would be a great outcome if storage is partitioned, but network and cookies are not. Ideally we offer the same boundary across the board.

@wanderview
Copy link
Author

A point that @johannhof brought up that I think is quite important is offering consistency to web developers. I don't think it would be a great outcome if storage is partitioned, but network and cookies are not. Ideally we offer the same boundary across the board.

I agree with this in principle, but I've been told there are important use cases for cookies that make the situation different from other storage. I would also note cookies are already quite different from other storage in other ways. Expiration, quota, network transmission, etc. They also tend to be used for different use cases; auth vs stateful app logic, etc. While undesirable, it doesn't seem too extreme to diverge on this nuanced aspect of partitioning.

@annevk
Copy link
Collaborator

annevk commented May 18, 2022

I opened #31, #32, and privacycg/CHIPS#40 for the various cookie-related issues around this. I agree that in principle we could end up with a different solution there, but I'm rather worried about subtle application bugs and web developer confusion. As well as making it difficult to offer consistent APIs around partitioning, such as whether a given environment is considered partitioned or not.

@wanderview
Copy link
Author

Note, this statement from the original post above ended up not being quite right:

This bit would be true if there are any sites between the current context and the top-level context that are cross-site to the current context.

We need to also check the top-level context and not just ancestors in-between. Basically we set the bit to true if site-for-cookies is not populated.

mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
This CL adds a number of new cases to the service worker SameSite
cookies test.  The cases break down into two general types:

1. Cases where A1 frames B frames A2, and then A2 calls window.open()
   to an A origin URL.
2. Cases where A1 frames B frames A2, and then A2 sets the location
   to an A origin URL.

For (1) we expect SameSite strict cookies to be sent because
window.open() creates a top-level context that will have a populated
site-for-cookies and the initiator is same-origin (regardless of the
cross-site ancestor chain).

For (2) we expect only SameSite=None cookies to be sent.  This is
because setting the location results in a navigation to an A1->B->A3
nested frame with an empty site-for-cookies.

We currently fail the passthrough and change-request cases for (2).
We plan to fix this as part of storage partitioning with an ancestor
chain bit in the StorageKey.  See:

privacycg/storage-partitioning#25

This CL also includes some minor cleanup of the WPT test and associated
resources.

Bug: 1115847
Change-Id: I9002e60a271ae95d1d702068d44b30bd0e33b5dc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3277058
Reviewed-by: Steven Bingler <bingler@chromium.org>
Commit-Queue: Ben Kelly <wanderview@chromium.org>
Cr-Commit-Position: refs/heads/main@{#944293}
NOKEYCHECK=True
GitOrigin-RevId: 53f29314ad19fc5e0e0500de0d9544b856a45e32
@guest271314
Copy link

This concept - implemented as default on Chromium 112 - is causing issues.

Before this I could append an iframe to a Web document, set src to chrome-extension: URL and the iframe would be set as a WindowClient of the MV3 ServiceWorker, and navigator.serviceWorker.getRegistrations() fulfilled to an array containing the MV3 ServiceWorker. With this enabbled by default neither occur.

We have to launch Chromium and Chrome with --disable-features=ThirdPartyStoragePartitioning to get the behaviour we expect.

Is the goal of this really for embedded iframes to not be a WindowClient of the ServiceWorker scope they are registered under; and for a document loaded under the scope of a ServiceWorker registration to not get a reference to the active ServiceWorker?

@wanderview
Copy link
Author

Please file a chromium bug:

https://bugs.chromium.org/p/chromium/issues/entry?labels=StoragePartitioning-trial-bugs&components=Blink%3EStorage

We have some affordances for extensions and we might be able to accomodate this as well. I think this is more of an extensions thing, though, and does not really belong in this issue.

@wanderview
Copy link
Author

wanderview commented Jan 30, 2023

I went ahead and filed https://crbug.com/1411422.

@guest271314
Copy link

You will have to see the matter re extensions through. Chromium authors banned me from filing bugs there. W3C (where Web Extensions has a group), and Google groups (extensions) banned me.

@wanderview
Copy link
Author

Understood. We'll take a look at it. At first glance it seems like something we should fix. Thanks for the report.

(And also, we have not enabled partitioning by default in any chrome release yet. I'm guessing maybe you have experimental web features enabled in chrome://flags or something.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants
@wanderview @annevk @guest271314 and others